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

[David Majnemer via cfe-commits]

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.


>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161019/03fd833c/attachment.html>