[COFF] Keep temporary symbols in object files on ARM64, as IMAGE_SYM_CLASS_LABEL

[mstorsjo]

Summary

On ARM64, many symbol references use an adrp+add/ldr pair. When the
references are against a temporary symbol, the references are
normally rewritten as a reference against the start of the section,
plus an immediate offset.

In the relocation for an adrp instruction, the offset from the
referenced symbol is stored in the opcode, which fits a 21 bit
immediate. Thus, if there's an adrp relocation against a temporary
symbol, it ends up out of range if the temporary symbol is further
in than 1 MB in a section.

This addresses the issue reported at
https://bugs.llvm.org/show_bug.cgi?id=52378. It can also be worked
around by using the options -ffunction-sections and -fdata-sections.

By keeping the temporary symbols in the object files (e.g. as the symbol
type IMAGE_SYM_CLASS_LABEL), the relocations only need to store the offset
from this symbol instead of the start of the section.

This roughly matches what MSVC does, where the produced object files
contain lots of temporary label symbols named like $LN<n>.

This does inflate the size of the intermediate object files somewhat,
but makes these relocations much more robust.

[rnk]

This roughly matches what MSVC does, where the produced object files
contain lots of temporary label symbols named like $LN<n>.

I kind of always thought it was sloppy to emit all these extra compiler-generated symbols, FWIW.

This does inflate the size of the intermediate object files somewhat,
but makes these relocations much more robust.

That is somewhat concerning. LLVM uses a lot of labels internally, I think we should be careful about this change.

Is it possible to relativize these relocations against the most recent previous external symbol instead? Given that the code compiles with function/data sections, that seems like it would work.

[efriedma]

The alternative here is that instead of relying on the symbols defined in the assembler source, we could automatically define a symbol every megabyte. If we expect the linker is throwing away the symbols anyway, though, the only effect on compiler-generated code is the number of symbols in the intermediate object, which isn't a big deal, I guess. We typically generate a lot of code labels, though. Do you have a number for how much we're inflating a typical object file?

[efriedma]

In D113865#3132268, @rnk wrote:

Is it possible to relativize these relocations against the most recent previous external symbol instead? Given that the code compiles with function/data sections, that seems like it would work.

This isn't any guarantee there's an external symbol in range, in general.

[mstorsjo]

In D113865#3132307, @efriedma wrote:

The alternative here is that instead of relying on the symbols defined in the assembler source, we could automatically define a symbol every megabyte. If we expect the linker is throwing away the symbols anyway, though, the only effect on compiler-generated code is the number of symbols in the intermediate object, which isn't a big deal, I guess. We typically generate a lot of code labels, though. Do you have a number for how much we're inflating a typical object file?

I guess it's hard to define what a typical object file is, but from a sample of 5 large-ish and normal sized object files, they grew by 13%, 13%, 10%, 5% and 1%. So clearly notable and not ideal, but not totally out of the question either.

In D113865#3132312, @efriedma wrote:

In D113865#3132268, @rnk wrote:

Is it possible to relativize these relocations against the most recent previous external symbol instead? Given that the code compiles with function/data sections, that seems like it would work.

This isn't any guarantee there's an external symbol in range, in general.

Yeah. I haven't studied the sequence ordering of what's fixed at what point in WinCOFFObjectWriter, but I'm wondering if the layout is known and fixed at the point when deciding whether to define a symbol or not - I'm afraid it isn't. If it was, we could choose to keep temporary symbols only when the previous external symbol is too far away.

The idea of producing extra symbols with 1 MB intervals sounds neat, but I'm wondering if it's possible to do that at the same time while doing choosing what symbols to define. Or maybe we can still add more symbols after fixing the layout? Then that would work.

But then, when doing the loop for fixing relocations, would that increase the complexity of that loop from O(n) into O(n^2), or at least O(n log n), as we have to search for the best symbol as reference point for each of them. Then again, if it only happens when the default choice is too far away, I guess it would be rare?

[efriedma]

It shouldn't be a problem to define additional symbols after layout. "layout" only cares about the contents of the sections.

But then, when doing the loop for fixing relocations, would that increase the complexity of that loop from O(n) into O(n^2), or at least O(n log n), as we have to search for the best symbol as reference point for each of them.

You'd need to binary search or something to find the closest symbol.

If we have a symbol exactly every 1MB, and emit relocations relative to those symbols, we can do a constant-time mapping from offset to symbol. (This might not find the closest symbol if the section has other symbols.)

[mstorsjo]

In D113865#3132841, @efriedma wrote:

It shouldn't be a problem to define additional symbols after layout. "layout" only cares about the contents of the sections.

But then, when doing the loop for fixing relocations, would that increase the complexity of that loop from O(n) into O(n^2), or at least O(n log n), as we have to search for the best symbol as reference point for each of them.

You'd need to binary search or something to find the closest symbol.

If we have a symbol exactly every 1MB, and emit relocations relative to those symbols, we can do a constant-time mapping from offset to symbol. (This might not find the closest symbol if the section has other symbols.)

