[lld-macho]Ensure canonicalization happen even for "skipped" referent sections.

[oontvoo]

Summary

Details:

See bug report: https://github.com/llvm/llvm-project/issues/63039

[oontvoo]

added test case

[oontvoo]

@int3 @thakis Any thought on this? :)

[smeenai]

I might not be understanding this diff correctly, but it seems like the code change just ensures that referent section canonicalization occurs for the UNSIGNED relocation following a SUBTRACTOR relocation. Is that the gist of the issue, and if so, could you update the summary to explain where such relocations are used in the unwind data and why it fixes the bug?

The test case is missing any assertions.

lld/test/MachO/fold-dwarf-lsda.s

5

Maybe use an explicit version in the triple which we know to not have compact unwind?

[int3]

+1 to what @smeenai said

Also re your commit message:

With assertion turned-off, lld would produce "bad" binary in which the gcc_except_table* got folded but not the related functions:

Is this actually a problem? Does the unwinder not like it when otherwise identical functions at two separate addresses share the same gcc_except_table?

lld/MachO/Writer.cpp

679–680

this is now being done in scanSymbols (I'm aware this was from an existing comment)

687–691

I think just copypasting the call to canonicalize() here would be cleaner -- no need for the skipNext state + would take fewer lines of code too

[oontvoo]

addressed review comments

[oontvoo]

In D151824#4393486, @int3 wrote:

+1 to what @smeenai said

I've updated the tests to add more assertions. But originally, the fact that the test didn't crash was sort of enough to cover to bug this aimed to fix.

Also re your commit message:

With assertion turned-off, lld would produce "bad" binary in which the gcc_except_table* got folded but not the related functions:

Is this actually a problem? Does the unwinder not like it when otherwise identical functions at two separate addresses share the same gcc_except_table?

Actually, the most serious issue here was that the linker crashed on the assertion (stacktrace attached in the bug report).
The rest of the symptoms are probably not as alarming. (the description/commit message was jsut trying to point out that the entries were pointing at the bad address).
Anyway, I've updated the tests with more assertions - hopefully that would make it a bit clearer.

Thanks!

lld/MachO/Writer.cpp

679–680

updated comment

687–691

couldn't simply copy the canonicalize() call here because we want to call it on the "next" entry (ie., the "skipped" one) as well.

In other words this it++ would have skipped over it .

[oontvoo]

removed extraneous NLs

[int3]

lld/MachO/Writer.cpp

687–691

I understand, but I meant like

++it;
if (auto *referentIsec = it->referent.dyn_cast<InputSection *>())
  it->referent = referentIsec->canonical();
continue;

lld/test/MachO/fold-dwarf-lsda.s

3

6

44

can the test case be made smaller? e.g. are the .cfi_personality directives necessary?

48

.cfi_def_cfa_register probably isn't necessary either

[oontvoo]

Addressed review comments.

[oontvoo]

Thanks!

[int3]

lgtm. Could you update the commit message to reflect the main problem too? Thanks!

[oontvoo]

rebase + edit commit message

lld/MachO/Writer.cpp

void Writer::scanRelocations() {
   for (size_t i = 0; i < inputSections.size(); ++i) {
     ConcatInputSection *isec = inputSections[i];
 
     if (isec->shouldOmitFromOutput())
       continue;
 
     for (auto it = isec->relocs.begin(); it != isec->relocs.end(); ++it) {
       lld::macho::Reloc &r = *it;
+
+      // Canonicalize the referent so that later accesses in Writer won't
+      // have to worry about it.
+      if (auto *referentIsec = r.referent.dyn_cast<InputSection *>())
+        r.referent = referentIsec->canonical();
+
       if (target->hasAttr(r.type, RelocAttrBits::SUBTRAHEND)) {
         // Skip over the following UNSIGNED relocation -- it's just there as the
         // minuend, and doesn't have the usual UNSIGNED semantics. We don't want
         // to emit rebase opcodes for it.
-        it++;
+        ++it;
+        // Canonicalize the referent so that later accesses in Writer won't
+        // have to worry about it.
+        if (auto *referentIsec = it->referent.dyn_cast<InputSection *>())
+          it->referent = referentIsec->canonical();
         continue;
       }
       if (auto *sym = r.referent.dyn_cast<Symbol *>()) {
         if (auto *undefined = dyn_cast<Undefined>(sym))
           treatUndefinedSymbol(*undefined, isec, r.offset);
         // treatUndefinedSymbol() can replace sym with a DylibSymbol; re-check.
         if (!isa<Undefined>(sym) && validateSymbolRelocation(sym, isec, r))
           prepareSymbolRelocation(sym, isec, r);
       } else {
-        // Canonicalize the referent so that later accesses in Writer won't
-        // have to worry about it. Perhaps we should do this for Defined::isec
-        // too...
-        auto *referentIsec = r.referent.get<InputSection *>();
-        r.referent = referentIsec->canonical();
         if (!r.pcrel) {
           if (config->emitChainedFixups)
             in.chainedFixups->addRebase(isec, r.offset);
           else
             in.rebase->addEntry(isec, r.offset);
         }
       }
     }

lld/test/MachO/fold-dwarf-lsda.s

+# REQUIRES: x86
+## Regression test for https://github.com/llvm/llvm-project/issues/63039
+
+## Use an old version to ensure we do *not* have any compact-unwind.
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos9 %s -o %t.o
+
+## Pre-cond: smoke-check that there is really no compact-unwind entries - only dwarfs.
+# RUN: llvm-objdump --macho --unwind-info --dwarf=frames %t.o | FileCheck %s --check-prefix=PRE
+# PRE-NOT: Contents of __compact_unwind section:
+# PRE-NOT: Entry at offset
+# PRE: .eh_frame contents:
+# PRE: {{[0-9a-f]+}} {{.*}} CIE
+# PRE:   Format:                DWARF32
+# PRE:   Version:               1
+
+## Link should succeed (ie., not crashed due to bug in icf code).
+# RUN: %lld -lSystem -lc++ --icf=all -arch x86_64 -arch x86_64 -platform_version macos 11.0 11.0 %t.o -o %t.out
+
+## Post-cond: verify that the final binary has expected eh-frame contents.
+# RUN: llvm-objdump --macho --syms --dwarf=frames %t.out | FileCheck %s --check-prefix=POST
+# POST-LABEL: SYMBOL TABLE:
+# POST: [[#%x,EXCEPT_ADDR:]] l   O __TEXT,__gcc_except_tab GCC_except_table0
+# POST: [[#%x,EXCEPT_ADDR]]  l   O __TEXT,__gcc_except_tab GCC_except_table1
+# POST: [[#%.16x,F0_ADDR:]]  g   F __TEXT,__text _f0
+# POST: [[#%.16x,F1_ADDR:]]  g   F __TEXT,__text _f1
+# POST: [[#%.16x,G_ADDR:]]   g   F __TEXT,__text _g
+
+# POST-LABEL: .eh_frame contents:
+# POST: {{.*}} FDE cie={{.+}} pc=[[#%x,G_ADDR]]...{{.+}}
+
+# POST: {{.*}} FDE cie={{.+}} pc=[[#%x,F0_ADDR]]...{{.+}}
+# POST: Format:       DWARF32 
+# POST: LSDA Address: [[#%.16x,EXCEPT_ADDR]]
+
+# POST: {{.*}} FDE cie={{.+}} pc=[[#%x,F1_ADDR]]...{{.+}}
+# POST Format:       DWARF32 
+# POST LSDA Address: [[#%.16x,EXCEPT_ADDR]]
+
+  .section        __TEXT,__text,regular,pure_instructions
+  .globl  _f0
+_f0:
+  .cfi_startproc
+  .cfi_lsda 16, Lexception0
+  .cfi_def_cfa_offset 16
+  .cfi_offset %rbp, -16
+  callq _g
+  retq
+  .cfi_endproc
+  
+  .section  __TEXT,__gcc_except_tab
+GCC_except_table0:
+Lexception0:
+  .byte 255
+                                
+  .section  __TEXT,__text,regular,pure_instructions
+  .globl  _f1
+_f1:
+  .cfi_startproc
+  .cfi_lsda 16, Lexception1
+  .cfi_def_cfa_offset 16
+  .cfi_offset %rbp, -16
+  callq _g
+  retq
+  .cfi_endproc
+  
+  .section  __TEXT,__gcc_except_tab
+GCC_except_table1:
+Lexception1:
+  .byte 255
+
+  .section  __TEXT,__text,regular,pure_instructions
+  .globl  _g
+_g:               
+  .cfi_startproc
+  .cfi_def_cfa_offset 16
+  .cfi_offset %rbp, -16
+  .cfi_def_cfa_register %rbp  
+  retq
+  .cfi_endproc
+  
+  .section  __TEXT,__text,regular,pure_instructions
+  .globl  _main             
+_main:                            
+  retq
+
+.subsections_via_symbols