Page MenuHomePhabricator

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

Authored by SouraVX on Jan 21 2020, 2:00 AM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
SouraVX added inline comments.Mar 17 2020, 3:36 AM
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 marked an inline comment as done.Mar 17 2020, 4:20 AM

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 marked 2 inline comments as done.Mar 17 2020, 5:38 AM

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?

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.

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

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 updated this revision to Diff 250862.Mar 17 2020, 12:40 PM

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 updated this revision to Diff 250988.Mar 17 2020, 10:13 PM

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

dblaikie added inline comments.Mar 17 2020, 10:45 PM
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 updated this revision to Diff 250997.Mar 17 2020, 11:35 PM

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.

SouraVX marked 5 inline comments as done.Mar 17 2020, 11:37 PM
dblaikie added inline comments.Mar 18 2020, 1:06 PM
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 added inline comments.Mar 19 2020, 6:21 AM
llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
491

Should we go ahead with this ?

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 updated this revision to Diff 251744.Mar 20 2020, 1:16 PM

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.

SouraVX marked 3 inline comments as done.Mar 20 2020, 1:17 PM
jhenderson added inline comments.Mar 23 2020, 2:51 AM
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 added inline comments.Mar 23 2020, 4:27 AM
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 added inline comments.Mar 24 2020, 2:29 AM
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 added inline comments.Mar 24 2020, 2:45 AM
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 added inline comments.Mar 24 2020, 3:17 AM
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 updated this revision to Diff 252256.Mar 24 2020, 3:38 AM

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.

SouraVX marked 2 inline comments as done.Mar 24 2020, 3:40 AM
jhenderson accepted this revision.Mar 25 2020, 1:56 AM

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;

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.

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 marked 2 inline comments as done.Mar 25 2020, 7:21 AM
SouraVX added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
327–329

Sure Thanks!
I'll take care of that!

ikudrin added inline comments.Mar 26 2020, 5:05 AM
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 updated this revision to Diff 252858.Mar 26 2020, 8:35 AM
SouraVX marked an inline comment as done.

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

SouraVX marked 3 inline comments as done.Mar 26 2020, 8:38 AM
SouraVX added inline comments.
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 marked 2 inline comments as done.Mar 26 2020, 8:43 AM
SouraVX added inline comments.
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 added inline comments.Mar 26 2020, 7:24 PM
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 marked an inline comment as done.Mar 26 2020, 10:09 PM
SouraVX added inline comments.
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 marked 2 inline comments as done.Mar 26 2020, 10:10 PM
SouraVX added inline comments.
llvm/include/llvm/BinaryFormat/Dwarf.h
75 ↗(On Diff #252858)

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

SouraVX marked 2 inline comments as done.Mar 28 2020, 1:35 AM
SouraVX added inline comments.
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 added inline comments.Mar 28 2020, 3:24 AM
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 added inline comments.Mar 28 2020, 6:04 AM
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 added inline comments.Mar 28 2020, 9:12 PM
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 updated this revision to Diff 253399.Mar 28 2020, 10:40 PM

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 added inline comments.Mar 30 2020, 12:24 AM
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 marked 2 inline comments as done.Mar 30 2020, 1:13 AM
SouraVX added inline comments.
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 added inline comments.Mar 30 2020, 1:27 AM
llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
300

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

jhenderson added inline comments.Mar 30 2020, 1:46 AM
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 marked 3 inline comments as done.Mar 30 2020, 1:56 AM
SouraVX added inline comments.
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 added inline comments.Mar 30 2020, 2:17 AM
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 updated this revision to Diff 253535.Mar 30 2020, 2:56 AM
SouraVX marked an inline comment as done.

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

SouraVX marked 3 inline comments as done.Mar 30 2020, 2:58 AM
ikudrin accepted this revision.Mar 30 2020, 3:30 AM

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

This revision is now accepted and ready to land.Mar 30 2020, 3:30 AM

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 accepted this revision.Mar 30 2020, 3:45 AM

Latest version looks good to me too.

@dblaikie are you Okay with these changes ?

dblaikie accepted this revision.Apr 2 2020, 4:38 PM

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

This revision was automatically updated to reflect the committed changes.