🔒 Locked

MS compiler optimization bug

Started by n0ob Apr 13, 2006 at 7:41 PM 18 replies 2.6k views

Original Post

Apr 13, 2006 at 7:41 PM

Today I was working on a project when I came across an incomprehensible bug. Well, it really has to do with the optimization generating faulty assembly, but I don't know exactly why. Just wondered if anyone has come across this bug before, and how I can let Microsoft know lol. Here's a link to a page detailing the bug: Compiler Bug Thanks!

n0ob

Author

n0ob

Apr 13, 2006 at 7:59 PM

Ah thanks! I'm glad to hear it works in vc2005 lol. I suppose there is no need to report it now :)

Promit

Mod

1,082

Promit

Apr 13, 2006 at 8:05 PM

I can confirm a repro, anyway. It does look like the generated code is wrong, under all levels of optimization (/O1, /O2, /Ox). Interestingly, marking the loop counter i as volatile fixes it. There's also a suspicious unreachable code warning related to the vector. I'll continue looking at this. (Clearly they noticed at some point, since it doesn't affect 2005, but I'm curious.)

deffer

Apr 13, 2006 at 8:08 PM

Interesting.
I compiled with VS 2003, with standard "Release", not necessarily the options you proposed, and the bug indeed WAS there.

I examined the assembly, and one instruction was strange:
cmp eax, 0FFFFFFFCh jbe... // unsigned comparison
where eax was holding variable i

Can't find any explanation for this instructions, other than bug.

jflanglois

Apr 13, 2006 at 8:10 PM

It seems to have been fixed in VC++ 2005 (Express). I tried all C++ optimization modes.

jfl.

n0ob

Author

n0ob

Apr 13, 2006 at 8:12 PM

Yay, glad to know I'm not crazy! I noticed the suspicious assembly instruction as well: eax is usually 0 when that line executes, instead of the 1 that i is being initialized to. I think it should be comparing ecx there (or at least in the version I was looking at) but I really have no idea :P Thanks for trying everyone! Edit: Thanks Simon :D I'm definately proud ^_^

n0ob

Author

n0ob

Apr 13, 2006 at 9:12 PM

