This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Implement --only-keep-debug for ELF
ClosedPublic

Authored by MaskRay on Sep 3 2019, 5:44 PM.

Details

Summary

--only-keep-debug is used to create separate debug files. It removes
contents of sections that are not useful for debugging purposes
(SHF_ALLOC sections that are not SHT_NOTE), by changing their section
types to SHT_NOBITS.

The intended use case is:

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

The current layout algorithm is incapable of deleting contents and
shrinking segments, so it is not suitable for implementing the
functionality.

This patch adds a new algorithm which assigns sh_offset to sections
first, then modifies p_offset/p_filesz of program headers. It bears a
resemblance to lld/ELF/Writer.cpp.

Event Timeline

MaskRay created this revision.Sep 3 2019, 5:44 PM
MaskRay abandoned this revision.Sep 3 2019, 8:34 PM
MaskRay retitled this revision from [llvm-objcopy] Add a new file offset assignment algorithm and support --only-keep-debug to [exposition-only][llvm-objcopy] Add a new file offset assignment algorithm and support --only-keep-debug.
MaskRay added a reviewer: pcc.Oct 24 2019, 3:45 PM
MaskRay added a subscriber: manojgupta.
MaskRay updated this revision to Diff 226509.Oct 25 2019, 3:33 PM
MaskRay retitled this revision from [exposition-only][llvm-objcopy] Add a new file offset assignment algorithm and support --only-keep-debug to [llvm-objcopy] Support --only-keep-debug.
MaskRay edited the summary of this revision. (Show Details)

Reclaim

Discussed with Manjoy, labath and others, we need a solution that rewrites p_offset/p_filesz so that we can do some symbolization without access to the original executable.

MaskRay removed a subscriber: jakehehrlich.

This looks highly promising to me BTW.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
574–577

I think this should happen last since it might affect the behavoir of other parts.

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

Funny enough this looks almost identical to the original layout algorithm used and is on of the primary reasons firstSection was exposed to begin with. Just a funny historical detail.

1976

Why not use Sec.ParentSegment->Align instead of MaxPageSize?

1986–1987

Can you add a comment explaining that FirstSec is nullptr generally means that we're in a non-allocated section.

1991

Flags not Type

1991–1994

I think only the non-allocated branch is needed here.

The first branch only makes sense if we already know that Sec.Type != SHT_NOBITS which we do because of the previous check for that above. The second branch makes complete since in both the allocated and non-allocated cases within the same segment.

2020–2026

This whole check feels really strange to me. Why would this happen? Why do we need to increase the size of the headers like this?

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

Please add a note here that this selects a layout algorithm and is *not* an instance of the config leaking into the ELFWriter which should never occur.

jakehehrlich added inline comments.Oct 28 2019, 11:53 AM
llvm/tools/llvm-objcopy/ELF/Object.cpp
2009

In the case where a Segment doesn't have a section this seems likely to go very poorly. For instance image a file with all the debug information after the allocated stuff but with a section-less segment in between the allocated content and the debug information. That would I think cause the offset of this segment to overlap with debug information which would be *very* bad. Perhaps we can add all of the section-less segments to a list and then assign them all MaxOffset with a FileSize of zero?

2020–2026

Ah I see why this is to handle the PT_LOAD covering the PT_PHDR. Ok this makes sense.

MaskRay updated this revision to Diff 226760.Oct 28 2019, 3:22 PM
MaskRay marked 11 inline comments as done.

Address review comments

MaskRay added inline comments.Oct 28 2019, 3:23 PM
llvm/tools/llvm-objcopy/ELF/Object.cpp
1976

Thanks. Deleting MaxPageSize can simplify the code a bit.

1991–1994

I agree. For non-first sections in a segment, we can just use its relative sh_offset distance to the first section.

I added it probably to emulate GNU objcopy. Reading it again, their choice looks weird and I don't think an executable that is linked by a linker (instead of hand-crafted) may need Off = Sec.Addr - FirstSec->Addr + FirstSec->Offset. I'll simplify the logic here.

2009

GNU objcopy seems to set p_offset of such PT_LOAD to 0. Rewriting p_offset and p_filesz is to make symbolization tools that rely on file offsets happy. I think such segments are not useful to the tool. p_filesz=MaxOffset probably is not better than p_filesz=old_p_filesz, so I will leave them unchanged.

Added another test.

MaskRay updated this revision to Diff 226763.Oct 28 2019, 3:28 PM

Forgot to update the second test

For clarity, is this change supposed to be instead of D67090? If so, please make sure the docs and help text are updated as part of it!

Also, for my own benefit, would you mind resummarising the expected behaviour of --only-keep-debug. It might be useful to add this as a comment in the code somewhere too.

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

It's not strictly needed, but it would be helpful to include the column headers here too, to make it easier to see which column is which.

50

If you specify an AddressAlign, you can ensure that this address will remain correct, even if more program headers are added or yaml2obj changes layout slightly. For example, you could specify AddressAlign: 0x400, and then Address: 0x400.

57

What happens if we have a progbits section before the note sections in this segment? NOBITS sections are usually at the end of segments.

62

