[llvm-objcopy]Don't implicitly strip sections in segments

[jhenderson]

Summary

This patch changes llvm-objcopy's behaviour to not strip sections that are in segments, if they otherwise would be due to a stripping operation (--strip-all, --strip-sections, --strip-non-alloc). This preserves the segment contents. It does not change the behaviour of --strip-all-gnu (although we could choose to do so), because GNU objcopy's behaviour in this case seems to be to strip the section, nor does it prevent removing of sections in segments with --remove-section (if a user REALLY wants to remove a section, we should probably let them, although I could be persuaded that warning might be appropriate). Tests have been added to show this latter behaviour.

I haven't modified --strip-debug's behaviour (i.e. a debug section in a segment will be stripped), but I'd be happy to.

This fixes https://bugs.llvm.org/show_bug.cgi?id=41006.

[grimar]

I approve, but please wait for somebody's else opinion too.

[jakehehrlich]

This isn't precisely what I had in mind but I think its a strict improvement. Tests look good and document the specific behavior well. I think the behavior with zeroing out sections in explicit cases or where we're looking to match GNU behavior precisely is a good idea (and would have been harder to impossible with what I had in mind). The only one I question is --strip-non-alloc but that was something I invented that turned out not to be useful. We could probably remove that and no one would care. There's not standard or use case to guide what should happen there. The guiding principal should probably be "when it doesn't make people scratch their heads, don't remove stuff in segments" which this change adhears to.

LGTM.

[rupprecht]

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

10 ↗

