- User Since
- May 16 2017, 11:34 AM (74 w, 4 h)
This just seems like a lit test in unit test form, why does this need to use unit tests and not lit tests?
LGTM, I'd file a bug about SmallString not taking const char* in its constructor and I'd also put a TODO somewhere to fix that once that's resolved.
Tue, Oct 9
Hey, sorry. I've been gone for 2 weeks between ICFP/Strange Loop and being in Seattle. Commit this now.
I think this is probably fine. I think having the error is better than not having it. I'm just concerned someone somewhere is using GNU objcopy in the way mentioned in the error. I'm fine treating this as an error and only switching behavior if and when someone hits an issue.
Fri, Sep 28
We have no such desire. I think we're fine being the test subjects for
Thu, Sep 20
So I'm good with this BTW.
Mon, Sep 17
Who put me in charge of this? I'm clearly unqualified.
-S should be an alias for --strip-all not --strip-all-gnu. There's seldom if ever going to be someone who rely's on the difference between the two. --strip-all-gnu exists as a way to allow people to hack llvm-objcopy into their system for entirely imagined use cases that we have not thus far actually seen. I'll be updating this patch.
Sep 12 2018
Sep 11 2018
I'll LGTM after Roland's fixes are done since I agree, that would be nice to also fix.
Oh also LGTM
When you write your diff use git diff -U<some big number> origin/master so that the diff is relative to what's upstream not your last change locally. I think git show might be understood by fabricator as well but I'm not 100% sure.
A couple nits but this LGTM.
Sep 10 2018
So I was thinking that we might try and detect compressed section at section construction time instead. The we can implement decompression in terms of CompressedSection...the only blemish seems to be that CompressedSection always allocates. I'm not super sure what the right solution to that is.
Sep 7 2018
Awesome! I'm looking at this now.
Sep 4 2018
Whoops...I meant to say the following already. I was confused why it was missing my LGTM.
Confirmed, using head solves this issue; I was just using a very stale checkout.
Hmm...this is odd...maybe I missed a change to yaml2obj. I'm rebuilding now.
When I run that yaml2obj test I actually get the following error rather than a silent failure.
Did you confirm that yaml2obj respects the explicit Link field?
Ah so I think this might be an issue with builders that don't build with zlib or that link zlib in in strange ways. I remember I mentioned (many weeks ago) that we'd need to #ifdef some of this stuff into compartments so that it didn't affect non zlib builds. I didn't remember to check for that before giving the LGTM. That is certainly killing at least some bots. I think the downstream bots that Petr mentioned are however actually linking in zlib. It isn't clear why this is breaking those but I'll try and look into that today.
Sorry, I didn't mean to accept this change. I was about to and then thought about the Link issue I mentioned in the test. I don't know how to cancel an approval in phabricator so I'm marking it as "request changes". Nothing is critically wrong here, I just want to make sure the test works as intended.
Aug 31 2018
I am satisfied that James' two remaining issues (as I understand them, more efficient looping in compressSections and minimal use of MCTargetOptions) have been solved and that mine have been solved.
Aug 29 2018
This is looking pretty close to good to me. I have a couple more nits about how alignment and RemovePred should work but other than that I think I'm good. If those are fixed and someone else accepts this revision before I get back to this feel free to land it without further input from me.
Aug 28 2018
Aug 27 2018
Aug 24 2018
Aug 23 2018
Aug 22 2018
Ok so a few higher level comments
Aug 21 2018
This is looking better!
Aug 14 2018
Aug 13 2018
Aug 9 2018
I don't think I want this option to do anything more than literally add a prefix to all section names. It isn't clear that those precise semantics are being relied on anywhere and the behavior of making relocation names and not renaming symtab etc... is a result of how bfd reconstructs the output binary without consideration for what the user initially put in which is opposite the stated goal of llvm-objcopy.
Sorry, I'm not ready to land this yet.
Awesome! I'd like to have someone else look over this but this pretty well LGTM.
Aug 8 2018
Nice. We need the std::string change for names for several other changes. LGTM.
oh god I apologize. I was wondering why this wasn't already submitted.
Aug 6 2018
So I think I want to remove -split-dwo as an option all together so that HandleArgs can just not worry about all of this.
Ah so I think I'm against BinaryReader being templated against ELFT. That is required to implement getElfType() however. getElfType drives which writer we create. That shouldn't be how things work however. I'll go and add my related comments to this to the previous patch. Sorry if I didn't see what was going on in the other patch and lead you astray. This patch makes things a bit more clear.
Aug 3 2018
Aug 2 2018
Make sure the commit message reflects this change but other than that LGTM
Speaking of that conflict I mentioned, it won't happen! So we're good to go. This change more or less LGTM. Please resolve Alex's comments however.
This LGTM other than one issue
Aug 1 2018
Due to the soon encroaching change where -I binary will be implemented I'm very much in favor of the OriginalData field being added to SectionBase. Really seems to handle everything very cleanly IMO.
I think I'd like to make a recommendation that we implement compression first and then decompression next.
I forsee a slight conflict between this and another change but it isn't clear how that will work out and I'm not sure what the best solution is. This change overall LGTM so maybe we'll just let things shake out and deal with that conflict as we have more concrete plans.
Compressed sections are a general kind of section. You can compress
literally any kind of section. All relocations are in terms of the
decompressed section. We can't use ELFSectionWriter on say symbol tables
because the actual data there isn't finalized until finalization. We don't
have to concern ourselves with how compression and modification interact
for debug sections because we don't modify those.
Jul 31 2018
Yeah I think (to within the margin of the details we've worked out) we seem to roughly agree @alexshap
I'm going to outline a proposal for how I think this stuff should work. It turns out I support creating a new section type after all (needed to support a few things like constructing the section name from a Twine and moving the compressed data into the Section's ownership in a clean way). I think this is a faithful version of what James (James please say so if this is not what you had in mind) and I have in mind for how to handle this (well I had to change my stance slightly). If the name ownership problem is solved and we figure out a nice way to plumb the 32-bit vs 64-bit information though that would be even better!
So here's an idea. It is ELF specific but it extends the SectionBase interface in a way I've considered for a while and would make both this and another change quite easy. If we add an OriginalData field (ArrayRef<uint8_t>) to SectionBase we can dump that inside of HandleArgs.
Can we add test? Otherwise this LGTM.
Jul 30 2018
Can you file a bug against me on the segfault? That sounds like a serious issue that needs to be addressed.
Thanks! I've been meaning to implement this forever. For instance this is the only way to dump non-allocated sections. I've run into cases debugging things where that was useful and had to use objcopy.
Jul 27 2018
Forgot to accept
LGTM, I had the same confusion James did. I have independently confirmed this is the correct behavior. As much as I dislike this behavior I think we need to match what GNU objcopy does here because this might result in observable differences in behavior (like a segfault if this is run on a ,o file and then linked)
Jul 26 2018
Also I wanted to say I'm pro this. This should be achieved regardless of which idea we settle on.
Yeah I really really think this should be handled by constructing new OwnedDataSections. The compression code should also be templated over an ELFT as well to enable using ELFT::Chdr.
Jul 25 2018
Also also, why do you need this? I ask because from talking with some people about what this is used for there is almost certainly a better way to accomplish any gain you get out of it. The main reason I can still see to do this is to achieve command line compatibility; if that is the goal we can probably noop or just noop with a section rename.
Also I think you can build llvm with or without zlib. I think that means you're going to need to #ifdef a lot of this code.
Jul 23 2018
Jul 20 2018
I want to keep the public interface of Object as small as possible but other than that LGTM
Jul 19 2018
The reason for it was just that this all seemed too complicated to me for "something so simple" but now that I've looked into the problem I think what you've done makes perfect sense.
Jul 18 2018
What would the consequence be of just converting the bytes to a string and writing that out? I have neither a windows box nor python 3 installed.
Jul 17 2018
Uh...I mean.."unobjectionable"...don't judge me.
Seems pretty non-objectionable to me. LGTM