[llvm-objcopy] - Strip sections before symbols.

[grimar]

Summary

This is a fix for https://bugs.llvm.org/show_bug.cgi?id=40007.
It is rebased on top of D59762.

Idea is to swap the order of stripping. So that we strip sections before
symbols what allows us to strip relocation sections without emitting
the error about relative symbols.

[jhenderson]

I think another interesting test might be -N <sym> -R <relocation section referencing sym>. I'm assuming that this fails currently, based on the --strip-all symptoms.

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

1 ↗

(On Diff #192081)

Probably worth renaming this test strip-all-relocations.test, since it's not specifically about stripping relocations.

3 ↗

(On Diff #192081)

Add "at the same time" to the end of this comment, as that's the important bit (it would have been possible to strip the relocation sections first, then the symbols as a workaround before).

[grimar]

• Addressed review comments.

[grimar]

In D59763#1441316, @jhenderson wrote:

I think another interesting test might be `-N <sym>

Something is wrong I guess here, as I do not see -N in llvm-objcopy help... I used --strip-symbol= instead.

[jhenderson]

In D59763#1441401, @grimar wrote:

In D59763#1441316, @jhenderson wrote:

I think another interesting test might be `-N <sym>

Something is wrong I guess here, as I do not see -N in llvm-objcopy help... I used --strip-symbol= instead.

I'm guessing short aliases aren't in the help text, but --strip-symbol is fine.

LGTM, with a couple of minor comments. Maybe worth leaving it overnight before committing though, in case any of the US contributors have any comments.

test/tools/llvm-objcopy/ELF/strip-all-relocations.test

1 ↗

(On Diff #192096)

Okay, since you've added the new test case to this test, this test is badly named again. Sorry! Perhaps "strip-symbol-and-relocation.test"?

29 ↗

(On Diff #192096)

Nit: you can delete the Flags, as they aren't relevant to the test.

[rupprecht]

Just a couple test changes, otherwise LG and thanks for fixing this bug!

test/tools/llvm-objcopy/ELF/strip-all-relocations.test

14 ↗

(On Diff #192096)

As a sanity check that this test is configured correctly, you might also want to run "not llvm-objcopy --strip-symbol=bar %t2" and make sure it prints the right error

14 ↗

(On Diff #192096)

This is missing %t as the source, it should be "llvm-objcopy %t %t2". This is "copying" %t2 in place.

[alexander-shaposhnikov]

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

545–549

I think it's worth adding a comment here explaining that the order is really important here.

[grimar]

• Addressed review comments.

test/tools/llvm-objcopy/ELF/strip-all-relocations.test

1 ↗

(On Diff #192096)

NP, thanks for the help with the naming things!

14 ↗

(On Diff #192096)

Done for both comments. Thanks!

29 ↗

(On Diff #192096)

Done. (Honestly, I am not sure here, .text is "ax" traditionally, and it seems fine to me to keep it as it should be, just in case (given that non-alloc sections are often an exception for something usually in tools/linkers/etc), but I have no solid arguments for that atm).

[rupprecht]

Thanks!

test/tools/llvm-objcopy/ELF/strip-symbol-and-relocation.test

+# RUN: yaml2obj %s > %t
+
+## Check we are able to strip all symbols and relocatable information at the same time.
+
+# RUN: llvm-objcopy -S %t %t2
+# RUN: llvm-objdump --section-headers %t2 | FileCheck %s
+
+# RUN: llvm-objcopy --strip-all-gnu %t %t2
+# RUN: llvm-objdump --section-headers %t2 | FileCheck %s
+
+# CHECK-NOT: .symtab
+# CHECK-NOT: .rela.text
+
+## Check we are able to strip the particular symbol if we
+## strip the corresponding relocation section at the same time.
+
+# RUN: llvm-objcopy --strip-symbol=bar -R .rela.text %t %t2
+# RUN: llvm-readelf -s -S %t2 | FileCheck %s --check-prefix=STRIPSYM
+
+# STRIPSYM-NOT: bar
+# STRIPSYM-NOT: .rela.text
+
+# RUN: not llvm-objcopy --strip-symbol=bar %t %t2 2>&1 | FileCheck %s --check-prefix=ERR
+# ERR: not stripping symbol 'bar' because it is named in a relocation
+ 
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ] 
+    Content:         00000000
+  - Name:            .rela.text
+    Type:            SHT_RELA
+    Link:            .symtab
+    Info:            .text
+    Relocations:
+      - Offset:          0x0000000000000000
+        Symbol:          bar
+        Type:            R_X86_64_32S
+Symbols:
+  Global:
+    - Name:     bar
+      Section:  .text

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

if (Error E =
             splitDWOToFile(Config, Reader, Config.SplitDWO, OutputElfType))
       return E;
 
   if (Config.OutputArch) {
     Obj.Machine = Config.OutputArch.getValue().EMachine;
     Obj.OSABI = Config.OutputArch.getValue().OSABI;
   }
 
-  if (Error E = updateAndRemoveSymbols(Config, Obj))
+  // It is important to remove the sections first. For example, we want to
+  // remove the relocation sections before removing the symbols. That allows
+  // us to avoid reporting the inappropriate  errors about removing symbols
+  // named in relocations.
+  if (Error E = replaceAndRemoveSections(Config, Obj))
     return E;
 
-  if (Error E = replaceAndRemoveSections(Config, Obj))
+  if (Error E = updateAndRemoveSymbols(Config, Obj))
     return E;
 
   if (!Config.SectionsToRename.empty()) {
     for (auto &Sec : Obj.sections()) {
       const auto Iter = Config.SectionsToRename.find(Sec.Name);
       if (Iter != Config.SectionsToRename.end()) {
         const SectionRename &SR = Iter->second;
         Sec.Name = SR.NewName;