[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

84–85

@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

603

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.

603

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

[int3]

llvm/lib/MC/MCMachOStreamer.cpp

603

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/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/LEB128.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
 #include <vector>
 
 namespace llvm {
 class MCInst;
 class MCStreamer;
 class MCSubtargetInfo;

 void MCMachOStreamer::createAddrSigSection() {
   MCAssembler &Asm = getAssembler();
   MCObjectWriter &writer = Asm.getWriter();
   if (!writer.getEmitAddrsigSection())
     return;
   MCSection *AddrSigSection =
       Asm.getContext().getObjectFileInfo()->getAddrSigSection();
   Asm.registerSection(*AddrSigSection);
-  new MCDataFragment(AddrSigSection);
+  auto *Frag = new MCDataFragment(AddrSigSection);
+  size_t SectionBytes = 0;
+  for (const MCSymbol *sym : writer.getAddrsigSyms())
+    SectionBytes += getULEB128Size(sym->getIndex());
+  Frag->getContents().resize(SectionBytes);
 }

llvm/lib/MC/MachObjectWriter.cpp

void MachObjectWriter::writeAddrsigSection(MCAssembler &Asm) {
   if (!fragmentList.size())
     return;
 
   assert(fragmentList.size() == 1);
   MCFragment *pFragment = &*fragmentList.begin();
   MCDataFragment *pDataFragment = dyn_cast_or_null<MCDataFragment>(pFragment);
   assert(pDataFragment);
 
+  pDataFragment->getContents().clear();
   raw_svector_ostream OS(pDataFragment->getContents());
   for (const MCSymbol *sym : this->getAddrsigSyms())
     encodeULEB128(sym->getIndex(), OS);
 }
 
 uint64_t MachObjectWriter::writeObject(MCAssembler &Asm,
                                        const MCAsmLayout &Layout) {
   uint64_t StartOffset = W.OS.tell();

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-objdump --macho --section-headers %t | FileCheck %s --check-prefix=SECTIONS
 ; RUN: llvm-readobj --hex-dump='__llvm_addrsig' %t | FileCheck %s --check-prefix=OBJ
 
+; 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 [[#%.8x,SIZE:]] [[#%.16x,ADDR:]]
+; SECTIONS-NEXT: __debug_line   {{[[:xdigit:]]+}} [[#%.16x, SIZE+ADDR]]
+
 ; 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

 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: !{})