[llvm-objcopy] - Strip undefined symbols if they are no longer referenced following --only-section

[grimar]

Summary

This is https://bugs.llvm.org/show_bug.cgi?id=40004.

In this patch I teach llvm-objcopy to remove undefined symbols if
them are not used anymore after applying -j/--only-section option.

[jhenderson]

The behaviour you've implemented removes all unreferenced symbols, even if they weren't previously referenced. Is this wise? Also, have you experimented with other stripping options, e.g. removing a section which references a symbol? The bug used --only-section, but there might be other similar cases.

test/tools/llvm-objcopy/ELF/only-sesction-strip-undefined.test

1–2 ↗

(On Diff #200972)

the undefined -> an undefined
if the source of reference was -> if all references to it have been

11 ↗

(On Diff #200972)

What about a test-case where there are two references to the same symbol, but only one of them is removed?

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

419 ↗

(On Diff #200972)

remove the undefined -> remove undefined
the source of reference -like relocation section was stripped > all references have been stripped

[grimar]

• Addressed review comments.

The behaviour you've implemented removes all unreferenced symbols, even if they weren't previously referenced. Is this wise?

This behavior follows GNU, i think it is probably OK. Do you think we should change it?

Also, have you experimented with other stripping options, e.g. removing a section which references a symbol?
The bug used --only-section, but there might be other similar cases.

Not yet, but that was my plan (after landing this). I was not going to close the bug before testing
other options and fixing them (I expect to see issues, yes).

test/tools/llvm-objcopy/ELF/only-sesction-strip-undefined.test

11 ↗

(On Diff #200972)

Done.

[grimar]

Oh and I also added a test for "The behaviour you've implemented removes all unreferenced symbols, even if they weren't previously referenced"

[jhenderson]

In D62317#1515526, @grimar wrote:

The behaviour you've implemented removes all unreferenced symbols, even if they weren't previously referenced. Is this wise?

This behavior follows GNU, i think it is probably OK. Do you think we should change it?

No, if it follows GNU, keep it as is.

Also, have you experimented with other stripping options, e.g. removing a section which references a symbol?
The bug used --only-section, but there might be other similar cases.

Not yet, but that was my plan (after landing this). I was not going to close the bug before testing
other options and fixing them (I expect to see issues, yes).

I think you should do that before landing this, because it might mean the implementation is a little simpler (e.g. we simply always remove unreferenced undefined globals, regardless of the switch).

test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test

1 ↗

(On Diff #201172)

Maybe worth removing "only-section" from the test title, as I think it would make sense for other options to fall under this test too.

10 ↗

(On Diff #201172)

Probably best to put a blank line before the CHECK line, for ease of reading.

14–17 ↗

(On Diff #201172)

Nit: remove the extra whitespace here and below.

21–22 ↗

(On Diff #201172)

You don't need these fields.

26–29 ↗

(On Diff #201172)

You don't need any of these fields. Ditto below.

48 ↗

(On Diff #201172)

You don't need this.

56 ↗

(On Diff #201172)

symbol -> symbols
it wasn't -> they weren't

72 ↗

(On Diff #201172)

You don't need this.

75–77 ↗

(On Diff #201172)

I don't think you need this symbol.

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

419 ↗

(On Diff #200972)

Apparently I failed at typing here:

remove the undefined -> remove undefined
the source of reference, like relocation section was stripped -> all references have been stripped

[grimar]

In D62317#1515541, @jhenderson wrote:

I think you should do that before landing this, because it might mean the implementation is a little simpler (e.g. we simply always remove unreferenced undefined globals, regardless of the switch).

Ok, done: imagine we have

.globl bar
.globl foo
.section .text,"ax"
  movl $bar, %edx

I.e. file with .text, .rela.text,
undefined bar, referenced by .rela.text and
undefined unreferenced foo.
(also, GNU as creates zero sized .data section, it is used below)

as in.s -o as.o

GNU objdump behavior:

1. objcopy in.o out.o:

both foo and bar are in the output.

2. objcopy in.o out.o -R .text

.text and .rela.text are removed, both symbols are there.

3. objcopy in.o out.o -j .text -j .rela.text

reports error: objcopy: out.o: symbol bar required but not present
(looks like a bug to me)

4. objcopy in.o out.o -j .data

.text and .rela.text are removed, both symbols are removed.

I.e. the behavior of this patch matches GNU now,
which does not removes undefined unreferenced symbols when -R is used and
does not remove them regardless of the switch.

Is there anything else you think worth to check?

[grimar]

• Addressed review comments.

[jhenderson]

In D62317#1515921, @grimar wrote:

Is there anything else you think worth to check?

Nope, what you've done seems fine to me. Please just address my inline comments. Note: I am away from the end of work today, for a week. I'm happy for somebody else to approve if my points are addressed.

test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test

1 ↗

(On Diff #201172)

Since this looks to be only for -j behaviour, don't do this after all.

[grimar]

test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test

1 ↗

(On Diff #201172)

:) I updated a few seconds before your comment. Will upload a new diff in a minute.

[grimar]

• Restored the original test case name.

[jhenderson]

test/tools/llvm-objcopy/ELF/strip-undefined.test

22 ↗

(On Diff #201235)

You shouldn't need these flags either.

56–57 ↗

(On Diff #201235)

Nit: remove some of the spacing here.

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

419 ↗

(On Diff #200972)

This comment is still outstanding.

[grimar]

• Addressed review comments.

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

419 ↗

(On Diff #200972)

Oops, sorry I missed this one.

[jhenderson]

LGTM.

[grimar]

In D62317#1516021, @jhenderson wrote:

LGTM.

Thanks for review, James!

llvm/trunk/test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test

+## Here we want to check that llvm-objcopy removes an undefined symbol
+## if all references to it have been stripped.
+
+# RUN: yaml2obj --docnum=1 %s -o %t.o
+# RUN: llvm-objcopy -j .other.section %t.o %t2.o
+# RUN: llvm-readobj --symbols %t2.o | FileCheck %s --implicit-check-not=bar
+
+# RUN: llvm-objcopy -j .text -j .rela.text1 %t.o %t2.o
+# RUN: llvm-readobj --symbols %t2.o | FileCheck %s --check-prefix=BAR
+
+# BAR: bar
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name:        .text
+    Type:        SHT_PROGBITS
+  - Name:        .rela.text1
+    Type:        SHT_RELA
+    Relocations:
+      - Offset:  0x0000000000000001
+        Symbol:  bar
+        Type:    R_X86_64_32
+  - Name:        .rela.text2
+    Type:        SHT_RELA
+    Relocations:
+      - Offset:  0x0000000000000001
+        Symbol:  bar
+        Type:    R_X86_64_32
+  - Name:        .other.section
+    Type:        SHT_PROGBITS
+Symbols:
+  - Name:    bar
+    Binding: STB_GLOBAL
+...
+
+## Check we remove unreferenced undefined symbols, even if
+## they weren't previously referenced. This follows GNU.
+
+# RUN: yaml2obj --docnum=2 %s -o %t.o
+# RUN: llvm-objcopy -j .keep_me %t.o %t2.o
+# RUN: llvm-readobj --symbols %t2.o | FileCheck %s --implicit-check-not=bar
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .keep_me
+    Type: SHT_PROGBITS
+Symbols:
+  - Name:    bar
+    Binding: STB_GLOBAL
+...

llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

Obj.SymbolTable->updateSymbols([&](Symbol &Sym) {
 
     if (!Config.SymbolsPrefix.empty() && Sym.Type != STT_SECTION)
       Sym.Name = (Config.SymbolsPrefix + Sym.Name).str();
   });
 
   // The purpose of this loop is to mark symbols referenced by sections
   // (like GroupSection or RelocationSection). This way, we know which
   // symbols are still 'needed' and which are not.
-  if (Config.StripUnneeded || !Config.UnneededSymbolsToRemove.empty()) {
+  if (Config.StripUnneeded || !Config.UnneededSymbolsToRemove.empty() ||
+      !Config.OnlySection.empty()) {
     for (auto &Section : Obj.sections())
       Section.markSymbols();
   }
 
   auto RemoveSymbolsPred = [&](const Symbol &Sym) {
     if (is_contained(Config.SymbolsToKeep, Sym.Name) ||
         (Config.KeepFileSymbols && Sym.Type == STT_FILE))
       return false;
auto RemoveSymbolsPred = [&](const Symbol &Sym) {
     if (is_contained(Config.SymbolsToRemove, Sym.Name))
       return true;
 
     if ((Config.StripUnneeded ||
          is_contained(Config.UnneededSymbolsToRemove, Sym.Name)) &&
         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;
+
     return false;
   };
 
   return Obj.removeSymbols(RemoveSymbolsPred);
 }
 
 static Error replaceAndRemoveSections(const CopyConfig &Config, Object &Obj) {
   SectionPred RemovePred = [](const SectionBase &) { return false; };