[llvm-objcopy] Add --only-keep-debug for ELF

[MaskRay]

Summary

Implements --only-keep-debug for ELF, previously only implemented for COFF.

[abrachet]

I'll want to add more tests later, along with making the current test better, its pretty bare right now. I'd like to test the keep debug, remove debug and add them back in with --add-gnu-debuglink too. But there are many things that I would like to iron out first, and just get the patch initially looked at, hopefully I'm not missing why this wasn't here before.

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

588

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 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.

[jhenderson]

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

llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test

19–21

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).

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

588

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?

llvm/tools/llvm-objcopy/ELF/Object.cpp

85

This would usually be llvm_unreachable. However, I'm wondering why you're not just ignoring this case?

[abrachet]

Updated test case

[abrachet]

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

588

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

llvm/tools/llvm-objcopy/ELF/Object.cpp

85

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?

[MaskRay]

It'll be good to check if the following workflow works:

# Say the executable is a
llvm-objcopy --only-keep-debug a a.dbg
llvm-objcopy --strip-debug a b
llvm-objcopy --add-gnu-debuglink=a.dbg b
# gdb b

llvm/tools/llvm-objcopy/ELF/Object.cpp

85

This will abort for DynamicRelocationSection (.rela.dyn)

1009

this-> is redundant.

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test

3–5

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.

17

Replace this with something like Size: 4, since the exact contents don't matter.

20

Why is this SHF_ALLOC?

21

I don't think your contents here need to be valid. Just an arbitrary size.

24

Ditto.

26

Let's use the precise SHF_* and SHT_* terminology, to avoid any ambiguity.

Also "Checks" -> "Check"

29

Nit here and throughout the rest of the test, reduce the double space to a single space.

37

Um... That Size doesn't match up with the specified contents?

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

588

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.

llvm/tools/llvm-objcopy/ELF/Object.cpp

85

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.

[MaskRay]

You need to update the help message as well:

// llvm-objcopy/ObjcopyOpts.td
def only_keep_debug
    : Flag<["--"], "only-keep-debug">,
      HelpText<"Clear sections that would not be stripped by --strip-debug. "
               "Currently only implemented for COFF.">;

[abrachet]

Thanks @MaskRay.

I have had some trouble completely emulating how GNU objcopy implements --only-keep-debug, but as @jakehehrlich mentioned in D63820 on this comment, it isn't imperative that this be as aggressive as GNU objcopy. Do you have thoughts on just doing the common case, just Sec.Flags & (SHF_ALLOC | SHF_GROUP) && Sec.Type != SHT_NOTE and not having SectionBase::clearSectionContents abort. Also I will move the sections to the start of their segments as @jhenderson described in D40523. Does that sound like an acceptable implementation? I fear that I am missing things that would make this a buggy implementation, which is worse than none I believe.

[MaskRay]

Do you have thoughts on just doing the common case, just Sec.Flags & (SHF_ALLOC | SHF_GROUP) && Sec.Type != SHT_NOTE and not having SectionBase::clearSectionContents abort

This rule looks good enough.

Also I will move the sections to the start of their segments as @jhenderson described in D40523.

My feeling is that segments are not important. The offsets of those NOBITS sections are not important. If segments are laid out weirdly after objcopy --only-keep-debug, I think it doesn't matter if llvm-objcopy also fails to layout the segments in a sensible way. The important thing here is that debuggers can correctly load debug info from such --only-keep-debug processed files. So I asked you test the workflow:

# Say the executable is a
llvm-objcopy --only-keep-debug a a.dbg
llvm-objcopy --strip-debug a b
llvm-objcopy --add-gnu-debuglink=a.dbg b
# gdb b  and lldb b

[jakehehrlich]

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.

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

588

This branch doesn't need to exist IMO. Unless you have a real world use case for this branch we need to drop it.

llvm/tools/llvm-objcopy/ELF/Object.h

420

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.

[jakehehrlich]

My feeling is that segments are not important. The offsets of those NOBITS sections are not important. If segments are laid out weirdly after objcopy --only-keep-debug, I think it doesn't matter if llvm-objcopy also fails to layout the segments in a sensible way. The important thing here is that debuggers can correctly load debug info from such --only-keep-debug processed files. So I asked you test the workflow:

I'm unclear on why sh_vaddr can't be used to match up sections. I'm curious as to how debuggers actually do this. It would be great if we can just ignore everything I said previously and only output sections. There's a third even easier way to do this if that's possible. We can just synthesize a section only version and have it go though the normal code paths separately.

