Page MenuHomePhabricator

[llvm-objcopy][llvm-strip] Support --only-keep-debug
Needs ReviewPublic

Authored by MaskRay on Sep 3 2019, 3:22 AM.

Details

Summary

--only-keep-debug is used to create separate debug files. In GNU
objcopy, it makes non-SHT_NOTE SHF_ALLOC|SHT_GROUP sections SHT_NOBITS.
The intended use case is:

llvm-objcopy --only-keep-debug a a.dbg  # or llvm-strip --only-keep-debug a -o a.dbg
llvm-objcopy --strip-debug a b
llvm-objcopy --add-gnu-debuglink=a.dbg b

The current sh_offset assignment algorithm is incapable of deleting
space used by sections that are made SHT_NOBITS. Add a new one
customized for --only-keep-debug.

After the rewrite of sh_offset fields, p_offset/p_filesz in program
headers are no longer correct. Just delete program headers. gdb and lldb
can still debug b.

We cannot delete the sections that are made SHT_NOBITS, because gdb will
warn:

// gdb/symfile.c:addr_info_make_relative
warning: section .got not found in /tmp/a.dbg
warning: section .bss.rel.ro not found in /tmp/c/ccls.dbg
...

lldb does not have the warnings.

GNU objcopy does some more things like making SHT_GROUP sections
SHT_NOBITS. --only-keep-debug is not intended for object files and
SHT_GROUP are not kept in ET_EXEC/ET_DYN. We don't copy the behavior.

Diff Detail

Event Timeline

MaskRay created this revision.Sep 3 2019, 3:22 AM
MaskRay updated this revision to Diff 218442.Sep 3 2019, 5:58 AM

Delete a MemSize change

Delete a comment (layoutBinarySections and layoutSections cannot be merged)

preserve-segment-contents.test I'm not sure how to test this. They new layout algorithm no longer leaves holes in the file (the behavior matches GNU objcopy).

I don't see any update to this test. What are you planning on doing with it? We have deliberately decided to deviate from GNU objcopy's behaviour here, because there are many cases where we want to preserve the contents of data within a segment that aren't covered by sections. A good example is 0xcc padding introduced to conform to alignment rules, or linker script modifications. Another example is when --strip-sections has been run on an object, there are no more sections to cover the data, so the bytes need to be preserved.

A fairly major deliberate decision we've agreed on previously in llvm-objcopy is that the layout of sections within a segment is immutable, as is the majority of a program header's fields (p_offset being the only exception I can think of currently). The only thing that can be done is explicitly removing a section, which will cause the resultant hole to be filled with null bytes instead of the old data content. This is because it is impossible to fix up address references etc without effectively relinking the entire program, should sections be compacted within a segment.

I haven't yet looked at the detail of this change, but I wanted to make sure you are aware of these points. I think we can make exceptions to specific aspects of the rules outlined above to ensure certain switches function well, but I'd be reluctant to completely redo the layout algorithm to support such situations, unless the above rules are preserved.

MaskRay added a comment.EditedSep 3 2019, 9:17 AM
[ 1] foo               PROGBITS        0000000000002000 002000 000004 00      0   0  0
[ 2] bar               PROGBITS        0000000000002004 002004 000004 00      0   0  0

GNU objcopy (and this patch) can rewrite sh_offset to smaller addresses starting from 0x900 (assuming sizeof(ehdr)+sizeof(phdr) = 0x900)

[ 1] foo               PROGBITS        0000000000002000 000900 000004 00      0   0  0
[ 2] bar               PROGBITS        0000000000002004 000904 000004 00      0   0  0

If we think this behavior is too subtle, and want segments as immutable as possible (e.g. PR41005),
we can enable the new layout algorithm only when --only-keep-debug is specified. You can find more info in the description of D64906.
My feeling is that such optimization probably only makes sense in the linker. So, it is my mistake to implement it in llvm-objcopy.
(In BFD, the file offset assignment code is shared by GNU ld and objcopy. So their behaviors are very similar.)

