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

[SouraVX]

Summary

Summary:
This patch adds parsing and dumping DWARFv5 macro section in llvm-dwarfdump, it does not introduce any new switch. Existing "--debug-macro" can be used to dump macinfo/macro section.

[SouraVX]

llvm/test/DebugInfo/X86/debug-macro-macinfo.s

3

No, Verbose mode isn't part of macro/macinfo since the beginning. I think it's due to nature of it's contents.

[ikudrin]

I am mostly OK with the changes at the moment. Please reach the consent with @jhenderson about code duplication and other topics he mentioned.

llvm/test/DebugInfo/X86/debug-macro-macinfo.s

43

Both strings, "DWARF_VERSION 4" and "DWARF_VERSION 5", are OK now.

48

Remove the comment "string offset=149". It is both useless and misleading. The real offset is 0.

[SouraVX]

In D73086#1926229, @ikudrin wrote:

I am mostly OK with the changes at the moment. Please reach the consent with @jhenderson about code duplication and other topics he mentioned.

Thanks for the consent @ikudrin!
@jhenderson, Are you okay with these changes?

[jhenderson]

In D73086#1926329, @SouraVX wrote:

In D73086#1926229, @ikudrin wrote:

I am mostly OK with the changes at the moment. Please reach the consent with @jhenderson about code duplication and other topics he mentioned.

Thanks for the consent @ikudrin!
@jhenderson, Are you okay with these changes?

You said you'd take a look at removing the duplication. Have you done that yet? That's what I'm waiting on.

[SouraVX]

You said you'd take a look at removing the duplication. Have you done that yet? That's what I'm waiting on.

Oh, Sorry! I think I mentioned that should be taken separately. https://reviews.llvm.org/D73086#1921509

[jhenderson]

In D73086#1926382, @SouraVX wrote:

You said you'd take a look at removing the duplication. Have you done that yet? That's what I'm waiting on.

Oh, Sorry! I think I mentioned that should be taken separately. https://reviews.llvm.org/D73086#1921509

If you want to do it separately, please do it before this patch. This patch makes the situation worse, which is obviously bad.

[SouraVX]

Refactored to remove code duplication.

Hi @jhenderson, @ikudrin, @dblaikie , I've refactored the dumping code to use a single getDWARFDebugMacro function for all dumping related to macro/macinfo. Thus removing all three redundant dumping functions getDebugMacro/Macinfo/MacinfoDWO . Resultant code looks cleaner/uniform.
@jhenderson I've briefly tried to work on https://reviews.llvm.org/D73086#1921509, hitting up multiple issues, Since this is not just macro/macinfo, a whole lot of sections are represented as DWARFSection and StringRef. This invites a huge rework.

[SouraVX]

Refactored static helper function(dumping macro/macinfo/macinfo.dwo) as a member function, making code more concise.

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

295–297

Returning a pointer to the parameter seems unnecessarily confusing - just return Error instead of Expected - callers can use the MacroPtr they passed in instead of retrieving the same thing from the return value.

301

Prefer make_unique:

MacroPtr = std::make_unique<DWARFDebugMacro>();

302–308

This probably shouldn't be dynamically allocated either - if DWARFDataExtractor is move constructible, which I guess it should be, then:

DWARFDataExtractor Data = IsMacro ? DWARFDataExtractor(... ) : DWARFDataExtractor(...);

If DWARFDataExtractor can't be move constructible then Optional<DWARFDataExtractor> could be used:

Optional<DWARFDataExtractor> Data;
if (IsMacro)
  Data.emplace(...);
else
  Data.emplace(...);

310–311

This doesn't look like it needs to be dynamically allocated - could it just be:

DWARFDataExtractor DataDWO(Context.getDWARFObj().getMacinfoDWOSection(), IsLittleEndian, 0);

314

Rather than nulling out this pointer - use a local unique_ptr and assign to MacroPtr at the end if no errors have occurred. (safer than trying to remember/make sure all the failure paths undo the work)

[SouraVX]

Addressed refactoring comments by @dblaikie. Thanks for this!
-Returned type changed to Error.
-Since DWARFDataExtractor is move constructible, removed dynamic construction of objects.
-Instead of resetting ptr, used local unique_ptr this will get moved to MacroPtr in case of successful parsing.

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

491

This (& similar elsewhere) should be written as "Macinfo->dump(OS)", the ".get()" is redundant.