[jakehehrlich]

Speaking with Roland on this it seems to be that the established protocol for how you match up vaddrs is to use the program headers. Roland invoked arcane scary references about prelinking and other sharp corners about other possible methods but I don't know or understand the details. The guidance I'm getting is that we need to keep the program headers virtual address space correct. He has stated the program headers are used by many existing tools and that the consumers are more varied than just the two major debuggers. If you need more than a "here be dragons" from Roland I'll probe for more information but I'm content enough with just knowing that the existing protocol requires using program headers.

[MaskRay]

We committed https://reviews.llvm.org/D67137

llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test

+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy --only-keep-debug %t %t.dbg
+# 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
+
+!ELF
+FileHeader:
+  Class:     ELFCLASS64
+  Data:      ELFDATA2LSB
+  Type:      ET_REL
+  Machine:   EM_X86_64
+Sections:
+  - Name:    .text
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+    Content: "c3c3c3c3"
+  - Name:    .note
+    Type:    SHT_NOTE
+    Flags:   [ SHF_ALLOC ]
+    Content: 040000001000000003000000474E55004FCB712AA6387724A9F465A32CD8C14B
+  - Name:    .debug
+    Type:    SHT_PROGBITS
+    Content: "031F"
+
+## Checks that alloc segment is marked no bits.
+## It has no contents but size should stay the same.
+# CHECK:       Index: 1
+# CHECK-NEXT:  Name: .text
+# CHECK-NEXT:  Type: SHT_NOBITS
+# CHECK-NEXT:  Flags [ (0x6)
+# CHECK-NEXT:   SHF_ALLOC (0x2)
+# CHECK-NEXT:   SHF_EXECINSTR (0x4)
+# CHECK-NEXT:  ]
+# CHECK-NEXT:  Address:
+# CHECK-NEXT:  Offset:
+# CHECK-NEXT:  Size: 64
+
+## SHT_NOTE does not change.
+# NOTE:        Index: 2
+# NOTE-NEXT:   Name: .note
+# NOTE-NEXT:   Type: SHT_NOTE
+# NOTE-NEXT:   Flags [ (0x2)
+# NOTE-NEXT:    SHF_ALLOC (0x2)
+
+## .debug sections should never change.
+# DEBUG:       Index: 3
+# DEBUG-NEXT:  Name: .debug
+# DEBUG-NEXT:  Type: SHT_PROGBITS
+# DEBUG-NEXT:  Flags [ (0x0)

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

else if (Config.DecompressDebugSections)
     replaceDebugSections(
         Obj, RemovePred,
         [](const SectionBase &S) { return isa<CompressedSection>(&S); },
         [&Obj](const SectionBase *S) {
           auto CS = cast<CompressedSection>(S);
           return &Obj.addSection<DecompressedSection>(*CS);
         });
 
-  return Obj.removeSections(Config.AllowBrokenLinks, RemovePred);
+  if (Error E = Obj.removeSections(Config.AllowBrokenLinks, RemovePred))
+    return E;
+
+  if (Config.OnlyKeepDebug) {
+    for (auto &Sec : Obj.sections()) {
+      if (Sec.Flags & (SHF_ALLOC | SHF_GROUP) && Sec.Type != SHT_NOTE) {
+        if (Sec.Type == SHT_SYMTAB) {
+          Sec.Flags &= ~SHF_ALLOC;
+          continue;
+        }
+        Sec.Type = SHT_NOBITS;
+        Sec.clearSectionContents();
+      }
+    }
+  }
+
+  return Error::success();
 }
 
 // This function handles the high level operations of GNU objcopy including
 // handling command line options. It's important to outline certain properties
 // we expect to hold of the command line operations. Any operation that "keeps"
 // should keep regardless of a remove. Additionally any removal should respect
 // any previous removals. Lastly whether or not something is removed shouldn't
 // depend a) on the order the options occur in or b) on some opaque priority

llvm/tools/llvm-objcopy/ELF/Object.h

public:
   removeSectionReferences(bool AllowBrokenLinks,
                           function_ref<bool(const SectionBase *)> ToRemove);
   virtual Error removeSymbols(function_ref<bool(const Symbol &)> ToRemove);
   virtual void accept(SectionVisitor &Visitor) const = 0;
   virtual void accept(MutableSectionVisitor &Visitor) = 0;
   virtual void markSymbols();
   virtual void
   replaceSectionReferences(const DenseMap<SectionBase *, SectionBase *> &);
