[llvm-objcopy,ELF] --discard-locals/--discard-all: allow and keep symbols referenced by relocations

[MaskRay]

In GNU objcopy, symbols referenced by relocations are retained. Our COFF
(https://reviews.llvm.org/D56480) and Mach-O
(https://reviews.llvm.org/D75104) ports port the behavior, but the ELF
port doesn't.

This PR implements the behavior for ELF.
Close #47468 (tcl has a use case that requires strip -x tclStubLib.o
to strip local symbols not referenced by relocations.)

[llvmbot]

@llvm/pr-subscribers-llvm-binary-utilities

Author: Fangrui Song (MaskRay)

Changes

In GNU objcopy, symbols referenced by relocations are retained. Our COFF
(https://reviews.llvm.org/D56480) and Mach-O
(https://reviews.llvm.org/D75104) ports port the behavior, but the ELF
port doesn't.

This PR implements the behavior for ELF.
Close #47468 (tcl has a use case that requires strip -x tclStubLib.o
to strip local symbols not referenced by relocations.)

---

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

5 Files Affected:

• (modified) llvm/docs/CommandGuide/llvm-objcopy.rst (+5-4)
• (modified) llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp (+14-12)
• (modified) llvm/test/tools/llvm-objcopy/ELF/discard-all.test (+19)
• (removed) llvm/test/tools/llvm-objcopy/ELF/discard-locals-rel.test (-26)
• (modified) llvm/test/tools/llvm-objcopy/ELF/discard-locals.test (+19)

diff --git a/llvm/docs/CommandGuide/llvm-objcopy.rst b/llvm/docs/CommandGuide/llvm-objcopy.rst
index 8dc1357635e1b..96f3247780b3b 100644
--- a/llvm/docs/CommandGuide/llvm-objcopy.rst
+++ b/llvm/docs/CommandGuide/llvm-objcopy.rst
@@ -57,9 +57,10 @@ multiple file formats.
 
 .. option:: --discard-all, -x
 
- Remove most local symbols from the output. Different file formats may limit
- this to a subset of the local symbols. For example, file and section symbols in
- ELF objects will not be discarded. Additionally, remove all debug sections.
+ Remove most local symbols not referenced by relocations from the output.
+ Different file formats may limit this to a subset of the local symbols. For
+ example, file and section symbols in ELF objects will not be discarded.
+ Additionally, remove all debug sections.
 
 .. option:: --dump-section <section>=<file>
 
@@ -386,7 +387,7 @@ them.
 
 .. option:: --discard-locals, -X
 
- Remove local symbols starting with ".L" from the output.
+ Remove local symbols starting with ".L" not referenced by relocations from the output.
 
 .. option:: --extract-dwo
 
diff --git a/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp b/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
index 1a31bfa2e0db3..43aeff18c41b2 100644
--- a/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
+++ b/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
@@ -364,7 +364,7 @@ static Error updateAndRemoveSymbols(const CommonConfig &Config,
   // (like GroupSection or RelocationSection). This way, we know which
   // symbols are still 'needed' and which are not.
   if (Config.StripUnneeded || !Config.UnneededSymbolsToRemove.empty() ||
-      !Config.OnlySection.empty()) {
+      !Config.OnlySection.empty() || Config.DiscardMode != DiscardType::None) {
     for (SectionBase &Sec : Obj.sections())
       Sec.markSymbols();
   }
@@ -386,22 +386,24 @@ static Error updateAndRemoveSymbols(const CommonConfig &Config,
     if (Config.StripDebug && Sym.Type == STT_FILE)
       return true;
 
-    if ((Config.DiscardMode == DiscardType::All ||
-         (Config.DiscardMode == DiscardType::Locals &&
-          StringRef(Sym.Name).starts_with(".L"))) &&
-        Sym.Binding == STB_LOCAL && Sym.getShndx() != SHN_UNDEF &&
-        Sym.Type != STT_FILE && Sym.Type != STT_SECTION)
-      return true;
-
     if ((Config.StripUnneeded ||
          Config.UnneededSymbolsToRemove.matches(Sym.Name)) &&
         (!Obj.isRelocatable() || isUnneededSymbol(Sym)))
       return true;
 
-    // We want to remove undefined symbols if all references have been stripped.
-    if (!Config.OnlySection.empty() && !Sym.Referenced &&
-        Sym.getShndx() == SHN_UNDEF)
-      return true;
+    if (!Sym.Referenced) {
+      if ((Config.DiscardMode == DiscardType::All ||
+           (Config.DiscardMode == DiscardType::Locals &&
+            StringRef(Sym.Name).starts_with(".L"))) &&
+          Sym.Binding == STB_LOCAL && Sym.getShndx() != SHN_UNDEF &&
+          Sym.Type != STT_FILE && Sym.Type != STT_SECTION)
+        return true;
+      // We want to remove undefined symbols if all references have been
+      // stripped.
+      if (!Config.OnlySection.empty() && !Sym.Referenced &&
+          Sym.getShndx() == SHN_UNDEF)
+        return true;
+    }
 
     return false;
   };
diff --git a/llvm/test/tools/llvm-objcopy/ELF/discard-all.test b/llvm/test/tools/llvm-objcopy/ELF/discard-all.test
index 50cad3ae9f533..ce65026715f71 100644
--- a/llvm/test/tools/llvm-objcopy/ELF/discard-all.test
+++ b/llvm/test/tools/llvm-objcopy/ELF/discard-all.test
@@ -32,6 +32,14 @@ Sections:
     Address:         0x1000
     AddressAlign:    0x0000000000000010
     Size:            64
+  - Name:            .rela.text
+    Type:            SHT_RELA
+    Link:            .symtab
+    Info:            .text
+    Relocations:
+      - Offset: 0
+        Symbol: Referenced
+        Type:   R_X86_64_PC32
 Symbols:
   - Name:     Local
     Type:     STT_FUNC
@@ -49,6 +57,8 @@ Symbols:
     Section:  .text
     Value:    0x1010
     Binding:  STB_GLOBAL
+  - Name:     Referenced
+    Section:  .text
   - Name:     Weak
     Type:     STT_FUNC
     Size:     8
@@ -85,6 +95,15 @@ Symbols:
 #CHECK-NEXT:    Section: Undefined
 #CHECK-NEXT:  }
 #CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: Referenced
+#CHECK-NEXT:    Value: 0x0
+#CHECK-NEXT:    Size: 0
+#CHECK-NEXT:    Binding: Local
+#CHECK-NEXT:    Type: None
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: .text
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
 #CHECK-NEXT:    Name: Global
 #CHECK-NEXT:    Value: 0x1010
 #CHECK-NEXT:    Size: 8
diff --git a/llvm/test/tools/llvm-objcopy/ELF/discard-locals-rel.test b/llvm/test/tools/llvm-objcopy/ELF/discard-locals-rel.test
deleted file mode 100644
index 00bb8fcf18205..0000000000000
--- a/llvm/test/tools/llvm-objcopy/ELF/discard-locals-rel.test
+++ /dev/null
@@ -1,26 +0,0 @@
-# RUN: yaml2obj %s -o %t
-# RUN: not llvm-objcopy --discard-locals %t %t2 2>&1 | FileCheck %s -DFILE=%t
-
-!ELF
-FileHeader:
-  Class:           ELFCLASS64
-  Data:            ELFDATA2LSB
-  Type:            ET_REL
-  Machine:         EM_X86_64
-Sections:
-  - Name:            .text
-    Type:            SHT_PROGBITS
-  - Name:            .rel.text
-    Type:            SHT_REL
-    Link:            .symtab
-    Info:            .text
-    Relocations:
-      - Offset: 0x1000
-        Symbol: .L.rel
-        Type:   R_X86_64_PC32
-Symbols:
-  - Name:     .L.rel
-    Type:     STT_FUNC
-    Section:  .text
-
-# CHECK: error: '[[FILE]]': not stripping symbol '.L.rel' because it is named in a relocation
diff --git a/llvm/test/tools/llvm-objcopy/ELF/discard-locals.test b/llvm/test/tools/llvm-objcopy/ELF/discard-locals.test
index 4aba1cd1c782a..c1fe0f2b02b6e 100644
--- a/llvm/test/tools/llvm-objcopy/ELF/discard-locals.test
+++ b/llvm/test/tools/llvm-objcopy/ELF/discard-locals.test
@@ -29,6 +29,14 @@ FileHeader:
 Sections:
   - Name:            .text
     Type:            SHT_PROGBITS
+  - Name:            .rela.text
+    Type:            SHT_RELA
+    Link:            .symtab
+    Info:            .text
+    Relocations:
+      - Offset: 0
+        Symbol: .L.referenced
+        Type:   R_X86_64_PC32
   - Name:            .LLVM.Custom.Section
     Type:            SHT_PROGBITS
 Symbols:
@@ -48,6 +56,8 @@ Symbols:
   - Name:     .L.undefined
   - Name:     .L.abs
     Index:    SHN_ABS
+  - Name:     .L.referenced
+    Section:  .text
   - Name:     .L.Global
     Type:     STT_FUNC
     Section:  .text
@@ -109,6 +119,15 @@ Symbols:
 # CHECK-NEXT:     Section: Undefined
 # CHECK-NEXT:   }
 # CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name: .L.referenced
+# CHECK-NEXT:     Value:
+# CHECK-NEXT:     Size:
+# CHECK-NEXT:     Binding: Local
+# CHECK-NEXT:     Type: None
+# CHECK-NEXT:     Other:
+# CHECK-NEXT:     Section: .text
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Symbol {
 # CHECK-NEXT:     Name: .L.Global
 # CHECK-NEXT:     Value:
 # CHECK-NEXT:     Size:

[jh7370]

LGTM. Probably worth a release note.

[jh7370]

LGTM, with typo fix.

[jh7370]

Suggested change

	* In llvm-objcopy's ELF port, `--discard-locals` and `--discard-all` now allow and preserve symbols references by relocations.
	* In llvm-objcopy's ELF port, `--discard-locals` and `--discard-all` now allow and preserve symbols referenced by relocations.

[nga888]

Shouldn't the docs for llvm-strip also be updated and perhaps mentioned in the release notes?