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

lebedev.ri abandoned this revision. lebedev.ri added a comment. Oh, i forgot that this review was here. I've just relanded this in rG16d03818412415c56efcd482d18c0cbdf712524c , after workarounding the thunk issue in

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

xbolva00 added a comment. In 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. > >

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

lebedev.ri added a comment. I have tried to fix this in D100388 , but i don't quite understand all the moving parts, so it's still not in a working state (even though it fixed those two tests), so i'm not sure how productive it is to develop it via a review

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

rjmccall added a comment. I agree. The arrangement logic is assuming that the exact pointer type doesn't matter because it's all just a pointer in the end, but of course that breaks down when we start inferring attributes. Pointer authentication also really wants to know the original

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

rsmith added a comment. In D99790#2681487 , @lebedev.ri wrote: > I think it should be fixed in `CodeGenVTables::maybeEmitThunk()`, likely in > `arrangeGlobalDeclaration()`. > Or is this wrong on the AST level, too? The AST-level `ThunkInfo` doesn't

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

sanwou01 added a comment. +1 on eagerly awaiting a fix. a 3% regression on astar (AArch64, LTO) bisects to @lebedev.ri 's revert: https://reviews.llvm.org/rG6270b3a1eafaba4279e021418c5a2c5a35abc002 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

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

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

lebedev.ri reopened this revision. lebedev.ri added a comment. Temporarily reverted in 6270b3a1eafaba4279e021418c5a2c5a35abc002 . Eagerly awaiting fix for that bug. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

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

jyknight added a comment. The OpenJDK bug was UB in the OpenJDK code: https://bugs.openjdk.java.net/browse/JDK-8229258 already fixed in JDK14. (But not backported to JDK 11 LTS, which is the version Brooks found an error in above.) They probably need to backport that patch. My only confusion

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

jyknight added a comment. 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 {

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

lebedev.ri added a comment. In D99790#2680366 , @brooksmoses wrote: > In D99790#2678674 , @lebedev.ri > wrote: > >> In D99790#2678384 , @brooksmoses >> wrote: >> >>> In

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

brooksmoses added a comment. In D99790#2678674 , @lebedev.ri wrote: > In D99790#2678384 , @brooksmoses > wrote: > >> In any case, thanks for the quick reply, and I'll figure out a small >> reproducer if we find

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

lebedev.ri added a comment. In D99790#2678384 , @brooksmoses wrote: > In D99790#2677919 , @lebedev.ri > wrote: > >> In D99790#2677917 , @brooksmoses >> wrote: >> >>> As a

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

brooksmoses added a comment. In D99790#2677919 , @lebedev.ri wrote: > In D99790#2677917 , @brooksmoses > wrote: > >> As a heads up, I'm seeing segfaults on internal code as a result of this >> change, as well as

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

jyknight added a comment. In D99790#2677917 , @brooksmoses wrote: > As a heads up, I'm seeing segfaults on internal code as a result of this > change, as well as errors in Eigen's unalignedassert.cpp test (specifically, > this line asserts: >

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

lebedev.ri added a comment. In D99790#2677917 , @brooksmoses wrote: > As a heads up, I'm seeing segfaults on internal code as a result of this > change, as well as errors in Eigen's unalignedassert.cpp test (specifically, > this line asserts: >

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

brooksmoses added a comment. As a heads up, I'm seeing segfaults on internal code as a result of this change, as well as errors in Eigen's unalignedassert.cpp test (specifically, this line asserts: https://github.com/madlib/eigen/blob/master/test/unalignedassert.cpp#L151). Repository: rG

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

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG0aa0458f1429: [CGCall] Annotate `this` argument with alignment (authored by lebedev.ri). Changed prior to commit:

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

lebedev.ri added a comment. I'll land this tomorrow if no one wants to object. As discussed in D99791 , this is an obvious, and legal, bugfix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99790/new/

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

lebedev.ri added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2314 + llvm::Align Alignment = + getNaturalTypeAlignment(ThisTy, /*BaseInfo=*/nullptr, + /*TBAAInfo=*/nullptr, arichardson wrote: >

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

arichardson added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2314 + llvm::Align Alignment = + getNaturalTypeAlignment(ThisTy, /*BaseInfo=*/nullptr, + /*TBAAInfo=*/nullptr, Shouldn't the alignment also

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

lebedev.ri created this revision. lebedev.ri added reviewers: efriedma, rjmccall, rsmith, erichkeane, nikic. lebedev.ri added a project: LLVM. Herald added a subscriber: lxfind. lebedev.ri requested review of this revision. Herald added a reviewer: jdoerfert. Herald added subscribers: cfe-commits,