[MC] .addrsig_sym: ignore unregistered symbols

[MaskRay]

Summary

.addrsig_sym forces registering the symbol regardless whether it is otherwise
registered. This creates an undefined symbol which is inconvenient/undesired:

• extern int x; void f() { (void)x; } has inconsistent behavior whether x is emitted as an undefined symbol. -O0 -faddrsig makes x undefined while other -O levels and -fno-addrsig eliminate the symbol.
• In ThinLTO, after a non-prevailing linkonce_odr definition is converted to available_externally, and then a declaration, the addrsig code emits a symbol while the symbol is otherwise unseen.

D135427 fixed a bug that a non-prevailing __cxx_global_var_init was
incorrectly retained. However, the IR declaration causes an undesired
.addrsig_sym __cxx_global_var_init. This can be addressed in a way similar
to D101512 (isTransitiveUsedByMetadataOnly) but the increased
OutStreamer->emitAddrsigSym(getSymbol(&GV)); complexity makes me nervous.
Just ignoring unregistered symbols circumvents the problem.

[MaskRay]

rebase after a Mach-O MC test

[chapuni]

llvm/test/MC/MachO/addrsig.s

2

It requires aarch64 target?

[MaskRay]

rebase on # REQUIRES: aarch64-registered-target

[MaskRay]

llvm/test/MC/MachO/addrsig.s

2

Thanks. Fixed in a separate commit.

[tejohnson]

@pcc should also take a look. I'm not very familiar with addrsig but it looks like he added support for it in D47744, including the source line and test that are being deleted here.

• extern int x; void f() { (void)x; } produces undefined x only in -O0 -faddrsig, other -O levels and -fno-addrsig don't produce the symbol.
• In ThinLTO, after a non-prevailing linkonce_odr definition is converted to available_externally, and then a declaration, the addrsig code emits a symbol while the symbol is otherwise unseen.

To make sure I understand the above properly, is it the case that both at "-O0 -faddrsig" and "-flto=thin -faddrsig" we get a reference to the symbol, but with "-O"x no ThinLTO we don't? I'm confused as to what was intended.

[rnk]

If we have this, should we revert D101512 as a code simplification?

Please update the .addrsig docs available at https://llvm.org/docs/Extensions.html#machine-specific-assembly-syntax to document that symbols that are neither defined nor referenced are not recorded as address-significant.

Also, I'm starting to reconsider D135427, which made non-prevailing local comdat members available_externally. Why did you choose available_externally? That is an external linkage. If there are any references to the local linkage symbol, they will now become references to an external symbol with the same name, rather than becoming errors, or relocations against discarded sections. The native linker *discards* non-prevailing comdat members, so we should do that too in complex cases. Using available_externally is something we do to support optimization (inlining).

[MaskRay]

Update Extensions.rst

[MaskRay]

In D135642#3849758, @tejohnson wrote:

@pcc should also take a look. I'm not very familiar with addrsig but it looks like he added support for it in D47744, including the source line and test that are being deleted here.

• extern int x; void f() { (void)x; } produces undefined x only in -O0 -faddrsig, other -O levels and -fno-addrsig don't produce the symbol.
• In ThinLTO, after a non-prevailing linkonce_odr definition is converted to available_externally, and then a declaration, the addrsig code emits a symbol while the symbol is otherwise unseen.

To make sure I understand the above properly, is it the case that both at "-O0 -faddrsig" and "-flto=thin -faddrsig" we get a reference to the symbol, but with "-O"x no ThinLTO we don't? I'm confused as to what was intended.

Clarified the description with this bullet point now:

* `extern int x; void f() { (void)x; }` has inconsistent behavior whether `x` is emitted as an undefined symbol.
  `-O0 -faddrsig` makes `x` undefined while other -O levels and -fno-addrsig eliminate the symbol.

[MaskRay]

In D135642#3849835, @rnk wrote:

If we have this, should we revert D101512 as a code simplification?

It can be considered a follow-up if this new behavior appears stable in the main branch.

Please update the .addrsig docs available at https://llvm.org/docs/Extensions.html#machine-specific-assembly-syntax to document that symbols that are neither defined nor referenced are not recorded as address-significant.

Done.

Also, I'm starting to reconsider D135427, which made non-prevailing local comdat members available_externally. Why did you choose available_externally? That is an external linkage. If there are any references to the local linkage symbol, they will now become references to an external symbol with the same name, rather than becoming errors, or relocations against discarded sections. The native linker *discards* non-prevailing comdat members, so we should do that too in complex cases. Using available_externally is something we do to support optimization (inlining).

