- User Since
- Jul 22 2016, 3:32 PM (124 w, 5 d)
oh, i see.
Fri, Dec 7
will address the comments and update this diff, huge thanks for the review
Thu, Dec 6
Hey! I've recently pinged multiple people via e-mail (sorry about the disturbance, I apologize for all the inconveniences and I do understand that people are probably very busy given that it's the end of the half),
@mtrent, @beanz , @ributzka @lhames, @echristo, @compnerd - just give me an idea if any of you will eventually have a look at it (imo it'd be good to have people from Apple involved in the code review since this code is for MachO, who knows - maybe you will find it useful at some point - i.e. as a potential replacement/replacements for cctools in the future). If it's not super-interesting - then it's okay...., let's just make some decision if there is a way to move forward here (which you would be comfortable with) or not. (The main reason why I'm bothering you & asking this question - this diff has been around for 3+ weeks without much visible progress)
Mon, Dec 3
@beanz Chris, would you have a minute to have a look at this diff ? (though this is just the beginning, so things will change)
Thu, Nov 29
i will get to this diff early next week, sorry about the delay. A small side note - probably the simple changes to include/llvm/Object/COFF.h can be factored out from this diff and reviewed separately (and probably much faster).
Wed, Nov 28
Thanks for cleaning this up.
Mon, Nov 26
Ping @echristo Eric, could you please help review this patch / or add smb else ?
Mon, Nov 19
ok, no worries, if smb has a look at it next week - that will be wonderful. Many thanks.
@mstorsjo, no, I'm not working on COFF, i have a fe
also, @jakehehrlich, @jhenderson, @rupprecht - what would you say to moving the existing tests (llvm/test/tools/llvm-objcopy) into the subfolder ELF (llvm/test/tools/llvm-objcopy/ELF) ? if it's ok - i can do that on a separate diff.
one more typo fixed
@jhenderson, yeah, this is the minimal amount of code i was able to come up with for coping over MachO object files without modifications, so essentially the only useful (hopefully) things here are serialization/deserialization. The "intermediate" model is a stub and will change in the future. However, even with this minimal capabilities we can do smth useful (in the nearest future), in particular, in a follow-up patch i was planning to add support for "fat" (universal) binaries, and as soon as it is done (together with this patch) we will be able to build an llvm-based drop-in replacement for the tool "lipo".
Address comments, improve the tests, fix some typos.
Sat, Nov 17
Wed, Nov 14
I would probably use more detailed help messages for the new options (i.e. what exactly the last two options mean (name's suffix) and how exactly the path is constructed from the build-id),
so that users can understand what's going on without looking at the actual implementation.
other than that - LGTM
Tue, Nov 13
Nov 10 2018
Oct 31 2018
Oct 29 2018
Oct 26 2018
Oct 25 2018
Oct 24 2018
@rupprecht Jordan, if you don't mind I will commit my patch https://reviews.llvm.org/D53311 (this probably will cause some minor rebase for you), and then I will cherry-pick your changes (locally) and send my future patch (to move elf-specific things into a separate file and subfolder) on top of yours
Oct 18 2018
Oct 15 2018
I see, many thanks. I've cherry-picked this patch locally and played with GDB - it appears to work fine with it.
I'm also interested and support this change since this would simplify the adoption of Fission by some build systems.
@dblaikie, @echristo - are there any particular concerns with moving this forward ?
Oct 11 2018
thanks, sorry for causing you to rebase
Oct 9 2018
@grimar, this is an interesting observation which I've had on my mind for quite some time as well; a couple of things which I have not double-checked yet - just in case - do both gold and lld completely ignore dwo-related sections ? (did you check that ?), and another small question - just wondering if the debuggers (GDB and LLDB) are okay with it / or smth needs to be adjusted or fixed on their side. I guess everything should be fine, but asking just in case.
Sep 21 2018
but, pls, also wait for Jake or James
Sep 20 2018
I've just had a look at LLD - it does smth similar, so to me this looks good. Pls, wait for Jake as well.
@jakehehrlich Jake, would you mind committing this ?
Sep 17 2018
also, pls, include the full context into the patch https://llvm.org/docs/Phabricator.html
svn diff --diff-cmd=diff -x -U999999
the change looks fine, but it needs a test - simply have a look at the existing ones and add one more invocation of the tool (with this alias).
To run the tests there are special targets: check-llvm-tools, check-all, etc
Sep 14 2018
Sep 13 2018
@plotfi , oh, sorry, i should have added more context. The thing is, that I've been thinking about this patch more and more, and it seems to me, that it's possible to eliminate DecompressableSection by making the class CompressedSection a bit more robust (probably what you mentioned at some point). For example (not insisting on this approach, but just one potential option): CompressedSection can store (internally) either the original content, or the already compressed content + you can add one more constructor to that class etc. In this case the full picture will probably look a bit nicer and easier to perceive: we will have two classes: DecompressedSection and CompressedSection which are responsible for managing this sort of transformation (replacing a given section with the "compressed" version of it or, instead, with the "decompressed" version of it).
although I'd probably try to make the implementation more symmetric (i mean "compression" and "decompression"). Would that be possible ?
this version looks much cleaner to me than the previous one!
Sep 11 2018
Sep 10 2018
Sep 6 2018
Side note: I would double check that nothing shady is going on with the comparator struct SectionCompare used by std::set. If it's broken or gets broken along the way a bunch of things can blow up
Sep 4 2018
To me looks good, but please wait for Jake.
Aug 31 2018
Aug 30 2018
okay, I agree with James' and Jake's comments (except the comment regarding the enum, although maybe i'm missing smth), but to me this otherwise looks good. Hopefully those comments can be addressed + please, wait for the approval from Jake and James.
Aug 27 2018
I've added a couple of inline comments, but I'm really glad to see this is moving forward and to me this looks very close to smth committable, many thanks for working on this and many thanks for your patience! Please, also address the remaining comments by Jake and James, I really hope we can wrap it up in the nearest future (~days) assuming that the decompression will be implemented in a follow-up patch.
Aug 21 2018
James, as for me, I think (though, don't insist) we might add a test for
it, but in this particular case it's not critical / long-term beneficial.
This is a temporary solution anyway. The thing is, that llvm-strip doesn't
support multiple inputs while binutils strip does.
So as soon as we fix it (add proper support for multiple input files), the
test would be gone / replaced with a meaningful test for multiple files.
Aug 20 2018
Aug 16 2018
Hi, I'm back from vacation and trying to catch up on all this stuff.
I'm also interested in this feature since I have a few places where binutils objcopy is called with these flags.
Aug 3 2018
Aug 2 2018
Aug 1 2018
Jul 31 2018
Jul 27 2018
I've added some inline comments, but otherwise this looks ok to me,
please, wait for James as well.
Jul 26 2018
@srhines - is there anything blocking this diff ?