[MC][MachO] Change addrsig format + ensure its size is properly set

[int3]

Summary

There were two problems with the previous setup:

1. We weren't setting its size, which caused problems when __llvm_addrsig wasn't the last section. In particular, __debug_line (if created) is generated and placed after __llvm_addrsig, and would result in an invalid object file w/ overlapping sections being emitted.

2. The symbol indices could be invalidated if e.g. llvm-strip ran on the object file. See discussion [here][1].

To fix both these issues, we use symbol relocations instead of encoding
symbol indices directly in the section contents. The section itself
doesn't contain any data. That sidesteps the layout problem in addition
to solving the second issue.

The corresponding LLD change to read in this new format: D128938: [lld-macho] Read in new addrsig format.
It will fix the icf-safe.ll test failure on this diff.

[1]: https://discourse.llvm.org/t/problems-with-mach-o-address-significance-table-generation/63392/

[int3]

llvm/test/CodeGen/AArch64/addrsig-macho.ll

91–93

@alx32: What's the purpose of these calls to dbg.value? What does it help us test?

[int3]

new approach

[int3]

use pointer sized relocs

[int3]

llvm/lib/MC/MCMachOStreamer.cpp

608

could use the actual machine pointer size here, but I'm not sure how to check for 64/32 bit elegantly here. MCMachObjectTargetWriter::is64Bit() exists, but that would require downcasting writer here which seems kinda ugly

[steven_wu]

LGTM in general. Some comments inline.

llvm/lib/MC/MCMachOStreamer.cpp

34

This is no longer needed.

608

You can just add is64Bit() to MachObjectWriter which just return TargetObjectWriter->is64Bit().

[int3]

llvm/lib/MC/MCMachOStreamer.cpp

608

Not sure I understand this. MachObjectWriter::is64Bit() already exists; the problem is that I don't seem to have access to MachObjectWriter here without doing a static_cast on writer.

(I know I said MCMachObjectTargetWriter instead of MachObjectWriter in my earlier comment, but I mixed the two up. Regardless they both have an is64Bit method.

[int3]

remove unnecessary include

[int3]

bump. I don't know if there's a simple solution to the is64Bit question, but I don't think it's super important either? At most we're just allocating an additional 4 bytes

[alx32]

LGTM, thanks for doing this !

[int3]

rebase

llvm/include/llvm/MC/MCMachObjectWriter.h

bool isSymbolRefDifferenceFullyResolvedImpl(const MCAssembler &Asm,
                                               const MCSymbol &B,
                                               bool InSet) const override;
 
   bool isSymbolRefDifferenceFullyResolvedImpl(const MCAssembler &Asm,
                                               const MCSymbol &SymA,
                                               const MCFragment &FB, bool InSet,
                                               bool IsPCRel) const override;
 
-  uint64_t writeObject(MCAssembler &Asm, const MCAsmLayout &Layout) override;
+  void populateAddrSigSection(MCAssembler &Asm);
 
-  void writeAddrsigSection(MCAssembler &Asm);
+  uint64_t writeObject(MCAssembler &Asm, const MCAsmLayout &Layout) override;
 };
 
 /// Construct a new Mach-O writer instance.
 ///
 /// This routine takes ownership of the target writer subclass.
 ///
 /// \param MOTW - The target specific Mach-O writer subclass.
 /// \param OS - The stream to write to.
 /// \returns The constructed object writer.
 std::unique_ptr<MCObjectWriter>
 createMachObjectWriter(std::unique_ptr<MCMachObjectTargetWriter> MOTW,
                        raw_pwrite_stream &OS, bool IsLittleEndian);
 
 } // end namespace llvm
 
 #endif // LLVM_MC_MCMACHOBJECTWRITER_H

llvm/lib/MC/MCMachOStreamer.cpp

 #include "llvm/MC/MCSectionMachO.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/MCSymbolMachO.h"
 #include "llvm/MC/MCValue.h"
 #include "llvm/MC/SectionKind.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
 #include <vector>
 
 namespace llvm {
 class MCInst;
 class MCStreamer;
 class MCSubtargetInfo;
 class Triple;
S->emitVersionForTarget(
       Target, Context.getObjectFileInfo()->getSDKVersion(),
       Context.getObjectFileInfo()->getDarwinTargetVariantTriple(),
       Context.getObjectFileInfo()->getDarwinTargetVariantSDKVersion());
   if (RelaxAll)
     S->getAssembler().setRelaxAll(true);
   return S;
 }
 
