VS 2013 Miscompiles

Aaron Ballman aaron at aaronballman.com
Wed Nov 6 09:09:30 PST 2013


Ping?

On Fri, Nov 1, 2013 at 5:41 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Fri, Nov 1, 2013 at 4:15 PM, Reid Kleckner <rnk at google.com> wrote:
>> Thanks for tackling and reducing this!  This was reported in July, and it's
>> a shame we didn't report it to them before they went from preview to
>> release: http://llvm.org/bugs/show_bug.cgi?id=16606
>
> Agreed!
>
> And from what I can tell, the patch I provided is probably the best
> way for us to work around the issue for now; the alternative is to
> remove the const version of the method from the base class.  The good
> news is, this only seems to affect situations involving this
> particular pattern, which doesn't appear to be very common in the
> source base, so we likely are not hitting other miscompiles.
>
> ~Aaron
>
>>
>>
>> On Fri, Nov 1, 2013 at 12:16 PM, Aaron Ballman <aaron at aaronballman.com>
>> wrote:
>>>
>>> I have this as narrowed down as I think I can get it, and I've
>>> reported it to Microsoft Connect.
>>>
>>>
>>> https://connect.microsoft.com/VisualStudio/feedback/details/807479/miscompile-involving-using-declarations-and-multiple-inheritance
>>>
>>> The miscompile seems to happen as a result of multiple inheritance
>>> (the thunk is missing in 2013 but there in 2012), and seems to have
>>> something to do with the using declaration importing two methods.  At
>>> least, from what I can tell, when I remove the const version of the
>>> method, the thunk comes back in 2013.
>>>
>>> ~Aaron
>>>
>>> On Thu, Oct 31, 2013 at 5:16 PM, Aaron Ballman <aaron at aaronballman.com>
>>> wrote:
>>> > While updating clang to VS 2013, I've seen numerous miscompiles on x86
>>> > that did not appear in VS 2012 related to the Redeclarable class.  It
>>> > seems that VS 2012 would properly calculate the offset to the base
>>> > class, while VS 2013 would not.  Specifically, VS 2012 would generate
>>> > the following code:
>>> >
>>> >   RecordDecl *getPreviousDecl() {
>>> > 025FF040  push        ebp
>>> > 025FF041  mov         ebp,esp
>>> > 025FF043  push        ecx
>>> > 025FF044  mov         dword ptr [this],0CCCCCCCCh
>>> > 025FF04B  mov         dword ptr [this],ecx
>>> >     return cast_or_null<RecordDecl>(TagDecl::getPreviousDecl());
>>> > 025FF04E  mov         ecx,dword ptr [this]
>>> > 025FF051  add         ecx,34h
>>> > 025FF054  call
>>> > clang::Redeclarable<clang::TagDecl>::getPreviousDecl (025FEF40h)
>>> > 025FF059  push        eax
>>> > 025FF05A  call
>>> > llvm::cast_or_null<clang::RecordDecl,clang::TagDecl> (025A9850h)
>>> > 025FF05F  add         esp,4
>>> >   }
>>> >
>>> > VS 2013, however, is missing the add ecx, 34h instruction:
>>> >
>>> >   RecordDecl *getPreviousDecl() {
>>> > 02500010  push        ebp
>>> > 02500011  mov         ebp,esp
>>> > 02500013  push        ecx
>>> > 02500014  mov         dword ptr [this],0CCCCCCCCh
>>> > 0250001B  mov         dword ptr [this],ecx
>>> >     return cast_or_null<RecordDecl>(TagDecl::getPreviousDecl());
>>> > 0250001E  mov         ecx,dword ptr [this]
>>> > 02500021  call
>>> > clang::Redeclarable<clang::TagDecl>::getPreviousDecl (024FFF10h)
>>> > 02500026  push        eax
>>> > 02500027  call
>>> > llvm::cast_or_null<clang::RecordDecl,clang::TagDecl> (024AA9B0h)
>>> > 0250002C  add         esp,4
>>> >
>>> > This would cause crashes in getPreviousDecl.  Unfortunately, this also
>>> > affects other calls on Redeclarable such as getMostRecentDecl.
>>> >
>>> > This patch "addresses" the miscompile such that it now compiles
>>> > properly for VS 2013 as well as VS 2012.  Instead of calling through a
>>> > scoped lookup on a method coming from Redeclarable, it statically
>>> > casts the this pointer and calls the method.  This seems to force
>>> > Visual Studio to generate the proper code.
>>> >
>>> > ~Aaron
>>
>>



More information about the cfe-commits mailing list