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

[oontvoo]

Summary

Related bug: 50812

[thakis]

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

[int3]

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.

[oontvoo]

In D105364#2856521, @thakis wrote:

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.

In D105364#2857202, @int3 wrote:

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]

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!

[oontvoo]

In D105364#2860523, @int3 wrote:

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

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]

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]

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

[oontvoo]

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]

added separate test

[int3]

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

[oontvoo]

In D105364#2866214, @int3 wrote:

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]

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.

[oontvoo]

In D105364#2868450, @int3 wrote:

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]

included diff from D105364

[int3]

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

[oontvoo]

Thanks!

[oontvoo]

addressed review comments

lld/MachO/UnwindInfoSection.cpp

void UnwindInfoSectionImpl<Ptr>::prepareRelocations(ConcatInputSection *isec) {
   // work. But since there are usually just few personality functions
   // that are referenced from many places, at least some of them likely
   // live, it wouldn't reduce number of got entries.
   for (size_t i = 0; i < isec->relocs.size(); ++i) {
     Reloc &r = isec->relocs[i];
     assert(target->hasAttr(r.type, RelocAttrBits::UNSIGNED));
 
     if (r.offset % sizeof(CompactUnwindEntry<Ptr>) == 0) {
-      if (auto *referentIsec = r.referent.dyn_cast<InputSection *>())
-        if (!cast<ConcatInputSection>(referentIsec)->shouldOmitFromOutput())
-          allEntriesAreOmitted = false;
+      InputSection *referentIsec;
+      if (auto *isec = r.referent.dyn_cast<InputSection *>())
+        referentIsec = isec;
+      else
+        referentIsec = cast<Defined>(r.referent.dyn_cast<Symbol *>())->isec;
+
       continue;
     }
 
     if (r.offset % sizeof(CompactUnwindEntry<Ptr>) !=
         offsetof(CompactUnwindEntry<Ptr>, personality))
       continue;
 
     if (auto *s = r.referent.dyn_cast<Symbol *>()) {
for (const ConcatInputSection *isec : compactUnwindSection->inputs) {
     uint8_t *buf =
         reinterpret_cast<uint8_t *>(cuVector.data()) + isec->outSecOff;
     memcpy(buf, isec->data.data(), isec->data.size());
 
     for (const Reloc &r : isec->relocs) {
       uint64_t referentVA = TombstoneValue<Ptr>;
       if (auto *referentSym = r.referent.dyn_cast<Symbol *>()) {
         if (!isa<Undefined>(referentSym)) {
-          assert(referentSym->isInGot());
           if (auto *defined = dyn_cast<Defined>(referentSym))
             checkTextSegment(defined->isec);
           // At this point in the link, we may not yet know the final address of
           // the GOT, so we just encode the index. We make it a 1-based index so
           // that we can distinguish the null pointer case.
           referentVA = referentSym->gotIndex + 1;
         }
       } else {

lld/test/MachO/relocs-syms-not-in-got.s

+# REQUIRES: x86
+## These yaml files were  from an object file produced with 'ld -r', specifically:
+##
+##  // foo.s
+## .text
+## .globl _main
+## _main:
+##   .cfi_startproc
+##   .cfi_def_cfa_offset 16
+##   .cfi_endproc
+##   nop
+##
+## llvm-mc -filetype=obj -triple=x86_64-apple-macos10.15 -o foo1.o foo.s
+## ld -r -o foo.o foo1.o
+
+
+# RUN: rm -rf %t; mkdir -p %t
+# RUN: yaml2obj %s -o %t/foo.o
+# RUN: %lld -o %t/a.out %t/foo.o
+
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x01000007
+  cpusubtype:      0x00000003
+  filetype:        0x00000001
+  ncmds:           2
+  sizeofcmds:      384
+  flags:           0x00000000
+  reserved:        0x00000000
+LoadCommands:
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         312
+    segname:         ''
+    vmaddr:          0
+    vmsize:          64
+    fileoff:         448
+    filesize:        64
+    maxprot:         7
+    initprot:        7
+    nsects:          2
+    flags:           0
+    Sections:
+      - sectname:        __text
+        segname:         __TEXT
+        addr:            0x0000000000000000
+        size:            1
+        offset:          0x000001C0
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x80000400
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+        content:         '90'
+      - sectname:        __compact_unwind
+        segname:         __LD
+        addr:            0x0000000000000020
+        size:            32
+        offset:          0x000001E0
+        align:           3
+        reloff:          0x00000200
+        nreloc:          1
+        flags:           0x02000000
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+        content:         '0000000000000000010000000000020200000000000000000000000000000000'
+        relocations:
+          - address:         0x00000000
+            symbolnum:       1
+            pcrel:           false
+            length:          3
+            extern:          true
+            type:            0
+            scattered:       false
+            value:           0
+  - cmd:             LC_SYMTAB
+    cmdsize:         24
+    symoff:          520
+    nsyms:           2
+    stroff:          552
+    strsize:         24
+LinkEditData:
+  NameList:
+    - n_strx:          8
+      n_type:          0x0E
+      n_sect:          2
+      n_desc:          0
+      n_value:         8
+    - n_strx:          2
+      n_type:          0x0F
+      n_sect:          1
+      n_desc:          32
+      n_value:         0
+  StringTable:
+    - ' '
+    - _main
+
+...