To make things easier, we can diverge from GNU objcopy further:
let --only-keep-debug delete all sections except SHT_NOTE and .debug*, and also delete all segments.
Then we can use a very simple layout algorithm:

off = 0
for each note section sec
  off = alignTo(off, sec->align)
  off += sec->size
for each debug section sec
  off = alignTo(off, sec->align)
  off += sec->size

If this scheme looks good, I'll check if debuggers are still happy without phdr and try fixing segment-shift.test with a simpler approach.

The desired behavior is to keep the program headers intact as much as possible. elf utils for instance just flatout ignores them in this case. Program headers are needed by many tools to matchup the virtual addresses with the sections. I think outside of virtual address and memory size there isn't much that's actully needed in practice however. Ideally we wouldn't even change the file size of segments but I think I've structured llvm-objcopy in a way that makes that difficult unfortunately. I'm 100% ok using a distinct layout algorithm for --only-keep-debug as its a special snowflake where the meaning of segments drastically changes and segments are critical to layout. I'm also ok changing the file size of segments as I don't think its likely to cause a problem with tools like debuggers (there also exist other tools that aren't debuggers that need to perform the same kinds of actions).

Proposed behavoir:

  • Program headers are maintained exactly as they went in except their file sizes are set to zero. I think in the past I've pushed back on this but this flag is becoming such a pain point it might be the way forward.
  • All sections are maintained except that allocated sections are swapped to be SHT_NOBITS sections
  • To maintain the invariant that program headers are not touched elsewhere I'd ask that we only touch program headers in the --only-keep-debug case

If we're willing to set FileSize to zero and we have a good way to replace allocated sections do we need a new layout algorithm? If we still do I'm 100% ok with using it in the --only-keep-debug case.

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

@jhenderson I seem to recall you had a tricky non-allocated data in a segment case. Is this still kosher? Perhaps in that case the sections can write themselves and we don't have to worry about the interstitial data?

1930

I just realized this comment wasn't correctly updated when we fixed the naming convention. OrderSegments should be orderSegments. Would be nice if you could fix this for us but it isn't a requirement for this change.

The desired behavior is to keep the program headers intact as much as possible. elf utils for instance just flatout ignores them in this case. Program headers are needed by many tools to matchup the virtual addresses with the sections. I think outside of virtual address and memory size there isn't much that's actully needed in practice however. Ideally we wouldn't even change the file size of segments but I think I've structured llvm-objcopy in a way that makes that difficult unfortunately. I'm 100% ok using a distinct layout algorithm for --only-keep-debug as its a special snowflake where the meaning of segments drastically changes and segments are critical to layout. I'm also ok changing the file size of segments as I don't think its likely to cause a problem with tools like debuggers (there also exist other tools that aren't debuggers that need to perform the same kinds of actions).

For a separate debug file created by --only-keep-debug, the program header is not used. I tried removing all insignificant sections but gdb would give warnings: .got .dynsym ... etc cannot be found. So I'll stick with the "making insignificant sections SHT_NOBITS" approach. Because sh_offset fields are now rewritten, they no longer match p_offset/p_filesz in program headers. To avoid confusion, I'll just delete program headers.

  if (Config.OnlyKeepDebug)
   RemovePred = [RemovePred, &Obj](const SectionBase &Sec) {
     if (RemovePred(Sec))
       return true;
     if (Obj.SectionNames == &Sec)
       return false;
     if (Obj.SymbolTable == &Sec ||
         (Obj.SymbolTable && Obj.SymbolTable->getStrTab() == &Sec))
       return false;
     return !(Sec.Type == SHT_NOTE || isDebugSection(Sec));
   };

// gdb warns:
    warning: section .got not found in /tmp/a.dbg
    warning: section .bss.rel.ro not found in /tmp/c/ccls.dbg

