- User Since
- Jul 22 2016, 3:32 PM (143 w, 5 d)
Mar 25 2019
Feb 4 2019
Feb 3 2019
Feb 1 2019
Jan 29 2019
Jan 25 2019
I agree with Jordan regarding tests, otherwise this change looks good to me.
Jan 24 2019
Jan 18 2019
i have one inline comment, but otherwise this looks good to me
Jan 17 2019
Jan 10 2019
I'd probably modify the test (to make it work on Windows (if it doesn't)) (or add a comment why it works if it does), other than that (and one minor inline comment) this diff looks like the right fix to me.
Jan 9 2019
on the other hand, i don't really have strong objections against using two dashes if you really want that consistency, not a big deal imo.
tbh i'm not sure about all the cases here (+ not sure if it's really worth doing),
one dash / two dashes - these things are described in tablegen opts files,
i remember there was a bug when 'one dash' was working while 'two dashes' wasn't (i don't already remember the details unfortunately),
so having at least some tests for both is not useless imo.
Jan 7 2019
Jan 4 2019
LGTM with one minor nit
Jan 2 2019
@echristo, oh, i see, thanks.
@lhames , can we proceed here ?
Dec 31 2018
Dec 29 2018
Dec 20 2018
I think this is a step in the right direction, although now we have some inconsistency with the existing code, but okay, I hope we will change the old code in the future as well. Plus I will change the not yet committed code for MachO myself.
Dec 19 2018
Thanks, definitely I'll wait for @lhames
Dec 18 2018
on my side i think this is fine, I don't want to block this diff.
I would wait for Jake since he is the code owner for llvm-objcopy, but to me this looks good enough (to start the ball rolling).
Dec 14 2018
I've looked at the code, there are few minor nits, but to me this code looks like a pretty good start, many thanks for working on this ))
Dec 13 2018
that's fine, my point was to "unload" some code from Object.cpp, in particular, the serialization/deserialization into separate files. (probably it won't change that much in the future, but the code for modifying Object will evolve as more features will be added).
sorry about the delay, I've also pinged some other people who know coff better than I do (at the moment).
What do think about keeping the implementation of Reader and Writer in their own files, and (in the future) put into Object.cpp only the logic for
modifying the "intermediate representation" (mutations of COFF) ?
Dec 11 2018
oh, i see.
Dec 7 2018
will address the comments and update this diff, huge thanks for the review
Dec 6 2018
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)
Dec 3 2018
@beanz Chris, would you have a minute to have a look at this diff ? (though this is just the beginning, so things will change)
Nov 29 2018
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).
Nov 28 2018
Thanks for cleaning this up.
Nov 26 2018
Ping @echristo Eric, could you please help review this patch / or add smb else ?
Nov 19 2018
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.
Nov 17 2018
Nov 14 2018
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
Nov 13 2018
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 ?