GNU objcopy has two complimentary options to split files into debug and non-debug parts. --strip-debug is very simple. It's complementary option --only-keep-debug is a bit more complicated. --only-keep debug attempts to keep all section headers but remove any non-debug information it can. GNU objcopy appears to accomplish this by making every allocated section into a SHT_NOBITS section. The GNU objcopy man page explains that this is because the section headers have to be kept so that the debugger knows what parts of the file are what and can match the parts of the two files up. Note I plan on adding --add-gnu-debuglink and an option akin to -split-dwarf that will do all of these things at once for you in another commit.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Honestly, I'm a little confused by the complexity of this patch. It seems to me like changing a section to SHT_NOBITS should be pretty straightforward, since it's just changing the value of one field in a section header. Perhaps you could explain this a little more, and where necessary, add some code comments to the same effect.
Also, looking at the man page (I haven't tested this), but it seems to me like it's not just allocatable sections that get replaced, but any non-debug section. In other words, I'd expect the inverse of the predicate for --strip-debug to be used as the predicate for replacing. Is that not what you've observed?
test/tools/llvm-objcopy/replace-text-symtab.test | ||
---|---|---|
1 | It's not clear to me what the purpose of the "symtab" bit is in the test name. Also, since this is testing the only-keep-debug switch, I think the test should say so in the name. The "replace" part isn't really relevant. Same sort of comment could apply to the other two tests. | |
tools/llvm-objcopy/Object.cpp | ||
163 | I don't think this case is tested at all. | |
166 | Is the "this->" needed here? | |
246 | Test needed for this error case. | |
327 | Unnecessary "this->". Is this how clang-format formats this block? It's quite surprising if it is! | |
684 | I don't think this should be called "ToRemove", since it isn't removing anything, just replacing. | |
686 | Why do we do this extra sorting? Wouldn't it be easier (and probably faster) to just have an if(!ToRemove(*Sec)){continue;} inside the later for loop? | |
690 | Test for this case? | |
691 | And for these cases? | |
698–708 | Why do we need to do all this? Can't we just have the one line: Sec->Type = SHT_NOBITS and not create a completely new section? | |
tools/llvm-objcopy/llvm-objcopy.cpp | ||
171 | Full stop. Why not do the replacing after the removal? Then there's less to replace, potentially. |
Honestly, I'm a little confused by the complexity of this patch. It seems to me like changing a section to SHT_NOBITS should be pretty straightforward, since it's just changing the value of one field in a section header. Perhaps you could explain this a little more, and where necessary, add some code comments to the same effect.
The type is how we know how to cast things. It's effectively our RTTI (perhaps that wasn't a good choice). To be honest I think changing to SHT_NOBITS would probably be harmless but that wouldn't be the case for other section types and I'm not 100% sure it's safe. Moreover I don't know that I want to add the mental burdon of worrying about weather or not a class's section type changed or not at some point. Also when we call writeSection every writeSection would have to check if it was a NOBITS section or not. We could also check that case in the Object::writeSections. Layout would happen correctly I think so no changes would need to occur there. removeSectionReference could throw an error as when it should as well. In general it isn't clear to me what all the repercussions would be to just changing the type. If we create a new section then it's clear that there won't be any surprising issues.
Also, looking at the man page (I haven't tested this), but it seems to me like it's not just allocatable sections that get replaced, but any non-debug section. In other words, I'd expect the inverse of the predicate for --strip-debug to be used as the predicate for replacing. Is that not what you've observed?
Yeah that doesn't wind up being what it does in practice and I don't think we really want it to do that since there are other things like .note, .comment, .gnu.warning, etc might be nice to have. As near as I can tell it only makes the allocated sections into NOBITS sections. In general since the debug information is quite big I think keeping around extra possibly helpful information isn't too big of a deal. It also saves us from explicitly keeping a bunch of particular magic sections.
test/tools/llvm-objcopy/replace-text-symtab.test | ||
---|---|---|
1 | So my intent here was a bit more subtle but certainly I haven't made that clear in the test. I was specifically trying to trigger the case where the symbol table needs to replace a section reference for a symbol. So maybe this should be "replace-text-ref-in-symtab.test"? only-keep-debug just happens to be the option I used to do the replacing. Thinking about it, I never actually tried testing only-keep-debug. I only tested certain code paths. I should add that test. | |
tools/llvm-objcopy/Object.cpp | ||
163 | In practice this can't come up with the options we have now but I think it's important to throw an error if someone tries to replace the wrong thing in code. It's an artifact of a more general interface existing that can be used. I don't really like that being the case but having the error and never being able to test it seems better than not having the error and one day the code silently failing. I can be convinced otherwise though. | |
246 | This is another one of those errors that I can't test with the current interface. There isn't a way to get yaml2obj to produce a relocation that dosn't have .symtab as its link and there isn't a way to make .symtab allocated so there isn't a way to get --only-keep-debug to trigger this error. | |
686 | derp...Yeah that's way better. I was just blindly trying to refactor the code from removeSections and it pretty clearly wasn't all that relevant in the end. | |
691 | This is another error case I can't test because yaml2obj can't produce an allocated .shstrtab and --only-keep-debug can't make a non-allocated section into a NOBITS section. Maybe I should add a hidden --make-nobits=<section> option for the sake of testing this stuff. |
Fixed loop and ToRemove/ToReplace predicate
tools/llvm-objcopy/Object.cpp | ||
---|---|---|
690 | Again, I can't make an allocated symbol table. |
Maybe I should add a hidden --make-nobits=<section> option for the sake of testing this stuff.
I kind of like this idea, but if you wanted to do it, I might take it one step further and do something like --change-type=<section>,<new type> or similar. Maybe even something like --edit-shdr=<section index or name>,<new section properties> where <new section properties> is similar to the assembler .section directive, so takes the form <new name>,<flags>,<type>,..., and then I see no reason to make it hidden, since a tool that can effectively binary edit the section header fields is quite useful for wider testing, at the very least.
Okay, that makes a lot of sense (although your function is "replaceWithNoBits", not "replace with arbitrary type"). I missed the fact that you are creating generic "Section" instances, rather than one of the various section sub-classes, and that it has no contents, so what you do there is reasonable. I do think it needs some code comments to explain some of this though.
Yeah that doesn't wind up being what it does in practice and I don't think we really want it to do that since there are other things like .note, .comment, .gnu.warning, etc might be nice to have. As near as I can tell it only makes the allocated sections into NOBITS sections. In general since the debug information is quite big I think keeping around extra possibly helpful information isn't too big of a deal. It also saves us from explicitly keeping a bunch of particular magic sections.
In other words - don't rely on the documentation being complete or accurate!
test/tools/llvm-objcopy/replace-text-symtab.test | ||
---|---|---|
1 | OK. The test rename, and a new test specifically testing only-keep-debug would work for me. Please rename the other tests in a similar manner. | |
tools/llvm-objcopy/Object.cpp | ||
163 | Okay, that makes sense. I'd have probably written this as an assert, since we can't get into the case currently, but I'm happy with your approach here. Once again though, this is something that we could unit-test if we figured out how to set up unit tests for llvm-objcopy. | |
685 | Unnecessary blank line? |
I kind of like this idea, but if you wanted to do it, I might take it one step further and do something like --change-type=<section>,<new type> or similar. Maybe even something like --edit-shdr=<section index or name>,<new section properties> where <new section properties> is similar to the assembler .section directive, so takes the form <new name>,<flags>,<type>,..., and then I see no reason to make it hidden, since a tool that can effectively binary edit the section header fields is quite useful for wider testing, at the very least.
I'm writing code for --add-gnu-debuglink right now which requires section adding. Section adding raises some interesting questions like "what should OriginalOffset be?" If I can resolve those issues in a consistent way then "--edit-shdr <section>=<stuff>" can be "-R <section> -add-section <section>=<stuff>". Adding a specific section like the debuglink section is a tad different but the major technical challenge is the same. That would let us add tests for these strange tests without modifying yaml2obj. In general it would let us get around all the cases where yaml2obj holds us back.
- Added a test for the actual feature being added in this change
- Added some comments in replaceWithNoBits
- Removed that line
- I feel like I did something else that was mentioned as a thing I should do but I can't remember what it was.
I just realised one fairly major aspect of this that isn't tested, namely the behaviour when there are segments in play. Could you add tests that demonstrate what the behaviour is here - I expect the contents to effectively be wiped, by changing the file size to zero (although I'm not sure if that is what actually happens here - could you point out where it does, if it does). GNU objcopy's behaviour is a little weird around here - the segment offsets in a simple case I just tried were outside the file! I don't think we need to match it, but I would expect something sensible.
Yes, I like this idea. However, we'd need the ability to explicitly specify where the new section is placed in the ELF. Otherwise, we end up moving the section at the same time as modifying it, which may not be desirable.
I think it was the test renaming.
test/tools/llvm-objcopy/only-keep-debug.test | ||
---|---|---|
30 ↗ | (On Diff #124822) | Could you make this symbol at a non-zero offset, and then test that the value is preserved, please. |
37–46 ↗ | (On Diff #124822) | You should probably check that the types of .debugfoo, .note, etc haven't been changed as well. I would also recommend adding non-zero content to all sections, and testing that it is preserved for those that are not replaced. |
tools/llvm-objcopy/Object.cpp | ||
694 | make whole -> make a whole What does "delete the old way" mean? | |
tools/llvm-objcopy/llvm-objcopy.cpp | ||
171 | Ping. |
So I spoke with Roland McGrath about what should be happening with the program headers and it's kind of strange. Basically they should be kept exactly as is but that shouldn't mean that all that space is consumed because of them which demands a more complex change that I desired. The program headers are sometimes needed to match the contents due to prelink.
This presents a complication because right now segments are always laid out where as in this case we really should just ignore their existence and output them without considering them during layout. It feels a bit hacky but I suppose we can add a boolean flag for "ignore program headers during layout". Honestly the whole behavoir of --only-keep-debug is quite exceptional; it dosn't really like obeying the rules.
I agree that it is all a bit weird. Further investigation suggests that objcopy does the following:
- All allocatable sections are converted to NOBITS.
- The size of these sections is left unchanged.
- The offset of these sections is moved to the start offset of the program headers, before the change.
- The file size field is the only field of the program headers that changes, with the NOBITS sections no longer contributing to its size, as is normal. The address and offset remains the same.
- Sections not in segments are moved as normal, i.e. based on the updated segment sizes from before. They may move to before segments, if there is space for them to fit. I assume (but I haven't experimented with this) that they cannot overlap the spot where a segment should be (e.g. if the segment is at offset two, a 2-byte-sized non-alloc section won't appear at offset 1).
- The output file size does not include segments with their contents stripped.
I get the feeling that none of the above will happen with the current layout scheme... I don't have a good proposal for you at this point, other than to maybe consider a separate layout scheme entirely, that observes the above rules.
Done and agreed.
- The offset of these sections is moved to the start offset of the program headers, before the change.
I don't think there's any terribly good reason to do this honestly. I'd rather keep their index if possible.
- The file size field is the only field of the program headers that changes, with the NOBITS sections no longer contributing to its size, as is normal. The address and offset remains the same.
This is tricky. I'm going to upload some code that changes the file size accordingly but please don't bother approving it as a) it's subject to change and b) I'm not sure it's right or that I understand all the moving parts here.
- Sections not in segments are moved as normal, i.e. based on the updated segment sizes from before. They may move to before segments, if there is space for them to fit. I assume (but I haven't experimented with this) that they cannot overlap the spot where a segment should be (e.g. if the segment is at offset two, a 2-byte-sized non-alloc section won't appear at offset 1).
Yeah I think this is right and this property would hold right now (but the segments wouldn't be any smaller, they would just take up lots of space).
- The output file size does not include segments with their contents stripped.
I'm confused how this is different from 4. Mind elaborating?
As long as consumers of this are fine with that, I have no issues either way, although a non-zero relative offset within a segment with file size zero doesn't make much sense (it implies that the section does not sit inside the segment). The relative order of the sections is still controlled by the address fields, which remain unchanged.
- Sections not in segments are moved as normal, i.e. based on the updated segment sizes from before. They may move to before segments, if there is space for them to fit. I assume (but I haven't experimented with this) that they cannot overlap the spot where a segment should be (e.g. if the segment is at offset two, a 2-byte-sized non-alloc section won't appear at offset 1).
Yeah I think this is right and this property would hold right now (but the segments wouldn't be any smaller, they would just take up lots of space).
I think the point of reducing the file size field of the segments is so that they actually do not take up any space in the output file, reducing the overall file size by quite a bit potentially. See below.
- The file size field is the only field of the program headers that changes, with the NOBITS sections no longer contributing to its size, as is normal. The address and offset remains the same.
- The output file size does not include segments with their contents stripped.
I'm confused how this is different from 4. Mind elaborating?
- deals with the file size field of a segment, and indeed, it's actual size on disk, whereas 6) is to do with the offsets of the segments and the overall object file size. Typically, NOBITS sections do not take up any disk footprint, so zero-sized segments make sense. However, usually a segment will always reside within the bounds of the output file, i.e. p_offset <= file size. In this case however, p_offset might be greater than file size, because the segments are not moved, but also require zero space on disk. objcopy appears to shrink the object to as small as it can, whilst still preserving the remaining non-replaced section contents at the location wherever that may be.
As long as consumers of this are fine with that, I have no issues either way, although a non-zero relative offset within a segment with file size zero doesn't make much sense (it implies that the section does not sit inside the segment). The relative order of the sections is still controlled by the address fields, which remain unchanged.
Sorry, I got confused with offset and index here. The behavior I'd like here is for the offset to be set by the existing layout algorithm which will frequently be the top but sometimes will be the bottom and occasionally will be somewhere in the middle depending on how we set segment sizes. In this current patch the size of PT_LOAD segments and their children segments will always be zero so everything will have the same offset (the only offset possible). I agree keeping the original offset doesn't make sense.
I think the point of reducing the file size field of the segments is so that they actually do not take up any space in the output file, reducing the overall file size by quite a bit potentially. See below.
Right. Roland's claim was that you should just copy the segments and then layout the sections as if the segments didn't matter. Since NOBITS sections don't really need to consider segments to be laid out and non-allocated sections generally are not in segments this is actually quite valid but the program headers become bizarrely special. Also this fails when you have non allocated sections in segments which is something we've supported so far (correctly so I'd say). Roland takes the stance that it isn't clear what would be wrong with changing the filesize but believes that not changing it is certainly correct.
As for this patch. I've slept on it and I'm ok with this going in if you think everything else is in order. Particularly look at how I change segment size BTW as I believe I'm being a bit more aggressive that even GNU objcopy is being (since it appears to keep around RELRO segments where as I am not)
As well as the new inline comments, there are a number of older points that need addressing still, when you next make changes.
It's not clear to me why you remove the new NOBITS sections from the segments. We don't remove other NOBITS sections from segments after all.
I did some thinking, and I'm pretty confident I understand more about the need for section offsets to change to the start of the segment (i.e. point 3 above). By not moving the section offset, but reducing the segment file size, the section to segment mapping (as displayed with readelf -l) no longer matches following stripping, whereas GNU objcopy behaviour (i.e. moving the section to the start offset of the segment) means that it continues to match (readelf appears to treat zero-sized sections at the end of a segment as being in that segment). Furthermore, if you consider the case where the sections were NOBITS in the linker input, they would be emitted at the start offsets. Not moving the sections is starting to feel simply wrong to me.
test/tools/llvm-objcopy/only-keep-debug-phdrs.test | ||
---|---|---|
1 ↗ | (On Diff #125846) | Could you please either extend this test, or add a new test that demonstrates the behaviour when a non-alloc section is in a segment. |
3 ↗ | (On Diff #125846) | You might want to break this line over multiple lines using the '\' end-of-line mechanism. |
15–16 ↗ | (On Diff #125846) | Is this section supposed to be empty explicitly? If so, could you rename it .empty (to make the intention explicit) or similar, and if not, please add some content. |
59 ↗ | (On Diff #125846) | You should check the address and offsets of these sections, since we are messing about with layout with this switch. |
83 ↗ | (On Diff #125846) | You should check the value of these symbols, for the same reason as testing the section properties. |
103–105 ↗ | (On Diff #125846) | I think these fields need testing as well for the program headers, to show the behaviour. |
tools/llvm-objcopy/Object.cpp | ||
695 | with the new -> with references to the new |
I did some thinking, and I'm pretty confident I understand more about the need for section offsets to change to the start of the segment (i.e. point 3 above). By not moving the section offset, but reducing the segment file size, the section to segment mapping (as displayed with readelf -l) no longer matches following stripping, whereas GNU objcopy behaviour (i.e. moving the section to the start offset of the segment) means that it continues to match (readelf appears to treat zero-sized sections at the end of a segment as being in that segment). Furthermore, if you consider the case where the sections were NOBITS in the linker input, they would be emitted at the start offsets. Not moving the sections is starting to feel simply wrong to me.
Ah I just realized that I have to set the OriginalOffset to be the same as the parent segment. I was thinking for some reason that by changing the FileSize of the segments layout would handle it all for me but of course that's not right at all. Sorry about that.
It's not clear to me why you remove the new NOBITS sections from the segments. We don't remove other NOBITS sections from segments after all.
They don't affect anything by being in a segment or not being in a segment and I want the references gone. That all said I don't know why that method still exists because none of those things have been used in forever. I'm just going to get rid of all those empty methods in Segment since they're kind of vestiges of ideas that either weren't needed.
It's not clear to me what the purpose of the "symtab" bit is in the test name. Also, since this is testing the only-keep-debug switch, I think the test should say so in the name. The "replace" part isn't really relevant. Same sort of comment could apply to the other two tests.