I'll give this a more thorough look at some point hopefully, but my instinct is the current situation is the worst of the three cases "good padding"/"excessive padding"/"no padding". I think a little bit of code complexity would be okay to improve the excessive padding situation. I'm not sure instinctively whether this is too much however. I'll give it some thought.

LGTM. I assume that the only changes aside from a literal cut + paste are due to a clang-format run?

LGTM.

LGTM. Thanks!

Thanks for the patch!

Aside from the inline comment, this looks good to me. Might want to get @MaskRay to approve before committing though.

Ping!

Ping?

Ping!

In D72843#1826229, @grimar wrote:

Hence the identation length is increased by 2 because of 2 round brackets printed additionally.

LGTM!

LGTM.

In both cases, I'm struggling to follow why specific numbers are used, e.g. where does the "-1" "-3" etc come from?

In D72358#1823248, @MaskRay wrote:

Ping:)

LGTM!

LGTM.

Please could you add this new switch to the documentation. I don't really understand what it does just by reading, so I can't do it myself!

LGTM.

In D71835#1817700, @MaskRay wrote:

I think our goal is to make llvm-readobj -d and llvm-readelf -d consistent.

FWIW, I don't see a massive need for this (we don't make, e.g. symbol output consistent), but I don't oppose attempting it as long as llvm-readelf output is consistent with GNU readelf.

As mentioned in D52810, GNU nm's behaviour is the same for both a symbol table with only a null symbol and a completely missing symbol table (both print "no symbols"). I think both need testing.

In D52810#1818209, @sbc100 wrote:

I've got a fix llvm-nm that I will upload, but funnily enough it doesn't actually solve the wasm issue. The reason is that with ELF we always generate at least one local symbols (the STT_FILE symbols) so no unstripped object will ever report "no symbols". However compiling the empty file on wasm does generate file with a symbol table of size zero.. which mean the fix I'm proposing doesn't actually fix our issue.

FWIW, if you create an ELF object from an assembly file, the STT_FILE symbol will only be created if you have a .file directive.

In D72154#1817101, @ikudrin wrote:

LGTM. Sorry for the delay.

In D71835#1816484, @grimar wrote:

The benefit I see is that we do not need to care about printing "Operating System specific" or "Processor Specific" text
and doing actions to align columns after that. I.e. code that prints "<unknown:>" is just a bit simpler.
It is not clear how much useful to say that unknown value is "Operating System specific" or "Processor Specific".
(I am OK to copy GNU's behavior here if we decide we want it, though)

No problem with the constness changes.

In D72337#1814124, @dblaikie wrote:

Ah, thanks for the context! Yeah, go ahead and commit it without a test case - the new implementation is strictly simpler/more obvious & I don't think the unit test really adds value/is likely to catch any regressions here.

Thanks. I didn't see your comment when I was doing the rebase, but I'll make sure to remove the extra unit test before landing it on Monday.

Rebased.

Rebased + changed to assuming the stated length is always correct. This required some additional test changes to better demonstrate the behaviour difference.

Rebase.

Rebased + changed to assume the stated opcode length is always correct.

Rebase.

In D72337#1813202, @dblaikie wrote:

In D72337#1811669, @jhenderson wrote:

Added unit test case that would trigger the assertion mentioned in the description. I'm not entirely convinced that this test case is worthwhile, since with the change in how size() is calculated, it really is moot (e.g. I wouldn't have needed it if the size() function had always been the way I'm proposing).

Ah, I was more expecting an end-to-end test (with llvm-dwarfdump or similar) when you said in the patch description "The old behaviour could cause an assertion in the debug line parser"

What were the circumstances where this assertion would fail? Was the failure reachable with llvm-dwarfdump, or only via a unit test/non-production API usage?

Rebase on master and add missed lld test.

In D72158#1813369, @dblaikie wrote:

As mentioned in another review - I'm not sure "assume bigger is correct" is a great strategy & not sure there's a lot of value in continuing in the face of a conflict like that. What sort of situations really benefit from such parsing optimism?

Making it easier for consumers to continue and try to do something with slightly bad output. One of the problems with using the unrecoverable errors is that it prevents people even trying to iterate over later tables.

In D72157#1813368, @dblaikie wrote:

I guess these error messages could be more precise about what's invalid & why it's invalid. In this case I guess it's invalid because the DW_LNCT_MD5 has a form that contains only one byte, not large enough to encode a whole MD5 sum?

In D72155#1813355, @dblaikie wrote:

Out of curiosity: what's your broader goal with this work? (it'll help understand what's in-scope and out of scope, and better understand the framing when reviewing changes)

In D72369#1812230, @hubert.reinterpretcast wrote:

In D72369#1812104, @jhenderson wrote:

I'd still really like to see a test that tests the other code path that tries to print DW_LNS_* (see the inline comment). The obvious thing to me to do would be to update the unit test "ParserPrintsStandardOpcodesWhenRequested", but it would require changes to the DwarfGenerator to create a custom prologue. If you don't want to do this, that's okay, as the code path is already untested.

Yes, that is rather out-of-the-way for what this patch is doing. My understanding is that you would be okay with letting this patch land?

I'd still really like to see a test that tests the other code path that tries to print DW_LNS_* (see the inline comment). The obvious thing to me to do would be to update the unit test "ParserPrintsStandardOpcodesWhenRequested", but it would require changes to the DwarfGenerator to create a custom prologue. If you don't want to do this, that's okay, as the code path is already untested.

Update missed test.

Address review comment.

Early ping - I'd like to get this and the other related reviews in before the release branch is created if possible.

Early ping - I'd like to get this and the other related reviews in before the release branch is created if possible.

Early ping - I'd like to get this and the other related reviews in before the release branch is created if possible.

Early ping - I'd like to get this and the other related reviews in before the release branch is created if possible.

Added unit test case that would trigger the assertion mentioned in the description. I'm not entirely convinced that this test case is worthwhile, since with the change in how size() is calculated, it really is moot (e.g. I wouldn't have needed it if the size() function had always been the way I'm proposing).

In D72313#1809709, @TH3CHARLie wrote:

In D72313#1809694, @jhenderson wrote:

Assuming the tests all pass, LGTM, thanks! Do you have commit access yet?

Thanks for the review! All tests passed and I've recently gained commit access and want to try to commit this. If there's nothing else, I will commit this

Assuming the tests all pass, LGTM, thanks! Do you have commit access yet?

I think a better solution is to print something more meaningful. For example, "DW_LNS_unknown_<value>" or something similar. You can see examples of that with the DW_AT_unknown_* printing in debug-abbrev.s. I don't think that will be significantly more complex and will provide more user-friendly output. See also my inline comment about where to put the fix.

In D72313#1807560, @TH3CHARLie wrote:

Well, if so, doesn't it duplicates what has been tested in sym.test?

Yes, but sym.test is not a very good test as it tries to test too many different things at once, which means it's hard to actually tell what it's testing or to maintain the test. I'd like to split it up/rewrite it/delete it etc at some point, although it's not high on my priority list. If we keep adding things to that test, it will actually make things worse. By putting this testing in the other test case, we have all our testing for this piece of functionality (i.e. what to do about unknown addresses) in one place, making it easy to find etc.

In D72313#1807539, @TH3CHARLie wrote:

In D72313#1807518, @jhenderson wrote:

Also, I think it would be good to expand invalid-input-address.test to cover this. In fact, you might want to put both your new case and something equivalent to the existing one in sym.test in the same place. That way, we're not lumping multiple different kinds of checks into the same test file.

I'm not quite sure about this so I'd like to confirm my understanding again before updating. Feel free to correct me if I misunderstand this: adding new tests(instead of moving existing ones in sym.test) to invalid-input-address.test that checks for invalid value like the sometext used in sym.test. And the testcases cover command line, ordinary textual file and response file.

Thanks for the patch! Could you add an additional test case for passing the values via a response file, rather than directly on the command-line, i.e. llvm-symbolizer <some args> @response.txt where response.txt contains "some text", inctwo and "some text2" etc.

LGTM from me on this. I can't think of anything else I'd like addressing, but please wait for @arichardson and anybody else who wants to comment to be happy.

LGTM.

LGTM. I have no idea why this was hex in the first place.

This change itself looks good to me. If I understand correctly from @rnk's comment, his suggestion is that there should be an option in llvm-objdump to revert to the old behaviour (which as you point it would be to just pass in 0 instead of the current address). This would mean that people used to the offset behaviour can choose to use that still. I think there is some benefit to this too, but I guess that it should actually be a change to D72172.

I don't see any problems with this, so LGTM, although with the caveat that I'm not familiar with some of the usage sites, so it might be worth getting others to confirm the value passed in for the address makes sense in some situations (e.g. llvm-mca).