Ok, this sounds doable. I'll try to give it a shot.

[mstorsjo]

In D113865#3132910, @mstorsjo wrote:

In D113865#3132841, @efriedma wrote:

It shouldn't be a problem to define additional symbols after layout. "layout" only cares about the contents of the sections.

But then, when doing the loop for fixing relocations, would that increase the complexity of that loop from O(n) into O(n^2), or at least O(n log n), as we have to search for the best symbol as reference point for each of them.

You'd need to binary search or something to find the closest symbol.

If we have a symbol exactly every 1MB, and emit relocations relative to those symbols, we can do a constant-time mapping from offset to symbol. (This might not find the closest symbol if the section has other symbols.)

Ok, this sounds doable. I'll try to give it a shot.

This was pretty straightforward, see D114340.

[mstorsjo]

This one was superseded by D114340.

llvm/lib/MC/WinCOFFObjectWriter.cpp

public:
 
   // Maps used during object file creation.
   section_map SectionMap;
   symbol_map SymbolMap;
 
   symbol_list WeakDefaults;
 
   bool UseBigObj;
+  bool KeepTemporarySymbols;
 
   bool EmitAddrsigSection = false;
   MCSectionCOFF *AddrsigSection;
   std::vector<const MCSymbol *> AddrsigSyms;
 
   MCSectionCOFF *CGProfileSection = nullptr;
 
   WinCOFFObjectWriter(std::unique_ptr<MCWinCOFFObjectTargetWriter> MOTW,

 
 //------------------------------------------------------------------------------
 // WinCOFFObjectWriter class implementation
 
 WinCOFFObjectWriter::WinCOFFObjectWriter(
     std::unique_ptr<MCWinCOFFObjectTargetWriter> MOTW, raw_pwrite_stream &OS)
     : W(OS, support::little), TargetObjectWriter(std::move(MOTW)) {
   Header.Machine = TargetObjectWriter->getMachine();
+  KeepTemporarySymbols = Header.Machine == COFF::IMAGE_FILE_MACHINE_ARM64;
 }
 
 COFFSymbol *WinCOFFObjectWriter::createSymbol(StringRef Name) {
   Symbols.push_back(std::make_unique<COFFSymbol>(Name));
   return Symbols.back().get();
 }
 
 COFFSymbol *WinCOFFObjectWriter::GetOrCreateCOFFSymbol(const MCSymbol *Symbol) {
if (Local) {
     Local->Data.Type = SymbolCOFF.getType();
     Local->Data.StorageClass = SymbolCOFF.getClass();
 
     // If no storage class was specified in the streamer, define it here.
     if (Local->Data.StorageClass == COFF::IMAGE_SYM_CLASS_NULL) {
       bool IsExternal = MCSym.isExternal() ||
                         (!MCSym.getFragment() && !MCSym.isVariable());
 
-      Local->Data.StorageClass = IsExternal ? COFF::IMAGE_SYM_CLASS_EXTERNAL
-                                            : COFF::IMAGE_SYM_CLASS_STATIC;
+      Local->Data.StorageClass =
+          IsExternal ? COFF::IMAGE_SYM_CLASS_EXTERNAL
+                     : MCSym.isTemporary() ? COFF::IMAGE_SYM_CLASS_LABEL
     }
   }
 
   Sym->MC = &MCSym;
 }
 
 // Maximum offsets for different string table entry encodings.
 enum : unsigned { Max7DecimalOffset = 9999999U };
void WinCOFFObjectWriter::executePostLayoutBinding(MCAssembler &Asm,
   }
 
   // "Define" each section & symbol. This creates section & symbol
   // entries in the staging area.
   for (const auto &Section : Asm)
     defineSection(static_cast<const MCSectionCOFF &>(Section));
 
   for (const MCSymbol &Symbol : Asm.symbols())
-    if (!Symbol.isTemporary())
+    if (!Symbol.isTemporary() ||
+        (KeepTemporarySymbols && !Symbol.getName().empty()))
       DefineSymbol(Symbol, Asm, Layout);
 }
 
 bool WinCOFFObjectWriter::isSymbolRefDifferenceFullyResolvedImpl(
     const MCAssembler &Asm, const MCSymbol &SymA, const MCFragment &FB,
     bool InSet, bool IsPCRel) const {
   // Don't drop relocations between functions, even if they are in the same text
   // section. Multiple Visual C++ linker features depend on having the
void WinCOFFObjectWriter::recordRelocation(MCAssembler &Asm,
   }
 
   COFFRelocation Reloc;
 
   Reloc.Data.SymbolTableIndex = 0;
   Reloc.Data.VirtualAddress = Layout.getFragmentOffset(Fragment);
 
   // Turn relocations for temporary symbols into section relocations.
-  if (A.isTemporary()) {
+  if (A.isTemporary() && (!KeepTemporarySymbols || A.getName().empty())) {
     MCSection *TargetSection = &A.getSection();
     assert(
         SectionMap.find(TargetSection) != SectionMap.end() &&
         "Section must already have been defined in executePostLayoutBinding!");
     Reloc.Symb = SectionMap[TargetSection]->Symbol;
     FixedValue += Layout.getSymbolOffset(A);
   } else {
     assert(

llvm/test/MC/AArch64/coff-relocations.s

 // RUN: llvm-mc -triple aarch64-windows -filetype obj -o %t.obj %s
 // RUN: llvm-readobj -r %t.obj | FileCheck %s
 // RUN: llvm-objdump -d %t.obj | FileCheck %s --check-prefix=DISASM
 // RUN: llvm-objdump -s %t.obj | FileCheck %s --check-prefix=DATA
+// RUN: llvm-readobj --syms %t.obj | FileCheck %s --check-prefix=SYMBOLS
 
 // IMAGE_REL_ARM64_ADDR32
 .Linfo_foo:
   .asciz "foo"
   .long foo
 
 // IMAGE_REL_ARM64_ADDR32NB
 .long func@IMGREL

 // CHECK: 0x4 IMAGE_REL_ARM64_ADDR32 foo
 // CHECK: 0x8 IMAGE_REL_ARM64_ADDR32NB func
 // CHECK: 0xC IMAGE_REL_ARM64_ADDR64 arr
 // CHECK: 0x14 IMAGE_REL_ARM64_BRANCH26 target
 // CHECK: 0x18 IMAGE_REL_ARM64_PAGEBASE_REL21 foo
 // CHECK: 0x1C IMAGE_REL_ARM64_PAGEOFFSET_12A foo
 // CHECK: 0x20 IMAGE_REL_ARM64_PAGEOFFSET_12L foo
 // CHECK: 0x24 IMAGE_REL_ARM64_PAGEBASE_REL21 bar
-// CHECK: 0x28 IMAGE_REL_ARM64_SECREL .text
+// CHECK: 0x28 IMAGE_REL_ARM64_SECREL .Linfo_bar
 // CHECK: 0x2C IMAGE_REL_ARM64_SECTION func
 // CHECK: 0x30 IMAGE_REL_ARM64_PAGEBASE_REL21 baz
 // CHECK: 0x34 IMAGE_REL_ARM64_PAGEOFFSET_12A foo
 // CHECK: 0x38 IMAGE_REL_ARM64_PAGEOFFSET_12L foo
 // CHECK: 0x3C IMAGE_REL_ARM64_PAGEOFFSET_12L foo
 // CHECK: 0x40 IMAGE_REL_ARM64_SECREL_LOW12A foo
 // CHECK: 0x44 IMAGE_REL_ARM64_SECREL_HIGH12A foo
 // CHECK: 0x48 IMAGE_REL_ARM64_SECREL_LOW12L foo
 // CHECK: 0x4C IMAGE_REL_ARM64_REL21 foo
 // CHECK: 0x50 IMAGE_REL_ARM64_BRANCH19 target
 // CHECK: 0x54 IMAGE_REL_ARM64_BRANCH14 target
 // CHECK:   }
 // CHECK:   Section (4) .rdata {
-// CHECK: 0x0 IMAGE_REL_ARM64_REL32 .text
-// CHECK: 0x4 IMAGE_REL_ARM64_REL32 .text
-// CHECK: 0x8 IMAGE_REL_ARM64_REL32 .text
+// CHECK: 0x0 IMAGE_REL_ARM64_REL32 .Linfo_bar
+// CHECK: 0x4 IMAGE_REL_ARM64_REL32 .Linfo_foo
+// CHECK: 0x8 IMAGE_REL_ARM64_REL32 .Linfo_foo
 // CHECK:   }
 // CHECK: ]
 
 // DISASM: 30:       20 1a 09 b0     adrp    x0, 0x12345000
 // DISASM: 34:       00 14 0d 91     add     x0, x0, #837
 // DISASM: 38:       00 14 4d 39     ldrb    w0, [x0, #837]
 // DISASM: 3c:       00 a4 41 f9     ldr     x0, [x0, #840]
 // DISASM: 40:       00 00 00 91     add     x0, x0, #0
 // DISASM: 44:       00 00 40 91     add     x0, x0, #0, lsl #12
 // DISASM: 48:       00 00 40 f9     ldr     x0, [x0]
 // DISASM: 4c:       20 1a 09 30     adr     x0, #74565
 
 // DATA: Contents of section .rdata:
-// DATA-NEXT:  0000 30000000 08000000
+// DATA-NEXT:  0000 04000000 08000000
+
+// SYMBOLS:      Symbol {
+// SYMBOLS:        Name: .Linfo_foo
+// SYMBOLS-NEXT:   Value: 0
+// SYMBOLS-NEXT:   Section: .text
+// SYMBOLS-NEXT:   BaseType: Null
+// SYMBOLS-NEXT:   ComplexType: Null
+// SYMBOLS-NEXT:   StorageClass: Label
+// SYMBOLS-NEXT:   AuxSymbolCount: 0
+// SYMBOLS-NEXT: }