Because marking such a symbol in a non-prevailing COMDAT available_externally is the only available and pratical mechanism, considering that it may be referenced by symbols in the same COMDAT. The symbol cannot just be eliminated (without introducing a GC pass).

External references to local linkage symbols in a non-prevailing COMDAT group is disallowed. The generic ABI says "... References to this symbol table entry from outside the group are not allowed."
(ld.lld error: relocation refers to a symbol in a discarded section: )
This property means that local linkage symbols in COMDAT groups are essentially roots in a reference graph. I think D135427 treatment has sane semantics.

[rnk]

I think this looks good, but make sure @tejohnson's concerns are addressed.

In D135642#3850471, @MaskRay wrote:

In D135642#3849835, @rnk wrote:

If we have this, should we revert D101512 as a code simplification?

It can be considered a follow-up if this new behavior appears stable in the main branch.

Sounds good

Please update the .addrsig docs available at https://llvm.org/docs/Extensions.html#machine-specific-assembly-syntax to document that symbols that are neither defined nor referenced are not recorded as address-significant.

Done.

Thanks!

Also, I'm starting to reconsider D135427, which made non-prevailing local comdat members available_externally. Why did you choose available_externally? That is an external linkage. If there are any references to the local linkage symbol, they will now become references to an external symbol with the same name, rather than becoming errors, or relocations against discarded sections. The native linker *discards* non-prevailing comdat members, so we should do that too in complex cases. Using available_externally is something we do to support optimization (inlining).

Because marking such a symbol in a non-prevailing COMDAT available_externally is the only available and pratical mechanism, considering that it may be referenced by symbols in the same COMDAT. The symbol cannot just be eliminated (without introducing a GC pass).

External references to local linkage symbols in a non-prevailing COMDAT group is disallowed. The generic ABI says "... References to this symbol table entry from outside the group are not allowed."
(ld.lld error: relocation refers to a symbol in a discarded section: )
This property means that local linkage symbols in COMDAT groups are essentially leaves in a reference graph. I think D135427 treatment has sane semantics.

I think what I'm saying is that yes, such references to local linkage symbols from outside of a comdat are not allowed. In a native link, they result in exactly that error message. How can we ensure that we issue an equivalent linker error in that case during ThinLTO? Anyway, we can discuss further on that review thread.

llvm/docs/Extensions.rst

380–381

I would elaborate: If sym is not otherwise referenced or defined anywhere else in the file, this directive is a no-op.

[MaskRay]

update doc

llvm/docs/Extensions.rst

 
 This instructs the assembler to emit an address-significance table. Without
 this directive, all symbols are considered address-significant.
 
 .. code-block:: gas
 
   .addrsig_sym sym
 
-This marks ``sym`` as address-significant.
+If ``sym`` is not otherwise referenced or defined anywhere else in the file,
+this directive is a no-op. Otherwise, mark ``sym`` as address-significant.
 
 ``SHT_LLVM_SYMPART`` Section (symbol partition specification)
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 This section is used to mark symbols with the `partition`_ that they
 belong to. An ``.llvm_sympart`` section consists of a null-terminated string
 specifying the name of the partition followed by a relocation referring to
 the symbol that belongs to the partition. It may be constructed as follows:

llvm/lib/MC/ELFObjectWriter.cpp

