Page MenuHomePhabricator

[lld-macho] Drop assertions that all symbols are in GOT
ClosedPublic

Authored by oontvoo on Jul 2 2021, 11:51 AM.

Details

Reviewers
int3
gkm
Group Reviewers
Restricted Project
Commits
rG3822e3d5b049: [lld-macho] Fix bug in handling unwind info from ld -r
Summary

Related bug: 50812

Diff Detail

Event Timeline

oontvoo created this revision.Jul 2 2021, 11:51 AM
Herald added a reviewer: gkm. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
oontvoo requested review of this revision.Jul 2 2021, 11:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2021, 11:51 AM

The "Related bug:" was fixed D104502 as far as as I know – can you say more about the motivation here?

int3 added a subscriber: MaskRay.Jul 4 2021, 1:09 PM

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.

The "Related bug:" was fixed D104502 as far as as I know – can you say more about the motivation here?

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

Right 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 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?

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.

int3 added a comment.EditedJul 6 2021, 12:24 PM

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!

. Running this through llvm-mc and ld -r should do the trick:

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

int3 added a comment.Jul 7 2021, 9:04 AM

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

oontvoo abandoned this revision.Jul 7 2021, 11:15 AM

Abandoning this revision because it looks like D105557 has also fixed this. =)
(ie., after sync'ing pass D105557, I was no longer able to reproduce the crash)

oontvoo reclaimed this revision.Jul 7 2021, 11:55 AM

On a second thought, still getting the error - just not with this simple test.

oontvoo planned changes to this revision.Jul 8 2021, 6:52 PM

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)
Aborted
oontvoo updated this revision to Diff 357413.Jul 8 2021, 9:10 PM

added separate test

int3 added a comment.Jul 8 2021, 9:26 PM

Q: what do the C files test that my small assembly test case doesn't cover?

oontvoo updated this revision to Diff 357418.Jul 8 2021, 10:20 PM

Q: what do the C files test that my small assembly test case doesn't cover?

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)

int3 added a comment.EditedJul 9 2021, 3:49 PM

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.

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.

Ah, ok - will do!

oontvoo updated this revision to Diff 357673.Jul 9 2021, 5:14 PM

included diff from D105364

int3 accepted this revision.Jul 9 2021, 5:54 PM

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

This revision is now accepted and ready to land.Jul 9 2021, 5:54 PM
oontvoo marked 4 inline comments as done.Jul 9 2021, 7:45 PM

Thanks!

oontvoo updated this revision to Diff 357689.Jul 9 2021, 7:45 PM

addressed review comments

This revision was landed with ongoing or failed builds.Jul 9 2021, 7:47 PM
This revision was automatically updated to reflect the committed changes.