Proposed behavoir:

  • Program headers are maintained exactly as they went in except their file sizes are set to zero. I think in the past I've pushed back on this but this flag is becoming such a pain point it might be the way forward.
  • All sections are maintained except that allocated sections are swapped to be SHT_NOBITS sections
  • To maintain the invariant that program headers are not touched elsewhere I'd ask that we only touch program headers in the --only-keep-debug case

    If we're willing to set FileSize to zero and we have a good way to replace allocated sections do we need a new layout algorithm? If we still do I'm 100% ok with using it in the --only-keep-debug case.
MaskRay updated this revision to Diff 218586.Sep 3 2019, 8:33 PM
MaskRay retitled this revision from [llvm-objcopy] Add a new file offset assignment algorithm and support --only-keep-debug to [llvm-objcopy][llvm-strip] Support --only-keep-debug.
MaskRay edited the summary of this revision. (Show Details)

Use the custom layout algorithm only for --only-keep-debug

This simplies the patch a lot.

The old approach is kept at D67137 for exposition purposes.

MaskRay marked 3 inline comments as done.Sep 3 2019, 8:55 PM
MaskRay added inline comments.
tools/llvm-objcopy/ELF/Object.cpp
1765

preserve-segment-contents.test is the tricky test. For the new approach for --only-keep-debug, program headers are deleted and sections' sh_offset are rewritten, writeSegmentData should not be called.

1930

There are a few others. Fixed in rL370838.

MaskRay edited the summary of this revision. (Show Details)Sep 3 2019, 8:57 PM
MaskRay edited the summary of this revision. (Show Details)Sep 3 2019, 9:03 PM
grimar added inline comments.Sep 4 2019, 2:02 AM
test/tools/llvm-objcopy/ELF/only-keep-debug.test
29

Perhaps not '.debug_*', but all non-alloc sections (looking at the logic implemented)?

What about adding non-alloc section that is not '.debug_*' to the test?

80

Do you need symbol?

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

Avoid auto?

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

Don't use auto?

MaskRay updated this revision to Diff 218627.Sep 4 2019, 2:54 AM
MaskRay marked 7 inline comments as done.

Address review comments: auto -> actual type

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

Thanks. I added a .comment to test it is not stripped.

80

This is to test --only-keep-debug does not strip .symtab

I guess it may be used by debuggers to help symbolize addresses, i.e. lib/DebugInfo/Symbolize* has logic to override DWARF results with .symtab. gdb and lldb may have similar behaviors but I'm not sure.

Added a comment.

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

Thanks. If I did D67143 first, I should have not written code like this..

Thanks for working on it. Have you tested if Linux perf can symbolize the perf traces using the split debug files especially with large binaries?

Meanwhile, I'll try the patch on Chrome OS to verify that gdb and perf still work.

MaskRay added a subscriber: labath.EditedSep 4 2019, 6:30 AM

@labath to confirm whether it works on lldb if the separate debug file (created by --only-keep-debug) has no program header.

I played with some executable and it appears to work.

labath added a comment.Sep 4 2019, 6:48 AM

@labath to confirm whether it works on lldb if the separate debug file (created by --only-keep-debug) has no program header.

I played with some executable and it appears to work.

Given how separate debug file handling is implemented in lldb (we just fetch some well known sections from the file (.debug_***, .symtab) and completely ignore everything else), I have a strong belief that this will not cause any problems in lldb. Even if it did cause some problem somehow, it does not sound like it would be too much to ask for lldb to handle this situation (the main file should always have the program headers, and it should be the authoritative source about the in-memory layout of the file).

We should certainly check perf. I'd also like to wait on this until Roland gets back in order for him to chime in since he had specific examples of use cases where program headers are required as I recall. Specifically the section virtual addresses are not sufficient in the arcane case of prelinking but prelinking isn't really something I know very much about.

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

ELFWriter is never allowed to depend on Config but you're more than welcome to add a flag of some kind to it to know if OnlyKeepDebug is being used.

You'll need to update the llvm-objcopy and llvm-strip documentation too, since the --only-keep-debug switch is currently listed as "COFF specific".

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

