Details
- Reviewers
rsmith alexander-shaposhnikov jdoerfert
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
clang/include/clang/Basic/ABI.h | ||
---|---|---|
182 | Instead of the changes you made to GlobalDecl, I think this should be a GlobalDecl itself. Different function variants could have different signatures, so in principle at least, what we want to know is the function variant not the function declaration. | |
clang/lib/AST/VTableBuilder.cpp | ||
1155–1157 | Can you modify the thunk info in-place, instead of this erase + insert dance? |
clang/lib/AST/VTableBuilder.cpp | ||
---|---|---|
1155–1157 | Absolutely. I had to change that because i needed to weed out all the places where Thunk is created. |
@rsmith thank you for taking a look!
This still feels pretty much like doing it while blindfolded.
I've tried to store GlobalDecl, but i'm clearly missing something else, since there are still 23 test failures,
and not all of then are simple tests that need updates.
E.g.:
******************** TEST 'Clang :: CodeGenCXX/debug-info-thunk.cpp' FAILED ******************** Script: -- : 'RUN: at line 1'; /builddirs/llvm-project/build-Clang11/bin/clang -cc1 -internal-isystem /builddirs/llvm-project/build-Clang11/lib/clang/13.0.0/include -nostdsysteminc /repositories/llvm-project/clang/test/CodeGenCXX/debug-info-thunk.cpp -triple=x86_64-pc-windows-msvc -debug-info-kind=limited -S -emit-llvm -o - | /builddirs/llvm-project/build-Clang11/bin/FileCheck /repositories/llvm-project/clang/test/CodeGenCXX/debug-info-thunk.cpp : 'RUN: at line 2'; /builddirs/llvm-project/build-Clang11/bin/clang -cc1 -internal-isystem /builddirs/llvm-project/build-Clang11/lib/clang/13.0.0/include -nostdsysteminc /repositories/llvm-project/clang/test/CodeGenCXX/debug-info-thunk.cpp -triple x86_64-unknown-linux-gnu -debug-info-kind=limited -S -emit-llvm -o - | /builddirs/llvm-project/build-Clang11/bin/FileCheck /repositories/llvm-project/clang/test/CodeGenCXX/debug-info-thunk.cpp -check-prefix=ITANIUM -- Exit Code: 2 Command Output (stderr): -- clang: /repositories/llvm-project/clang/lib/CodeGen/CGVTables.cpp:518: llvm::Constant *clang::CodeGen::CodeGenVTables::maybeEmitThunk(clang::GlobalDecl, const clang::ThunkInfo &, bool): Assertion `OldThunkFn->isDeclaration() && "Shouldn't replace non-declaration"' failed. PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script. Stack dump: 0. Program arguments: /builddirs/llvm-project/build-Clang11/bin/clang -cc1 -internal-isystem /builddirs/llvm-project/build-Clang11/lib/clang/13.0.0/include -nostdsysteminc /repositories/llvm-project/clang/test/CodeGenCXX/debug-info-thunk.cpp -triple=x86_64-pc-windows-msvc -debug-info-kind=limited -S -emit-llvm -o - 1. /repositories/llvm-project/clang/test/CodeGenCXX/debug-info-thunk.cpp:221:1: current parser token 'namespace' 2. /repositories/llvm-project/clang/test/CodeGenCXX/debug-info-thunk.cpp:200:11: LLVM IR generation of declaration 'Test9' 3. /repositories/llvm-project/clang/test/CodeGenCXX/debug-info-thunk.cpp:216:9: Generating code for declaration 'Test9::D::foo1' #0 0x00007f9f54af2e43 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /repositories/llvm-project/llvm/lib/Support/Unix/Signals.inc:565:13 #1 0x00007f9f54af09a0 llvm::sys::RunSignalHandlers() /repositories/llvm-project/llvm/lib/Support/Signals.cpp:77:18 #2 0x00007f9f54af335f SignalHandler(int) /repositories/llvm-project/llvm/lib/Support/Unix/Signals.inc:407:1 #3 0x00007f9f597ee140 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14140) #4 0x00007f9f54400ce1 raise ./signal/../sysdeps/unix/sysv/linux/raise.c:51:1 #5 0x00007f9f543ea537 abort ./stdlib/abort.c:81:7 #6 0x00007f9f543ea40f get_sysdep_segment_value ./intl/loadmsgcat.c:509:8 #7 0x00007f9f543ea40f _nl_load_domain ./intl/loadmsgcat.c:970:34 #8 0x00007f9f543f9662 (/lib/x86_64-linux-gnu/libc.so.6+0x34662) #9 0x00007f9f58958c30 clang::CodeGen::CodeGenVTables::maybeEmitThunk(clang::GlobalDecl, clang::ThunkInfo const&, bool) /repositories/llvm-project/clang/lib/CodeGen/CGVTables.cpp:0:0
******************** FAIL: Clang :: CodeGenCXX/thunks.cpp (29 of 1127) ******************** TEST 'Clang :: CodeGenCXX/thunks.cpp' FAILED ******************** Script: -- : 'RUN: at line 3'; /builddirs/llvm-project/build-Clang11/bin/clang -cc1 -internal-isystem /builddirs/llvm-project/build-Clang11/lib/clang/13.0.0/include -nostdsysteminc /repositories/llvm-project/clang/test/CodeGenCXX/thunks.cpp -triple=sparc64-pc-linux-gnu -munwind-tables -emit-llvm -o - | /builddirs/llvm-project/build-Clang11/bin/FileCheck --check-prefixes=CHECK,CHECK-CLONE,CHECK-NONOPT /repositories/llvm-project/clang/test/CodeGenCXX/thunks.cpp : 'RUN: at line 5'; /builddirs/llvm-project/build-Clang11/bin/clang -cc1 -internal-isystem /builddirs/llvm-project/build-Clang11/lib/clang/13.0.0/include -nostdsysteminc /repositories/llvm-project/clang/test/CodeGenCXX/thunks.cpp -triple=sparc64-pc-linux-gnu -debug-info-kind=standalone -dwarf-version=5 -munwind-tables -emit-llvm -o - | /builddirs/llvm-project/build-Clang11/bin/FileCheck --check-prefixes=CHECK,CHECK-CLONE,CHECK-NONOPT,CHECK-DBG /repositories/llvm-project/clang/test/CodeGenCXX/thunks.cpp : 'RUN: at line 7'; /builddirs/llvm-project/build-Clang11/bin/clang -cc1 -internal-isystem /builddirs/llvm-project/build-Clang11/lib/clang/13.0.0/include -nostdsysteminc /repositories/llvm-project/clang/test/CodeGenCXX/thunks.cpp -triple=sparc64-pc-linux-gnu -munwind-tables -emit-llvm -o - -O1 -disable-llvm-passes | /builddirs/llvm-project/build-Clang11/bin/FileCheck --check-prefixes=CHECK,CHECK-CLONE,CHECK-OPT /repositories/llvm-project/clang/test/CodeGenCXX/thunks.cpp : 'RUN: at line 11'; /builddirs/llvm-project/build-Clang11/bin/clang -cc1 -internal-isystem /builddirs/llvm-project/build-Clang11/lib/clang/13.0.0/include -nostdsysteminc /repositories/llvm-project/clang/test/CodeGenCXX/thunks.cpp -triple=x86_64-pc-linux-gnu -munwind-tables -emit-llvm -o - -O1 -disable-llvm-passes | /builddirs/llvm-project/build-Clang11/bin/FileCheck --check-prefixes=CHECK,CHECK-TAIL,CHECK-OPT /repositories/llvm-project/clang/test/CodeGenCXX/thunks.cpp : 'RUN: at line 15'; /builddirs/llvm-project/build-Clang11/bin/clang -cc1 -internal-isystem /builddirs/llvm-project/build-Clang11/lib/clang/13.0.0/include -nostdsysteminc /repositories/llvm-project/clang/test/CodeGenCXX/thunks.cpp -triple=x86_64-windows-msvc -munwind-tables -emit-llvm -o - -O1 -disable-llvm-passes | /builddirs/llvm-project/build-Clang11/bin/FileCheck --check-prefixes=WIN64 /repositories/llvm-project/clang/test/CodeGenCXX/thunks.cpp -- Exit Code: 2 Command Output (stderr): -- clang: /repositories/llvm-project/llvm/lib/IR/Value.cpp:500: void llvm::Value::doRAUW(llvm::Value *, llvm::Value::ReplaceMetadataUses): Assertion `New->getType() == getType() && "replaceAllUses of value with new value of different type!"' failed. PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script. Stack dump: 0. Program arguments: /builddirs/llvm-project/build-Clang11/bin/clang -cc1 -internal-isystem /builddirs/llvm-project/build-Clang11/lib/clang/13.0.0/include -nostdsysteminc /repositories/llvm-project/clang/test/CodeGenCXX/thunks.cpp -triple=sparc64-pc-linux-gnu -munwind-tables -emit-llvm -o - 1. /repositories/llvm-project/clang/test/CodeGenCXX/thunks.cpp:390:1: current parser token 'namespace' 2. /repositories/llvm-project/clang/test/CodeGenCXX/thunks.cpp:342:11: LLVM IR generation of declaration 'Test12' 3. /repositories/llvm-project/clang/test/CodeGenCXX/thunks.cpp:354:9: Generating code for declaration 'Test12::C::f' #0 0x00007ff8c7c7ae43 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /repositories/llvm-project/llvm/lib/Support/Unix/Signals.inc:565:13 #1 0x00007ff8c7c789a0 llvm::sys::RunSignalHandlers() /repositories/llvm-project/llvm/lib/Support/Signals.cpp:77:18 #2 0x00007ff8c7c7b35f SignalHandler(int) /repositories/llvm-project/llvm/lib/Support/Unix/Signals.inc:407:1 #3 0x00007ff8cc976140 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14140) #4 0x00007ff8c7588ce1 raise ./signal/../sysdeps/unix/sysv/linux/raise.c:51:1 #5 0x00007ff8c7572537 abort ./stdlib/abort.c:81:7 #6 0x00007ff8c757240f get_sysdep_segment_value ./intl/loadmsgcat.c:509:8 #7 0x00007ff8c757240f _nl_load_domain ./intl/loadmsgcat.c:970:34 #8 0x00007ff8c7581662 (/lib/x86_64-linux-gnu/libc.so.6+0x34662) #9 0x00007ff8c825b8d5 llvm::cast_retty<llvm::BasicBlock, llvm::Value*>::ret_type llvm::cast<llvm::BasicBlock, llvm::Value>(llvm::Value*) /repositories/llvm-project/llvm/include/llvm/Support/Casting.h:269:3 #10 0x00007ff8c825b8d5 llvm::Value::doRAUW(llvm::Value*, llvm::Value::ReplaceMetadataUses) /repositories/llvm-project/llvm/lib/IR/Value.cpp:523:38 #11 0x00007ff8cbade0fb clang::CodeGen::CodeGenFunction::GenerateVarArgsThunk(llvm::Function*, clang::CodeGen::CGFunctionInfo const&, clang::GlobalDecl, llvm::FunctionCallee, clang::ThunkInfo const&) /repositories/llvm-project/clang/lib/CodeGen/CGVTables.cpp:184:10 #12 0x00007ff8cbae0a0f clang::CodeGen::CodeGenVTables::maybeEmitThunk(clang::GlobalDecl, clang::ThunkInfo const&, bool) /repositories/llvm-project/clang/lib/CodeGen/CGVTables.cpp:584:36
clang/include/clang/Basic/Thunk.h | ||
---|---|---|
157 ↗ | (On Diff #337567) | Isn't this the same as BaseMethod.getDecl()? (I'd expect "overridee" and "overridden" to be synonymous, contrasting with the "overrider" which would be the derived-class function.) |
clang/lib/AST/VTableBuilder.cpp | ||
1630–1635 | This looks suspicious: we're going to add two vtable slots, one for the deleting destructor and one for the complete object destructor. It doesn't matter in practice because we never form a thunk for a destructor, but it would seem more correct to pass the CXXMethodDecl* into AddMethod and have it form the appropriate GlobalDecl(s) itself. | |
clang/lib/CodeGen/CGVTables.cpp | ||
451–452 | Is this done somewhere else now? | |
465–466 | This bitcast seems to have gone missing. | |
485 | When do we get here for a destructor? I can't see any evidence that this is reachable at all: in our testsuite there doesn't seem to be any mention of a destructor thunk mangling. Generally, though, thunk mangling uses the mangled name of the target of the thunk, which should be GD.getDtorType() here. (This might matter if we somehow emitted a thunk from the complete destructor to the base subobject destructor via this function.) | |
493 | I think this should probably be checking the base method type, but the difference probably doesn't matter in practice. | |
505–508 | It's confusing to call these Base* when (in the vtable thunk case) they're information about the derived-class function. Maybe Target*? | |
508 | This doesn't look right: the target of the thunk shouldn't itself be treated as a musttail thunk. Presumably we should unconditionally use arrangeGlobalDeclaration(GD) for the target. | |
524 | Hm, I think we probably want to use MD rather than BaseMethod here. Eg, if the base method is [[noreturn]] but the overrider is not, we want a non-[[noreturn]] thunk, and conversely if the overrider is [[noreturn]] the thunk should be regardless of whether the base method was. Perhaps there are some attributes for which we want to look at the BaseMethod instead. If so, I suppose we'll need to pass both the caller-side declaration and the callee-side declaration to ConstructAttributeList (or maybe extend CGFunctionInfo to store both) and let it pick which one it wants to inspect for each attribute. | |
546 | This should be passing GD rather than BaseMethod. | |
557 | This should be passing GD rather than BaseMethod. | |
592 | This should be passing GD rather than BaseMethod. |
You know, somehow i don't think me wasting 30 more hours on this to experimentally find a working permutation is productive.
clang/include/clang/Basic/Thunk.h | ||
---|---|---|
157 ↗ | (On Diff #337567) | Oh, hmm. That makes more sense indeed. |
@rsmith i was able to make some forward progress here.
What do you think about this?
(I haven't updated all tests because i don't want to waste hours doing that if this is still wrong)
Made a couple suggestions to make this easier to review.
The test changes you've made so far seem reasonable.
Is there some specific section of the code you want feedback on?
clang/include/clang/Basic/Thunk.h | ||
---|---|---|
1 ↗ | (On Diff #339757) | Can you split the new Thunk.h, and the minimal set of changes required to use it, into a separate NFC patch? |
clang/lib/CodeGen/CGVTables.cpp | ||
744 | Can land this separately. |
Thank you for taking a look!
Basically, i'm not sure this is right, and i'm not sure what is going wrong.
For example
$ ./bin/llvm-lit /repositories/llvm-project/clang/test/CodeGen/available-externally-hidden.cpp -v llvm-lit: /repositories/llvm-project/llvm/utils/lit/lit/llvm/config.py:428: note: using clang: /builddirs/llvm-project/build-Clang12/bin/clang -- Testing: 1 tests, 1 workers -- FAIL: Clang :: CodeGen/available-externally-hidden.cpp (1 of 1) ******************** TEST 'Clang :: CodeGen/available-externally-hidden.cpp' FAILED ******************** Script: -- : 'RUN: at line 1'; /builddirs/llvm-project/build-Clang12/bin/clang -cc1 -internal-isystem /builddirs/llvm-project/build-Clang12/lib/clang/13.0.0/include -nostdsysteminc -O2 -fvisibility hidden -std=c++11 -emit-llvm -o - -triple x86_64-apple-darwin10 /repositories/llvm-project/clang/test/CodeGen/available-externally-hidden.cpp | /builddirs/llvm-project/build-Clang12/bin/FileCheck /repositories/llvm-project/clang/test/CodeGen/available-externally-hidden.cpp -- Exit Code: 2 Command Output (stderr): -- /repositories/llvm-project/clang/test/CodeGen/available-externally-hidden.cpp:14:16: error: cannot compile this thunk for forward declaration yet virtual bool Send(Message* msg) = 0; ^ 1 error generated. FileCheck error: '<stdin>' is empty. FileCheck command line: /builddirs/llvm-project/build-Clang12/bin/FileCheck /repositories/llvm-project/clang/test/CodeGen/available-externally-hidden.cpp -- ******************** ******************** Failed Tests (1): Clang :: CodeGen/available-externally-hidden.cpp Testing Time: 0.18s Failed: 1
Would it be reasonable to instead start with a stopgap measure of not adding attributes for this/return of thunks?
Would it be reasonable to instead start with a stopgap measure of not adding attributes for this/return of thunks?
You mean, add align attributes to "this" on regular methods, but not thunks? I can't see how that could do any harm.
clang/lib/CodeGen/CGVTables.cpp | ||
---|---|---|
450 | "IsUnprototyped" means that we have to generate a thunk, but we don't have the proper types. We emit some special IR with known incorrect types, and mark it with the "thunk" attribute to let the optimizer know what we're doing. It should be fine to continue using a fake type on the IsUnprototyped codepath. |
Yes, just so the rG6270b3a1eafaba4279e021418c5a2c5a35abc002 can be reverted.
Unless of course we can get this going..
I can't see how that could do any harm.
clang/lib/CodeGen/CGVTables.cpp | ||
---|---|---|
450 | IsUnprototyped is false here for that test though.. |
clang/lib/CodeGen/CGVTables.cpp | ||
---|---|---|
450 | Oh, hmm, right. My comment still holds, but doesn't apply to that test. In that test, the derived class method is defined, but the base class method isn't. You should be able to compute the LLVM function type for the base class method in that case. The types have to be the same, barring pointer variance, so being able to compute the type of the derived class method implies you can compute the type of the base class method. So I'm not sure why you'd need to error out in that case; the computation should work the same way whether or not we can see the definition of the base class method. |
clang-tidy: warning: 'auto *CD' can be declared as 'const auto *CD' [llvm-qualified-auto]
not useful