C/C++ compiler generated wrong code with optimization - by Bobris

Status : 

  Postponed<br /><br />
		Due to current priorities, the product team decided to postpone the resolution of this item.<br /><br />
		A more detailed explanation for the resolution of this particular item may have been provided in the comments section.


2
0
Sign in
to vote
ID 534515 Comments
Status Closed Workarounds
Type Bug Repros 0
Opened 2/18/2010 5:30:03 AM
Access Restriction Public

Description

We just testing VS2010 RC and found regression in our product due to wrong code generated by C compiler from ICU library:
http://source.icu-project.org/repos/icu/icu/trunk/source/common/ubidiln.c
inside function:
U_CFUNC UBool
ubidi_getRuns(UBiDi *pBiDi, UErrorCode *pErrorCode)

is this code:
            limit=0;
            for(i=0; i<runCount; ++i) {
                ADD_ODD_BIT_FROM_LEVEL(runs[i].logicalStart, levels[runs[i].logicalStart]);
                limit=runs[i].visualLimit+=limit;
            }
dissasembly of this is attached in wrong.txt
If I change this code to:
            limit=0;
            for(i=0; i<runCount; ++i) {
                ADD_ODD_BIT_FROM_LEVEL(runs[i].logicalStart, levels[runs[i].logicalStart]);
                limit+=runs[i].visualLimit;
                runs[i].visualLimit=limit;
            }
it generates correct code attached good.txt

That macro does this:
#define ADD_ODD_BIT_FROM_LEVEL(x, level)  ((x)|=((int32_t)level<<31))

Compiler from VS2005 generates correct code.
Sign in to post a comment.
Posted by Bobris on 2/26/2010 at 3:47 PM
No problem for me. My workaround is ok for me. I completely understand that it will not be fixed in RTM.
Posted by Shayne [MSFT] on 2/26/2010 at 3:29 PM
I created a runnable exe with your instructions, and I am seeing the bad output. We will now investigate and fix the issue.

Unfortunately, regardless of how complicated the fix is, the fix will not make it into Visual Studio 2010, as this bug does not meet the bar for VS2010 this late in the product cycle. However, we may include this fix in future service packs for VS2010.

Let me know if your current workaround is not sufficient, and I'll help come up with other workarounds.

Thanks.
Posted by Shayne [MSFT] on 2/26/2010 at 2:48 PM
Oops, sorry for sending the same comment twice (hit refresh by mistake).
Posted by Shayne [MSFT] on 2/26/2010 at 2:47 PM
Thank you for the repro. I'm now getting the same asm that you are getting. However, the particular instruction you refer to occurs because of an optimization (which is legal). So I need some more information to better understand the issue. What are the details about your products regression that pointed you to this code?

Bascially I'm trying to understand the inputs to ubidi_getRuns_3_2, and outputs fom it, that made you realize that ubidi_getRuns_3_2 is incorrectly compiled.

Source that compiles to a runnable exe that reports the wrong output (after calling ubidi_getRuns_3_2), would also help me isolate what is causing the bad code generation.
Posted by Bobris on 2/26/2010 at 2:18 PM
I am now already away from work. But I try to explain what this cycle do. That line with macro ADD_ODDBIT_FROM_LEVEL is not interesting for understanding of visualLimit correction (optimization of this probably corupt code for next line). Simply before this cycle visualLimit contains lengths of runs (runs are left to right, right to left parts of string). After this cycle visual limit should contain end offsets of each run.
Try to change code in way of my workaround and see how massively resulting code is different.
runCount (in EBX) is 3
levels[] = { 0,1,1,1,0 }
runs[].logicalStart = { 0,1,4 }
runs[].visualLimit = { 1,3,1 }

after this cycle variables should be:
runs[].logicalStart = { 0,1+(1<<31),4 }
runs[].visualLimit = { 1,4,5 }

but due to wrong code visualLimit will stay unmodified.
Posted by Shayne [MSFT] on 2/26/2010 at 1:07 PM
Thank you for the repro. I'm now getting the same asm that you are getting. However, the particular instruction you refer to occurs because of an optimization (which is legal). So I need some more information to better understand the issue. What are the details about your products regression that pointed you to this code?

Bascially I'm trying to understand the inputs to ubidi_getRuns_3_2, and outputs fom it, that made you realize that ubidi_getRuns_3_2 is incorrectly compiled.

Source that compiles to a runnable exe that reports the wrong output (after calling ubidi_getRuns_3_2), would also help me isolate what is causing the bad code generation.
Posted by Bobris on 2/25/2010 at 4:00 AM
Ok, I did it what you wanted. All interesting info is in attached proof.zip.
Just original source code is slightly different (older version of ICU) then in original report. But anyway resulting asm code is similiary wrong. I think first wrong instruction is on line 30415 in ICU32CommonAll.cod.
We don't compile ubidiln.c directly (but with much more files together to speed up compilation and linking, also better code), but I don't think this makes that problem, that's why I just written that ubidiln.c.

Posted by Shayne [MSFT] on 2/22/2010 at 12:36 PM
Can you please provide a repro for this issue? (I'm unable to produce the asm you are showing in "wrong.txt".)

To get a repro, compile ubidiln.c as you normally do, but also add /P to the cl.exe command. This will generate ubidiln.i. That one file (plus the command arguments used to create it) should be enough for me to reproduce this issue.

To create ubidiln.i from Visual studio, go to properties for your project, and from Configuration Properties->C/C++->Preprocessor change "Generate Preprocessed File" to "With Line Number (/P)"

When you compile ubidiln.c, ubidiln.i will be in the same directory as ubidiln.c. You can get the command args passed to cl.exe from BuildLog.htm, located in the build directory, ie: common\x86\Release

Thanks.
Posted by Microsoft on 2/18/2010 at 7:04 PM
Thank you for your feedback, we are currently reviewing the issue you have submitted. If this issue is urgent, please contact support directly(http://support.microsoft.com)