- User Since
- May 16 2017, 11:34 AM (92 w, 3 d)
Wed, Feb 20
This emphatically looks good to me. Thanks for doing this.
This change looks good to me and the semantics seem sound. I'd still appreciate a use case however. This is small and compact enough and I can imagine you might want to do this in cases where a process doesn't fix itself up at all and consequently there are multiple valid entry points but by and large this seems quite odd to me.
Sorry to be the Grinch again but what is the use case for this? This isn't a feature in GNU objcopy as far as I can tell and the semantics of such a change are quite involved. I think I'd prefer to discuss this in llvm-dev first. I belive this can be given meaningful and consistent semantics and I think it could have a use case but right now I don't see one. This seems like a complicated feature to add without a use case in mind.
If we're just giving a set we might as well give the user a function instead right? That's a bit more general of an interface and easier to use. many-sections.o.gz is smaller than 3.3 MB right? I went to a lot of pains to make sure that I was uploading as small a file as was possible. Do we have large file support
Mon, Feb 18
- What's the use case for this?
- What's the interaction for program headers?
Thu, Feb 14
cc @mcgrathr who can comment more in depth on how this works in GNU objcopy and how useful it is. He already noted that anything relaying on segment resizing behavior from GNU objcopy is already fragile.
Can we get an explicit real use case for this or a case where not removing these sections is detrimental? I believe this to be fundamentally wrong otherwise. Imagine what would happen if you first ran --strip-sections and then ran literally anything again. It's also a primary tenant so far that we not modify program headers. That's how we maintain that stripping not affect run time behavior so trivially. This diff changes that.
Tue, Jan 29
OMG thank you. I've been wanting this to happen so badly. LGTM.
Mon, Jan 28
Even though this is a large change I think I'm happy with it. Everything was either factoring out common code or handling error messages. I didn't see anything about how error messages were handled that was objectionable.
Fri, Jan 25
Thu, Jan 24
Does overwriting a file create a new file? --build-id-link-dir and friends relay on that behavior. In general tools should *never* modify a file in place. Consider any build that uses GN (like Fuchsia, or Chromeium, and now optionally llvm/clang). Say they have a rule (like how we strip in Fuchsia) that writes a file. Now say a copy rule is used to put that somewhere else or you use --build-id-link-dir to copy into a .build-id directory (both happen in Fuchsia). The correct behavior should be for a new file to be created on disk, not modification of the existing one. This for instance keeps the file in the .build-id directory correct. I think this truncates the same existing inode.
Sorry, meant to request changes.
Jan 23 2019
Pending 1 nit, LGTM.
Jan 18 2019
Jan 4 2019
Super informative error messages!
LGTM as well.
Jan 3 2019
As discussed offline I realized I missed something.
LGTM. However make sure to at least CC some people outside, if not add them as reviewers on everything.
Jan 2 2019
This is a preety nice solution. I'd been working on removing a lot of the fields from SectionBase, and making an object that had all the sizes of various things listed in it, and constructing that from the ELFT and moving all the calculation of such things into the Writer. That changes kept getting larger and more hairy. This is still quite clean and far less invasive. Also I think a mutable section visitor is a generically useful thing to have around. Nice work. I'd wait for one more LGTM. Sorry I wasn't able to get the change I promised out in time.
Dec 20 2018
Head's up I'm about to go home for the evening and I'm pretty well off for until the new year after that. I think others are in a similar boat. I'm not ready to LGTM this yet and I don't have time to give more review so this might have to wait until January 2nd.
Dec 19 2018
One broad comment is that we should probably implement these in different patches. Just pick one for now and we can build up to the others.
LGTM, I'd like James's approval and my only comment is about a feature we can add after this if its needed. James is also out on leave so go ahead and land this.
I think this looks pretty good. I'd like an explicit public approval from someone who knows MachO better but pending that and a few relatively minor requests I think I'm happy. I've gone over the high level structure and approve of it. I see major obvious issues in the code, it looks preety good to me. I'll defer to someone else on the MachO details. Testing looks pretty good but I don't know the full space very well so I'd like a MachO person to look at that as well.
Dec 18 2018
Other than me realizing I made a mistake in previous reviews and a minor little issue (NeededLibCount), this LGTM.
The structure looks most good but I've got a lot of little requests.
High level structure/interface looks good to me. I don't see in obvious code level issues. I'll trust @rnk's assessment of the COFF specific parts of this. I haven't reviewed the testing inputs but it looks like you're able to copy some substantial object files which looks like a solid start. I'll trust that James and Alex have looked at enough of this to fill in the gaps that I might have missed. We can start iterating now if there are any issues. Land it.
Dec 12 2018
Dec 11 2018
I'd like someone outside of our team like @jhenderson to review this. Landing a new tool without some outside eyes looking at it isn't very community friendly IMO.
Dec 10 2018
Dec 7 2018
Looks like a simple mechanical transformation. LGTM.
Dec 6 2018
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.
Dec 5 2018
Rebased and used -only-section (no s at the end)
Dec 3 2018
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.
Nov 27 2018
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.
Nov 26 2018
Nov 19 2018
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.
Nov 15 2018
Nov 14 2018
- Switched to expected
- Used slice
- Formatted td file
Nov 13 2018
If we wanted we could also add an alias for -k but I don't really think that matters too much.
Nov 12 2018
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.
Nov 10 2018
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.