I've added a few reviewers who are probably better placed to review things from the DWARF/DW_OP perspective than me.

Looks good from my perspective. I'll leave it up to @MaskRay to approve once you've taken a look at his suggestion.

Do you think a regular llvm-ar archive test is sufficient for this testing, or do you think a mach-o fat archive is important to test independently? I don't see a massive need for the latter personally.

LGTM, from a stylistic point of view. Not sure I can comment much on the behaviour parts that I haven't commented on. Please wait for other reviewers to approve too.

LGTM, with one fix.

LGTM. Please wait for @MaskRay to approve too, since I don't know very much about the details here.

Address review comments.

Remove unneeded test input.

Mention the static analyzer and other tools explicitly.

Looks fine to me. I'd like to see an example of it being used in a real test case, so I'm holding off on the LGTM until that point.

LGTM, once my last comment has been addressed and question answered (assuming that the question doesn't highlight an issue in the patch itself). Thanks for putting up with my lengthy review!

LGTM, with nit.

With all of our tools with GNU equivalents, we should avoid single letter aliases unless we can get GNU to buy into the new option too. Long names are less of an issue, as it's unlikely we'll end up with a clash.

LGTM.

LGTM too.

I think this looks okay to me. LGTM. Caveat: I don't know much about the disassembler, so I leave it up to you whether you think somebody else should review.

Just to make sure I understand the issue: if DT_STRTAB is small enough to fit within a segment, according to the segment's size/offset fields, we currently crash, if the size/offset fields themselves are invalid? What about if only part of the DT_STRTAB range is within the file (i.e. DT_STRTAB is less than file size but DT_STRTAB + DT_STRSZ is past the end)?

Latest veresion basically looks fine. One suggestion and one question remaining from me.

I haven't really reviewed the funcional parts of this change in the attribute parser stuff, but everything else LGTM. Please wait for somebody else to review the attribute parser bits.

In D76572#1942024, @Quuxplusone wrote:

Btw, @jhenderson and @rsmith, if you wanted to land your own files' pieces of this patch while waiting on whoever-it-is to approve the pieces in other files, that'd be cool. As I mentioned initially, I don't have the access necessary to land any part of it myself.

LGTM.

We might want to change the iterators to use the fallible iterator pattern described in the programmer's manual. That should ensure that all clients can cope with bad iteration somehow. I'm not sure I have any particular further insight into how to proceed here beyond that. I think imitating GNU's behaviour for now is a good start, and further improvements can be in later patches.

Code all looks fine now, just a few minor test suggestions.

Two minor comments. Otherwise, looks good to me.

In D73086#1940876, @jhenderson wrote:

It might make sense to get one of the other reviewers formal approval before landing this.

Given @ikudrin requested changes earlier, he'll need to approve it for this patch to move to "Approved" state officially, not that it matters that much.

LGTM, with one minor suggestion. It might make sense to get one of the other reviewers formal approval before landing this.

Thanks for the effort. This is nearly there.

LGTM, with nit, with regards to the llvm-readobj and libObject parts. I haven't looked at the other bits though.

What, if any, are the actual behaviour changes in this patch? I don't see any test updates, but the patch isn't labelled NFC.

In D74023#1937220, @HsiangKai wrote:

In D74023#1933427, @jhenderson wrote:

@HsiangKai, have you noticed that there are some test failures according to the harbourmaster bot? They look like they could be related to this somehow.

@jhenderson, yes, I found test failures in harbormaster. The failures are occurred after I rebased my patch on master branch. After digging into error messages, I found the failures are triggered by find_if(). Maybe I misuse find_if() in this patch? Do you have any idea about this?
By the way, I also found some patch, D75015, landed even harbormaster is failed. I am curious about is it a necessary condition to pass harbormaster before landing?

LGTM, with one minor suggestion.

@Quuxplusone - did you see that there are several clang-format warnings reported against these changes? Please could you fix them.

In D76498#1933793, @probinson wrote:

I suppose this should lead to questions about whether mis-encoded ULEBs can cause parsing sadness, and are those handled in a reasonable way?

@HsiangKai, have you noticed that there are some test failures according to the harbourmaster bot? They look like they could be related to this somehow.

Make --implicit-check-not patterns tighter.

(Woops, one comment didn't get added yesterday).

Add colons to --implicit-check-not=warning (missed because they didn't appear in the diff, as they were in the old test).

Address review comments from @MaskRay.

Ping?

LGTM, with a couple of nits.

Sorry, I've been quite busy with having about 15-20 different active reviews at once, some of which are not trivial in complexity. It may take me a couple of working days to respond to things.

Address review comments.

Address review comments.

I've created D76425 to remove the check in llvm-readobj for fully linked output. This would work as an alternative to this linker change, to fix the PR (although there is one minor caveat - see the bug comment/that review), but I don't think this linker change hurts still. The llvm-readobj patch solves the issue for the case where stack_sizes sections have all been concatenated together via linker script.

LGTM. @Higuoxing, would you mind writing a separate dedicated test for --dynamic? It would allow for testing the corner cases, which are outside the scope of this test file. It can be a separate patch, no problem.

Forgot to mention: given this is intended to be common across other file formats, and there is a need to test the isDebugSection functions, I'd recommend adding tests for COFF and/or Mach-O as well as the ELF one. They don't need to be comprehensive necessarily.

LGTM, but I'd like to see at least one other change that makes use of this to be ready before you commit this, as otherwise this is just dead and untested code. I agree a unit test probably doesn't make much sense.

LGTM, with comment updates.

LGTM.

In D75342#1903392, @MaskRay wrote:

@jhenderson listed many good test cases. It requires some work. I've left some notes.

1. Standalone empty segments

GNU ld can create a p_memsz=p_filesz=0 PT_LOAD. lld doesn't.

I'm happy with this idea in principle, but does it work for users who have .stack_sizes grouped in linker scripts explicitly? (I'm betting not, and yes, such linker scripts do exist).

In D76250#1929314, @MaskRay wrote:

I was also of the opinion that "llvm-objdump" (argv[0]) and "objdump" (argv[0]) should reject single-dash -long-option.

However, @compnerd mentioned that on Mach-O platforms, -long-option may be used. Xcode uses the source code of llvm-objdump but it is not clear how they use the tool. They have patches and tests which are not upstreamed. If they provided a concrete tool name, I would like to allow --long-option when argv[0] matches that tool name (may be otool or something).

If we know how they use the tool, for example, if they always pass --macho, we can get rid of the alias -m.

Fix unchecked Expected.