@jhenderson - did you want to roll the error handling/dumping into the common code too? (looks like the "if/handle/else/dump" could go in the common function (& rename the function to parseAndDump or something) too?

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

491

Should we go ahead with this ?

[jhenderson]

Sorry, I've been quite busy with having about 15-20 different active reviews at once, some of which are not trivial in complexity. It may take me a couple of working days to respond to things.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

297–298

It might make the code nicer if IsDWO and IsMacro are folded into one small enum e.g. MacroSecType. You can then switch on that value, and it also avoids multiple boolean arguments, which are easy to get wrong.

314

It looks to me like @dblaikie wanted the MacroPtr = std::make_unique<DWARFDebugMacro>(); line to be replaced with auto Ptr = std::make_unique<DWARFDebugMacro>(); and only assign to MacroPtr at the end.

If there is an error, then MacroPtr will remain unchanged (i.e. unitialised). This is very similar to having a strong exception safety guarantee in concept, i.e. the state hasn't changed if there's an error.

491

Since this is all within the same library, we can put the recoverable error handler calls inside the function, and simplify the interface further.

@dblaikie's first comment about get though should also be addressed.

[SouraVX]

Addressed comments/suggestion by @jhenderson and @dblaikie. Thanks a lot for this!
-Removed both boolean types and introduced new helper enum MacroSecType. Now content parsing/dumping is based on this enum.
-Simplified interface further by moving Error handling inside function, renamed this function as parseAndDumpMacroMacinfo.

[jhenderson]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

290–293

Dump -> dump a

and macinfo -> or macinfo

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

327–329

Comment indentation is slightly off. Might be worth reporting a warning in the case that this is hit?

489

I'm not particularly keen that dumping and parsing happen in the same function. It feels like those are two separate tasks that should be kept independent. Perhaps a small lambda near here instead would make sense to factor out the sequence "parse -> check not null -> dump"?

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

489

Are you suggesting something sort of -
parseMacroOrMacinfo handles parsing and error handling, this can be a member function instead of a lamba for uniformities sake.

if (shouldDump(Explicit, ".debug_macro", DIDT_ID_DebugMacro,
                  DObj->getMacroSection().Data)) {
     parseMacroOrMacinfo(Macro, OS, DumpOpts, /*IsLittleEndian=*/true,
                              dwarf::Macro);
     if (Macro)
     Macro->dump(OS);
   }

I'm okay with this too :)

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

489

Most of the other sections have a pattern of "get/parse" followed by "dump", often in one line, without the error check. I think we should follow that if possible (but with the error check, or possibly by initialising the pointer to some "default" empty section in the event of a failure). In other words, yes, esssentially what you suggested. In particular, this will allow the parsing code to be used and tested independently of the printing.

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

327–329

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?

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

327–329

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.

[SouraVX]

Addressed @jhenderson comments, Thanks a lot for this!

• Refactored the code based on comments.
• Empty case MacroDWO: is still there with FIXME, just for appeasing a build warning warning: enumeration value ‘MacroDWO’ not handled in switch [-Wswitch]
• No regression found after check-all.

FIXME: might still look a bit misplaced(couple of spaces), but clang-format is doing that.

[jhenderson]

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

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

327–329

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;

[jhenderson]

In D73086#1940876, @jhenderson wrote:

It might make sense to get one of the other reviewers formal approval before landing this.

Given @ikudrin requested changes earlier, he'll need to approve it for this patch to move to "Approved" state officially, not that it matters that much.

[SouraVX]

In D73086#1940879, @jhenderson wrote:

In D73086#1940876, @jhenderson wrote:

It might make sense to get one of the other reviewers formal approval before landing this.

Given @ikudrin requested changes earlier, he'll need to approve it for this patch to move to "Approved" state officially, not that it matters that much.

Thanks a lot @jhenderson, for investing time in this!
I'll land this after other reviewers @ikudrin and @dblaikie acceptance.

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

327–329

Sure Thanks!
I'll take care of that!

[ikudrin]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

479

I am not sure that Macro as an in-out parameter of a void method is the perfect design. It would be better if parseMacroOrMacinfo() would just return std::unique_ptr<DWARFDebugMacro>. The code here, in that case, will look like:

if (!Macro)
  Macro = parseMacroOrMacinfo(...);
if (Macro)
  Macro->dump(OS);

Much more clear, no?

485–486

Before the patch, isLittleEndian() was used, but now you always pass the constant true. Could you please elaborate, why the behavior has been changed?

[SouraVX]

Addressed comments by @ikudrin Thanks for this!
-made return type std_unique_ptr<DWARFDebugMacro>.
-littleEndian replaced and some cleanup.

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

479

Removed this

if (!Macro)  --this seemed redundant to me since during first parsing of section, this will always be NULL.
  Macro = parseMacroOrMacinfo(...);

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

485–486

Sorry, some how in effort to keeping the code concise this crept in. I've removed this since parseMacroMacinfo is a member function of DWARFContext.

[ikudrin]

llvm/include/llvm/BinaryFormat/Dwarf.h

75 ↗

(On Diff #252858)

I guess that it is better to make this enum private inside DWARFContext.

You may also want to remove MACRODWO because it is used only for an empty ("TODO") case in parseMacroOrMacinfo(). Just remove that empty case and move the "TODO" comment to this enum.

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

276

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.

[SouraVX]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

276

It was part of effort of removing the redundant code that @jhenderson and you were discussing about https://reviews.llvm.org/D73086#1927338

[SouraVX]

llvm/include/llvm/BinaryFormat/Dwarf.h

75 ↗

(On Diff #252858)

Thanks, I'll take care of that in next revision.

[SouraVX]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

276

@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.

[ikudrin]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

276

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.

[SouraVX]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

276

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!

[ikudrin]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

276

Almost. The getters should cache the result of parseMacroOrMacinfo() and return raw pointers because unique_ptr transfers ownership.

[SouraVX]

Addressed comments by @ikudrin Thanks for this!
-Replaced the return type to raw pointer. Now no un-necessary transfer of ownership involved here.
-Also changed parseMacroOrMacifo to return const DWARFDebugMacro*, in case of errors this will return nullptr. And subsequently checked while dumping.

[jhenderson]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

111–113

Why did you change the naming style of this? LLVM coding standards say that enums should use the same naming scheme as types: https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly.

Also, why the = 1?

114–115

Use C++ style comments, and put this all on one line. Also add missing trailing full stop.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

300

ParseAndDump. This is a (callable) object, not a function.

[SouraVX]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

111–113

Ah Sorry, I renamed them this way, to avoid confusion of naming while calling
parseMacroOrMacinfo(Macro, getRecoverableErrorHandler(), MACRO);

I'll change this as per Coding Standard.

114–115

Ah, seems like clang-formatting is getting in way(maybe due to /**/ style comment), I'll correct it such that clang-format does't put a new line.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

300

Sorry, didn't get what you meant here ?

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

300

Is naming not conforming to standard are you referring here parseAndDump ?

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

300

Right. Lambdas are objects, not functions, and so should follow the variable naming scheme, not the function naming scheme.

[SouraVX]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

111–113

Just want to drop a forward note here since, naming enum also as Macro Macinfoand so forth was colliding with previous definition std::unique_ptr<DWARFDebugMacro> Macro;
error: Macro conflicts with a previous declaration Macro
So I've extended enum names as
MacroSection MacinfoSection MacinfoDwoSection.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

300

Thanks! done locally, we'll wait for @ikudrin before revising this.

[ikudrin]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

287

• Make parseMacroOrMacinfo() private.
• parseMacroOrMacinfo() should return a newly created std::unique_ptr<DWARFDebugMacro>.
• Remove the MacroPtr argument. Caching the result is the responsibility of getters.
• Remove the RecoverableErrorHandler argument. All getters pass the same value, and this method, being a non-static member of the same class, also has access to it.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

485–487

This might be simplified to

if (auto Macinfo = getDebugMacinfo())
  Macinfo->dump(OS);

862–863

The getters should look like

const DWARFDebugMacro *DWARFContext::getDebugMacinfo() {
  if (!Macinfo)
    Macinfo = parseMacroOrMacinfo(MACINFO);
  return Macinfo.get();
}

[SouraVX]

Addressed comments by @jhenderson and @ikudrin Thanks for this!

[ikudrin]

LGTM. Please wait for the comments from @jhenderson and @dblaikie.

[SouraVX]

In D73086#1949559, @ikudrin wrote:

LGTM. Please wait for the comments from @jhenderson and @dblaikie.

Thanks a lot for investing time in this, I'll wait for @jhenderson and @dblaikie. In meantime can you also share some final thoughts on D72828 Mostly it's already reviewed.

[jhenderson]

Latest version looks good to me too.

[SouraVX]

@dblaikie are you Okay with these changes ?

[dblaikie]

I think it's OK - best I can tell at this point, given all the review iterations/comments/etc. *fingers crossed*