Is this a compiler error in Visual Studio 2010?

Question

I have a bug in this conditional:

while(CurrentObserverPathPointDisplacement > lengthToNextPoint && CurrentObserverPathPointIndex < (PathSize - 1) )
{
     CurrentObserverPathPointIndex = CurrentObserverPathPointIndex + 1;
     CurrentObserverPathPointDisplacement -= lengthToNextPoint;
     lengthToNextPoint = (CurrentObserverPath->pathPoints[min((PathSize - 1),CurrentObserverPathPointIndex + 1)] - CurrentObserverPath->pathPoints[CurrentObserverPathPointIndex]).length();
}

It seems to get stuck in an infinite loop while in Release mode. Works fine in debug mode, or more interstingly when I put a debug print on the last line

OutputInDebug("Here");

Here is the generated assembly for the conditional itself:

            while(CurrentObserverPathPointDisplacement > lengthToNextPoint && CurrentObserverPathPointIndex < (PathSize - 1) )
00F074CF  fcom        qword ptr [dist]  
00F074D2  fnstsw      ax  
00F074D4  test        ah,5  
00F074D7  jp          ModelViewData::moveCameraAndCenterOnXYPlaneForwardBackward+27Eh (0F0753Eh)  
00F074D9  mov         eax,dword ptr [dontRotate]  
00F074DC  cmp         eax,ebx  
00F074DE  jge         ModelViewData::moveCameraAndCenterOnXYPlaneForwardBackward+27Eh (0F0753Eh)  
            {

You can see that for the second condition, it seems to move the value of 'dontRotate', a function parameter of type bool, into eax, and then compare against it, yet dontRotate is used nowhere near that bit of code.

I understand that this may be a bit little data, but it seems like an obvious compiler error personally. But sadly, i'm not sure how to distill it down to a self contained enough problem to actually produce a bug report.

Edit: Not the actual decelerations, but the types:

double CurrentObserverPathPointDisplacement;
double lengthToNextPoint;
int CurrentObserverPathPointIndex;
int PathSize;
vector<vector3<double>> CurrentObserverPath::pathPoints;

Edit2:

Once I add in the debug print statement to the end of the while, this is the assembly that gets generated, which no longer expresses the bug:

            while(CurrentObserverPathPointDisplacement > lengthToNextPoint && CurrentObserverPathPointIndex < (PathSize - 1) )
00B1751E  fcom        qword ptr [esi+208h]  
00B17524  fnstsw      ax  
00B17526  test        ah,5  
00B17529  jp          ModelViewData::moveCameraAndCenterOnXYPlaneForwardBackward+2D6h (0B175A6h)  
00B1752B  mov         eax,dword ptr [esi+200h]  
00B17531  cmp         eax,ebx  
00B17533  jge         ModelViewData::moveCameraAndCenterOnXYPlaneForwardBackward+2D6h (0B175A6h)  
            {

That does not look like instructions corresponding to any of the code. — wallyk, Feb 22 '12 at 00:17
First of all, you could have shortened the identifiers a bit. However, to figure out what's happening it would be helpful to see what types are involved. — bitmask, Feb 22 '12 at 00:20
@bitmask I've added the types, I would have shortened identifiers, but since I wasn't actually able to get the bug to manifest in a shorter program, I would have had to manually modify anything in the generated ASM also, and I wanted to make sure human error didn't contribute to the confusion. — Arelius, Feb 22 '12 at 00:38
@wallyk That's the generated code the the condition, the code is copied directly out of the VS asm window, and the source code is inline, so that you can verify that it is indeed the code being generated. Just seemingly generated poorly. — Arelius, Feb 22 '12 at 00:41
How are you calling `.length()` on a `double`? Either you made a mistake copying the code, or a mistake figuring out the types. — Ben Voigt, Feb 22 '12 at 00:44
@Arelius: Okay, one more thing: what is the value of `PathSize`? I have a hunch. — bitmask, Feb 22 '12 at 00:45
@Bitmask it can vary, but with the recent data I've been using, it's 4. — Arelius, Feb 22 '12 at 00:49
@BenVoigt Sorry, you are correct, It's actually a 3D vector of doubles, and .length() is returning a double. — Arelius, Feb 22 '12 at 00:52
I suspect there's undefined behavior nearby, which is permitting the optimizer to do things it normally couldn't. — Ben Voigt, Feb 22 '12 at 01:10
Have you done a full clean and build? There's a semi-common bug with Visual Studio on large projects, where it 'forgets' to fully recompile changes, resulting in code that doesn't make sense. — Collin Dauphinee, Feb 22 '12 at 04:30
@BenVoigt it's a struct Vector3 { double x, y, z; }; With a lot of extra code. — Arelius, Feb 22 '12 at 17:57

bitmask · Accepted Answer · 2012-02-22T01:12:30.863

1

Here:

while(/* foo */ && CurrentObserverPathPointIndex < (PathSize - 1) )
{
     CurrentObserverPathPointIndex = CurrentObserverPathPointIndex + 1;

Since this is the only point (unless min does something really nasty) in the loop where CurrentObserverPathPointIndex is changed and both CurrentObserverPathPointIndex and PathSize are signed integers of the same size (and PathSize is small enough to rule out integer promotion issues), the remaining floating point fiddling is irrelevant. The loop must terminate eventually (it may take quite a long time if the initial value of CurrentOvserverPathPointIndex is small compared to PathSize, though).

This allows only one conclusion; If the compiler generates code that does not terminate (ever), the compiler is wrong.

edited Feb 22 '12 at 01:12

answered Feb 22 '12 at 00:53

bitmask

32,434
14
99
159

An interesting development is that if I mark CurrentObserverPathPointIndex as volatile, the program then behaves as expected. I suppose it's most valuable to see if I can find some sort of reproduction for this exact error. – Arelius Feb 22 '12 at 00:57
@Arelius: To submit a bug report, this will be helpful anyway (however, I've no clue how MSVC bug reports work, but I don't expect they differ much from gcc's). – bitmask Feb 22 '12 at 01:00
You probably meant "if the initial value ... is too negative"? – Ben Voigt Feb 22 '12 at 01:08
@BenVoigt: Yep, thanks. I was referring to the absolute value, but if it's positive the invariant would be violated immediately, of course. Fixed. – bitmask Feb 22 '12 at 01:11
In addition to the application not seeming to actually terminate. If I step through it in assembly, the compared hardware registers are actually the same every step through, which implies that the program actually doesn't terminate. Combined with the fact that making the type Volatile fixes it, I'm marking this as an accepted answer, as it confirms why, this actually seems to be a compiler error. – Arelius Feb 22 '12 at 18:02

score 0 · Answer 2 · answered Feb 22 '12 at 00:30

0

It looks like PathSize doesn't change in the loop, so the compiler can compute PathSize - 1 before looping and coincidentally use the same memory location as dontRotate, whatever that is.

More importantly, how many elements are there in CurrentObserverPath->pathPoints?

Your loop condition includes this test:

CurrentObserverPathPointIndex < (PathSize - 1)

Inside your loop is this assignment:

CurrentObserverPathPointIndex = CurrentObserverPathPointIndex + 1;

followed by this further incremented subscript:

[min((PathSize - 1),CurrentObserverPathPointIndex + 1)]

Maybe your code appeared to work in debug mode because random undefined behaviour appeared to work?

answered Feb 22 '12 at 00:30

Windows programmer

7,871
1
22
23

Actually looking at the variable values, ebx is getting the value of PathSize - 1, and the register just remains untouched. There are PathSize points in CurrentObserverPath->pathPoints. – Arelius Feb 22 '12 at 00:32
2

@Arelius : I find it bizarre that you're fixiated on the generated assembly when the problem is much more likely to be in the semantics of your code (which you haven't properly shown us)... – ildjarn Feb 22 '12 at 00:37
@ildjarn could you clarify which semantics you would like to see, I cannot actually find a reproduction case that is smaller then the entirety of my program quite yet. But my fixations exist due to the fact that the generated code is so clearly wrong. To help express my confidence in the problem with the generated code, I've edited the existing question to contain the generated code when I add in the debug statement. – Arelius Feb 22 '12 at 00:45
@ildjarn Considering that the incremented subscript is not used in one of the primary conditions of the for loop, and also does not actually modify either PathSize, or CurrentObserverPathPointIndex, that can safely be ruled out as any aspect of the issue in a well behaved compiler. Thus the only matters of importance are the first two code comments in the answer. Assuming that PathSize and CurrentObserverPathPointIndex are both integers, and there is no code that can run concurrently, Thus the remaining semantics are irrelevant to anything except the code generator, and the optimizer. – Arelius Feb 22 '12 at 18:18

Is this a compiler error in Visual Studio 2010?

2 Answers2