VS 2013 Miscompiles

[Aaron Ballman]

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
>
>