[PATCH] D63807: [llvm-objcopy] Add --only-keep-debug for ELF

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

At least two attempts were made to implement --only-keep-debug before. See D40523 <https://reviews.llvm.org/D40523> and D43475 <https://reviews.llvm.org/D43475>. Make sure to read through the comments on those reviews too.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:18-20
+# CHECK-NEXT:       Name: .text
+# CHECK-NEXT:       Type: SHT_NOBITS
+# CHECK-NEXT:       Flags [ (0x6)
----------------
You don't need to match the whitespace exactly. I'd just line these all up, close to the CHECK-NEXT column like so:
```
# CHECK:     Index: 1
# CHECK-NEXT: Name: .text
# CHECK-NEXT: Type: SHT_NOBITS
# CHECK-NEXT: Flags [ (0x6)
```
Also, I'd be explicit about checking the set flags (i.e. SHF_ALLOC, SHF_EXECINSTR), and I'd also check the Size field.

You also need checks for the behaviour for other sections (non-alloc, SHT_NOTE, SHF_GROUP, .debug_* sections etc).


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:590
+      if (Sec.Flags & (SHF_ALLOC | SHF_GROUP) && Sec.Type != SHT_NOTE) {
+        if (Sec.Type == SHT_SYMTAB) {
+          Sec.Flags &= ~SHF_ALLOC;
----------------
abrachet wrote:
> This is temporary!
> 
> I've been looking at how GNU objcopy deals with --only-keep-debug, I cannot figure out what is different about an SHT_SYMTAB with SHF_ALLOC flag, but it treats them differently. I need to look into this further but would love some help because I really can't figure it out. 
> 
> The code can be found [[ https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=binutils/objcopy.c;h=28b9d3bf9291518bffe93e98aa9b9b50a8fff3e2;hb=refs/heads/master#l3799 | here ]]. It seems to me like find_section_list will always return nullptr, meaning the if I have on 589 should be enough. Clearly this isn't the case, though. 
I've not got time to dig into the GNU code, but it's worth noting that a SHF_ALLOC, SHT_SYMTAB makes little sense. SHF_ALLOC sections represent sections loaded into memory, for which there is little purpose for SHT_SYMTAB sections - that's what SHT_DYNSYM sections are for. Ultimately, I therefore think that we don't need to worry about what GNU does here. What led you to investigate SHT_SYMTAB + SHF_ALLOC?


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:85
+void SectionBase::clearSectionContents() {
+  assert(0 && "Cannot clear this sections contents");
+}
----------------
This would usually be `llvm_unreachable`. However, I'm wondering why you're not just ignoring this case?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63807/new/

https://reviews.llvm.org/D63807