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

2016-10-19 Thread Peter Collingbourne via cfe-commits 
On Wed, Oct 19, 2016 at 3:38 PM, David Majnemer 
wrote:

>
>
> On Wed, Oct 19, 2016 at 2:04 PM, Hans Wennborg via cfe-commits <
> cfe-commits@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@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
>


-- 
-- 
Peter
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-10-19 Thread David Majnemer via cfe-commits 
On Wed, Oct 19, 2016 at 2:04 PM, Hans Wennborg via cfe-commits <
cfe-commits@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.


>
> 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@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-10-19 Thread Hans Wennborg via cfe-commits 
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/MicrosoftCXXABI.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(Base)
+ : Layout.getBaseClassOffset(Base);
 if (BaseOffset <= Offset && BaseOffset >= MaxBaseOffset) {
   MaxBase = Base;
   MaxBaseOffset = BaseOffset;

Added: cfe/trunk/test/CodeGenCXX/pr30731.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/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@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits