Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MC] Rename temporary symbols of empty name to ".L0 " #89693

Merged

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Apr 23, 2024

Temporary symbols generated for .eh_frame and .debug_line have an empty
name, which appear in .symtab in the presence of RISC-V style linker
relaxation and will not be discarded by ld/objcopy --discard-locals
(-X).

In contrast, GNU assembler's riscv port assigns a fake name ".L0 " (with
a trailing space) to these symbols so that will be discarded by
ld/objcopy --discard-locals.

This patch matches the GNU behavior. Since Clang's RISC-V targets pass
-X to ld, and GNU ld defaults to -X for RISC-V targets, these ".L0 "
symbols will be discarded after linking by default, as expected by
users.

The llvm-symbolizer special case for RISC-V SF_FormatSpecific symbols
https://reviews.llvm.org/D98669 needs to be adjusted.

Note: "": in assembly currently crashes.

Created using spr 1.3.5-bogner
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 23, 2024

@llvm/pr-subscribers-debuginfo
@llvm/pr-subscribers-lld-elf
@llvm/pr-subscribers-lld

@llvm/pr-subscribers-backend-risc-v

Author: Fangrui Song (MaskRay)

Changes

Temporary symbols generated for .eh_frame and .debug_line have an empty
name, which will not be discarded by ld/objcopy --discard-locals (-X)

In contrast, GNU assembler assigns a fake name ".L0" to these symbols
so that will be discarded by ld/objcopy --discard-locals.

This patch matches the GNU behavior. Since Clang's RISC-V targets pass
-X to ld, and GNU ld defaults to -X for RISC-V targets, these ".L0"
symbols will be discarded after linking by default, as expected by
users.

The llvm-symbolizer special case for RISC-V SF_FormatSpecific symbols
https://reviews.llvm.org/D98669 needs to be adjusted.

Note: "": in assembly currently crashes.


Full diff: https://github.com/llvm/llvm-project/pull/89693.diff

13 Files Affected:

  • (modified) lld/test/ELF/mips-eh_frame-pic.s (+2-2)
  • (modified) llvm/include/llvm/Object/ELFObjectFile.h (+1-1)
  • (modified) llvm/lib/MC/ELFObjectWriter.cpp (+6)
  • (modified) llvm/test/CodeGen/RISCV/fixups-diff.ll (+1-1)
  • (modified) llvm/test/DebugInfo/LoongArch/dwarf-loongarch-relocs.ll (+9-9)
  • (modified) llvm/test/DebugInfo/RISCV/dwarf-riscv-relocs.ll (+6-6)
  • (modified) llvm/test/DebugInfo/RISCV/relax-debug-frame.ll (+5-5)
  • (renamed) llvm/test/DebugInfo/Symbolize/ELF/riscv-temporary-symbol.s (+3-2)
  • (modified) llvm/test/ExecutionEngine/JITLink/RISCV/anonymous_symbol.s (+1-1)
  • (modified) llvm/test/MC/ELF/RISCV/gen-dwarf.s (+13-13)
  • (modified) llvm/test/MC/RISCV/cfi-advance.s (+26-6)
  • (modified) llvm/test/MC/RISCV/fde-reloc.s (+1-1)
  • (modified) llvm/test/MC/RISCV/scoped-relaxation.s (+3-3)
diff --git a/lld/test/ELF/mips-eh_frame-pic.s b/lld/test/ELF/mips-eh_frame-pic.s
index a84c36b0e5ecdb..c04dbdf57b08ad 100644
--- a/lld/test/ELF/mips-eh_frame-pic.s
+++ b/lld/test/ELF/mips-eh_frame-pic.s
@@ -36,8 +36,8 @@
 # RELOCS:            .rel{{a?}}.eh_frame {
 # ABS32-RELOCS-NEXT:   0x1C R_MIPS_32 .text
 # ABS64-RELOCS-NEXT:   0x1C R_MIPS_64/R_MIPS_NONE/R_MIPS_NONE .text
-# PIC64-RELOCS-NEXT:   0x1C R_MIPS_PC32/R_MIPS_NONE/R_MIPS_NONE <null>
-# PIC32-RELOCS-NEXT:   0x1C R_MIPS_PC32 <null>
+# PIC64-RELOCS-NEXT:   0x1C R_MIPS_PC32/R_MIPS_NONE/R_MIPS_NONE .L0
+# PIC32-RELOCS-NEXT:   0x1C R_MIPS_PC32 .L0
 # RELOCS-NEXT:       }
 
 # ABS64-EH-FRAME: Augmentation data: 0C
diff --git a/llvm/include/llvm/Object/ELFObjectFile.h b/llvm/include/llvm/Object/ELFObjectFile.h
index 1d457be93741f2..9bd258ddd875fd 100644
--- a/llvm/include/llvm/Object/ELFObjectFile.h
+++ b/llvm/include/llvm/Object/ELFObjectFile.h
@@ -803,7 +803,7 @@ Expected<uint32_t> ELFObjectFile<ELFT>::getSymbolFlags(DataRefImpl Sym) const {
       StringRef Name = *NameOrErr;
       // Mark empty name symbols (used for label differences) and mapping
       // symbols.
-      if (Name.empty() || Name.starts_with("$d") || Name.starts_with("$x"))
+      if (Name == ".L0" || Name.starts_with("$d") || Name.starts_with("$x"))
         Result |= SymbolRef::SF_FormatSpecific;
     } else {
       // TODO: Actually report errors helpfully.
diff --git a/llvm/lib/MC/ELFObjectWriter.cpp b/llvm/lib/MC/ELFObjectWriter.cpp
index 005521bad6e014..3f62bf90cc10b9 100644
--- a/llvm/lib/MC/ELFObjectWriter.cpp
+++ b/llvm/lib/MC/ELFObjectWriter.cpp
@@ -725,7 +725,13 @@ void ELFWriter::computeSymbolTable(
         HasLargeSectionIndex = true;
     }
 
+    // Temporary symbols generated for certain assembler features (.eh_frame,
+    // .debug_line) of an empty name may be referenced by relocations due to
+    // linker relaxation. Rename them to ".L0" to match the gas fake label name
+    // and allow ld/objcopy --discard-locals to discard such symbols.
     StringRef Name = Symbol.getName();
+    if (Name.empty())
+      Name = ".L0";
 
     // Sections have their own string table
     if (Symbol.getType() != ELF::STT_SECTION) {
diff --git a/llvm/test/CodeGen/RISCV/fixups-diff.ll b/llvm/test/CodeGen/RISCV/fixups-diff.ll
index cc1c87b1fe377f..a9043a75452623 100644
--- a/llvm/test/CodeGen/RISCV/fixups-diff.ll
+++ b/llvm/test/CodeGen/RISCV/fixups-diff.ll
@@ -27,7 +27,7 @@ entry:
 ; CHECK:      }
 
 ; CHECK:      Section {{.*}} .rela.eh_frame {
-; CHECK-NEXT:   0x1C R_RISCV_32_PCREL <null> 0x0
+; CHECK-NEXT:   0x1C R_RISCV_32_PCREL .L0 0x0
 ; CHECK-NEXT: }
 
 !llvm.dbg.cu = !{!0}
diff --git a/llvm/test/DebugInfo/LoongArch/dwarf-loongarch-relocs.ll b/llvm/test/DebugInfo/LoongArch/dwarf-loongarch-relocs.ll
index d6a1d8d6e1366f..2076530f4f82d7 100644
--- a/llvm/test/DebugInfo/LoongArch/dwarf-loongarch-relocs.ll
+++ b/llvm/test/DebugInfo/LoongArch/dwarf-loongarch-relocs.ll
@@ -18,21 +18,21 @@
 ; RELOCS-BOTH:         Section ({{.*}}) .rela.debug_frame {
 ; RELOCS-NORL-NEXT:      0x1C R_LARCH_32 .debug_frame 0x0
 ; RELOCS-NORL-NEXT:      0x20 R_LARCH_64 .text 0x0
-; RELOCS-ENRL-NEXT:      0x1C R_LARCH_32 <null> 0x0
-; RELOCS-ENRL-NEXT:      0x20 R_LARCH_64 <null> 0x0
-; RELOCS-ENRL-NEXT:      0x28 R_LARCH_ADD64 <null> 0x0
-; RELOCS-ENRL-NEXT:      0x28 R_LARCH_SUB64 <null> 0x0
-; RELOCS-ENRL-NEXT:      0x3F R_LARCH_ADD6 <null> 0x0
-; RELOCS-ENRL-NEXT:      0x3F R_LARCH_SUB6 <null> 0x0
+; RELOCS-ENRL-NEXT:      0x1C R_LARCH_32 .L0 0x0
+; RELOCS-ENRL-NEXT:      0x20 R_LARCH_64 .L0 0x0
+; RELOCS-ENRL-NEXT:      0x28 R_LARCH_ADD64 .L0 0x0
+; RELOCS-ENRL-NEXT:      0x28 R_LARCH_SUB64 .L0 0x0
+; RELOCS-ENRL-NEXT:      0x3F R_LARCH_ADD6 .L0 0x0
+; RELOCS-ENRL-NEXT:      0x3F R_LARCH_SUB6 .L0 0x0
 ; RELOCS-BOTH-NEXT:    }
 ; RELOCS-BOTH:         Section ({{.*}}) .rela.debug_line {
 ; RELOCS-BOTH-NEXT:      0x22 R_LARCH_32 .debug_line_str 0x0
 ; RELOCS-BOTH-NEXT:      0x31 R_LARCH_32 .debug_line_str 0x2
 ; RELOCS-BOTH-NEXT:      0x46 R_LARCH_32 .debug_line_str 0x1B
 ; RELOCS-NORL-NEXT:      0x4F R_LARCH_64 .text 0x0
-; RELOCS-ENRL-NEXT:      0x4F R_LARCH_64 <null> 0x0
-; RELOCS-ENRL-NEXT:      0x5F R_LARCH_ADD16 <null> 0x0
-; RELOCS-ENRL-NEXT:      0x5F R_LARCH_SUB16 <null> 0x0
+; RELOCS-ENRL-NEXT:      0x4F R_LARCH_64 .L0 0x0
+; RELOCS-ENRL-NEXT:      0x5F R_LARCH_ADD16 .L0 0x0
+; RELOCS-ENRL-NEXT:      0x5F R_LARCH_SUB16 .L0 0x0
 ; RELOCS-BOTH-NEXT:    }
 ; RELOCS-BOTH-NEXT:  ]
 
diff --git a/llvm/test/DebugInfo/RISCV/dwarf-riscv-relocs.ll b/llvm/test/DebugInfo/RISCV/dwarf-riscv-relocs.ll
index e5de1713f4e00d..808c462f9f3bfd 100644
--- a/llvm/test/DebugInfo/RISCV/dwarf-riscv-relocs.ll
+++ b/llvm/test/DebugInfo/RISCV/dwarf-riscv-relocs.ll
@@ -6,14 +6,14 @@
 
 ; Check that we actually have relocations, otherwise this is kind of pointless.
 ; READOBJ-RELOCS:  Section ({{.*}}) .rela.debug_info {
-; READOBJ-RELOCS:    0x1B R_RISCV_ADD32 <null> 0x0
-; READOBJ-RELOCS-NEXT:    0x1B R_RISCV_SUB32 <null> 0x0
+; READOBJ-RELOCS:    0x1B R_RISCV_ADD32 .L0 0x0
+; READOBJ-RELOCS-NEXT:    0x1B R_RISCV_SUB32 .L0 0x0
 ; READOBJ-RELOCS:  Section ({{.*}}) .rela.debug_frame {
-; READOBJ-RELOCS:    0x20 R_RISCV_ADD32 <null> 0x0
-; READOBJ-RELOCS-NEXT:    0x20 R_RISCV_SUB32 <null> 0x0
+; READOBJ-RELOCS:    0x20 R_RISCV_ADD32 .L0 0x0
+; READOBJ-RELOCS-NEXT:    0x20 R_RISCV_SUB32 .L0 0x0
 ; READOBJ-RELOCS:  Section ({{.*}}) .rela.debug_line {
-; READOBJ-RELOCS:    0x5A R_RISCV_ADD16 <null> 0x0
-; READOBJ-RELOCS-NEXT:    0x5A R_RISCV_SUB16 <null> 0x0
+; READOBJ-RELOCS:    0x5A R_RISCV_ADD16 .L0 0x0
+; READOBJ-RELOCS-NEXT:    0x5A R_RISCV_SUB16 .L0 0x0
 
 ; Check that we can print the source, even with relocations.
 ; OBJDUMP-SOURCE: Disassembly of section .text:
diff --git a/llvm/test/DebugInfo/RISCV/relax-debug-frame.ll b/llvm/test/DebugInfo/RISCV/relax-debug-frame.ll
index f655a7c0a7ef42..36d23557f7400d 100644
--- a/llvm/test/DebugInfo/RISCV/relax-debug-frame.ll
+++ b/llvm/test/DebugInfo/RISCV/relax-debug-frame.ll
@@ -4,11 +4,11 @@
 ; RUN:     | FileCheck -check-prefix=RELAX-DWARFDUMP %s
 ;
 ; RELAX:      Section ({{.*}}) .rela.eh_frame {
-; RELAX-NEXT:   0x1C R_RISCV_32_PCREL <null> 0x0
-; RELAX-NEXT:   0x30 R_RISCV_32_PCREL <null> 0x0
-; RELAX-NEXT:   0x44 R_RISCV_32_PCREL <null> 0x0
-; RELAX-NEXT:   0x48 R_RISCV_ADD32 <null> 0x0
-; RELAX-NEXT:   0x48 R_RISCV_SUB32 <null> 0x0
+; RELAX-NEXT:   0x1C R_RISCV_32_PCREL .L0 0x0
+; RELAX-NEXT:   0x30 R_RISCV_32_PCREL .L0 0x0
+; RELAX-NEXT:   0x44 R_RISCV_32_PCREL .L0 0x0
+; RELAX-NEXT:   0x48 R_RISCV_ADD32 .L0 0x0
+; RELAX-NEXT:   0x48 R_RISCV_SUB32 .L0 0x0
 ; RELAX-NEXT:  }
 
 ; RELAX-DWARFDUMP-NOT: error: failed to compute relocation
diff --git a/llvm/test/DebugInfo/Symbolize/ELF/riscv-empty-name-symbol.s b/llvm/test/DebugInfo/Symbolize/ELF/riscv-temporary-symbol.s
similarity index 71%
rename from llvm/test/DebugInfo/Symbolize/ELF/riscv-empty-name-symbol.s
rename to llvm/test/DebugInfo/Symbolize/ELF/riscv-temporary-symbol.s
index 1e0fa8a3061830..6b003274e2d505 100644
--- a/llvm/test/DebugInfo/Symbolize/ELF/riscv-empty-name-symbol.s
+++ b/llvm/test/DebugInfo/Symbolize/ELF/riscv-temporary-symbol.s
@@ -1,10 +1,11 @@
 # REQUIRES: riscv-registered-target
-## Ignore empty name symbols.
+## Ignore .L0 symbols that are generated by LLVM integrated assembler and GNU
+## assembler for .debug_line/.eh_frame related assembler directives.
 
 # RUN: llvm-mc -filetype=obj -triple=riscv64 %s -o %t
 # RUN: llvm-readelf -s %t | FileCheck %s --check-prefix=SYM
 
-# SYM: 0000000000000004  0 NOTYPE LOCAL  DEFAULT [[#]] {{$}}
+# SYM: 0000000000000004  0 NOTYPE LOCAL  DEFAULT [[#]] .L0{{$}}
 # SYM: 0000000000000000  0 NOTYPE GLOBAL DEFAULT [[#]] foo
 
 ## Make sure we test at an address larger than or equal to an empty name symbol.
diff --git a/llvm/test/ExecutionEngine/JITLink/RISCV/anonymous_symbol.s b/llvm/test/ExecutionEngine/JITLink/RISCV/anonymous_symbol.s
index a5038022dfe0c3..9393290f0bd432 100644
--- a/llvm/test/ExecutionEngine/JITLink/RISCV/anonymous_symbol.s
+++ b/llvm/test/ExecutionEngine/JITLink/RISCV/anonymous_symbol.s
@@ -7,7 +7,7 @@
 # the section start and section end. So that by relocating these symbol, the section length
 # can be calculated.
 #
-# CHECK: Creating defined graph symbol for ELF symbol ""
+# CHECK: Creating defined graph symbol for ELF symbol ".L0"
 # CHECK: Creating defined graph symbol for ELF symbol "main"
         .text
         .globl main
diff --git a/llvm/test/MC/ELF/RISCV/gen-dwarf.s b/llvm/test/MC/ELF/RISCV/gen-dwarf.s
index 342ed1cc0e7ef9..8e0aca01f1b212 100644
--- a/llvm/test/MC/ELF/RISCV/gen-dwarf.s
+++ b/llvm/test/MC/ELF/RISCV/gen-dwarf.s
@@ -40,28 +40,28 @@
 # CHECK-NEXT: 0x00000020: [DW_RLE_end_of_list ]
 
 # RELOC:      Section ([[#]]) .rela.eh_frame {
-# RELOC-NEXT:   0x1C R_RISCV_32_PCREL <null> 0x0
-# RELOC-NEXT:   0x20 R_RISCV_ADD32 <null> 0x0
-# RELOC-NEXT:   0x20 R_RISCV_SUB32 <null> 0x0
-# RELOC-NEXT:   0x25 R_RISCV_SET6 <null> 0x0
-# RELOC-NEXT:   0x25 R_RISCV_SUB6 <null> 0x0
-# RELOC-NEXT:   0x34 R_RISCV_32_PCREL <null> 0x0
+# RELOC-NEXT:   0x1C R_RISCV_32_PCREL .L0 0x0
+# RELOC-NEXT:   0x20 R_RISCV_ADD32 .L0 0x0
+# RELOC-NEXT:   0x20 R_RISCV_SUB32 .L0 0x0
+# RELOC-NEXT:   0x25 R_RISCV_SET6 .L0 0x0
+# RELOC-NEXT:   0x25 R_RISCV_SUB6 .L0 0x0
+# RELOC-NEXT:   0x34 R_RISCV_32_PCREL .L0 0x0
 # RELOC-NEXT: }
 
 # RELOC:      Section ([[#]]) .rela.debug_rnglists {
 # RELOC-NEXT:   0xD R_RISCV_64 .text.foo 0x0
-# RELOC-NEXT:   0x15 R_RISCV_SET_ULEB128 <null> 0x0
+# RELOC-NEXT:   0x15 R_RISCV_SET_ULEB128 .L0 0x0
 # RELOC-NEXT:   0x15 R_RISCV_SUB_ULEB128 .text.foo 0x0
 # RELOC-NEXT:   0x17 R_RISCV_64 .text.bar 0x0
 # RELOC-NEXT: }
 
 # RELOC:      Section ([[#]]) .rela.debug_line {
-# RELOC:        R_RISCV_ADD16 <null> 0x0
-# RELOC-NEXT:   R_RISCV_SUB16 <null> 0x0
-# RELOC-NEXT:   R_RISCV_ADD16 <null> 0x0
-# RELOC-NEXT:   R_RISCV_SUB16 <null> 0x0
-# RELOC-NEXT:   R_RISCV_ADD16 <null> 0x0
-# RELOC-NEXT:   R_RISCV_SUB16 <null> 0x0
+# RELOC:        R_RISCV_ADD16 .L0 0x0
+# RELOC-NEXT:   R_RISCV_SUB16 .L0 0x0
+# RELOC-NEXT:   R_RISCV_ADD16 .L0 0x0
+# RELOC-NEXT:   R_RISCV_SUB16 .L0 0x0
+# RELOC-NEXT:   R_RISCV_ADD16 .L0 0x0
+# RELOC-NEXT:   R_RISCV_SUB16 .L0 0x0
 # RELOC:      }
 
 # RELOC:      Hex dump of section '.eh_frame':
diff --git a/llvm/test/MC/RISCV/cfi-advance.s b/llvm/test/MC/RISCV/cfi-advance.s
index c4af390be757da..00af9a50eafa65 100644
--- a/llvm/test/MC/RISCV/cfi-advance.s
+++ b/llvm/test/MC/RISCV/cfi-advance.s
@@ -1,13 +1,27 @@
 # RUN: llvm-mc -filetype=obj -triple riscv32 %s -o %t.o  
-# RUN: llvm-readobj -r %t.o | FileCheck -check-prefix=CHECK %s
+# RUN: llvm-readelf -sr %t.o | FileCheck %s
 # RUN: llvm-dwarfdump --debug-frame %t.o 2>&1 \
 # RUN:     | FileCheck -check-prefix=CHECK-DWARFDUMP %s
 
-# CHECK:      .rela.eh_frame {
-# CHECK-NEXT:   0x1C R_RISCV_32_PCREL <null> 0x0
-# CHECK-NEXT:   0x35 R_RISCV_SET6 <null> 0x0
-# CHECK-NEXT:   0x35 R_RISCV_SUB6 <null> 0x0
-# CHECK-NEXT: }
+
+# CHECK:      Relocation section '.rela.text1' at offset {{.*}} contains 1 entries:
+# CHECK-NEXT:  Offset     Info    Type                Sym. Value  Symbol's Name + Addend
+# CHECK-NEXT: 00000000  00000313 R_RISCV_CALL_PLT       00000004   .L0 + 0
+# CHECK-EMPTY:
+# CHECK-NEXT: Relocation section '.rela.eh_frame' at offset {{.*}} contains 3 entries:
+# CHECK:       Offset     Info    Type                Sym. Value  Symbol's Name + Addend
+# CHECK-NEXT: 0000001c  00000139 R_RISCV_32_PCREL       00000000   .L0 + 0
+# CHECK-NEXT: 00000035  00000b35 R_RISCV_SET6           00010178   .L0 + 0
+# CHECK-NEXT: 00000035  00000934 R_RISCV_SUB6           0001016e   .L0 + 0
+# CHECK-EMPTY:
+# CHECK:      Symbol table '.symtab' contains 15 entries:
+# CHECK-NEXT:    Num:    Value  Size Type    Bind   Vis       Ndx Name
+# CHECK-NEXT:      0: 00000000     0 NOTYPE  LOCAL  DEFAULT   UND
+# CHECK-NEXT:      1: 00000000     0 NOTYPE  LOCAL  DEFAULT     2 .L0
+# CHECK:           3: 00000004     0 NOTYPE  LOCAL  DEFAULT     2 .L0
+# CHECK:           9: 0001016e     0 NOTYPE  LOCAL  DEFAULT     2 .L0
+# CHECK:          11: 00010178     0 NOTYPE  LOCAL  DEFAULT     2 .L0
+
 # CHECK-DWARFDUMP: DW_CFA_advance_loc1: 104
 # CHECK-DWARFDUMP-NEXT: DW_CFA_def_cfa_offset: +8
 # CHECK-DWARFDUMP-NEXT: DW_CFA_advance_loc2: 259
@@ -23,6 +37,9 @@
 test:
         .cfi_startproc
         nop
+## Even if the label shares the name with temporary symbols generated for .eh_frame,
+## the assembler does not conflate it with temporary symbols.
+.L0:
         .zero 100, 0x90
         .cfi_def_cfa_offset 8
         nop
@@ -36,3 +53,6 @@ test:
         .cfi_def_cfa_offset 8
         nop
         .cfi_endproc
+
+.section .text1,"ax"
+call .L0
diff --git a/llvm/test/MC/RISCV/fde-reloc.s b/llvm/test/MC/RISCV/fde-reloc.s
index 1db8929e074703..d3bcedfa876b9c 100644
--- a/llvm/test/MC/RISCV/fde-reloc.s
+++ b/llvm/test/MC/RISCV/fde-reloc.s
@@ -12,7 +12,7 @@ func:
 	.cfi_endproc
 
 # CHECK:   Section (4) .rela.eh_frame {
-# CHECK-NEXT:   0x1C R_RISCV_32_PCREL <null> 0x0
+# CHECK-NEXT:   0x1C R_RISCV_32_PCREL .L0 0x0
 # CHECK-NEXT: }
 # CHECK:      Hex dump of section '.eh_frame':
 # CHECK-NEXT: 0x00000000 10000000 00000000 017a5200 017c0101
diff --git a/llvm/test/MC/RISCV/scoped-relaxation.s b/llvm/test/MC/RISCV/scoped-relaxation.s
index 0b797ee5aca5eb..3b1fcf21f29537 100644
--- a/llvm/test/MC/RISCV/scoped-relaxation.s
+++ b/llvm/test/MC/RISCV/scoped-relaxation.s
@@ -9,7 +9,7 @@
 .dword function - .
 
 # CHECK: 0x0 R_RISCV_ADD64 function 0x0
-# CHECK-NEXT: 0x0 R_RISCV_SUB64 <null> 0x0
+# CHECK-NEXT: 0x0 R_RISCV_SUB64 .L0 0x0
 
 # Relaxed reference, this will resolve to a pair of `RISCV_ADD64` and
 # `RISCV_SUB64` relocation.
@@ -19,7 +19,7 @@
 .option pop
 
 # CHECK: 0x8 R_RISCV_ADD64 function 0x0
-# CHECK-NEXT: 0x8 R_RISCV_SUB64 <null> 0x0
+# CHECK-NEXT: 0x8 R_RISCV_SUB64 .L0 0x0
 
 # Unrelaxed reference, this will resolve to a pair of `RISCV_ADD64` and
 # `RISCV_SUB64` relocation due to relaxation being sticky to the file.
@@ -29,6 +29,6 @@
 .option pop
 
 # CHECK: 0x10 R_RISCV_ADD64 function 0x0
-# CHECK-NEXT: 0x10 R_RISCV_SUB64 <null> 0x0
+# CHECK-NEXT: 0x10 R_RISCV_SUB64 .L0 0x0
 
 # CHECK: }

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Fixes" link to the issue?

LGTM in principle, but might want another review from someone with a little more knowledge of e.g. RISC-V.

StringRef Name = Symbol.getName();
if (Name.empty())
Name = ".L0";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, interesting point. If these symbols are "discarded after linking by default", is that symbol_is_valid filtering relevant only when the defaults are overridden and they are not being discarded?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out. I invoked readelf -x .strtab a.o but did not read the output carefully to notice the space.
Switched to .L0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that fake labels .L0\x01 for other ports cannot be retained in .symtab . Even if they do, LLVM likely doesn't want to customize on this.

Created using spr 1.3.5-bogner
@MaskRay MaskRay changed the title [MC] Rename temporary symbols of empty name to ".L0" [MC] Rename temporary symbols of empty name to ".L0 " Apr 23, 2024
Created using spr 1.3.5-bogner
@MaskRay MaskRay merged commit 96c45a7 into main Apr 24, 2024
3 of 4 checks passed
@MaskRay MaskRay deleted the users/MaskRay/spr/mc-rename-temporary-symbols-of-empty-name-to-l0 branch April 24, 2024 20:16
joker-eph added a commit that referenced this pull request Apr 24, 2024
joker-eph added a commit that referenced this pull request Apr 24, 2024
Reverts #89693

This broke the premerge bot (bolt tests failing)
MaskRay added a commit that referenced this pull request Apr 24, 2024
Temporary symbols generated for .eh_frame and .debug_line have an empty
name, which appear in .symtab in the presence of RISC-V style linker
relaxation and will not be discarded by ld/objcopy --discard-locals
(-X).

In contrast, GNU assembler's riscv port assigns a fake name ".L0 " (with
a trailing space) to these symbols so that will be discarded by
ld/objcopy --discard-locals.

This patch matches the GNU behavior. Since Clang's RISC-V targets pass
-X to ld, and GNU ld defaults to -X for RISC-V targets, these ".L0 "
symbols will be discarded after linking by default, as expected by
users.

The llvm-symbolizer special case for RISC-V `SF_FormatSpecific` symbols
https://reviews.llvm.org/D98669 needs to be adjusted.

Note: `"":` in assembly currently crashes.
aaupov added a commit that referenced this pull request Apr 26, 2024
MaskRay added a commit that referenced this pull request Apr 26, 2024
The special case is unneeded after #89693.

Pull Request: #90004
@joker-eph
Copy link
Collaborator

The revert of this PR broke the test and somehow this all conflicted with #68977 ; can you all figure it out @aaupov / @MaskRay ?

The premerge is still broken right now because of this.

@aaupov
Copy link
Contributor

aaupov commented Apr 26, 2024

@MaskRay – can this be relanded now?

MaskRay added a commit that referenced this pull request Apr 26, 2024
Temporary symbols generated for .eh_frame and .debug_line have an empty
name, which appear in .symtab in the presence of RISC-V style linker
relaxation and will not be discarded by ld/objcopy --discard-locals
(-X).

In contrast, GNU assembler's riscv port assigns a fake name ".L0 " (with
a trailing space) to these symbols so that will be discarded by
ld/objcopy --discard-locals.

This patch matches the GNU behavior. Since Clang's RISC-V targets pass
-X to ld, and GNU ld defaults to -X for RISC-V targets, these ".L0 "
symbols will be discarded after linking by default, as expected by
users.

The llvm-symbolizer special case for RISC-V `SF_FormatSpecific` symbols
https://reviews.llvm.org/D98669 needs to be adjusted.

Note: `"":` in assembly currently crashes.

Note: bolt tests used /usr/bin/clang before
llvmorg-19-init-9532-g59bfc3106874.
The revert llvmorg-19-init-9531-g28b55342e1a8 actually broke
bolt/test/RISCV/fake-label-no-entry.c
@MaskRay
Copy link
Member Author

MaskRay commented Apr 26, 2024

@MaskRay – can this be relanded now?

I relanded this patch as it should actually fix bolt/test/RISCV/fake-label-no-entry.c.

bolt/test/RISCV/fake-label-no-entry.c is too sensitive to clang versions. It can probably be removed if we still see failures.
The original purpose that bolt/test/RISCV/fake-label-no-entry.c was contributed/worked around was properly fixed by this patch anyway.

@MaskRay
Copy link
Member Author

MaskRay commented Apr 26, 2024

@aaupov

IIUC the issue was that premerge bots and https://lab.llvm.org/buildbot/#/builders/272 disagreed on what clang to use.
Before 59bfc31 [CI] Use trunk Clang in BOLT testing, I saw /usr/bin/clang used in one and path/to/build/bin/clang in another.

This discrepancy would cause bolt/test/RISCV/fake-label-no-entry.c to fail in one of the bots.

We should remove these /usr/bin/clang settings as otherwise the bolt tests would be too sensitive to LLVMMC changes.

@aaupov
Copy link
Contributor

aaupov commented Apr 26, 2024

@aaupov

IIUC the issue was that premerge bots and https://lab.llvm.org/buildbot/#/builders/272 disagreed on what clang to use. Before 59bfc31 [CI] Use trunk Clang in BOLT testing, I saw /usr/bin/clang used in one and path/to/build/bin/clang in another.

This discrepancy would cause bolt/test/RISCV/fake-label-no-entry.c to fail in one of the bots.

We should remove these /usr/bin/clang settings as otherwise the bolt tests would be too sensitive to LLVMMC changes.

It should no longer be used in pre-merge checks, and PR linked above should eliminate the uses on buildbots.

aaupov added a commit to llvm/llvm-zorg that referenced this pull request Apr 26, 2024
As a follow-up to the issue unearthed by
llvm/llvm-project#89693, remove all uses of
BOLT_CLANG_EXE to prevent similar blockers in the future and reduce
builder
maintenance overhead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants