[PATCH] D99790: [CGCall] Annotate `this` argument with alignment

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a subscriber: aaron.ballman.
lebedev.ri added a comment.

ping @rsmith @rjmccall @aaron.ballman

To spell this out explicitly: i'm afraid i'm not really sure how to fix this preexisting thunk irgen bug.
I think it should be fixed in `CodeGenVTables::maybeEmitThunk()`, likely in `arrangeGlobalDeclaration()`.
Or is this wrong on the AST level, too?

In D99790#2680857 <https://reviews.llvm.org/D99790#2680857>, @jyknight wrote:

> It seems like there's a bug with vtable thunks getting the wrong information. This appears to be a pre-existing bug, but this change has caused it to start actively breaking code.
>
> Test case:
>
>   class Base1 {
>       virtual void Foo1();
>   };
>   
>   class Base2 {
>       virtual void Foo2();
>   };
>   
>   class alignas(16) Obj : public Base1, public Base2 {
>      void Foo1() override;
>      void Foo2() override;
>      ~Obj();
>   };
>   
>   void Obj::Foo1() {}
>   void Obj::Foo2() {}
>
> emits three method definitions:
>
>   define dso_local void @_ZN3Obj4Foo1Ev(%class.Obj* nonnull align 16 dereferenceable(16) %0) unnamed_addr #0 align 2 !dbg !7 {
>   define dso_local void @_ZN3Obj4Foo2Ev(%class.Obj* nonnull align 16 dereferenceable(16) %0) unnamed_addr #0 align 2 !dbg !25 {
>   define dso_local void @_ZThn8_N3Obj4Foo2Ev(%class.Obj* nonnull align 16 dereferenceable(16) %0) unnamed_addr #2 align 2 !dbg !29 {
>
> (See https://godbolt.org/z/MxhYMe1q7, for now at least)
>
> That third method declaration is bogus -- its argument is _not_ an `Obj*` at all! In fact, it's pointing at `Obj + 8` -- at the embedded `Base2` object! As such, `align 16` is incorrect, as is `dereferenceable(16)`. The additional 8 bytes of dereferenceable claim apparently hasn't broken anything noticeable, but the alignment claim is causing actual trouble.
>
> As such, I suggest to revert this change, separately commit a fix to that underlying bug, and then re-submit this change after that.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99790/new/

https://reviews.llvm.org/D99790