-// Create the AddrSig section and first data fragment here as its layout needs
-// to be computed immediately after in order for it to be exported correctly.
+// The AddrSig section uses a series of relocations to refer to the symbols that
+// should be considered address-significant. The only interesting content of
+// these relocations is their symbol; the type, length etc will be ignored by
+// the linker. The reason we are not referring to the symbol indices directly is
+// that those indices will be invalidated by tools that update the symbol table.
+// Symbol relocations OTOH will have their indices updated by e.g. llvm-strip.
 void MCMachOStreamer::createAddrSigSection() {
   MCAssembler &Asm = getAssembler();
   MCObjectWriter &writer = Asm.getWriter();
   if (!writer.getEmitAddrsigSection())
     return;
+  // Create the AddrSig section and first data fragment here as its layout needs
+  // to be computed immediately after in order for it to be exported correctly.
   MCSection *AddrSigSection =
       Asm.getContext().getObjectFileInfo()->getAddrSigSection();
   Asm.registerSection(*AddrSigSection);
-  new MCDataFragment(AddrSigSection);
+  auto *Frag = new MCDataFragment(AddrSigSection);
+  // We will generate a series of pointer-sized symbol relocations at offset
+  // 0x0. Set the section size to be large enough to contain a single pointer
+  // (instead of emitting a zero-sized section) so these relocations are
+  // technically valid, even though we don't expect these relocations to
+  // actually be applied by the linker.
+  Frag->getContents().resize(8);
 }

llvm/lib/MC/MachObjectWriter.cpp

static MachO::LoadCommandType getLCFromMCVM(MCVersionMinType Type) {
   case MCVM_OSXVersionMin:     return MachO::LC_VERSION_MIN_MACOSX;
   case MCVM_IOSVersionMin:     return MachO::LC_VERSION_MIN_IPHONEOS;
   case MCVM_TvOSVersionMin:    return MachO::LC_VERSION_MIN_TVOS;
   case MCVM_WatchOSVersionMin: return MachO::LC_VERSION_MIN_WATCHOS;
   }
   llvm_unreachable("Invalid mc version min type");
 }
 
