Page MenuHomePhabricator

[BROKEN][clang] Try to fix thunk function types
Needs ReviewPublic

Authored by lebedev.ri on Tue, Apr 13, 8:07 AM.

Details

Summary

@rsmith this seems to work on the two testcases from D99790,
but apparently doesn't work on many more tests.
I don't know what i'm doing here.

Diff Detail

Unit TestsFailed

TimeTest
40 msx64 debian > Clang.CodeGen::available-externally-hidden.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/13.0.0/include -nostdsysteminc -O2 -fvisibility hidden -std=c++11 -emit-llvm -o - -triple x86_64-apple-darwin10 /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/available-externally-hidden.cpp | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/available-externally-hidden.cpp
120 msx64 debian > Clang.CodeGenCXX::arm.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/13.0.0/include -nostdsysteminc /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenCXX/arm.cpp -triple=thumbv7-apple-ios6.0 -fno-use-cxa-atexit -target-abi apcs-gnu -emit-llvm -std=gnu++98 -o - -fexceptions | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck -check-prefix=CHECK -check-prefix=CHECK98 /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenCXX/arm.cpp
100 msx64 debian > Clang.CodeGenCXX::constructor-destructor-return-this.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/13.0.0/include -nostdsysteminc /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenCXX/constructor-destructor-return-this.cpp -emit-llvm -o - -triple=i686-unknown-linux | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck --check-prefix=CHECKGEN /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenCXX/constructor-destructor-return-this.cpp
500 msx64 debian > Clang.CodeGenCXX::debug-info-ms-dtor-thunks.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/13.0.0/include -nostdsysteminc -triple i686--windows -emit-llvm -debug-info-kind=line-tables-only -x c++ /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenCXX/debug-info-ms-dtor-thunks.cpp -fms-extensions -o - | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenCXX/debug-info-ms-dtor-thunks.cpp
780 msx64 debian > Clang.CodeGenCXX::debug-info-thunk.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/13.0.0/include -nostdsysteminc /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenCXX/debug-info-thunk.cpp -triple=x86_64-pc-windows-msvc -debug-info-kind=limited -S -emit-llvm -o - | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenCXX/debug-info-thunk.cpp
View Full Test Results (62 Failed)

Event Timeline

lebedev.ri created this revision.Tue, Apr 13, 8:07 AM
lebedev.ri requested review of this revision.Tue, Apr 13, 8:07 AM
Herald added a project: Restricted Project. · View Herald Transcript
rsmith added inline comments.Wed, Apr 14, 11:57 AM
clang/include/clang/Basic/ABI.h
43

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
1157–1159

Can you modify the thunk info in-place, instead of this erase + insert dance?

lebedev.ri added inline comments.Wed, Apr 14, 11:59 AM
clang/lib/AST/VTableBuilder.cpp
1157–1159

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
rsmith added inline comments.Wed, Apr 14, 3:50 PM
clang/include/clang/Basic/Thunk.h
158

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
1649–1654

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.

516

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.)

525–527

I think this should probably be checking the base method type, but the difference probably doesn't matter in practice.

532–533

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.

538–541

It's confusing to call these Base* when (in the vtable thunk case) they're information about the derived-class function. Maybe Target*?

553

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.

575

This should be passing GD rather than BaseMethod.

586

This should be passing GD rather than BaseMethod.

620

This should be passing GD rather than BaseMethod.

lebedev.ri abandoned this revision.Thu, Apr 15, 2:46 AM
lebedev.ri updated this revision to Diff 337679.
lebedev.ri marked 10 inline comments as done.

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
158

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)

ping @rsmith - any further thoughts here?

@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)

ping @rsmith / @efriedma - if there are any further thoughts on the problem, i would love to hear them

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

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
750

Can land this separately.