Related bug: 50812
Details
Details
- Reviewers
int3 gkm - Group Reviewers
Restricted Project - Commits
- rG3822e3d5b049: [lld-macho] Fix bug in handling unwind info from ld -r
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
The "Related bug:" was fixed D104502 as far as as I know – can you say more about the motivation here?
Comment Actions
The motivation was from D105210 -- the test case initially constructed while fixing PR50812 had symbol relocs in compact unwind, so I asked @oontvoo to split it out -- thanks for doing it :)
However, given that the cause of the bug is unrelated to N_STABS, I think we should have it in a different test file. Also, is this not something that we can test via assembly, instead of YAML?
Perhaps we should also rename the test (like @MaskRay suggested) -- in addition to making it easier for the reader to understand context, it also encourages grouping tests by functionality tested, instead of by bug origin.
Comment Actions
The ultimate motivation is to be able to link the Cronet framework :)
The simplest repro case for that is to link this:
echo "int main() {return 0;}" > test.cc
clang -g -c -g -o test1.o test.cc
ld -r -o test2.o test1.o;
ld64.lld.darwinnew -platform_version macos 11.3 11.0 -arch x86_64 test2.oRight now it tripped on the assert(isInGOT).
Previously, it was complaining about invalid symbols, then about non-section relocs.
Yes, it seems D104502 has handled the bug in non-section relocs, but the inGoT assert is still there, hence this patch.
The problem is right now you can't produce a an object file that would be similar to what ld -r would produce (ie., with debug symols).
Perhaps we should also rename the test (like @MaskRay suggested) -- in addition to making it easier for the reader to understand context, it also encourages grouping tests by functionality tested, instead of by bug origin.
Good point - I'd missed the comment.
Will address it.
Comment Actions
Got it, thanks for the explanation! In that case, can we have a separate file for this test, and include as a comment the build steps you used to make the YAML? The small repro case you described is good, though having the input be assembly instead of C would make for even more minimal YAML. Running this through llvm-mc and ld -r should do the trick:
.text .globl _main _main: .cfi_startproc .cfi_def_cfa_offset 16 .cfi_endproc nop
It's worth adding some of that detail to the commit message as well. Thanks!
Comment Actions
Using ld directly in the test? (what if the test gets run on linux, how does it know where to find ld?)
anyway we could make this more linux friendly since I do most of the dev on linux
Comment Actions
In that case, can we have a separate file for this test, and include as a comment the build steps you used to make the YAML? The small repro case you described is good, though having the input be assembly instead of C would make for even more minimal YAML.
I was suggesting how to make a minimal YAML test case. Running ld directly in a test is never an option :)
Comment Actions
Here is the simplest set repro steps:
echo 'int main() {return 0;}' > foo.c
echo 'int getX() {return 123;}' > bar.c
clang -c -g -o foo.o foo.c
clang -c -g -o bar_1.o bar.c
ld -r -o bar.o bar_1.o
ld64.lld.darwinnew --error-limit=0 -dynamic -arch x86_64 -platform_version macos 11.0.0 14.5 foo.o bar.o
----------------------------------------
ld64.lld.darwinnew: /mnt/ssd/repo/llvm-project/lld/MachO/UnwindInfoSection.cpp:265: void relocateCompactUnwind(lld::macho::ConcatOutputSection*, std::vector<lld::macho::CompactUnwindEntry<Ptr> >&) [with Ptr = long unsigned int]: Assertion `referentSym->isInGot()' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0. Program arguments: ./bin/ld64.lld.darwinnew --error-limit=0 -dynamic -arch x86_64 -platform_version macos 11.0.0 14.5 cpp/foo_cpp.o cpp/bar_cpp_2.o
#0 0x000055f75a27eefd llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /mnt/ssd/repo/llvm-project/llvm/lib/Support/Unix/Signals.inc:565:22
#1 0x000055f75a27efb4 PrintStackTraceSignalHandler(void*) /mnt/ssd/repo/llvm-project/llvm/lib/Support/Unix/Signals.inc:632:1
#2 0x000055f75a27cfaa llvm::sys::RunSignalHandlers() /mnt/ssd/repo/llvm-project/llvm/lib/Support/Signals.cpp:76:20
#3 0x000055f75a27e950 SignalHandler(int) /mnt/ssd/repo/llvm-project/llvm/lib/Support/Unix/Signals.inc:407:1
#4 0x00007fc016723140 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14140)
#5 0x00007fc016206ce1 raise ./signal/../sysdeps/unix/sysv/linux/raise.c:51:1
#6 0x00007fc0161f0537 abort ./stdlib/abort.c:81:7
#7 0x00007fc0161f040f get_sysdep_segment_value ./intl/loadmsgcat.c:509:8
#8 0x00007fc0161f040f _nl_load_domain ./intl/loadmsgcat.c:970:34
#9 0x00007fc0161ff662 (/lib/x86_64-linux-gnu/libc.so.6+0x34662)
#10 0x000055f75a7edb8a void relocateCompactUnwind<unsigned long>(lld::macho::ConcatOutputSection*, std::vector<lld::macho::CompactUnwindEntry<unsigned long>, std::allocator<lld::macho::CompactUnwindEntry<unsigned long> > >&) /mnt/ssd/repo/llvm-project/lld/MachO/UnwindInfoSection.cpp:266:48
#11 0x000055f75a7f2dee UnwindInfoSectionImpl<unsigned long>::finalize() /mnt/ssd/repo/llvm-project/lld/MachO/UnwindInfoSection.cpp:391:43
#12 0x000055f75a7ba989 (anonymous namespace)::Writer::assignAddresses(lld::macho::OutputSegment*) /mnt/ssd/repo/llvm-project/lld/MachO/Writer.cpp:1019:27
#13 0x000055f75a7ba592 (anonymous namespace)::Writer::finalizeAddresses() /mnt/ssd/repo/llvm-project/lld/MachO/Writer.cpp:973:22
#14 0x000055f75a7bb705 void (anonymous namespace)::Writer::run<lld::macho::LP64>() /mnt/ssd/repo/llvm-project/lld/MachO/Writer.cpp:1095:26
#15 0x000055f75a7c5ec8 void lld::macho::writeResult<lld::macho::LP64>() /mnt/ssd/repo/llvm-project/lld/MachO/Writer.cpp:1100:49
#16 0x000055f75a733407 lld::macho::link(llvm::ArrayRef<char const*>, bool, llvm::raw_ostream&, llvm::raw_ostream&) /mnt/ssd/repo/llvm-project/lld/MachO/Driver.cpp:1422:24
#17 0x000055f75a1d2582 lldMain(int, char const**, llvm::raw_ostream&, llvm::raw_ostream&, bool) /mnt/ssd/repo/llvm-project/lld/tools/lld/lld.cpp:152:12
#18 0x000055f75a1d2953 main /mnt/ssd/repo/llvm-project/lld/tools/lld/lld.cpp:206:19
#19 0x00007fc0161f1d0a __libc_start_main ./csu/../csu/libc-start.c:308:16
#20 0x000055f75a1d17aa _start (./bin/ld64.lld.darwinnew+0xe367aa)
AbortedComment Actions
Not much - I'd just forgot to simplify the yaml - I've regenerated the yaml files from your small assembly test and updated the test. PTAL
(but after D104502, the code path changed a bit - and the simplest test case. I could come up with was still using TWO inputs)
Comment Actions
and the simplest test case. I could come up with was still using TWO inputs
This was really surprising to me so I dug into it. Turns out it's a "regression" from D105557: [lld/mac] Don't crash when dead-stripping removes all unwind info (cc @thakis)
D105750: [not for review][lld-macho] Set allEntriesAreOmitted correctly for unwind info from ld -r should fix things so that we can test this with just one file. Wanna fold the contents of D105750 into this diff? We can't test that diff anyway without removing the isInGot() assert.
Comment Actions
we should be able to have just one file in the test now ;)
please also update the commit message to add a bit more detail, and mention the issue the other diff fixed too
gonna stamp this since there's been enough back and forth, but please fix all the issues before landing :)
| lld/test/MachO/relocs-syms-not-in-got.s | ||
|---|---|---|
| 3 | no need for FIXME, I doubt we're going to implement ld -r any time soon, and even if we did, I hope we don't emulate this output | |
| 4–5 | since we're now generating the yaml from assembly, I guess this comment should be updated | |
| 10 | bunch of unnecessary flags here, but I also think it's unnecessary? we're consuming the output of ld -r, not lld | |
| 17 | I don't think the other flags are necessary | |
no need for FIXME, I doubt we're going to implement ld -r any time soon, and even if we did, I hope we don't emulate this output