+  virtual void clearSectionContents();
 };
 
 class Segment {
 private:
   struct SectionCompare {
     bool operator()(const SectionBase *Lhs, const SectionBase *Rhs) const {
       // Some sections might have the same address if one of them is empty. To
       // fix this we can use the lexicographic ordering on ->Addr and the
public:
   explicit Section(ArrayRef<uint8_t> Data) : Contents(Data) {}
 
   void accept(SectionVisitor &Visitor) const override;
   void accept(MutableSectionVisitor &Visitor) override;
   Error removeSectionReferences(bool AllowBrokenLinks,
       function_ref<bool(const SectionBase *)> ToRemove) override;
   void initialize(SectionTableRef SecTable) override;
   void finalize() override;
+  void clearSectionContents() override;
 };
 
 class OwnedDataSection : public SectionBase {
   MAKE_SEC_WRITER_FRIEND
 
   std::vector<uint8_t> Data;
 
 public:
OwnedDataSection(const Twine &SecName, uint64_t SecAddr, uint64_t SecFlags,
     Addr = SecAddr;
     Flags = SecFlags;
     OriginalOffset = SecOff;
   }
 
   void appendHexData(StringRef HexData);
   void accept(SectionVisitor &Sec) const override;
   void accept(MutableSectionVisitor &Visitor) override;
+  void clearSectionContents() override;
 };
 
 class CompressedSection : public SectionBase {
   MAKE_SEC_WRITER_FRIEND
 
   DebugCompressionType CompressionType;
   uint64_t DecompressedSize;
   uint64_t DecompressedAlign;

llvm/tools/llvm-objcopy/ELF/Object.cpp

template <class ELFT> void ELFWriter<ELFT>::writeShdr(const SectionBase &Sec) {
   Shdr.sh_offset = Sec.Offset;
   Shdr.sh_size = Sec.Size;
   Shdr.sh_link = Sec.Link;
   Shdr.sh_info = Sec.Info;
   Shdr.sh_addralign = Sec.Align;
   Shdr.sh_entsize = Sec.EntrySize;
 }
 
+void SectionBase::clearSectionContents() {
+  assert(0 && "Cannot clear this sections contents");
+}
+
 template <class ELFT> void ELFSectionSizer<ELFT>::visit(Section &Sec) {}
 
 template <class ELFT>
 void ELFSectionSizer<ELFT>::visit(OwnedDataSection &Sec) {}
 
 template <class ELFT>
 void ELFSectionSizer<ELFT>::visit(StringTableSection &Sec) {}
 
void OwnedDataSection::appendHexData(StringRef HexData) {
   assert((HexData.size() & 1) == 0);
   while (!HexData.empty()) {
     Data.push_back(checkedGetHex<uint8_t>(HexData.take_front(2)));
     HexData = HexData.drop_front(2);
   }
   Size = Data.size();
 }
 
+void OwnedDataSection::clearSectionContents() {
+  Data.clear();
+}
+
 void BinarySectionWriter::visit(const CompressedSection &Sec) {
   error("cannot write compressed section '" + Sec.Name + "' ");
 }
 
 template <class ELFT>
 void ELFSectionWriter<ELFT>::visit(const CompressedSection &Sec) {
   uint8_t *Buf = Out.getBufferStart() + Sec.Offset;
   if (Sec.CompressionType == DebugCompressionType::None) {
LinkSection =
       SecTable.getSection(Link, "Link field value " + Twine(Link) +
                                     " in section " + Name + " is invalid");
   if (LinkSection->Type == ELF::SHT_SYMTAB)
     LinkSection = nullptr;
 }
 
 void Section::finalize() { this->Link = LinkSection ? LinkSection->Index : 0; }
 
+void Section::clearSectionContents() {
+  this->Contents = ArrayRef<uint8_t>();
+}
+
 void GnuDebugLinkSection::init(StringRef File) {
   FileName = sys::path::filename(File);
   // The format for the .gnu_debuglink starts with the file name and is
   // followed by a null terminator and then the CRC32 of the file. The CRC32
   // should be 4 byte aligned. So we add the FileName size, a 1 for the null
   // byte, and then finally push the size to alignment and add 4.
   Size = alignTo(FileName.size() + 1, 4) + 4;
   // The CRC32 will only be aligned if we align the whole section.