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

Wed Mar 25 02:08:20 PDT 2020

jhenderson accepted this revision.
jhenderson added a comment.

LGTM, with one minor suggestion. It might make sense to get one of the other reviewers formal approval before landing this.

================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:327-329
+    // FIXME: Add support for debug_macro.dwo section.
+  case dwarf::MacroDWO:
+    break;
----------------
jhenderson wrote:
> SouraVX wrote:
> > jhenderson wrote:
> > > Comment indentation is slightly off. Might be worth reporting a warning in the case that this is hit?
> > Before revising this patch, I want to have your opinion on this -
> > I'm considering removing this case `MacroDWO` instead of emitting a warning(I'm not against it). But since that part of code case(MacroDWO) will never be hit, since we don't do it explicitly in first place here --
> > ```
> > if (shouldDump(Explicit, ".debug_macro.dwo", DIDT_ID_DebugMacro,
> >                  DObj->getMacroDWOSection()))
> > parseMacroOrMacinfo(MacroDWO, OS, DumpOpts, /*IsLittleEndian=*/true,
> >                               dwarf::MacroDWO);
> > ```
> > And it's not needed atleast for this patch.
> > Should I go ahead remove this case and just leave a FIXME?
> > 
> If the behaviour from prior to your patch is unchanged, I think that's fine to do from my point of view. I wouldn't want to see things regress though.
I think the problem is that you've put the FIXME above the case statement, whereas it should be below it. If you move it to before the break, it'll look right, and it's probably the right place for it:

```
  case dwarf::MacroDWO:
    // FIXME: Add support for debug_macro.dwo section.
    break;
```

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

https://reviews.llvm.org/D73086