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

Igor Kudrin via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sat Mar 28 21:28:30 PDT 2020


ikudrin added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:276
-  /// Get a pointer to the parsed DebugMacro object.
-  const DWARFDebugMacro *getDebugMacinfo();
-
----------------
SouraVX wrote:
> ikudrin wrote:
> > SouraVX wrote:
> > > SouraVX wrote:
> > > > ikudrin wrote:
> > > > > Maybe I am missing something, but I cannot find the discussion where the removal of these public methods was approved.
> > > > > 
> > > > > `getDebugMacinfo()` and `getDebugMacinfoDWO()` should not be removed from the public interface. `getDebugMacro()` should be added. `parseMacroOrMacinfo()` should be private and be called from these methods.
> > > > It was part of effort of removing the redundant code that @jhenderson and you were discussing about https://reviews.llvm.org/D73086#1927338
> > > @ikudrin While naming `parseMacroMacinfo` or `getMacroMacinfo` can still be debatable, but semantically this `parseMacroMacinfo` function is doing essentially the same thing which these 2 + 1(getDebugMacro) were doing i.e parsing the contents and returning to the caller for dumping the contents.
> > Not exactly. Look from the perspective of a user of the `DWARFContext` class. Before the patch, they could just call `Context->getDebugMacinfo()`, but now they need to pass way more arguments, some of which are not that obvious (namely, `DumpOpts`) and others are references to a private member of `DWARFContext` (namely, `MacroPtr`). This is a significant degradation of the public interface of `DWARFContext`.
> Thanks for pointing this out! I've drafted a rough patch that does not degrade the existing public interface of `DWARFContext` and doesn't bloat the code too much.
> Here's snippet from that, there will be 3 functions(`getDebugMacro/Macinfo/MacinfoDWO`) calling `parseMacroOrMacinfo` for doing the math associated with parsing and error handling.
> ```
> std::unique_ptr<DWARFDebugMacro> DWARFContext::getDebugMacro() {
>    return parseMacroOrMacinfo(Macro, getRecoverableErrorHandler(),
>               MACRO);
> }
> ``` 
> In subsequent dumping code we'll just check for `nullptr` and dump accordingly.
> ```
>  Macro = getDebugMacro();
>     if (Macro)
>       Macro->dump(OS);
> ```
> Could you please share your opinion on this!
Almost. The getters should cache the result of `parseMacroOrMacinfo()` and return raw pointers because `unique_ptr` transfers ownership.


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

https://reviews.llvm.org/D73086





More information about the llvm-commits mailing list