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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:830
+  if (auto Err = MacinfoDWO->parse(this->getStringExtractor(), MacinfoDWOData,
+                                   false /* IsMacro */))
+    WithColor::defaultErrorHandler(std::move(Err));
----------------
jhenderson wrote:
> dblaikie wrote:
> > SouraVX wrote:
> > > dblaikie wrote:
> > > > Here and below, I believe the format LLVM prefers for naming boolean parameters is:
> > > > ```
> > > > /*IsMacro=*/false
> > > > ```
> > > > Having the comment before the literal, and using '='
> > > > 
> > > > 
> > > Will do Thanks! for correcting this.
> > > BTW, do you have any thoughts on error handling, in this implementation ?
> > > Currently this implementation, handles errors while parsing(mostly macro section, but also bloats up `macinfo` too,/due to shared execution paths/), if any error encountered return(recoverable error) to caller and continue subsequent execution of `dump` routine which will eventually dump whatever has been correctly parsed and terminate finally.
> > I haven't been paying too much attention to error handling - but that sounds reasonable/consistent with other work in improving error handling in libDebugInfoDWARF.
> > 
> > @jhenderson might have the most context on error handling philosophies here, I think
> Using the RecoverableErrorHandler allows programs to either continue or stop depending on the library client, and seems to be a good fit here. I haven't yet had a chance to look at the whole thing to be certain, but in general we should avoid directly reporting errors within library code, and instead use handlers or return llvm::Error/Expected to provide the error information back up the pile.
> 
> You may want to consider whether there are cases where it makes sense to keep parsing within the section, even if something goes wrong. In that case, you'll need to pass the handler down the stack, call it, and then continue parsing.
> 
> If you're interested more in the topic, I did a lightning talk on it a couple of years ago, in relation to some older work I did in the .debug_line code. See https://www.youtube.com/watch?v=YSEY4pg1YB0 for the video.
As the code is written at the moment, the usage of `RecoverableErrorHandler()` is not correct. If an error occurs, the parsing is terminated. Moreover, the `Macro` object is left in an inconsistent state. Well, that is not for this method, but for `getDebugMacro()`: an error is not possible in this method, at least for now. Anyway, all these methods should probably follow the same pattern.

What about returning `Expected<const DWARFDebugMacro *>`? Something similar to `DWARFContext::getLineTableForUnit()` (the second version)?


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:5
+# CHECK: .debug_abbrev contents:
+# CHECK: DW_AT_macros    DW_FORM_sec_offset
+# CHECK: DW_AT_macro_info        DW_FORM_sec_offset
----------------
Your patch does not touch anything related to these attributes, right? If so, you do not need to care about them in this test. Thus, the test source might be greatly reduced, removing anything not directly related to the aspect of the path the test is aimed to demonstrate.


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:7
+# CHECK: DW_AT_macro_info        DW_FORM_sec_offset
+# 
+# CHECK: debug_macro contents:
----------------
There is trailing whitespace in this line which should be removed.


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:14
+# CHECK-NEXT: DW_MACRO_end_file
+# 
+# CHECK: .debug_macinfo contents:
----------------
Another trailing whitespace is here.


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

https://reviews.llvm.org/D73086