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

Thu Jun 27 02:21:24 PDT 2019

jhenderson added inline comments.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:3-5
+# RUN: llvm-readobj --sections %t.dbg | FileCheck %s
+# RUN: llvm-readobj --sections %t.dbg | FileCheck --check-prefix=NOTE %s
+# RUN: llvm-readobj --sections %t.dbg | FileCheck --check-prefix=DEBUG %s
----------------
No need to repeat the llvm-readobj line three times. Just do it all in one run of FileCheck, by just changing "NOTE" and "DEBUG" to "CHECK" below.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:17
+    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+    Content: "c3c3c3c3"
+  - Name:    .note
----------------
Replace this with something like `Size: 4`, since the exact contents don't matter.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:20
+    Type:    SHT_NOTE
+    Flags:   [ SHF_ALLOC ]
+    Content: 040000001000000003000000474E55004FCB712AA6387724A9F465A32CD8C14B
----------------
Why is this `SHF_ALLOC`?

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:21
+    Flags:   [ SHF_ALLOC ]
+    Content: 040000001000000003000000474E55004FCB712AA6387724A9F465A32CD8C14B
+  - Name:    .debug
----------------
I don't think your contents here need to be valid. Just an arbitrary size.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:24
+    Type:    SHT_PROGBITS
+    Content: "031F"
+
----------------
Ditto.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:26
+
+## Checks that alloc segment is marked no bits.
+## It has no contents but size should stay the same.
----------------
Let's use the precise SHF_* and SHT_* terminology, to avoid any ambiguity.

Also "Checks" -> "Check"

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:29
+# CHECK:       Index: 1
+# CHECK-NEXT:  Name: .text
+# CHECK-NEXT:  Type: SHT_NOBITS
----------------
Nit here and throughout the rest of the test, reduce the double space to a single space.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:37
+# CHECK-NEXT:  Offset:
+# CHECK-NEXT:  Size: 64
+
----------------
Um... That Size doesn't match up with the specified contents?

================
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:
> jhenderson wrote:
> > 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?
> > a SHF_ALLOC, SHT_SYMTAB makes little sense.
> I thought so too.
> > What led you to investigate SHT_SYMTAB + SHF_ALLOC?
> There is an object file in test/llvm-objcopy/ELF/Inputs called alloc-symtab.o, I wasn't sure why we were testing this but of course my program was crashing at the assert(!"cant clear contents"). 
> > that's what SHT_DYNSYM sections are for
> This case definitely isn't handled yet, I'll look there as well 
So we should definitely handle a SHF_ALLOC symbol table (i.e. we shouldn't crash or emit an error), but we don't need to match GNU's behaviour, I reckon. Take a look at what exactly we do with SHT_SYMTAB and SHF_ALLOC sections elsewhere, and use that as a guideline. I think a case could be made for preserving the contents, or for clearing it. I'd probably go for the preserving contents, since it's safer.

================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:85
+void SectionBase::clearSectionContents() {
+  assert(0 && "Cannot clear this sections contents");
+}
----------------
MaskRay wrote:
> abrachet wrote:
> > jhenderson wrote:
> > > This would usually be `llvm_unreachable`. However, I'm wondering why you're not just ignoring this case?
> > I'll change this later, for now I think its a good way to check when it is being called on an unexpected section. Although probably llvm_unreachable will also aborts with the message?
> This will abort for DynamicRelocationSection (.rela.dyn)
I think this suggests that the default implementation should be to just not do anything. I think there'll be other section kinds that might get here too, either now or in the future.

Basically, I see two options:

1) If no section type should ever get here, it should be enforced by the virtual interface, i.e. make the function a pure virtual function without a definition in the base class. I don't think having the default behaviour of "this is bad" should be enforced at run time. We can do this at compile time.
2) If for most sections it really doesn't make sense, I'd recommend a default empty implementation.

> Although probably llvm_unreachable will also aborts with the message?
Yes, that's the purpose of it.

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

https://reviews.llvm.org/D63807