void ELFWriter::computeSymbolTable(
   for (uint32_t Index : ShndxIndexes)
     write(Index);
   SecEnd = W.OS.tell();
   SectionOffsets[SymtabShndxSection] = std::make_pair(SecStart, SecEnd);
 }
 
 void ELFWriter::writeAddrsigSection() {
   for (const MCSymbol *Sym : OWriter.AddrsigSyms)
+    if (Sym->getIndex() != 0)
       encodeULEB128(Sym->getIndex(), W.OS);
 }
 
 MCSectionELF *ELFWriter::createRelocationSection(MCContext &Ctx,
                                                  const MCSectionELF &Sec) {
   if (OWriter.Relocations[&Sec].empty())
     return nullptr;
 
   const StringRef SectionName = Sec.getName();

llvm/lib/MC/MCObjectStreamer.cpp

void MCObjectStreamer::emitFileDirective(StringRef Filename,
   // TODO: add additional info to integrated assembler.
 }
 
 void MCObjectStreamer::emitAddrsig() {
   getAssembler().getWriter().emitAddrsigSection();
 }
 
 void MCObjectStreamer::emitAddrsigSym(const MCSymbol *Sym) {
-  getAssembler().registerSymbol(*Sym);
   getAssembler().getWriter().addAddrsigSymbol(Sym);
 }
 
 void MCObjectStreamer::finishImpl() {
   getContext().RemapDebugPaths();
 
   // If we are generating dwarf for assembly source files dump out the sections.
   if (getContext().getGenDwarfForAssembly())

llvm/lib/MC/MachObjectWriter.cpp

static MachO::LoadCommandType getLCFromMCVM(MCVersionMinType Type) {
   llvm_unreachable("Invalid mc version min type");
 }
 
 void MachObjectWriter::populateAddrSigSection(MCAssembler &Asm) {
   MCSection *AddrSigSection =
       Asm.getContext().getObjectFileInfo()->getAddrSigSection();
   unsigned Log2Size = is64Bit() ? 3 : 2;
   for (const MCSymbol *S : getAddrsigSyms()) {
+    if (!S->isRegistered())
+      continue;
     MachO::any_relocation_info MRE;
     MRE.r_word0 = 0;
     MRE.r_word1 = (Log2Size << 25) | (MachO::GENERIC_RELOC_VANILLA << 28);
     addRelocation(S, AddrSigSection, MRE);
   }
 }
 
 uint64_t MachObjectWriter::writeObject(MCAssembler &Asm,

llvm/lib/MC/WinCOFFObjectWriter.cpp

uint64_t WinCOFFObjectWriter::writeObject(MCAssembler &Asm,
   }
 
   // Create the contents of the .llvm_addrsig section.
   if (EmitAddrsigSection) {
     auto Frag = new MCDataFragment(AddrsigSection);
     Frag->setLayoutOrder(0);
     raw_svector_ostream OS(Frag->getContents());
     for (const MCSymbol *S : AddrsigSyms) {
+      if (!S->isRegistered())
+        continue;
       if (!S->isTemporary()) {
         encodeULEB128(S->getIndex(), OS);
         continue;
       }
 
       MCSection *TargetSection = &S->getSection();
       assert(SectionMap.find(TargetSection) != SectionMap.end() &&
              "Section must already have been defined in "

llvm/test/MC/COFF/addrsig.s

 // RUN: llvm-mc -filetype=obj -triple x86_64-pc-win32 %s -o - | llvm-readobj -S --symbols --sd --addrsig - | FileCheck %s
 
 // CHECK:      Name: .llvm_addrsig
 // CHECK-NEXT: VirtualSize: 0x0
 // CHECK-NEXT: VirtualAddress: 0x0
 // CHECK-NEXT: RawDataSize: 4
 // CHECK-NEXT: PointerToRawData:
 // CHECK-NEXT: PointerToRelocations: 0x0
 // CHECK-NEXT: PointerToLineNumbers: 0x0
 // CHECK-NEXT: RelocationCount: 0
 // CHECK-NEXT: LineNumberCount: 0
 // CHECK-NEXT: Characteristics [ (0x100800)
 // CHECK-NEXT:   IMAGE_SCN_ALIGN_1BYTES (0x100000)
 // CHECK-NEXT:   IMAGE_SCN_LNK_REMOVE (0x800)
 // CHECK-NEXT: ]
 // CHECK-NEXT: SectionData (
-// CHECK-NEXT:   0000: 080A0B02
+// CHECK-NEXT:   0000: 080B0A02
 // CHECK-NEXT: )
 
 // CHECK: Symbols [
-// CHECK: Name: .text
+// CHECK:      Name:
+// CHECK-SAME: {{^}} .text
 // CHECK: AuxSectionDef
-// CHECK: Name: .data
+// CHECK:      Name:
+// CHECK-SAME: {{^}} .data
 // CHECK: AuxSectionDef
-// CHECK: Name: .bss
+// CHECK:      Name:
+// CHECK-SAME: {{^}} .bss
 // CHECK: AuxSectionDef
-// CHECK: Name: .llvm_addrsig
+// CHECK:      Name:
+// CHECK-SAME: {{^}} .llvm_addrsig
 // CHECK: AuxSectionDef
-// CHECK: Name: g1
-// CHECK: Name: g2
-// CHECK: Name: g3
-// CHECK: Name: local
+// CHECK:      Name:
+// CHECK-SAME: {{^}} g1
+// CHECK:      Name:
+// CHECK-SAME: {{^}} g2
+// CHECK:      Name:
+// CHECK-SAME: {{^}} local
+// CHECK:      Name:
+// CHECK-SAME: {{^}} g3
+// CHECK-NOT:  Name:
+// CHECK: }
 
 // CHECK:      Addrsig [
 // CHECK-NEXT:   Sym: g1 (8)
-// CHECK-NEXT:   Sym: g3 (10)
-// CHECK-NEXT:   Sym: local (11)
+// CHECK-NEXT:   Sym: g3 (11)
+// CHECK-NEXT:   Sym: local (10)
 // CHECK-NEXT:   Sym: .data (2)
 // CHECK-NEXT: ]
 
+.globl g1
+
 .addrsig
 .addrsig_sym g1
 .globl g2
 .addrsig_sym g3
 .addrsig_sym local
 .addrsig_sym .Llocal
+.addrsig_sym .Lunseen
+.addrsig_sym unseen
 
 local:
+.globl g3
 
 .data
 .Llocal:

llvm/test/MC/ELF/addrsig-error.s

-// RUN: not llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o - 2>&1 | FileCheck %s
-// CHECK: Undefined temporary symbol
-
-.addrsig
-.addrsig_sym .Lundef

llvm/test/MC/ELF/addrsig.s

 // CHECK-NEXT:   Size:
 // CHECK-NEXT:   Binding:
 // CHECK-NEXT:   Type:
 // CHECK-NEXT:   Other:
 // CHECK-NEXT:   Section:
 // CHECK-NEXT: }
 // CHECK-NEXT: Symbol {
 // CHECK-NEXT:   Name: g3
+// CHECK-NOT:  Symbol {
 
 // CHECK:      Addrsig [
 // CHECK-NEXT:   Sym: g1 (3)
 // CHECK-NEXT:   Sym: g3 (5)
 // CHECK-NEXT:   Sym: local (2)
 // CHECK-NEXT:   Sym:  (1)
 // CHECK-NEXT: ]
 
+.globl g1
+
 // ASM:      .addrsig
 // ASM-NEXT: .addrsig_sym g1
 .addrsig
 .addrsig_sym g1
 .globl g2
 // ASM:      .addrsig_sym g3
 // ASM-NEXT: .addrsig_sym local
 // ASM-NEXT: .addrsig_sym .Llocal
+// ASM-NEXT: .addrsig_sym .Lunseen
+// ASM-NEXT: .addrsig_sym unseen
 .addrsig_sym g3
 .addrsig_sym local
 .addrsig_sym .Llocal
+.addrsig_sym .Lunseen
+.addrsig_sym unseen
 
 local:
 .Llocal:
 
+.globl g3
+
 // DWO-NOT: .llvm_addrsig

llvm/test/MC/MachO/addrsig.s

 # REQUIRES: aarch64-registered-target
 # RUN: llvm-mc -filetype=obj -triple arm64-apple-darwin %s -o %t
 # RUN: llvm-readobj -r -s %t | FileCheck %s
 
 # CHECK:      Section __llvm_addrsig {
 # CHECK-NEXT:   0x0 0 3 1 ARM64_RELOC_UNSIGNED 0 .Llocal
 # CHECK-NEXT:   0x0 0 3 1 ARM64_RELOC_UNSIGNED 0 local
 # CHECK-NEXT:   0x0 0 3 1 ARM64_RELOC_UNSIGNED 0 g3
 # CHECK-NEXT:   0x0 0 3 1 ARM64_RELOC_UNSIGNED 0 g1
 # CHECK-NEXT: }
 
 # CHECK:      Symbols [
 # CHECK:        Symbol {
 # CHECK-NEXT:     Name: ltmp0
 # CHECK:        Symbol {
 # CHECK-NEXT:     Name: local
 # CHECK:        Symbol {
-# CHECK-NEXT:     Name: .Llocal
-# CHECK:        Symbol {
 # CHECK-NEXT:     Name: ltmp1
 # CHECK:        Symbol {
+# CHECK-NEXT:     Name: .Llocal
+# CHECK:        Symbol {
 # CHECK-NEXT:     Name: g1
 # CHECK:        Symbol {
 # CHECK-NEXT:     Name: g2
 # CHECK:        Symbol {
 # CHECK-NEXT:     Name: g3
 # CHECK-NOT:    Symbol {
 # CHECK:      ]
 
 .globl g1
 
 .addrsig
 .addrsig_sym g1
 .globl g2
 .addrsig_sym g3
 .addrsig_sym local
 .addrsig_sym .Llocal
+.addrsig_sym .Lunseen
+.addrsig_sym unseen
 
 local:
   nop
 .globl g3
 
 .data
 .Llocal:
 
 .subsections_via_symbols