Do we need to consider the weird layout of .tbss sections at all?

67

Any reason for this arbitrary size?

73

Ditto.

79

Does this matter?

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
591–592

If the symbol or string tables have SHF_ALLOC set, this will make them SHT_NOBITS. I'm not sure if that matters or not, but wanted to point it out.

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

Maybe worth updating this comment to say that maximum page size is the segment alignment.

Is this comment strictly true? What if there's a hole in the segment before the first section?

2001

Nit: sh_offset are -> sh_offset values have been

2020

I think the first part only needs to be Seg->Offset < HdrOffset. A segment at the end of the headers doesn't need to be related to them in any way.

2056

Here and elsewhere, could you rename this to HdrEnd or similar please? I thought it was the start of the headers when reading before, which would be the normal situation for an "Offset".

I had another thought. Right now the sections first layout algorithm is actually pretty general and doesn't make too much use of being specific to a particular flag. That's a good thing. That bad bit is that we're changing the section type.

As a consequence SHT_NOBITS has to be set for allocated sections in HandleArgs. However there are many allocated section types that require casting and this makes the casting break. In many contexts this winds up being valid but its a non-local contract that SHT_NOBITS is special for the sake of --only-keep debug and all casts have to consider it as potentially special. There are at least two ways out of this

  1. Add a separate type information field to SectionBase that remains unchanged...perhaps OrigionalType so that we can mutate Type. Then change all the classof functions to use OrigionalType. For consistency OrigionalFlags might be nice as well so that we don't have to do this again later.
  2. Remove the mutation on Type and leave it all functioning as is then add a check on OnlyKeepDebug in section header writing code so that the Type is always written out as SHT_NOBITS if the section is allocated. Additionally inside of the new layout algorithm you'll have to say "if the section is nobits or unallocated" rather than just checking for SHT_NOBITS as we did before.

I prefer #2 personally but it does make the new layout algorithm --only-keep-debug specific and probably won't allow it to be used for any future unforeseen flags that are as difficult as --only-keep-debug

jakehehrlich added inline comments.Oct 29 2019, 10:38 AM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
591–592

It matters. Thanks for bringing that up.

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

+1 this was really confusing to me as well.

MaskRay updated this revision to Diff 226965.Oct 29 2019, 2:12 PM
MaskRay marked 22 inline comments as done.

Add more tests

MaskRay added inline comments.Oct 29 2019, 2:12 PM
llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test
57

Added

## Only the tail of a segment can be trimmed. .text still occupies space because
## it is followed by .note which is not SHT_NOBITS.
# CHECK2:      Name        Type     Address          Off    Size   ES Flg Lk Inf Al
# CHECK2:      .text       NOBITS   0000000000000200 000200 000001 00  AX  0   0 512
# CHECK2-NEXT: .note       NOTE     0000000000000201 000201 000001 00   A  0   0  0
62

Added .tbss to demonstrate that .tbss can be handled as well.

67

This arbitrary non-zero size is used to show that p_filesz of the containg PT_TLS gets rewritten to 0.

67

Added a comment.

73

Added a comment.

79

Added a comment.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
591–592

For SHF_ALLOC .shstrtab, we will get: error: e_shstrndx field value 5 in elf header is not a string table

GNU objcopy handles the section by dropping SHF_ALLOC. I haven't ever seen a tool that can make e_shstrndx SHF_ALLOC, so it probably does not matter.


Added a test for SHF_ALLOC .symtab and .strtab. The behavior is consistent with GNU objcopy. The ELF spec says this is allowed but I don't think GNU ld or lld can create such components.

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

This looks stale after I removed MaxPageSize.

Updated the comment.

2056

Thanks for the suggestion. I agree HdrEnd is a much better name.

MaskRay updated this revision to Diff 226966.Oct 29 2019, 2:15 PM

reword a comment

Only two minor nits from me currently, plus these comments which I made out-of-line:

please make sure the docs and help text are updated

Also, for my own benefit, would you mind resummarising the expected behaviour of --only-keep-debug. It might be useful to add this as a comment in the code somewhere too.

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

You use hex for AddressAlign above and also for Addresses. I think it would make sense to make it hex here and throughout the rest of the file too.

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

Nit: "equals to" -> "equals" or "is equal to"

MaskRay updated this revision to Diff 227146.Oct 30 2019, 11:20 AM
MaskRay marked 3 inline comments as done.

Address review comments

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

Thanks!

Thanks. The existing changes look good, but the docs/help text still need updating, and I think the whole --only-keep-debug feature could do with an explanation somewhere in the code (i.e. what it does).

MaskRay updated this revision to Diff 227302.Oct 31 2019, 10:03 AM
MaskRay retitled this revision from [llvm-objcopy] Support --only-keep-debug to [llvm-objcopy] Implement --only-keep-debug for ELF.
MaskRay edited the summary of this revision. (Show Details)

Add comment in Object.cpp: assignOffsets

Add documentation

I'd still like to avoid changing the section type for the reason mentioned, at least until the appropriate point in finalization. I don't want this dangling assumption that casts don't always do what you think they should do.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
591–592

