LGTM!

LGTM, with one suggestion.

Objcopy aspects look good, thanks.

Sorry for the delay - I've had a lot on my plate.

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).

I'm uncertain, but should we have a test case where the output directory doesn't exist?

LGTM.

LGTM, thanks!

LGTM!

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).

In D124865#3601518, @DiggerLin wrote:

In D124865#3601036, @jhenderson wrote:

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.

There is already has "llvm-nm --print-armap" in the llvm/test/tools/llvm-ar/delete.test , the test case is disable in AIX OS, it enable in AIX in the patch by deleting the "# XFAIL: system-aix"

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.

LGTM, but you probably want to wait for @peter.smith and/or @MaskRay.

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.

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).

Seems good to me, but might want to give a chance for AArch64 contributors to have a say in the matter.

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.

LGTM.

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.

In D121346#3580806, @rahmanl wrote:

Thanks for the explanation. If we use -function-sections it also means that we'll generate a unique LLVM_BB_ADDR_MAP per function. In this case, I believe the version data will be repeated for every function. Correct? I think we can live with that for now. It's only one or two bytes per function.

In D126904#3577758, @jhuber6 wrote:

Addressing some comments for now. Let me know if this is sufficient for now, or if I should move to address the other main comments, the section identification scheme and using binary blobs.

In D121346#3574742, @rahmanl wrote:

IIUC, your suggestion is to embed the version in the section data. The problem with this approach is that the linker must read and deduplicate the version data when linking the sections (unless if we store the version for each function separately).
Also, if we compile with different compiler versions, the linker must create multiple LLVM_BB_ADDR_MAP sections if multiple versions exist.
For these reasons I am a bit hesitant to add linker dependency to the feature, even though section-name-independence is great to have. I'd be happy to change course if we can avoid involving the linker. Any thoughts?

Sorry for the delay - I ended up off sick for a big chunk of last week.

In D127135#3561301, @pzheng wrote:

In D127135#3561269, @thieta wrote:

I think you can drop the content lines from the test cases. But other that that it looks fine.

I tried removing the content lines from the test case, but got the following error somehow. Any idea?

"truncated or malformed archive (offset to next archive member past the end of the archive after member foo-arm64ec.obj)"

Latest updates look good again.

In D126509#3554020, @dschuff wrote:

@jhenderson or others, would you like to review this too, or is sbc's review sufficient?

In D126904#3559278, @MaskRay wrote:

Note: you probably need to consider jhenderson as a blocking reviewer for this change, even if folks who usually review offload changes give an approval.

Looks like there's no test case?

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.

LGTM!

All looks good to me!

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).

I've added @MaskRay as he's done a number of cl::opt -> OptTable transitions in other tools.

Code change looks good to me. Are the TODO cases where the test fails if changing them?

In D124560#3512784, @MaskRay wrote:

Looks great! But best to wait for @jhenderson

No more comments from me, but someone with RISCV knowledge should also review.

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.

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.

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.

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.

In D124607#3494410, @dmcyk wrote:

The builds look good, maybe @jhenderson or @t.p.northover, can you land this for me, please? I don't have commit access.

LGTM!

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.

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.