• Add comment which explains the zero offset.

• Remove regex for whitespace

Thanks David, I appreciate your feedback!

• Remove relative path in test.

• Address review comments from Fred and David.
• Convert test to YAML with instruction on how to reproduce the binary.

LGTM

• Upload the correct patch (Thanks Adrian!)

• Moved printing outside switch.
• Un-verbosed test cases.

Thanks Dave, I've updated the diff with your feedback.

In D37971#873777, @aprantl wrote:

Doing this during parsing doesn't seem too intrusive and looks like it is the right trade-off.
Do you have a test case, too?

Feedback Adrian.

Thanks! I've updated the diff and description to make .eh_frame an alias for .debug_frame.

In D37696#869811, @dblaikie wrote:

Also - how does this work (what does it do, does it catch things correctly) when there are DIEs without ranges - such as compile_unit{namespace{subprogram}} - does it still check that the subprogram is contained within the compile_unit?

Add support and test case for nested functions, which should not be contained by their parent.

Changing the default value uncovered some bugs related to the DIDumpOptions not being properly propagated.

Thanks Dave! There's no logic that deals specifically with nested functions, so I don't think it'll handle this particular case. Before hacking this in, I think it might be good to obtain a real example rather than an artificial one, to ensure we don't make the verification broader than necessary.

ping

LGTM!

Hi Dave,

Thanks Adrian!

Abandoning in favor of D37696

Use assert instead of unreachable.

In D37439#860575, @kcc wrote:

Looks good (but I don't know this code).
Thanks for adding the test input -- once the shallow bugs are cleaned up I'll use test/Object/Inputs/ as the seed corpus.

@dblaikie wrote:

Might be worth looking at the previous review on this: https://reviews.llvm.org/D32821

(yeah, it stalled out - partly my fault, I did go back to try to work up the changes I'd been suggesting, but hit some other issues that I gave up on - might be worth discussing design first a bit before diving into the code changes)

I imagine a test for this could be reduced beyond 700 lines of YAML - happy to help with that if you get stuck/have trouble reducing/simplifying it.

(I mean there's several things to test for here, but one basic one should be well reduced to ~10s of lines, hopefully)

Thanks George, this has been very helpful!

Thanks! I've updated the diff to include the section name.

Use report_fatal_error

Thanks Alex!

Thanks for the review Adrian!

In D36993#858121, @dblaikie wrote:

In D36993#858093, @JDevlieghere wrote:

David, apologies for missing your e-mail. I really hate that it doesn't automatically show up in Phabricator! 🙁

If the tag doesn't have a name attribute, everything will go through this function except: DW_TAG_pointer_type, DW_TAG_ptr_to_member_type, DW_TAG_reference_type, DW_TAG_rvalue_reference_type. The first part explains why class and struct don't show up. I prefer this approach because it's guaranteed to be robust. Every DW_TAG_*_type encountered without a name will have something meaningful printed.

IIRC, the original switch had between 20 and 25 cases.

I'm curious what those 20-25 cases were - do you have a copy/roughly describe their contents? Because while 'const' does print nicely, (& volatile would be similar) I'm not sure what the other 10 or so cases might be and whether that's a reasonable way to print them.

David, apologies for missing your e-mail. I really hate that it doesn't automatically show up in Phabricator! 🙁

David: Do you want to discuss this further or is this okay to land as is?

Improved test as suggested by @dblaikie

@dblaikie: I've added the positive tests you mentioned in D36835 as I was changing the file anyway. I also like the idea of omitting the prefixes in brief mode, but that's definitely something for a separate differential.

Thanks @friss, I've updated the diff with your feedback.

In D36993#855777, @dblaikie wrote:

In D36993#855144, @JDevlieghere wrote:

In D36993#854984, @dblaikie wrote:

Where's the 'const' being printed from (couldn't spot it from a quick glance at the code) for the test cases like "const Struct"?

The function dumpTypeTagName extracts the 'type name' from a tag. For the example the function dumps DW_TAG_const_type as const.

That seems a bit subtle - does that codepath trigger/do things for other tag types, or just const? Especially if it's just const, I'd probably handle const where reference, array, pointer, etc, are handled. Seems clearer.

• Feedback David

In D36993#854984, @dblaikie wrote:

Where's the 'const' being printed from (couldn't spot it from a quick glance at the code) for the test cases like "const Struct"?

Thanks for the review @aprantl! About the comment for the Incomplete member: can this strictly occur for members or has this a broader scope?