Not sure if it works in this situation, but a trick I've used on a number of occasions is to set AddressAlign to some nice round number. yaml2obj then aligns the section offset in the file accordingly, and it's much less likely that the address looks like just a random number.

For example:

- Name: .note
    Address: 0x400
    AddressAlign: 0x400
tools/llvm-objcopy/ELF/Object.cpp
1765

My feeling is that with --only-keep-debug we don't need to do the segment content preservation. The examples I had were a) the section headers were stripped, leaving data in the segments, and b) arbitrary data written to gaps between sections in a segment (e.g. padding bytes). I think in both cases, they are not going to be to do with debugging, so --only-keep-debug can ignore it.

In the case of a section within a segment that is non-alloc, we'd need to write the data still, if we want to strictly conform to what GNU objcopy does (I do slightly question some of the decisions it makes though - why keep NOTEs and arbitrary non-debug PROGBITS sections for example?). An obvious example of this is non-alloc SHT_NOTE sections, but I have seen other examples of non-NOTE sections inside program headers before (particularly platform-specific program headers). However, in every case I know of (aside from the NOTE sections), there's no reason to keep that data.

Anyway, my understanding of the latest version of the code is that sections in segments will still have their data written, so much of this is moot.

MaskRay updated this revision to Diff 218870.Sep 5 2019, 2:32 AM
MaskRay marked 2 inline comments as done.

Update docs/CommandGuide/llvm-objcopy.rst
Use a bool instead of const Config & in ELFWriter

MaskRay added a comment.EditedSep 5 2019, 3:13 AM

Thanks for working on it. Have you tested if Linux perf can symbolize the perf traces using the split debug files especially with large binaries?

Meanwhile, I'll try the patch on Chrome OS to verify that gdb and perf still work.

I've learned that @manojgupta actually has a different use case: symbolization without requiring access to the stripped binary. To support that use case, stripping program headers + simple section layout:

+static uint64_t layoutSectionsForOnlyKeepDebug(Object &Obj, uint64_t Off) {
+  uint32_t Index = 1;
+  for (SectionBase &Sec : Obj.sections()) {
+    Sec.Index = Index++;
+    Sec.Offset = alignTo(Off, Sec.Align ? Sec.Align : 1);
+    if (Sec.Type != SHT_NOBITS)
+      Off = Sec.Offset + Sec.Size;
+  }
+  return Off;
+}

cannot be used. We'll need a full-fledged layout algorithm that rewrites p_offset/p_filesz as well as sh_offset - much like the one used in lld/ELF/Writer.cpp (https://reviews.llvm.org/D67137). The problem will be conflicting sources of information.

@jakehehrlich may have a similar use case:

We should certainly check perf. I'd also like to wait on this until Roland gets back in order for him to chime in since he had specific examples of use cases where program headers are required as I recall. Specifically the section virtual addresses are not sufficient in the arcane case of prelinking but prelinking isn't really something I know very much about.

I think going the program header rewriting route needs more thoughts. It should be a separate change. It is also fairly difficult to validate because an end-to-end test going through their symbolization pipeline is likely required.

@kongyi and @pirama What's your use case? Do you need program headers?

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

It works for the first section but not for subsequent sections which are in different PT_LOAD segments.

I want to teach yaml2obj to write section headers at the end so that the offsets will not change when we add/delete sections.

labath added a comment.Sep 5 2019, 4:05 AM

Thanks for working on it. Have you tested if Linux perf can symbolize the perf traces using the split debug files especially with large binaries?

Meanwhile, I'll try the patch on Chrome OS to verify that gdb and perf still work.

I've learned that @manojgupta actually has a different use case: symbolization without requiring access to the stripped binary.

I don't want to digress to much, but since you've mentioned this use case... The main problem I encountered when trying to debug something (with lldb) without the stripped binary was that the separate debug file does not contain the eh_frame section. This meant that lldb was unable to unwind past the first frame, or if it did, the backtrace was largely inaccurate. If you're doing unwinding in this scenario, I'd be curious to hear how you get around this problem.