r284624 - MS ABI: Fix assert when generating virtual function call with virtual bases and -flto (PR30731)

[Peter Collingbourne via cfe-commits]

On Wed, Oct 19, 2016 at 3:38 PM, David Majnemer <david.majnemer at gmail.com>
wrote:

>
>
> On Wed, Oct 19, 2016 at 2:04 PM, Hans Wennborg via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> Author: hans
>> Date: Wed Oct 19 13:04:27 2016
>> New Revision: 284624
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=284624&view=rev
>> Log:
>> MS ABI: Fix assert when generating virtual function call with virtual
>> bases and -flto (PR30731)
>>
>> getClassAtVTableLocation() was calling
>> ASTRecordLayout::getBaseClassOffset() on a virtual base, causing an
>> assert.
>>
>> Differential Revision: https://reviews.llvm.org/D25779
>>
>> Added:
>>     cfe/trunk/test/CodeGenCXX/pr30731.cpp
>> Modified:
>>     cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
>>
>> Modified: cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/Mi
>> crosoftCXXABI.cpp?rev=284624&r1=284623&r2=284624&view=diff
>> ============================================================
>> ==================
>> --- cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp Wed Oct 19 13:04:27 2016
>> @@ -1773,15 +1773,8 @@ static const CXXRecordDecl *getClassAtVT
>>    CharUnits MaxBaseOffset;
>>    for (auto &&B : RD->bases()) {
>>      const CXXRecordDecl *Base = B.getType()->getAsCXXRecordDecl();
>> -    CharUnits BaseOffset = Layout.getBaseClassOffset(Base);
>> -    if (BaseOffset <= Offset && BaseOffset >= MaxBaseOffset) {
>> -      MaxBase = Base;
>> -      MaxBaseOffset = BaseOffset;
>> -    }
>> -  }
>> -  for (auto &&B : RD->vbases()) {
>> -    const CXXRecordDecl *Base = B.getType()->getAsCXXRecordDecl();
>> -    CharUnits BaseOffset = Layout.getVBaseClassOffset(Base);
>> +    CharUnits BaseOffset = B.isVirtual() ? Layout.getVBaseClassOffset(Bas
>> e)
>> +                                         : Layout.getBaseClassOffset(Base
>> );
>>      if (BaseOffset <= Offset && BaseOffset >= MaxBaseOffset) {
>>        MaxBase = Base;
>>        MaxBaseOffset = BaseOffset;
>>
>
> I don't think this code is correct.
> It uses a base's layout to find virtual base offsets, that seems wrong
> because the virtual base offsets can be different in a base and the most
> derived class.
> I think we need to do getVBaseOffset with the MDC's layout similar to how
> VTableBuilder.cpp's findPathsToSubobject operates.
>

Thanks, I will put it on my todo list to try and understand how we need to
handle vbases here.

Peter

>
>
>>
>> Added: cfe/trunk/test/CodeGenCXX/pr30731.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCX
>> X/pr30731.cpp?rev=284624&view=auto
>> ============================================================
>> ==================
>> --- cfe/trunk/test/CodeGenCXX/pr30731.cpp (added)
>> +++ cfe/trunk/test/CodeGenCXX/pr30731.cpp Wed Oct 19 13:04:27 2016
>> @@ -0,0 +1,21 @@
>> +// RUN: %clang_cc1 -triple i386-pc-win32 -emit-llvm -flto -std=c++11 -o
>> - %s | FileCheck %s
>> +
>> +struct A {
>> +  virtual ~A();
>> +};
>> +
>> +struct B {};
>> +
>> +struct C {
>> +  virtual void f();
>> +};
>> +
>> +struct S : A, virtual B, C {
>> +  void f() override;
>> +};
>> +
>> +void f(S* s) { s->f(); }
>> +
>> +// CHECK-LABEL: define void @"\01?f@@YAXPAUS@@@Z"
>> +// CHECK: call
>> +// CHECK: ret void
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
>


-- 
-- 
Peter
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161019/99a37da8/attachment.html>