- User Since
- Jan 18 2017, 2:49 AM (284 w, 21 h)
LGTM, with one suggestion.
Objcopy aspects look good, thanks.
Sorry for the delay - I've had a lot on my plate.
Tue, Jun 28
Two nits, otherwise LGTM.
@plotfi, you were the one who originally added compression support, including zlib-gnu support. Do you know of a use-case for zlib-gnu support? If not, I think we can drop it, given that GNU has. (Looking back at https://reviews.llvm.org/D49678#1189341 which was a comment I made on the review to add support, @plotfi originally had this marked as deprecated).
Mon, Jun 27
I'm uncertain, but should we have a test case where the output directory doesn't exist?
Thu, Jun 23
LGTM. Does llvm-objdump need something equivalent? (I don't remember whether we interpret platform-specific program headers in llvm-objdump)
There's a potentially relevant pre-merge failure in mri-create.test on debian (see above), which you should look at before landing this (I suspect it's the missing %errc substitution I highlighted above).
Wed, Jun 22
I feel like this code is missing testing for the non-error cases. In particular, I'd expect to see some sort of test that shows that the archive symbol table can be read successfully. I think you can do that using llvm-nm --print-armap. I'd be surprised if there aren't already equivalent test cases for other formats that do this, so you may be able to copy/modify those to cover big archives too.
Tue, Jun 21
I'm going to leave my review there, but haven't finished the test. Please address my comments and then I'll finish reviewing the test case.
Mon, Jun 20
LGTM, but maybe give @alexander-shaposhnikov a chance to comment (i.e. give it a day or two to land this or until he responds).
Fri, Jun 17
Seems good to me, but might want to give a chance for AArch64 contributors to have a say in the matter.
Wed, Jun 15
The pre-merge checks are showing up failures caused by this patch. Please fix them.
No further comments from me at this time, but I'm not knowledgeable enough about this area to be able to LGTM it.
Have you got any reference material that I can read for the format of offloading binaries? I can't really review that this implementation does the right thing without knowing the format!
Seems reasonable to me, but needs a Power PC person to do the review, I think.
Tue, Jun 14
Mon, Jun 13
Sorry for the delay - I ended up off sick for a big chunk of last week.
Tue, Jun 7
Latest updates look good again.
Mon, Jun 6
May 26 2022
Looks like there's no test case?
May 24 2022
Both llvm-symbolizer and llvm-cxxfilt, if called without positional arguments, are run interactively, essentially taking a line of input then writing some output before waiting for more input. This would allow you to force output to be written (by writing to stdin) after the pipe has been closed. Maybe that'll solve the issue, although I'm not sure I understand why the python script doesn't work in its current form: llvm-nm should try to write many bytes of output (> 1 byte), but the pipe is closed after the very first byte is written. Before going and changing the script, I'd like to understand whether this statement is actually true, and if so, why:
That does not appear to be enough. You can either run a tool that runs longer or run llvm-nm with something that produces more output.
May 23 2022
May 19 2022
All looks good to me!
May 17 2022
Has the spec for this been finalised anywhere? My main conceren is the use of section names to have semantic importance. ELF generally tries to avoid this, hence the use of section types, and it would be a shame to introduce this approach when there are other options. It would be far more preferable to include the version number in the section data somewhere, a bit like how most DWARF sections are encoded. I can think of one other possible way of doing this: change the section type value for version 1 and upwards, and rename the original value to something like SHT_LLVM_BB_ADDR_MAP_V0. Add the version field as the first N bytes (2 or 4 probably) of the new section type. Parsers understanding the old data structure only won't recognise the new section type as a recognised format. This is good because it doesn't mislead people by printing incorrect offsets (in addition to not needing to rely on the section name).
May 16 2022
I've added @MaskRay as he's done a number of cl::opt -> OptTable transitions in other tools.
May 15 2022
Code change looks good to me. Are the TODO cases where the test fails if changing them?
No more comments from me, but someone with RISCV knowledge should also review.
May 13 2022
Actually, according to the pre-merge checks, one of the tests is failing, and I suspect it might be related to this patch?
LGTM, apart from a typo and a couple of unnecessary steps in the test.
May 12 2022
All looks good to me, but I think it would be worth getting a second pair of eyes on it (@Esme?)
Just some nits left from me.
May 11 2022
Okay, nothing else from me, but @dblaikie or another debuginfo person should review it too.
Three nits, but otherwise LGTM, with the caveat that I'm not a user or regular developer of this tool, so there might be some cases I haven't considered. If there's someone else available to sign off too, that would be good.
May 9 2022
Thanks for the updates! Unfortunately, I don't know anything about the specifics of Darwin archives, so I think it's best if one of the other reviewers signs off on the logic of this patch.
One remaining nit from my point of view, but as noted, I haven't attempted to review the disassembly aspects in depth.
LGTM, but please rename the test file as indicated, before landing.
May 5 2022
I don't know the disassembly code well enough to feel comfortable reviewing it or its corresponding testing, without sinking more time into this review than I have available, so someone else better look at those parts at least.
I'm wondering if it would be better to use python to drive this test rather than a somewhat non-trivial shell script? I'm mostly concerned about the potential flakiness of the sleep 2 command, since on an overloaded system, it's possible this won't have the desired effect. If you use python, you could attach a reader to the stdout of the llvm-nm process, that only read a bit of output (e.g. 1 byte) before stopping and closing itself. This in turn should trigger the error on stderr, if I'm following the behaviour right.
LGTM, with @MaskRay's summary change suggestion.
May 4 2022
I've run out of time to properly review this today, but some more nits to be getting on with.
I think otool changes should be split off into a separate patch. They should also be tested and documented in isolation.