(On Diff #190396)

Does od work on windows?

tools/llvm-objcopy/StripOpts.td

34 ↗

(On Diff #190396)

The spacing around quotes is inconsistent; clang-format past rC356099 should be able to automatically fix this

[jakehehrlich]

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

10 ↗

(On Diff #190396)

We've been using in practice so...I think so?

[jhenderson]

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

10 ↗

(On Diff #190396)

Yes, od is part of GnuWin32, which is a pre-requisite for running the lit tests on Windows.

llvm/trunk/test/tools/llvm-objcopy/ELF/remove-section-in-segment.test

+# Show that --remove-section removes sections in segments.
+
+# RUN: yaml2obj %s -o %t
+
+# Validate the offsets for the later parts of the test.
+# RUN: llvm-objcopy %t %t.copy
+# Skip to the start of the sections, which should be immediately after the
+# program header table (i.e. sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) == 120).
+# RUN: od %t.copy -t x1 -N 16 -j 120 | FileCheck %s --check-prefix=COPY
+
+# COPY: 11 22 33 44 55 66 77 88
+
+# RUN: llvm-objcopy --remove-section non_alloc %t %t.nonalloc
+# RUN: llvm-readelf --sections %t.nonalloc | FileCheck %s --check-prefix=NONALLOC-SHDRS
+# RUN: od %t.nonalloc -t x1 -N 16 -j 120 | FileCheck %s --check-prefix=NONALLOC-BYTES
+
+# NONALLOC-SHDRS-NOT: non_alloc
+# NONALLOC-BYTES: 11 22 33 44 00 00 00 00
+
+# RUN: llvm-objcopy --remove-section shf_alloc %t %t.alloc
+# RUN: llvm-readelf --sections %t.alloc | FileCheck %s --check-prefix=ALLOC-SHDRS
+# RUN: od %t.alloc -t x1 -N 16 -j 120 | FileCheck %s --check-prefix=ALLOC-BYTES
+
+# ALLOC-SHDRS-NOT: shf_alloc
+# ALLOC-BYTES: 00 00 00 00 55 66 77 88
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name:    shf_alloc
+    Type:    SHT_PROGBITS
+    Flags:   [SHF_ALLOC]
+    Content: '11223344'
+  - Name:    non_alloc
+    Type:    SHT_PROGBITS
+    Flags:   []
+    Content: '55667788'
+ProgramHeaders:
+  # Use an arbitrary segment type to show that the segment type is unimportant.
+  - Type: 0x61234567
+    Sections:
+      - Section: shf_alloc
+      - Section: non_alloc

llvm/trunk/test/tools/llvm-objcopy/ELF/strip-all-gnu.test

 # RUN: yaml2obj %s > %t
-# RUN: cp %t %t1
 # RUN: llvm-objcopy --strip-all-gnu %t %t2
 # RUN: llvm-strip --strip-all-gnu %t -o %t3
 # RUN: llvm-readobj --file-headers --sections %t2 | FileCheck %s
 # RUN: cmp %t2 %t3
 
+# Show that the debug section in a segment was removed, to match GNU.
+# First validate that the offset in use is correct.
+# RUN: llvm-objcopy %t %t4
+# RUN: od %t4 -t x1 -N 4 -j 120 | FileCheck %s --check-prefix=COPY-BYTES
+# RUN: od %t2 -t x1 -N 4 -j 120 | FileCheck %s --check-prefix=STRIP-BYTES
+
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:
+  - Name:            .debug_in_segment
+    Type:            SHT_PROGBITS
+    Flags:           [ ]
+    Content:         "deadbeef"
   - Name:            .dynstr
     Type:            SHT_STRTAB
     Flags:           [ SHF_ALLOC ]
   - Name:            .symtab.dyn
     Type:            SHT_SYMTAB
     Flags:           [ SHF_ALLOC ]
     Type:            SHT_NOBITS
   - Name:            .text
Sections:
   - Name:            .debug_not_a_real_debug_section
     Type:            SHT_PROGBITS
   - Name:            .rel.text
     Type:            SHT_REL
     Info:            .text
   - Name:            .rela.text
     Type:            SHT_RELA
     Info:            .text
+ProgramHeaders:
+  # Use an arbitrary segment type to show that the segment type is unimportant.
+  - Type: 0x61234567
+    Sections:
+      - Section: .debug_in_segment
 
 # CHECK: SectionHeaderCount: 8
 
 # CHECK:      Name: .dynstr
 # CHECK:      Name: .symtab.dyn
 # CHECK:      Name: .text
 # CHECK:      Name: .debug_loc
 # CHECK:      Name: .comment
 # CHECK:      Name: .random_section_name
 # CHECK:      Name: .shstrtab
+
+# COPY-BYTES:  de ad be ef
+# STRIP-BYTES: 00 00 00 00

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

 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:
+  - Name:            non_alloc_in_segment
+    Type:            SHT_PROGBITS
+    Flags:           [ ]
+    Size:            4
   - Name:            .bss
     Type:            SHT_NOBITS
     Flags:           [ SHF_ALLOC ]
   - Name:            .text
     Type:            SHT_PROGBITS
     Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
   - Name:            .blarg
     Type:            SHT_PROGBITS
     Flags:           [ ]
   - Name:            .gnu.warning.foo
     Type:            SHT_PROGBITS
+ProgramHeaders:
+  # Use an arbitrary segment type to show that the segment type is unimportant.
+  - Type: 0x61234567
+    Sections:
+      - Section: non_alloc_in_segment
 
-# CHECK: SectionHeaderCount: 5
+# CHECK: SectionHeaderCount: 6
 
+# CHECK: Name: non_alloc_in_segment
 # CHECK: Name: .bss
 # CHECK: Name: .text
 # CHECK: Name: .gnu.warning.foo
 # CHECK: Name: .shstrtab

llvm/trunk/test/tools/llvm-objcopy/ELF/strip-non-alloc.test

 # RUN: yaml2obj %s > %t
 # RUN: llvm-objcopy --strip-non-alloc %t %t2
 # RUN: llvm-readobj --file-headers --sections %t2 | FileCheck %s
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:
+  - Name:            non_alloc_in_segment
+    Type:            SHT_PROGBITS
+    Flags:           [ ]
+    Size:            4
   - Name:            .bss
     Type:            SHT_NOBITS
     Flags:           [ SHF_ALLOC ]
   - Name:            .text
     Type:            SHT_PROGBITS
     Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
   - Name:            .blarg
     Type:            SHT_PROGBITS
     Flags:           [ ]
+ProgramHeaders:
+  # Use an arbitrary segment type to show that the segment type is unimportant.
+  - Type: 0x61234567
+    Sections:
+      - Section: non_alloc_in_segment
 
-# CHECK: SectionHeaderCount: 4
+# CHECK: SectionHeaderCount: 5
 
+# CHECK: Name: non_alloc_in_segment
 # CHECK: Name: .bss
 # CHECK: Name: .text
 # CHECK: Name: .shstrtab

llvm/trunk/test/tools/llvm-objcopy/ELF/strip-sections.test

FileHeader:
   Type:            ET_EXEC
   Machine:         EM_X86_64
 Sections:
   - Name:            .text
     Type:            SHT_PROGBITS
     Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
     AddressAlign:    0x0000000000001000
     Content:         "DEADBEEF"
+  - Name:            .non_alloc_in_segment
+    Type:            SHT_PROGBITS
+    Flags:           [ ]
+    Content:         "CAFEBABE"
 ProgramHeaders:
 - Type: PT_LOAD
   Flags: [ PF_X, PF_R ]
   Sections:
     - Section: .text
+    - Section: .non_alloc_in_segment
 
-#DATA: 0010000 de ad be ef
+#DATA: 0010000 de ad be ef ca fe ba be
 
 #CHECK: ElfHeader {
 #CHECK-NEXT:  Ident {
 #CHECK-NEXT:     Magic: (7F 45 4C 46)
 #CHECK-NEXT:     Class: 64-bit (0x2)
 #CHECK-NEXT:     DataEncoding: LittleEndian (0x1)
 #CHECK-NEXT:     FileVersion: 1
 #CHECK-NEXT:     OS/ABI: SystemV (0x0)

 #CHECK-NEXT: }
 
 #CHECK:     ProgramHeaders [
 #CHECK-NEXT:  ProgramHeader {
 #CHECK-NEXT:    Type: PT_LOAD (0x1)
 #CHECK-NEXT:    Offset: 0x1000
 #CHECK-NEXT:    VirtualAddress: 0x0
 #CHECK-NEXT:    PhysicalAddress: 0x0
-#CHECK-NEXT:    FileSize: 4
-#CHECK-NEXT:    MemSize: 4
+#CHECK-NEXT:    FileSize: 8
+#CHECK-NEXT:    MemSize: 8
 #CHECK-NEXT:    Flags [ (0x5)
 #CHECK-NEXT:      PF_R (0x4)
 #CHECK-NEXT:      PF_X (0x1)
 #CHECK-NEXT:    ]
 #CHECK-NEXT:    Alignment: 4096
 #CHECK-NEXT:  }
 #CHECK-NEXT:]

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

RemovePred = [RemovePred, &Obj](const SectionBase &Sec) {
       case SHT_STRTAB:
         return true;
       }
       return isDebugSection(Sec);
     };
 
   if (Config.StripSections) {
     RemovePred = [RemovePred](const SectionBase &Sec) {
-      return RemovePred(Sec) || (Sec.Flags & SHF_ALLOC) == 0;
+      return RemovePred(Sec) || Sec.ParentSegment == nullptr;
     };
   }
 
   if (Config.StripDebug) {
     RemovePred = [RemovePred](const SectionBase &Sec) {
       return RemovePred(Sec) || isDebugSection(Sec);
     };
   }
 
   if (Config.StripNonAlloc)
     RemovePred = [RemovePred, &Obj](const SectionBase &Sec) {
       if (RemovePred(Sec))
         return true;
       if (&Sec == Obj.SectionNames)
         return false;
-      return (Sec.Flags & SHF_ALLOC) == 0;
+      return (Sec.Flags & SHF_ALLOC) == 0 && Sec.ParentSegment == nullptr;
     };
 
   if (Config.StripAll)
     RemovePred = [RemovePred, &Obj](const SectionBase &Sec) {
       if (RemovePred(Sec))
         return true;
       if (&Sec == Obj.SectionNames)
         return false;
       if (StringRef(Sec.Name).startswith(".gnu.warning"))
         return false;
+      if (Sec.ParentSegment != nullptr)
+        return false;
       return (Sec.Flags & SHF_ALLOC) == 0;
     };
 
   // Explicit copies:
   if (!Config.OnlySection.empty()) {
     RemovePred = [&Config, RemovePred, &Obj](const SectionBase &Sec) {
       // Explicitly keep these sections regardless of previous removes.
       if (is_contained(Config.OnlySection, Sec.Name))

llvm/trunk/tools/llvm-objcopy/ObjcopyOpts.td

 
 defm set_section_flags
     : Eq<"set-section-flags",
          "Set section flags for a given section. Flags supported for GNU "
          "compatibility: alloc, load, noload, readonly, debug, code, data, "
          "rom, share, contents, merge, strings.">,
       MetaVarName<"section=flag1[,flag2,...]">;
 
-def strip_all
-    : Flag<["-", "--"], "strip-all">,
-      HelpText<
-          "Remove non-allocated sections other than .gnu.warning* sections">;
+def strip_all : Flag<["-", "--"], "strip-all">,
+                HelpText<"Remove non-allocated sections outside segments. "
+                         ".gnu.warning* sections are not removed">;
 def S : Flag<["-"], "S">, Alias<strip_all>;
 def strip_all_gnu : Flag<["-", "--"], "strip-all-gnu">,
                     HelpText<"Compatible with GNU objcopy's --strip-all">;
 def strip_debug : Flag<["-", "--"], "strip-debug">,
                   HelpText<"Remove all debug information">;
 def g : Flag<["-"], "g">, Alias<strip_debug>,
         HelpText<"Alias for --strip-debug">;
 def strip_dwo : Flag<["-", "--"], "strip-dwo">,
                 HelpText<"Remove all DWARF .dwo sections from file">;
-def strip_sections : Flag<["-", "--"], "strip-sections">,
-                     HelpText<"Remove all section headers">;
-def strip_non_alloc : Flag<["-", "--"], "strip-non-alloc">,
-                      HelpText<"Remove all non-allocated sections">;
+def strip_sections
+    : Flag<["-", "--"], "strip-sections">,
+      HelpText<"Remove all section headers and all sections not in segments">;
+def strip_non_alloc
+    : Flag<["-", "--"], "strip-non-alloc">,
+      HelpText<"Remove all non-allocated sections outside segments">;
 def strip_unneeded : Flag<["-", "--"], "strip-unneeded">,
                      HelpText<"Remove all symbols not needed by relocations">;
-defm strip_unneeded_symbol 
+defm strip_unneeded_symbol
     : Eq<"strip-unneeded-symbol",
          "Remove symbol <symbol> if it is not needed by relocations">,
       MetaVarName<"symbol">;
 defm strip_unneeded_symbols
     : Eq<"strip-unneeded-symbols",
          "Reads a list of symbols from <filename> and removes them "
          "if they are not needed by relocations">,
       MetaVarName<"filename">;

llvm/trunk/tools/llvm-objcopy/StripOpts.td

def U : Flag<["-"], "U">,
         HelpText<"Alias for --disable-deterministic-archives">;
 
 defm output : Eq<"o", "Write output to <file>">, MetaVarName<"output">;
 
 def preserve_dates : Flag<["-", "--"], "preserve-dates">,
                      HelpText<"Preserve access and modification timestamps">;
 def p : Flag<["-"], "p">, Alias<preserve_dates>;
 
-def strip_all
-    : Flag<["-", "--"], "strip-all">,
-      HelpText<
-          "Remove non-allocated sections other than .gnu.warning* sections">;
+def strip_all : Flag<["-", "--"], "strip-all">,
+                HelpText<"Remove non-allocated sections outside segments. "
+                         ".gnu.warning* sections are not removed">;
 def s : Flag<["-"], "s">, Alias<strip_all>;
 
 def strip_all_gnu : Flag<["-", "--"], "strip-all-gnu">,
                     HelpText<"Compatible with GNU strip's --strip-all">;
 def strip_debug : Flag<["-", "--"], "strip-debug">,
                   HelpText<"Remove debugging symbols only">;
 def d : Flag<["-"], "d">, Alias<strip_debug>;
 def g : Flag<["-"], "g">, Alias<strip_debug>;