[lld-macho] Fix assertion when two symbols at same addr have unwind info

[int3]

Summary

If there are multiple symbols at the same address, our unwind info
implementation assumes that we always register unwind entries to a
single canonical symbol.

This assumption was violated by the registerEhFrame code.

Fixes #56570.

[oontvoo]

lld/test/MachO/Inputs/double-unwind-info.yaml

2

would be great to have comment on how this yaml was created (ie., the .s file and/or any special flag?) in case we need to update it in the future

[int3]

lld/test/MachO/Inputs/double-unwind-info.yaml

2

See the comments in double-unwind-info.s :)

[thakis]

Stamp, but:

lld/MachO/InputFiles.cpp

1536

We hit this assert in "normal" builds: https://bugs.chromium.org/p/chromium/issues/detail?id=1346125#c1

Do you want a repro file for that?

Anyways, addressing the assert quickly would be good :) (either revert if hitting this on a normal build is surprising and needs more investigation, or by landing this fix here)

[int3]

lld/MachO/InputFiles.cpp

1536

Just to confirm, are you sure the "normal" build doesn't pull in any 3rd-party lib that could've been passed through ld -r?

I wasn't able to generate it the problem via llvm-mc in my testing, but I could certainly have missed something. Repro file wouldn't really help to figure out the compiler flags that trigger it though...

It doesn't look like the assert triggers at all on chromium_framework at least, so I think it's safe to say that it's a fairly uncommon code path

[int3]

lld/MachO/InputFiles.cpp

1536

It doesn't look like the assert triggers at all on chromium_framework at least, so I think it's safe to say that it's a fairly uncommon code path

More precisely, an assert(false) in this if branch doesn't get tripped while linking chromium_framework.

Landing this now, we can tweak the comment as needed later

[abrachet]

lld/MachO/InputFiles.cpp

1536

Hi this is crashing for us too I've uploaded a reproducer here https://drive.google.com/file/d/1tz3feLMfR-2N8SMxT5dqLqAN3dUVSGRQ/view?usp=sharing. Though it seems like not all input files correctly made it into the reproducer.
See https://ci.chromium.org/ui/p/fuchsia/builders/prod/clang-mac-xarm64/b8807891867843444577/overview which is an lto build of llvm. If it's helpful here's what funcSym looks like before the crash

(lldb) p *funcSym
(lld::macho::Defined) $9 = {
  lld::macho::Symbol = {
    gotIndex = 4294967295
    lazyBindOffset = 4294967295
    stubsHelperIndex = 4294967295
    stubsIndex = 4294967295
    symtabIndex = 4294967295
    symbolKind = DefinedKind
    nameData = 0x000000011986014f "__ZNSt3__212basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEC1B6v15000IDnEEPKc"
    file = 0x000000011a849a00
    nameSize = 82
    isUsedInRegularObj = true
    used = false
  }
  overridesWeakDef = false
  privateExtern = true
  includeInSymtab = true
  wasIdenticalCodeFolded = false
  thumb = false
  referencedDynamically = false
  noDeadStrip = false
  interposable = false
  weakDefCanBeHidden = false
  weakDef = true
  external = true
  isec = nullptr
  value = 0
  size = 0
  unwindEntry = nullptr
}

[abrachet]

lld/MachO/InputFiles.cpp

1536

Ignore the "too" in my comment. I incorrectly read @thakis's comment as him saying this was causing an assertion failure not fixing one.

[int3]

lld/MachO/InputFiles.cpp

1536

whoops, guess it's bug whackamole season. Sorry about that. D130409 should fix.

[thakis]

Sorry for the slow turnaround – was at a conference last week, and don't build the iOS code all that often.

Here's a repro file: https://drive.google.com/file/d/1wMTTtgtLgrnXPZ--qpx1jUJs3spXeLmo/view?usp=sharing

The assertion used to fire for Users/thakis/src/chrome/src/out/gnios/obj/ios/chrome/browser/ui/omnibox/popup/popup_swift/omnibox_popup_view_provider.o. That .o file is created by compiling https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/ui/omnibox/popup/omnibox_popup_view_provider.swift;l=1?q=omnibox_popup_view_provider.&sq= (I can get the exact compile command if it helps). It's a swift file, so maybe this is needed for all swift .o files? If so, this would probably slow down links of projects using swift a bit (?)