The issue is more the dynamic string table. The dynamic string table *should* be SHT_NOBITS but what matters it that the type information changes. Similar things need to happen for .dynamic and such. At the very least I don't want to carry around the assumption that the type information can change.

MaskRay marked an inline comment as done.Oct 31 2019, 11:15 AM
MaskRay added inline comments.
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
591–592

If changing the section type here does not look good, should I do it in ELFObject.cpp, after

if (!Config.SetSectionFlags.empty()) {
  for (auto &Sec : Obj.sections()) {
    const auto Iter = Config.SetSectionFlags.find(Sec.Name);
    if (Iter != Config.SetSectionFlags.end()) {
      const SectionFlagsUpdate &SFU = Iter->second;
      setSectionFlagsAndType(Sec, SFU.NewFlags);
    }
  }
}

?

MaskRay updated this revision to Diff 227316.Oct 31 2019, 11:18 AM

Move SHT_NOBITS code to handleArgs.

jhenderson added inline comments.Nov 1 2019, 2:32 AM
llvm/docs/CommandGuide/llvm-objcopy.rst
72

I don't think "separate" should be used here, as it implies to me that the output of this operation is not the normal output file. Perhaps change this sentence to "Produce a debug file as the output that only preserves contents of sections useful for debugging purposes."

75–76

I'd add "by making them SHT_NOBITS and shrinking the program headers where possible."

llvm/tools/llvm-objcopy/CommonOpts.td
89–90

Same comment as the documentation.

MaskRay updated this revision to Diff 227469.Nov 1 2019, 9:36 AM
MaskRay marked 5 inline comments as done.

Update documentation

jakehehrlich added inline comments.Nov 1 2019, 12:52 PM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
591–592

I'd like to avoid changing Type all together. See my original two proposals for how to fix it. One of them is a very minor tweak to the existing code that would only require changing a few lines of this patch.

I had another thought. Right now the sections first layout algorithm is actually pretty general and doesn't make too much use of being specific to a particular flag. That's a good thing. That bad bit is that we're changing the section type.

As a consequence SHT_NOBITS has to be set for allocated sections in HandleArgs. However there are many allocated section types that require casting and this makes the casting break. In many contexts this winds up being valid but its a non-local contract that SHT_NOBITS is special for the sake of --only-keep debug and all casts have to consider it as potentially special. There are at least two ways out of this

  1. Add a separate type information field to SectionBase that remains unchanged...perhaps OrigionalType so that we can mutate Type. Then change all the classof functions to use OrigionalType. For consistency OrigionalFlags might be nice as well so that we don't have to do this again later.
  2. Remove the mutation on Type and leave it all functioning as is then add a check on OnlyKeepDebug in section header writing code so that the Type is always written out as SHT_NOBITS if the section is allocated. Additionally inside of the new layout algorithm you'll have to say "if the section is nobits or unallocated" rather than just checking for SHT_NOBITS as we did before.

I prefer #2 personally but it does make the new layout algorithm --only-keep-debug specific and probably won't allow it to be used for any future unforeseen flags that are as difficult as --only-keep-debug

Sorry, I missed this comment. I understand your concern regarding the mutability of llvm::objcopy::elf::SectionBase::Type. Type is used by various static bool classof(const SectionBase *) overloads and mutating Type can change its result in different stages. My feeling is that we may actually need OriginalTypes and OriginalFlags, because Flags is also used in classof methods.

#2 requires us to do:

--- a/llvm/tools/llvm-objcopy/ELF/Object.cpp
+++ b/llvm/tools/llvm-objcopy/ELF/Object.cpp
@@ -70,7 +70,10 @@ template <class ELFT> void ELFWriter<ELFT>::writeShdr(const SectionBase &Sec) {
   uint8_t *B = Buf.getBufferStart() + Sec.HeaderOffset;
   Elf_Shdr &Shdr = *reinterpret_cast<Elf_Shdr *>(B);
   Shdr.sh_name = Sec.NameIndex;
-  Shdr.sh_type = Sec.Type;
+  if (OnlyKeepDebug && (Sec.Flags & SHF_ALLOC) && Sec.Type != SHT_NOTE)
+    Shdr.sh_type = SHT_NOBITS;
+  else
+    Shdr.sh_type = Sec.Type;

and add some code to both layoutSectionsForOnlyKeepDebug and layoutSegmentsForOnlyKeepDebug.

#1 as this patch currently does, can remain simple if we introduce OriginalTypes and OriginalFlags (D69739).

jakehehrlich accepted this revision.Nov 4 2019, 2:09 PM

LGTM

You did it!!!

This revision is now accepted and ready to land.Nov 4 2019, 2:09 PM
This revision was automatically updated to reflect the committed changes.
grimar added a subscriber: grimar.Jan 26 2021, 2:10 AM
grimar added inline comments.
llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test
218

There is an issue revealed by D95364 here. This should be SHT_SYMTAB, right?

llvm-objcopy reports an error when I change it:
error: Symbol table has link index of 4 which is not a string table

I guess the desired behavior is to drop the sh_link value as the new section is of SHT_NOBITS type?

llvm/docs/CommandGuide/llvm-objcopy.rst