[PATCH] D73086: [DWARF5] Added support for debug_macro section parsing and dumping in llvm-dwarfdump.

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX marked 20 inline comments as done.
SouraVX added a comment.

In D73086#1907596 <https://reviews.llvm.org/D73086#1907596>, @ikudrin wrote:

> The test should be extended to use all supported kinds of entries. There should also be another macro unit without the `debug_line_offset` field. There should be tests for other combinations of flags in the header to show that the correct diagnostics for unsupported flags are generated.


Thank you for your comments! Okay, seems I mis-communicated/mis-understood when you guys said "reducing test". This test case seems a bit longer but as you may notice, it's not just the section content(macro) verified here, test is also checking coverage for `DW_AT_macros` attribute `form` and it's `value` (At this moment, It may seem trivial to check this in test case, but if you consider the case multiple CU's, we need to make sure different CU's `DW_AT_macros` points to appropriate offset in macro section) hence all this churn/baggage of section "abbrev/info/str_offsets".

+ if you look at `parse` function, here the content parsing is happening/done based on `Context.getMaxVersion() >= 5`, So for all this you need info section contents(hence others) and hence if you remove any of this you'll end up parsing content in-correctly or even a crash.

> Do you know why it prints this content for this macro? It looks suspicious.

This is reduced/hacked from a asm containing macro info, so that it just fits the purpose, otherwise if we take the whole thing(macro from CU), their will >200 macros(system defined?) pop-up in asm/object file.

> There should be tests for other combinations of flags in the header to show that the correct diagnostics for unsupported flags are generated.

I'm revising first not including diagnostics test cases. From this test case I've tried removing all the un-necessary stuff, only bare minimum is their. Please note abbrev/info/str_offset are still their because of the dependency(accessing DWARFContext to getMaxVersion) while parsing. And I think same pattern(i.e all necessary sections) will/should be followed for other(diagnostics) test cases to be added even when the purpose of the test case is to just print the diagnostics. 
@Ikudrin Are you Okay with this ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73086/new/

https://reviews.llvm.org/D73086