[thakis]

(For my own notes:

args.gn:

use_goma = true
dcheck_always_on = false
is_debug = false
symbol_level = 0
target_os = "ios"
target_cpu = "arm64"
ios_enable_code_signing = false

At chromium rev 3695eeb95bcf70bcbad32a4166c0d0748ed61d99.

Ran:

% time ninja -C out/gnios ios_chrome_unittests -j250     
% rm out/gnios/obj/ios/chrome/test/arm64/ios_chrome_unittests
% LLD_REPRODUCE=repro-ios.tar ninja -C out/gnios obj/ios/chrome/test/arm64/ios_chrome_unittests

)

lld/MachO/InputFiles.cpp

for (auto it = isec->relocs.begin(); it != isec->relocs.end(); ++it) {
         funcAddrRelocIt = it++; // Found subtrahend; skip over minuend reloc
       else if (lsdaAddrOpt && it->offset == lsdaAddrOff)
         lsdaAddrRelocIt = it++; // Found subtrahend; skip over minuend reloc
     }
 
     Defined *funcSym;
     if (funcAddrRelocIt != isec->relocs.end()) {
       funcSym = targetSymFromCanonicalSubtractor(isec, funcAddrRelocIt);
+      // Canonicalize the symbol. If there are multiple symbols at the same
+      // address, we want both `registerEhFrame` and `registerCompactUnwind`
+      // to register the unwind entry under same symbol.
+      // This is not particularly efficient, but we should run into this case
+      // infrequently (only when handling the output of `ld -r`).
+      funcSym = findSymbolAtOffset(cast<ConcatInputSection>(funcSym->isec),
+                                   funcSym->value);
     } else {
       funcSym = findSymbolAtAddress(sections, funcAddr);
       ehRelocator.makePcRel(funcAddrOff, funcSym, target->p2WordSize);
     }
     // The symbol has been coalesced, or already has a compact unwind entry.
     if (!funcSym || funcSym->getFile() != this || funcSym->unwindEntry) {
       // We must prune unused FDEs for correctness, so we cannot rely on
       // -dead_strip being enabled.

lld/MachO/UnwindInfoSection.cpp

 void UnwindInfoSection::addSymbol(const Defined *d) {
   if (d->unwindEntry)
     allEntriesAreOmitted = false;
   // We don't yet know the final output address of this symbol, but we know that
   // they are uniquely determined by a combination of the isec and value, so
   // we use that as the key here.
   auto p = symbols.insert({{d->isec, d->value}, d});
   // If we have multiple symbols at the same address, only one of them can have
-  // an associated CUE.
+  // an associated unwind entry.
   if (!p.second && d->unwindEntry) {
     assert(!p.first->second->unwindEntry);
     p.first->second = d;
   }
 }
 
 void UnwindInfoSectionImpl::prepareRelocations() {
   // This iteration needs to be deterministic, since prepareRelocations may add

lld/test/MachO/Inputs/double-unwind-info.yaml

+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x1000007
+  cpusubtype:      0x3
+  filetype:        0x1
+  ncmds:           4
+  sizeofcmds:      384
+  flags:           0x2000
+  reserved:        0x0
+LoadCommands:
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         312
+    segname:         ''
+    vmaddr:          0
+    vmsize:          96
+    fileoff:         448
+    filesize:        96
+    maxprot:         7
+    initprot:        7
+    nsects:          3
+    flags:           0
+    Sections:
+      - sectname:        __text
+        segname:         __TEXT
+        addr:            0x0
+        size:            2
+        offset:          0x1C0
+        align:           0
+        reloff:          0x0
+        nreloc:          0
+        flags:           0x80000400
+        reserved1:       0x0
+        reserved2:       0x0
+        reserved3:       0x0
+        content:         '9090'
+      - sectname:        __eh_frame
+        segname:         __TEXT
+        addr:            0x8
+        size:            56
+        offset:          0x1C8
+        align:           3
+        reloff:          0x220
+        nreloc:          4
+        flags:           0x0
+        reserved1:       0x0
+        reserved2:       0x0
+        reserved3:       0x0
+        content:         1400000000000000017A520001781001100C0708900100001C00000004000000F8FFFFFFFFFFFFFF0100000000000000000E080000000000
+        relocations:
+          - address:         0x1C
+            symbolnum:       3
+            pcrel:           false
+            length:          2
+            extern:          true
+            type:            5
+            scattered:       false
+            value:           0
+          - address:         0x1C
+            symbolnum:       4
+            pcrel:           false
+            length:          2
+            extern:          true
+            type:            0
+            scattered:       false
+            value:           0
+          - address:         0x20
+            symbolnum:       4
+            pcrel:           false
+            length:          3
+            extern:          true
+            type:            5
+            scattered:       false
+            value:           0
+          - address:         0x20
+            symbolnum:       2
+            pcrel:           false
+            length:          3
+            extern:          true
+            type:            0
+            scattered:       false
+            value:           0
+      - sectname:        __compact_unwind
+        segname:         __LD
+        addr:            0x40
+        size:            32
+        offset:          0x200
+        align:           3
+        reloff:          0x240
+        nreloc:          1
+        flags:           0x2000000
+        reserved1:       0x0
+        reserved2:       0x0
+        reserved3:       0x0
+        content:         '0000000000000000010000000000010200000000000000000000000000000000'
+        relocations:
+          - address:         0x0
+            symbolnum:       2
+            pcrel:           false
+            length:          3
+            extern:          true
+            type:            0
+            scattered:       false
+            value:           0
+  - cmd:             LC_SYMTAB
+    cmdsize:         24
+    symoff:          584
+    nsyms:           5
+    stroff:          664
+    strsize:         40
+  - cmd:             LC_BUILD_VERSION
+    cmdsize:         32
+    platform:        1
+    minos:           659200
+    sdk:             0
+    ntools:          1
+    Tools:
+      - tool:            3
+        version:         50069504
+  - cmd:             LC_DATA_IN_CODE
+    cmdsize:         16
+    dataoff:         584
+    datasize:        0
+LinkEditData:
+  NameList:
+    - n_strx:          2
+      n_type:          0xE
+      n_sect:          1
+      n_desc:          0
+      n_value:         0
+    - n_strx:          10
+      n_type:          0xE
+      n_sect:          1
+      n_desc:          0
+      n_value:         1
+    - n_strx:          16
+      n_type:          0xE
+      n_sect:          1
+      n_desc:          0
+      n_value:         1
+    - n_strx:          21
+      n_type:          0xE
+      n_sect:          2
+      n_desc:          0
+      n_value:         8
+    - n_strx:          31
+      n_type:          0xE
+      n_sect:          2
+      n_desc:          0
+      n_value:         32
+  StringTable:
+    - ' '
+    - _spacer
+    - ltmp1
+    - _foo
+    - EH_Frame1
+    - func.eh
+    - ''
+...

lld/test/MachO/double-unwind-info.s

+## When changing the assembly input, uncomment these lines to re-generate the
+## YAML.
+# COM: llvm-mc --emit-dwarf-unwind=always -filetype=obj -triple=x86_64-apple-macos10.15 %s -o %t.o
+# COM: ld -r %t.o -o %t-r.o
+# COM: obj2yaml %t-r.o > %S/Inputs/double-unwind-info.yaml
+
+# RUN: yaml2obj %S/Inputs/double-unwind-info.yaml > %t-r.o
+# RUN: %lld -dylib -lSystem %t-r.o -o /dev/null
+
+.text
+## eh_frame function address relocations are only emitted if the function isn't
+## at address 0x0.
+_spacer:
+  nop
+
+## Check that we perform unwind info registration correctly when there are
+## multiple symbols at the same address. This would previously hit an assertion
+## error (PR56570).
+_foo:
+ltmp1:
+  .cfi_startproc
+  .cfi_def_cfa_offset 8
+  nop
+  .cfi_endproc
+
+.subsections_via_symbols