Ah, here's mine:

	const char* TestBug()	{004016E0  push        ebx  		bool bug_success = false;004016E1  xor         bl,bl 004016E3  xor         eax,eax 004016E5  mov         edx,2 004016EA  lea         ebx,[ebx] 		{				if(i > 0)004016F0  cmp         eax,0FFFFFFFCh 004016F3  ja          CompilerBug::TestBug+19h (4016F9h) 			{}			else				//You see, logically this code should not be reachable				bug_success = true;004016F5  mov         bl,1 			if(i > 0)004016F7  jmp         CompilerBug::TestBug+22h (401702h) 		for(unsigned i = 1; i < testSize; ++i)004016F9  mov         ecx,dword ptr [CompilerBug::intVector+4 (411E04h)] 				intVector[i-1]++;004016FF  inc         dword ptr [eax+ecx] 00401702  add         eax,4 00401705  dec         edx  00401706  jne         CompilerBug::TestBug+10h (4016F0h) 		}		return bug_success ? "Bug succeeded!" : "Bug failed.";00401708  test        bl,bl 0040170A  pop         ebx  0040170B  mov         eax,offset KERNEL32_NULL_THUNK_DATA+20h (40D11Ch) 00401710  jne         CompilerBug::TestBug+37h (401717h) 00401712  mov         eax,offset KERNEL32_NULL_THUNK_DATA+30h (40D12Ch) 	}00401717  ret

[Edited by - n0ob on April 13, 2006 9:12:05 PM]

DukeAtreides076

140

DukeAtreides076

Apr 13, 2006 at 9:30 PM

Here is the code and dissasembly at O2:

      const char* TestBug()      {004016C0  push        ebx              bool bug_success = false; 004016C1  xor         bl,bl 004016C3  xor         eax,eax 004016C5  mov         edx,2 004016CA  lea         ebx,[ebx]             {                       if(i > 0)004016D0  cmp         eax,0FFFFFFFCh 004016D3  ja          CompilerBug::TestBug+19h (4016D9h)                   {}                  else                        //You see, logically this code should not be reachable                        bug_success = true;004016D5  mov         bl,1                   if(i > 0)004016D7  jmp         CompilerBug::TestBug+22h (4016E2h)             for(unsigned i = 1; i < testSize; ++i)004016D9  mov         ecx,dword ptr [CompilerBug::intVector+4 (411E04h)]                         intVector[i-1]++;004016DF  inc         dword ptr [eax+ecx] 004016E2  add         eax,4 004016E5  dec         edx  004016E6  jne         CompilerBug::TestBug+10h (4016D0h) 			}            return bug_success ? "Bug succeeded!" : "Bug failed.";004016E8  test        bl,bl 004016EA  pop         ebx  004016EB  mov         eax,offset string "Bug succeeded!" (40D128h) 004016F0  jne         CompilerBug::TestBug+37h (4016F7h) 004016F2  mov         eax,offset string "Bug failed." (40D11Ch)       }004016F7  ret

And without optimization (runtime stack checks disabled):

      const char* TestBug()      {00416E60  push        ebp  00416E61  mov         ebp,esp 00416E63  sub         esp,4Ch 00416E66  push        ebx  00416E67  push        esi  00416E68  push        edi              bool bug_success = false; 00416E69  mov         byte ptr [bug_success],0             for(unsigned i = 1; i < testSize; ++i)00416E6D  mov         dword ptr ,1 00416E74  jmp         CompilerBug::TestBug+1Fh (416E7Fh) 00416E76  mov         eax,dword ptr  00416E79  add         eax,1 00416E7C  mov         dword ptr ,eax 00416E7F  cmp         dword ptr ,3 00416E83  jae         CompilerBug::TestBug+5Ah (416EBAh)             {                       if(i > 0)00416E85  cmp         dword ptr ,0 00416E89  jbe         CompilerBug::TestBug+2Dh (416E8Dh)                   {}                  else00416E8B  jmp         CompilerBug::TestBug+31h (416E91h)                         //You see, logically this code should not be reachable                        bug_success = true;00416E8D  mov         byte ptr [bug_success],1                   if(i > 0)00416E91  cmp         dword ptr ,0 00416E95  jbe         CompilerBug::TestBug+58h (416EB8h)                         intVector[i-1]++;00416E97  mov         eax,dword ptr  00416E9A  sub         eax,1 00416E9D  push        eax  00416E9E  mov         ecx,offset CompilerBug::intVector (44C240h) 00416EA3  call        std::vector<int,std::allocator<int> >::operator[] (415816h) 00416EA8  mov         dword ptr [ebp-4Ch],eax 00416EAB  mov         ecx,dword ptr [ebp-4Ch] 00416EAE  mov         edx,dword ptr [ecx] 00416EB0  add         edx,1 00416EB3  mov         eax,dword ptr [ebp-4Ch] 00416EB6  mov         dword ptr [eax],edx 			}00416EB8  jmp         CompilerBug::TestBug+16h (416E76h)             return bug_success ? "Bug succeeded!" : "Bug failed.";00416EBA  movzx       eax,byte ptr [bug_success] 00416EBE  test        eax,eax 00416EC0  je          CompilerBug::TestBug+6Bh (416ECBh) 00416EC2  mov         dword ptr [ebp-4Ch],offset string "Bug succeeded!" (4430D8h) 00416EC9  jmp         CompilerBug::TestBug+72h (416ED2h) 00416ECB  mov         dword ptr [ebp-4Ch],offset string "Bug failed." (4430C8h) 00416ED2  mov         eax,dword ptr [ebp-4Ch]       }00416ED5  pop         edi  00416ED6  pop         esi  00416ED7  pop         ebx  00416ED8  mov         esp,ebp 00416EDA  pop         ebp  00416EDB  ret

Edit: the problem seems to be here:

004016F0 cmp eax,0FFFFFFFCh
004016F3 ja CompilerBug::TestBug+19h (4016F9h)

I have no idea why it decided it could remove that from the loop...but that condition check definitely seems wrong. It looks like it removed that entire if from the loop. Funky. Maybe they'll give you a tshirt and stuff for finding that.

Promit

Mod

1,082

Promit

Apr 13, 2006 at 10:06 PM

Quote:
Original post by DukeAtreides076
I have no idea why it decided it could remove that from the loop...but that condition check definitely seems wrong. It looks like it removed that entire if from the loop.

That's what tipped me off to seeing what happens if you mark i as volatile, which does indeed fix it. For some reason the compiler decides that it can drop that out of the loop, which results in some totally bogus values and comparisons, and volatile prevents it from doing that.

iMalc

182

iMalc

Apr 13, 2006 at 10:36 PM

I'd like to know what happens when you actually initialise the vector, instead of incrementing uninitialised values (which to be fair, is itself a bug in your code).
Oh and you'd be best to use the resulting vector values as well, really.

Adam Hamilton

Apr 14, 2006 at 11:31 PM

The compiler should have done this

            bool bug_success = false;            intVector[0]++;            intVector[1]++;            intVector[2]++;            return "Bug failed.";

It is a quirky bug. I think I see the problem.

This set of instruction here are to initilise ecx with the address of the vector's data however there is an issue with the value it chose to compare against. In this code, the jump will never be taken because eax is only ever incremented by 4 but edx being the counting variable will ensure that the loop is broken
004016D0 cmp eax,0FFFFFFFCh
004016D3 ja CompilerBug::TestBug+19h (4016D9h)

This leads me to wonder about ecx that is never initialised to its proper value so that the vector is addressed properly.

N0ob: When you ran the program, did you get an access violation error?

n0ob

Author

n0ob

Apr 15, 2006 at 1:40 AM

I'm not quite sure how an access violation can be generated. The loop should terminate at the right time to prevent access of an invalid index right? I never had a crash like that anyway. My ORIGINAL problem was an access violation, trying to dereference a null pointer, since I was trying to assign pointers inside the (i > 0) condition's body :P

joanusdmentia

Apr 15, 2006 at 2:29 AM

Quote:
Original post by Promit
Quote:
Original post by DukeAtreides076
I have no idea why it decided it could remove that from the loop...but that condition check definitely seems wrong. It looks like it removed that entire if from the loop.
That's what tipped me off to seeing what happens if you mark i as volatile, which does indeed fix it. For some reason the compiler decides that it can drop that out of the loop, which results in some totally bogus values and comparisons, and volatile prevents it from doing that.

It hasn't been removed from the loop, the jne at 004016E6 for the loop jumps back to the comparison. What's really interesting is that changing from unsigned to signed (for both testSize and i) will completely inline the function into main(), removes the redundant loop counter in EDX (it can use EAX instead) and (as mentioned by n0ob) works correctly. What seems to be happenning is that the compiler is confusing signed/unsigned in the comparison. Compiling with the volatile keyword for some reason fixes this, and instead of the cmp eax,FFFFFFFC it generates test eax,eax. When compiling with signed values it generates the same cmp instruction but jumps with jg instead of ja. Very strange indeed.....

Furthermore using an int array instead of the std::vector results in the correct test instruction being used instead of the incorrect cmp.

n0ob

Author

n0ob

Apr 15, 2006 at 2:45 AM

I updated my little page on the bug with a slightly better example that I made tonight. Now we can say the array is being initialized (the bug test function is actually the array initialization function, for example), making the bug especially frightening! The while loop has two conditions, and the if test just after the while loop verifies that the i > 0 test must have failed, or else the i < testSize + 1 is failing every single time such that i eventually overflows to 0 (but the array accessing would most definately crash it if this were the case). Enjoy!

Also, regarding the comments to the uninitialized vector again. Is unsigned(), or int() for that matter, undefined? I'm assuming that it's not standard? I guess it's always the best idea to explicitly initialize things though ;) Thanks!

joanusdmentia

Apr 15, 2006 at 2:50 AM

Quote:
Original post by n0ob
Also, regarding the comments to the uninitialized vector again. Is unsigned(), or int() for that matter, undefined? I'm assuming that it's not standard? I guess it's always the best idea to explicitly initialize things though ;) Thanks!

It's defined, but with built-in types storage is not initialized by default (even though it is with classes).

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.