-// Encode addrsig data as symbol indexes in variable length encoding.
-void MachObjectWriter::writeAddrsigSection(MCAssembler &Asm) {
+void MachObjectWriter::populateAddrSigSection(MCAssembler &Asm) {
   MCSection *AddrSigSection =
       Asm.getContext().getObjectFileInfo()->getAddrSigSection();
-  MCSection::FragmentListType &fragmentList = AddrSigSection->getFragmentList();
-  if (!fragmentList.size())
-    return;
-
-  assert(fragmentList.size() == 1);
-  MCFragment *pFragment = &*fragmentList.begin();
-  MCDataFragment *pDataFragment = dyn_cast_or_null<MCDataFragment>(pFragment);
-  assert(pDataFragment);
-
-  raw_svector_ostream OS(pDataFragment->getContents());
-  for (const MCSymbol *sym : this->getAddrsigSyms())
-    encodeULEB128(sym->getIndex(), OS);
+  unsigned Log2Size = is64Bit() ? 3 : 2;
+  for (const MCSymbol *S : getAddrsigSyms()) {
+    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,
                                        const MCAsmLayout &Layout) {
   uint64_t StartOffset = W.OS.tell();
 
+  populateAddrSigSection(Asm);
+
   // Compute symbol table information and bind symbol indices.
   computeSymbolTable(Asm, LocalSymbolData, ExternalSymbolData,
                      UndefinedSymbolData);
-  writeAddrsigSection(Asm);
 
   if (!Asm.CGProfile.empty()) {
     MCSection *CGProfileSection = Asm.getContext().getMachOSection(
         "__LLVM", "__cg_profile", 0, SectionKind::getMetadata());
     MCDataFragment *Frag = dyn_cast_or_null<MCDataFragment>(
         &*CGProfileSection->getFragmentList().begin());
     assert(Frag && "call graph profile section not reserved");
     Frag->getContents().clear();

llvm/test/CodeGen/AArch64/addrsig-macho.ll

 ; RUN: llc -filetype=asm %s -o - -mtriple arm64-apple-ios -addrsig | FileCheck %s
 ; RUN: llc -filetype=obj %s -o %t -mtriple arm64-apple-ios -addrsig
-; RUN: llvm-readobj --hex-dump='__llvm_addrsig' %t | FileCheck %s --check-prefix=OBJ
+; RUN: llvm-objdump --macho --section-headers %t | FileCheck %s --check-prefix=SECTIONS
+; RUN: llvm-objdump --macho --reloc %t | FileCheck %s --check-prefix=RELOCS
 
 ; CHECK:      .addrsig{{$}}
 ; CHECK-NEXT: .addrsig_sym _func03_takeaddr
 ; CHECK-NEXT: .addrsig_sym _f1
 ; CHECK-NEXT: .addrsig_sym _metadata_f2
 ; CHECK-NEXT: .addrsig_sym _result
 ; CHECK-NEXT: .addrsig_sym _g1
 ; CHECK-NEXT: .addrsig_sym _a1
 ; CHECK-NEXT: .addrsig_sym _i1
 
-; OBJ: Hex dump of section '__llvm_addrsig':
-; OBJ-NEXT:0x{{[0-9a-f]*}}
+; The __debug_line section (which should be generated for the given input file)
+; should appear immediately after the addrsig section.  Use it to make sure
+; addrsig's section size has been properly set during section layout. This
+; catches a regression where the next section would overlap addrsig's
+; contents.
+; SECTIONS:      __llvm_addrsig 00000008          [[#%.16x,ADDR:]]
+; SECTIONS-NEXT: __debug_line   {{[[:xdigit:]]+}} [[#%.16x,8+ADDR]]
+
+; RELOCS: Relocation information (__DATA,__llvm_addrsig) 7 entries
+; RELOCS: address  pcrel length extern type    scattered symbolnum/value
+; RELOCS: 00000000 False ?( 3)  True   UNSIGND False     _i1
+; RELOCS: 00000000 False ?( 3)  True   UNSIGND False     _a1
+; RELOCS: 00000000 False ?( 3)  True   UNSIGND False     _g1
+; RELOCS: 00000000 False ?( 3)  True   UNSIGND False     _result
+; RELOCS: 00000000 False ?( 3)  True   UNSIGND False     _metadata_f2
+; RELOCS: 00000000 False ?( 3)  True   UNSIGND False     _f1
+; RELOCS: 00000000 False ?( 3)  True   UNSIGND False     _func03_takeaddr
 
 target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
 target triple = "arm64-apple-ios7.0.0"
 
 @result = global i32 0, align 4
 
 ; Function Attrs: minsize nofree noinline norecurse nounwind optsize ssp uwtable
 define void @func01() local_unnamed_addr #0 {

 entry:
   %0 = load volatile i32, ptr @result, align 4
   %add = add nsw i32 %0, 1
   store volatile i32 %add, ptr @result, align 4
   ret void
 }
 
 ; Function Attrs: minsize nofree noinline norecurse nounwind optsize ssp uwtable
-define void @func03_takeaddr() #0 {
+define void @func03_takeaddr() #0 !dbg !9 {
 entry:
   %0 = load volatile i32, ptr @result, align 4
   %add = add nsw i32 %0, 1
   store volatile i32 %add, ptr @result, align 4
   ret void
 }
 
 ; Function Attrs: minsize nofree norecurse nounwind optsize ssp uwtable
define void()* @f1() {
   %g2 = bitcast i32* @g2 to i8*
   %g3 = bitcast i32* @g3 to i8*
   %dllimport = bitcast i32* @dllimport to i8*
   %tls = bitcast i32* @tls to i8*
   %a1 = bitcast i32* @a1 to i8*
   %a2 = bitcast i32* @a2 to i8*
   %i1 = bitcast void()* @i1 to i8*
   %i2 = bitcast void()* @i2 to i8*
-  call void @llvm.dbg.value(metadata i8* bitcast (void()* @metadata_f1 to i8*), metadata !5, metadata !DIExpression()), !dbg !7
-  call void @llvm.dbg.value(metadata i8* bitcast (void()* @metadata_f2 to i8*), metadata !5, metadata !DIExpression()), !dbg !7
+  call void @llvm.dbg.value(metadata i8* bitcast (void()* @metadata_f1 to i8*), metadata !6, metadata !DIExpression()), !dbg !8
+  call void @llvm.dbg.value(metadata i8* bitcast (void()* @metadata_f2 to i8*), metadata !6, metadata !DIExpression()), !dbg !8
   call void @f4(i8* bitcast (void()* @metadata_f2 to i8*))
   unreachable
 }
 
 declare void @f4(i8*) unnamed_addr
 
 declare void @metadata_f1()
 declare void @metadata_f2()
 

 @a2 = internal local_unnamed_addr alias i32, i32* @g2
 
 @i1 = ifunc void(), void()* ()* @f1
 @i2 = internal local_unnamed_addr ifunc void(), void()* ()* @f2
 
 declare void @llvm.dbg.value(metadata, metadata, metadata)
 
 attributes #0 = { noinline }
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3}
 
-!3 = distinct !DISubprogram(scope: null, isLocal: false, isDefinition: true, isOptimized: false)
-!4 = !DILocation(line: 0, scope: !3)
-!5 = !DILocalVariable(scope: !6)
-!6 = distinct !DISubprogram(scope: null, isLocal: false, isDefinition: true, isOptimized: false)
-!7 = !DILocation(line: 0, scope: !6, inlinedAt: !4)
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, emissionKind: FullDebug)
+!1 = !DIFile(filename: "test.c", directory: "/tmp")
+!2 = !{i32 7, !"Dwarf Version", i32 4}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+
+!4 = distinct !DISubprogram(scope: null, isLocal: false, isDefinition: true, isOptimized: false, unit: !0)
+!5 = !DILocation(line: 0, scope: !4)
+!6 = !DILocalVariable(scope: !7)
+!7 = distinct !DISubprogram(scope: null, isLocal: false, isDefinition: true, isOptimized: false, unit: !0)
+!8 = !DILocation(line: 0, scope: !7, inlinedAt: !5)
+!9 = distinct !DISubprogram(scope: null, file: !1, line: 1, type: !10, unit: !0)
+!10 = !DISubroutineType(types: !{})