- User Since
- May 16 2017, 11:34 AM (101 w, 1 d)
To be clear I'm only really giving my thumbs up to a specific high level plan that I believe handles a case that llvm-elfabi wouldn't otherwise handle on its own. I consider the specifics like the output format still in the air right now. I strongly believe that a unique yaml or json format should be used for this. We should discuss the specifics of what should go in that format and have a discussion on that format. The trick is to capture everything in .o files that could wind up mattering to a .tbe file and nothing more. I haven't thought though what that entails completely yet but I'll think about it and throw up my proposal. Once we have at least one proposal we can start iterating on the specifics of it.
The strong motivation here is to avoid having the list be maintained outside of the source code. I am not too strongly tied to the YAML approach as long as the binaries that we get at the end are equivalent to the pruned ELF binaries from the YAML2ELF approach. However, the trade off here is valuable - it allows for the source code to be the source of truth for the interface set. The intermediate emission is due to the fact that this requires program level visibility and having the intermediates an extra step to merge is relatively simpler. In fact, you could emit the TBE files from this and check that into the tree if you really wanted (but allowing the source to remain the source of truth I think is a valid and valuable trade off). The reason for the selection of YAML is simply pragmatism - that is already available in the tree. If the TBE linker is available now, I have no issue with that being the intermediate representation (assuming that the representation is easy to parse programatically).
I actually don't see why something like this couldn't be used in both scenarios (ie some users generating the files at build time, and others using -emit-ifso to periodically generate and land changes to checked in files).
Why is this better than producing output from the output of the static linker at the end? Also how do we plan on integrating this with llvm-elfabi so that we don't have unnecessarily duplicated efforts? Why does that tool not suffice?
Mon, Apr 22
I'm not misunderstanding, please seem my post on llvm-dev where I layout the different issues involved. You're underestimating the complexities involved here I think. Trivializing what the linker does to "the linker just merges these" is a gross oversimplification that doesn't account for symbol consuming vs exporting, size, alignment, visibility, linkage, symbol type, symbol versioning (which is impossible to handle here without consuming version scripts as was pointed out by someone else), which symbols are untimely exported/consumed or even intermediate operations on .o files via tools like objcopy (which is uncommon and shouldn't be a hard blocker here but something to think about). Also all linkers behave just a bit differently with respect to these issues so 1) the reality is that the thing that merges these will behave just a bit differently than any existing linker but 2) even if it trys to match one linker it will fail to match the other so it will only be compaitable in all cases with atmost one linker (but probably none when edge cases are considered. I also already pointed out the issue with having to search all source files on the tree. I agree that if you were to use the linker to accomplish this that would would have to look at all source files. Further more many flags passed to the linker untimely affect these things making matters more complicated. The "linker" you describe for these cases is far from "cat".
There's a lot of code to review here. I'll keep reviewing it everyday but this is going to take a while to review. Any help on splitting this up and making into smaller chunks would be helpful. Splitting reading and writing up into two separate patches would be helpful and removing features that we can add later would be helpful.
Can you elaborate on the use case for this? Like can you explain end to end how this would be used?
Wed, Apr 17
I feel like we didn't get a solid conclusion here but that the general consensus is that this change is more ideal. I think the argument that this is incompatible with merged short options and that that might have compatibility issues is a strong argument personally. The other not-quite-conclusion-but-leaning-that-way seems to be that we can't really make this same choice for all binaries. Do we feel like the conclusion of the llvm-dev post supports yet further fragmenting the tools? I don't know and I'm concerned about making that choice. We have to way tool consistency with GNU compatibility here and that's a hard choice. I'll add an email later to see if we can flesh this out more.
This seems like a good idea and looks good from an architectural standpoint. Don't wait on me to land it (e.g. I'm not giving an LGTM because I haven't reviewed it properly but I've reviewed it enough to know that this is both a) a good idea and b) nothing bad is being introduced). I'm positive Jordan and George's review have already filled what I haven't done.
A few comments.
So overall this looks good and literally yesterday I'd have just given a +2 but James noticed an edge case we should think about. If there are ever reasons to have a dynamic relocation without a symbol table (I don't know of any) we should hold off and account for the edge case. O think that concern only applies to relocatable files however.
Tue, Apr 9
I haven't bread or considered this patch. I just want to clarify when I think we should make changes of this type.
Wed, Apr 3
This seems like the wrong fix to me. Shouldn't we be fixing the alignment bug with respect to segments not changing things like this?
Ah well then we should implement that behavior. We should get rid of the n^2 loop however.
Tue, Apr 2
I'm not sure I like renaming and I'm not sure anyone requires that behavior. It also introduces n^2 behavior which I don't like. That should be removed at the very least.
Wed, Mar 27
Added Roland's proposed fix
Mar 22 2019
Right sorry about that.
I can think of two ways
Roland pointed out that this will be a regression after those special high alignment notes are flushed from the ecosystem (this is apparently a recent addition and the issue will be resolved in the months to come). A possibility is that if we see a note section with greater than 4-byte alignment then we use this method but otherwise fallback to the old method. This means that on Linux binaries for the time being (while this new ABI thing is being flushed out of the ecosystem) will be slightly larger but as soon as its fixed ld.lld will be back to fully functioning. This is a general strategy too because no matter size the notes are, the reader will bump by 4-byte alignment. That seems fine to me and I'll upload that change today hopefully.
Ok, I think how the test functions is fine and it's probably a good test. It certainly tests every case I'd consider. I haven't checked that all the offsets are correct but they don't look wrong at a glance.
Mar 21 2019
LGTM as well. Thanks for checking the spec!
Mar 19 2019
The current implementation in this change was what I had in mind but I have questions now. I think this implementation is wrong
Use one PT_NOTE per SHT_NOTE now instead of trying to minimize the number by assuming the layout algorithm. Changed tests to match this reality.
We don't know addresses at this point, and indeed we can't because the layout of the rest of the output file depends on how many program headers there are, which is decided by this function.
If you think about TLS as being in a sort of different address space (which was not a way I had thought about this before) then this is the function that determines if a section is in the same address area as a segment so its a semantically consistent idea I think. Layout should also always succeed because the PT_TLS will be given the respective PT_LOAD as a parent which will fix its offset correctly and then any sections assigned only to PT_TLS and not a PT_LOAD will still be laid out correctly (which should always be the case for any overlapping segment, we should have tests for this already). We are however changing what this function means even if in practice it's true/false in almost all of the same cases. For non-NOBITS Segment.VAddr - Section.Addr == Segment.Offset - Section.OriginalOffset and those sections will never fall in the range past Segment.FileSize so the predicate should return true in all the same cases for non-NOBITS. It would also now assign sections to the "expected" segment even if today that has no affect one way or the other.
I think pcc's case is valid and we should be able to update the script to account for it (I think anyway). This issue arises when the offset/vaddr of the output section is already known to be larger than gap that note traversal would assume. My current algorithm makes assumptions about how section layout occurs.
I contest that this change is correct. This predicate is specifically designed to capture which bytes overlap in the file, not the run time semantic goal you're looking for. I'm also concerned that it will interact badly with PT_TLS, . It was for this exact reason (handling of PT_TLS) that we chose to use Offset and Filesize *instead* of VAddr and MemSize as was my first instinct.
I mean in the handler for the option you can re-traverse the program headers for each section and handle your check.
Mar 18 2019
For your use case you can simply traverse the program headers for each to see if a section is indeed loadable. e.g. you want to strip all non-loadable allocated sections. That seems like a good thing but this probably isn't the right way to handle it IMO.
The code looks good to me but the test is going to take me a while to review. I don't think I'll get to this today.
If .bss has the desired offset this will work. Also .tbss, .tdata, PT_TLS, and PT_LOAD interact in very strange ways that we need to be careful about. In particular the TLS image will exist in a PT_LOAD read-only segment but the PT_TLS segment will have a bogus vaddr and the .tbss will extend into the part of the PT_LOAD that covers .rodata, .dynstr, etc...
Rather than updating a section in place we should extend the "replaceSectionReferences" method to work in every section type, and then swap the object out for a new object rather than updating in place.
Maybe we should make "prepareForLayout" a top level operation that we can just run on all sections?
Mar 14 2019
Oh also we can add PHDR tests to llvm-objcopy with this!
I think there are some really funky tests in llvm-objcopy that we can fix now but it's going to take a careful eye to see which ones.
This is much needed. Thanks!
Mar 13 2019
This isn't precisely what I had in mind but I think its a strict improvement. Tests look good and document the specific behavior well. I think the behavior with zeroing out sections in explicit cases or where we're looking to match GNU behavior precisely is a good idea (and would have been harder to impossible with what I had in mind). The only one I question is --strip-non-alloc but that was something I invented that turned out not to be useful. We could probably remove that and no one would care. There's not standard or use case to guide what should happen there. The guiding principal should probably be "when it doesn't make people scratch their heads, don't remove stuff in segments" which this change adhears to.
Mar 11 2019
So this change and the issue that James hit made me think about a few short comings in llvm-objcopy that I think should be addressed.
Mar 8 2019
This looks good to me but it is perhaps confusing that the replaceSectionReferences method is unimplemented for many types where it makes sense. Implementing it for all types makes --only-keep-debug possible for instance.
- Fixed typos
- Added context (sorry about that, rookie mistake)
Mar 7 2019
Would be nice to have a test where 2 sections are out of order, and this change puts them into the correct order. It's not clear from this if the build ID section is naturally placed at the end or not.
Mar 6 2019
Feb 20 2019
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
Feb 18 2019
- What's the use case for this?
- What's the interaction for program headers?
Feb 14 2019
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.
Jan 29 2019
OMG thank you. I've been wanting this to happen so badly. LGTM.
Jan 28 2019
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.
Jan 25 2019
Jan 24 2019
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