Page MenuHomePhabricator

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

Authored by abrachet on Jun 26 2019, 12:07 AM.

Details

Summary

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

Diff Detail

Event Timeline

abrachet created this revision.Jun 26 2019, 12:07 AM
abrachet marked an inline comment as done.Jun 26 2019, 12:19 AM

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.

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 this revision to Diff 206789.Jun 27 2019, 12:52 AM
abrachet marked an inline comment as done.

Updated test case

abrachet marked an inline comment as done.Jun 27 2019, 12:56 AM
abrachet added inline comments.
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?

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 added inline comments.Jun 27 2019, 2:19 AM
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.

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

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 added a comment.EditedJul 2 2019, 12:03 AM
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

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.

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.

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.

kongyi added a subscriber: kongyi.Aug 7 2019, 3:53 PM
kongyi added a subscriber: pirama.