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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

I think this change as is should not go in. I think I'd like to explain some details that I've learned about this flag and how it functions in practice since I've had to consume binaries produced by this and it's equivalent in eu-strip.

1. Program headers must keep the virtual address and memory size the same. If program headers aren't kept there is no use case for this option. This is vital for how debuggers match up sections.
2. At least eu-strip actually just copies the program headers over without performing layout or any kind of modification. That's hard to do here because of how llvm-objcopy is designed (sorry). There's an argument to be made that p_filesz should be kept the same (see bullet point regarding this) but I think we're free to change p_offset even if eu-strip doesn't. We should test what GNU objcopy does to program headers here as well but we're probably ok allowing ourselves to modify p_offset and p_filesz.
3. You have to change all links and pointers properly. This is what has killed the last two attempts. replaceSectionReferences is designed to do exactly this but you'll need to extend the implementation for many more section types to make this work generally.
4. If you don't decrease p_filesz but you go though the current layout algorithm with no modification your file will be the same size as it is now by doing the no-op. e.g. nothing will have been achieved. That's what the code is doing currently. Actually this would be a slight improvement because it would make the file more compressible. If your goal is to save on disk space there isn't much point but if your point is to save disk space for compressed files or reduce bits over the wire then this is valuable. If we just want to zero these sections out however there are better ways to do it.

I've thought of two ways to accomplish valid and near minimal results for this option but neither of them are easy to implement:

1. Use replaceSectionReferences and modify p_filesz to be correctly minimized to save that bit of space. This will work with the current writer but the whole thing requires getting a lot of details right.
2. Create a new writer that writes everything out in a totally distinct manner and does layout in a distinct way. This would most closely match what eu-strip does (and what I suspect GNU objcopy does). The layout algorithm would be the hardest part to get right but it really only needs to get the section header layout correct and can ignore program headers and their interplay with sections completely. The only extra challenging part is that you have to treat SHF_ALLOC sections just like we already do SHT_NOBITS and then make sure that you write out SHF_ALLOC sections as SHT_NOBITS instead of their respective types.

Method 2) is a recent-ish idea. I've been saying for a while that method 1) is the only way to do this but that was based on a belief that program headers had to be valid when in fact the opposite is true, you don't really want them to be valid.



================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:588
+      if (Sec.Flags & (SHF_ALLOC | SHF_GROUP) && Sec.Type != SHT_NOTE) {
+        if (Sec.Type == SHT_SYMTAB) {
+          Sec.Flags &= ~SHF_ALLOC;
----------------
This branch doesn't need to exist IMO. Unless you have a real world use case for this branch we need to drop it.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:420
   replaceSectionReferences(const DenseMap<SectionBase *, SectionBase *> &);
+  virtual void clearSectionContents();
 };
----------------
I don't think this is the sort of general operation that we want TBH. It leaves sections in an odd state and changes their meaning without changing their underlying type. That's very unideal. Perhaps the only "non-general" operation we've added so far is 'markSymbols' and I fought against that preety hard. It's not at all clear to me we need this method yet.


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

https://reviews.llvm.org/D63807