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

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked an inline comment as done.
abrachet added inline comments.


================
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;
----------------
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 


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:85
+void SectionBase::clearSectionContents() {
+  assert(0 && "Cannot clear this sections contents");
+}
----------------
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?


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

https://reviews.llvm.org/D63807