- User Since
- May 16 2017, 11:34 AM (26 w, 6 d)
Fri, Nov 17
- Added a ForDefinition parameter to setGlobalVisibility and merged visibility setting behaviors for setGlobalVisibility and setLinkageAndVisibilityForGV
- Renamed setLinkageAndVisibilityForGV to setLinkageForGV
- refactored calls to these two functions to match their new form.
Wed, Nov 15
- I implemented the semantics I presented above. Also note that my proposal for llvm-objcopy simplifies to "copies have priority over removes". I describe the behavior this way in the comment I added for this behavior.
- I added a -keep operation
- All the SectionDump non-sense is gone
Tue, Nov 14
Ok the following behavior seems consistent with my testing. There are 3 things that can happen to a section (and a 4th default thing sort of)
Ah crap this isn't right either because this dosn't do what GNU objcopy does in the -j case where multiple sections are kept. I'll investigate more.
Yikes, here's an update on what GNU objcopy does. It interprets "keep" and "remove" differently. Namely, "keep" is not "remove all but". If you "remove" and "keep" a section then an error is thrown. --only-keep appears to be "remove all but" plus "keep". I don't think this behavior is so terrible to implement. We would implement --only-keep so that it, in an order independent way, removes all sections but the named one (and certain special sections obviously) then additionally adds the section to a keep list. We then, before performing removal, iterate though the keep list to see if we're about to remove anything on the keep list. If we detect that we are, we throw an error. This a general way of implementing this keep functionality so as new options come up there will be a standard way of handling it.
- Added test for setting alignment of first segment to zero
- Added description of the parent cycle test.
Mon, Nov 13
Replaced explicit types with auto as recommended. Thanks Davide!
- Added test (derp, it turns out I wrote it but didn't run git add on it so it didn't make it into the diff)
- renamed "only-keep-alloc" with "strip-non-alloc" everywhere.
Whoops! Turns out I added a bug in this change where certain kinds of cycles in ParentSegment could form because there wasn't a check to make sure that compareSegments(Parent, Child) held if Child->ParentSegment was nullptr. This caused -O binary to fail in our kernel dump. I fixed this and added a test case. Additionally the way I modified the first segment wasn't exactly right.
This has already landed, I'm not sure why it wasn't automatically closed.
I find it slightly surprising that yaml2obj automatically assumes relocation sections with SHF_ALLOC to be dynamic relocation sections, given we're working with an ELF of ET_REL type. Given that though, this is okay.
Fri, Nov 10
Knowing nothing about elf2yaml this LGTM
Ok I think this should just follow what GNU objcopy does for mass consistency but I'm also going to add --only-keep-allocated to do what I intended this to do in the first place
Mostly looks good to me, I want someone to review the ELFDumper and elf2yaml changes but the ELFYAML and yaml2elf changes look good. I just have one request about making .dynstr allocated (see inline).
Another option would be to restrict CLANG_DEFAULT_OBJCOPY to contain objcopy or even to only allow objcopy or llvm-objcopy. I don't know if that's sensible though...
Thu, Nov 9
Fixed more comments
So I'm wondering if we should be following what GNU objcopy does here. After talking with Roland for a bit I'm back to thinking that removing non-allocated sections is the way to go. What I think we want --strip-all to do is minimize the size of a fully linked executable while maintaining any relevant section headers of the content still in the file. For instance I want .dyn* sections to be kept so that I can run --strip-all on shared objects and still link against them. In general I'm not sure we should be mimicking what GNU objcopy does just for the sake of doing what GNU objcopy does as long as the feature in question is attached with some understandable intent. I think --strip-all has just such an intent; namely it should be used when you want to reduce the size of a fully linked object but you don't want to change what can be done with the object with the exception of debugging.
Wed, Nov 8
I improved the test to test the removal cases I mentioned.
- Changed the removal criteria to be conservative rather than aggressive
Oh and one more behavoir I found. It will never remove an allocated section, even if it is named debug_*. This makes a lot of sense for dynamic symbol tables and relocation and it makes more sense than not for debug sections.
- Fixed some comments
- Added reference in loop over AllocatedSections
Tue, Nov 7
I'm happy with D39749 and using that to resolve the ordering isue. I'm not a reviewer here but this looks good to me other than the ternary operator nit.
Overall looks good to me. I just have one nit about the lookup function.
Mon, Nov 6
Fri, Nov 3
It would appear some kind stranger made some sweeping improvements to the code base that caused conflicts with the code I wrote. I never added those 'static' clauses (but they're good) and I never used the "using" keyword (but I support it). See here: https://reviews.llvm.org/rL317123
Thu, Nov 2
In general this looks like a good approach to me. I have concerns about deciding section indexes for users as mentioned but other than that I just have a couple nits.
Can you add tests?
Also, as more stripping options become available I think coming up with a system to handle the removal predicate would be nice. I might write that patch soon.
When you talk about relative offsets of sections, are you talking about relative to the segment start? If so, I agree that appears to be the objcopy behaviour, and I'm happy for this to be done in llvm-objcopy. With that change, we should be able to drop the SectionDump class, as the BinaryObject class should give us the functionality we expect for -j, along with the other cases. I think it might be wise to make these changes in a separate pre-requisite review, since it's really a separate issue (e.g. the behaviour we currently have for -R is different to gnu objcopy's, if the first or last section is stripped).
Wed, Nov 1
Can you show me which inputs were not preserving relative offsets of sections within the same segment? I gave up trying to read the code and went back to empirical testing. After getting many more data points my theory about the behavior is the following
- Cleaned up command line argument description. I decided to keep the split-dwo option. For the one use case of dwarf fission that I'm aware of (clang) that's a better option to use. I agree that not applying options to the DWO file is more sensible.
- removed/replaced all references to "GSplitDwarf"
Tue, Oct 31
ping, I haven't heard back on this yet.
- Added test for -O binary case
- That test still made no sense. It should test the fact that even though the second segment has alignment 0x1000 it can't still have offset 0x1008.
- Added section count to tests to avoid false positives.
- Fixed option descriptions.
- Repalced "Dwo" with "DWO" everywhere including in cl::opt types.
- Changed "-gsplit-dwarf" to "-split-dwo" but I think this should actually be removed. I'm waiting to hear back before I remove it.
- Moved some comments around and altered them.
- Removed extraneous passing of Obj and additionally removed the template.
- Deleted config.host_arch as a feature
- Changed host-arch-is64bit to llvm-64-bits
- Changed code to by more idiomatic as pointed out by Zach
Mon, Oct 30
Fixed the test case with all the strange addresses that made no sense.
Rafael recommended that I split this into two separate changes. This change makes that split by removing the if-statment change that fixed default alignment.
- Fixed typos
- Changed --only-keep so that it keeps the symbol table and string table for the symbol table
- Fixed comments