- User Since
- May 16 2017, 11:34 AM (81 w, 5 d)
Fri, Dec 7
Looks like a simple mechanical transformation. LGTM.
Thu, Dec 6
Also I'll put up a review for a change that triggers the overflow error and other things. One moment.
Thanks! This looks good to me.
Wed, Dec 5
Rebased and used -only-section (no s at the end)
Mon, Dec 3
Also don't focus on byte for byte accuracy, test semantically so that we can make layout changes independent of content changes. It's clear to me now that we shouldn't even bother attempting exact binary matches even in the first patch.
On the whole ELFObject vs Object thing. I'm in favor of just using Object but I don't really care too much. Having both the namespace and the name prefix is kind of verbose. A while back we had two kinds of Objects instead of just one but this was determined overtime to be a mistake for the ELF backend. If you want to do conversions between formats, even if the format is "binary" you'll probably want to avoid having two kinds of Object or if you do make sure you provide a sufficiently rich base interface to them to allow most code to be written in terms of that.
Aight, uploading the final patch so people can see exactly what I'm going to push.
Tue, Nov 27
Oh and you'll need to remove the dependency on libObject as discussed. I pointed out that only changes I think you'll need to make but you might find a few more.
LGTM with nits. Please make sure Jurgen and you agree on the namespace and folder scheme first but other than than this LGTM.
Would there be an objection to using llvm::tapi::MachO or llvm::tapi::macho? At least for ELF it doesn't make much sense to put the tapi code in the same namespace as BinaryFormat/ELF so it would be nice to add the extra namespace. I'm not married to the idea however.
I think this is fine. GNU compatibility is a tricky subject with lots of nuances that I keep discovering but it is decidedly a stated goal.
I had a few bike-shed level comments. I'll dig into the code more deeply soon but my near complete lack of knowledge of MachO makes giving any actually useful feedback hard.
Added Jurgen who should be able to help with the MachO review (or finding a reviewer) as well.
Responded to latest comments.
Mon, Nov 26
Mon, Nov 19
Also I'll be gone this week for Thanksgiving. I might look at this off and on but don't expect too much this week
I like the planned direction for features here! I'm going to add @echristo to this since I think he knows MachO reasonably well and would know someone who can review those aspects for us.
Thu, Nov 15
Wed, Nov 14
- Switched to expected
- Used slice
- Formatted td file
Tue, Nov 13
If we wanted we could also add an alias for -k but I don't really think that matters too much.
Mon, Nov 12
Not technically something we can't work around, the following code solves my problem now that I understand it.
So I fixed this issue: https://reviews.llvm.org/D54451
- Added test that would have caused undefined behavior before but should be solid now.
Sat, Nov 10
I started to rewrite it in terms of ELFFile. The main challenge nis dealing
with the Error object that we don't really care about.
Nov 9 2018
Nov 6 2018
I'm not where I can use phab but the diff and reason for the change look
good to me.
Nov 5 2018
Oct 31 2018
Let's not break compaitableity then and just check the index. The fewer
cases this happens in the better IMO. I guess basically what you had at the
start was actually the best.
Oct 30 2018
I spoke with Roland. Roland basically agrees that there's no good reason for this to function this way (also pointed out that SHN_COMMON is a legacy thing)...but he also agrees that none of that matters because we need to support this case. So LGTM pending anyone else's code/test specific issues.
Ah fair. That looks like many cases we've unfortunately seen before (see
SHF_LINK and SHF_COMPRESSED) that are not always respected (and thus we
must support the old case as well). So disregard my comment about using the
type. That's what I get for blindly reading the spec.
Oct 29 2018
Looks pretty close. My major concerns are with the currently chosen interface but I think we can shake that out later.
Another thought I had was that we could add an escape hatch for specific symbols the way "--keep" adds an escape hatch for specific sections. It's as if we have a few operations now
This seems correct to me. LGTM.
Oct 25 2018
I spoke a bit offline with Jordan. I'm going to put a patch up by end of day that refactors things so that this can be done in the Writer. It's a bit hariy but I really don't want setArch or anything like it. The issue we were aware of before was that size was format dependent and we (me really) kluded around it. This actully points out that many other things are as well. The general shift that I've liked here is to slowy remove state from the internal representation and push those things toward constants in the writers. This change I have in mind will remove a lot of state from sections and additionally add that section headers be written on a section by section basis.
Oct 24 2018
Oct 23 2018
Looks like a strict improvement to me and a nice test case fix. Thanks!
Oct 15 2018
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.
Oct 9 2018
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.
Sep 28 2018
We have no such desire. I think we're fine being the test subjects for
Sep 20 2018
So I'm good with this BTW.
Sep 17 2018
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.