quality of visual studio 2017 compiler?

[stratum]

Hi

Is there anybody other than me who see the visual studio 2017 optimizer fail on relatively trivial things? That's completely unbelievable. The code below works correctly with #pragma optimize( "", off ) and fails with #pragma optimize( "", on )

Code: Select all

          // testing db read/write methods		
          if (p_array[idx].parameter1 != t_value.parameter1 ||
			p_array[idx].parameter2 != t_value.parameter2 ||
			p_array[idx].parameter3 != t_value.parameter3 ||
			p_array[idx].parameter4 != t_value.parameter4 ||
			p_array[idx].parameter5 != t_value.parameter5 ||
			p_array[idx].parameter6 != t_value.parameter6 ||
			p_array[idx].parameter7 != t_value.parameter7 ||
			(fabs(p_array[idx].parameter8 - t_value.parameter8) > 1E-6) ||
			(fabs(p_array[idx].parameter9 - t_value.parameter9) > 1E-6) ||
			(fabs(p_array[idx].parameter10 - t_value.parameter10) > 1E-6))
        {
			std:cerr << "Incorrect values returned by GetRecord:\n";
			std:cerr << "parameter1: '" << p_array[idx].parameter1 << "' vs '" <<t_value.parameter1 << "'\n";
			std:cerr << "parameter2: '" << p_array[idx].parameter2 << "' vs '" << t_value.parameter2 << "'\n";
			std:cerr << "parameter3: '" << p_array[idx].parameter3 << "' vs '" << t_value.parameter3 << "'\n";
			std:cerr << "parameter4: '" << p_array[idx].parameter4 << "' vs '" << t_value.parameter4 << "'\n";
			std:cerr << "parameter5: '" << p_array[idx].parameter5 << "' vs '" << t_value.parameter5 << "'\n";
			std:cerr << "parameter6: '" << p_array[idx].parameter6 << "' vs '" << t_value.parameter6 << "'\n";
			std:cerr << "parameter7: '" << p_array[idx].parameter7 << "' vs '" << t_value.parameter7 << "'\n";
			std:cerr << "parameter8: '" << p_array[idx].parameter8 << "' vs '" << t_value.parameter8 << "'\n";			
			std:cerr << "parameter9: '" << p_array[idx].parameter9 << "' vs '" << t_value.parameter9 << "'\n";
			std:cerr << "parameter10: '" << p_array[idx].parameter10 << "' vs '" << t_value.parameter10 << "'\n";
			return false;
        }
 

Thanks!

[vortico]

More context is needed to answer this question. You can use Godbolt with msvc 19.x to check how it compiles. https://godbolt.org/z/dXHWS6

[quikquak]

What type are parameter# etc?
And what fails?

[stratum]

What type are parameter# etc?

The ones that are compared with "fabs(a-b) >1E-6" are double. The others are std::string. Perhaps there were also one or two integers.

The if condition evalutates to true in spite of the fact that it should evaluate to false and the std::cerr trace that follows clearly shows that the compared values are the same.

We had a thread about this some time ago, and the code that had failed during that time was unrelated but the symptom was the same: Incorrect evalutation of if condition. The faulty code was in the same process though, so there may be a relation.

I suspect that there is a memory bug in this program but oddly compiler optimizations reveal it.

[camsr]

I know a better way.. accumulate the results of the comparisons outside the branch. Then test the accumulated variable.

[stratum]

More context is needed to answer this question. You can use Godbolt with msvc 19.x to check how it compiles. https://godbolt.org/z/dXHWS6

That's a very interesting tool. Thanks. My assembly reading skills need a bit improvement but comment tooltips over assembly instructions are very helpful.

[quikquak]

Isn't comparing floating point vars a really bad idea?

[stratum]

Isn't comparing floating point vars a really bad idea?

Maybe. But that's a unittest code that's part of the actual product. Don't ask why that's how a one band man does design+coding+testing and packaging alone.
The comparision was done with fabs(a-b)>1E-6 and the trace code prints a=0.001 and b=0.001

[matt42]

Could be a memory allocation issue? In debug builds VS "helpfully" allocates extra memory to arrays for example. The idea is this extra memory helps in debugging somehow, unfortunately this "help" doesn't extend to identifying access to this out of bounds memory area. So debug builds may well run while optimised builds fail. Using a data structure with an assert() for bounds checking can save serious headaches I learned the hard way.

The only other time I ran into optimised version failure related to memory alignment. SIMD operations relying on specific alignment may well run in debug mode only to fail in release.

I'm unsure what other conditions can cause optimised builds to fail.

[Miles1981]

brrr, C mixed with C++... (fabs is C, not C++).

[stratum]

brrr, C mixed with C++... (fabs is C, not C++).

I love that style.

[stratum]

Could be a memory allocation issue? In debug builds VS "helpfully" allocates extra memory to arrays for example. The idea is this extra memory helps in debugging somehow, unfortunately this "help" doesn't extend to identifying access to this out of bounds memory area. So debug builds may well run while optimised builds fail. Using a data structure with an assert() for bounds checking can save serious headaches I learned the hard way.

The only other time I ran into optimised version failure related to memory alignment. SIMD operations relying on specific alignment may well run in debug mode only to fail in release.

I'm unsure what other conditions can cause optimised builds to fail.

It may be anything and I left counting the number of source code lines at 300K. And that's just our code not including a few dozen open source dependencies. And Miles hates C mixed with C++. Perhaps we should tell that to open source community, but that wouldn't be at the top of my list of requests. A little more documentation and examples would be much more appreciated.

edit. Looks like it's about 1000K lines of c/c++ code (excluding open source dependencies) now and there is a memory bug somewhere that doesn't get revealed in debug builds (or in release builds without optimization).

[mystran]

Isn't comparing floating point vars a really bad idea?

Comparing inexact floating point values for exact equality is typically a bad idea, simply because the chance that two inexact values happen to be exactly the same is usually pretty poor. If you know that the values are exact (eg. storing integers that fit into the mantissa) then exact equality testing is fine. If you have inexact values, then typically the right thing to do is to compare the absolute difference against some small threshold value as is done here.

[mystran]

Isn't comparing floating point vars a really bad idea?

Maybe. But that's a unittest code that's part of the actual product. Don't ask why that's how a one band man does design+coding+testing and packaging alone.
The comparision was done with fabs(a-b)>1E-6 and the trace code prints a=0.001 and b=0.001

Sanity check: if you're only printing 3 decimals, then that's not really enough to tell whether or not the values are the same at 6 decimal places. So make sure you're printing at least 7 decimals so you see what's really going on.

[stratum]

Isn't comparing floating point vars a really bad idea?

Maybe. But that's a unittest code that's part of the actual product. Don't ask why that's how a one band man does design+coding+testing and packaging alone.
The comparision was done with fabs(a-b)>1E-6 and the trace code prints a=0.001 and b=0.001

Sanity check: if you're only printing 3 decimals, then that's not really enough to tell whether or not the values are the same at 6 decimal places. So make sure you're printing at least 7 decimals so you see what's really going on.

I would normally think so if I had not seen the another if condition in the same process (unrelated to floats) evaluated incorrectly.