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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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. |
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
If you want to do it separately, please do it before this patch. This patch makes the situation worse, which is obviously bad.
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.
Refactored static helper function(dumping macro/macinfo/macinfo.dwo) as a member function, making code more concise.
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) |
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.
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? |
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. |
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.
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"? |
llvm/lib/DebugInfo/DWARF/DWARFContext.cpp | ||
---|---|---|
489 | Are you suggesting something sort of - 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 :) |
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. |
llvm/lib/DebugInfo/DWARF/DWARFContext.cpp | ||
---|---|---|
327–329 | Before revising this patch, I want to have your opinion on this - 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. |
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. |
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.
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; |
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.
llvm/lib/DebugInfo/DWARF/DWARFContext.cpp | ||
---|---|---|
327–329 | Sure Thanks! |
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? |
Addressed comments by @ikudrin Thanks for this!
-made return type std_unique_ptr<DWARFDebugMacro>.
-littleEndian replaced and some cleanup.
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(...); |
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. |
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. |
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 |
llvm/include/llvm/BinaryFormat/Dwarf.h | ||
---|---|---|
75 ↗ | (On Diff #252858) | Thanks, I'll take care of that in next revision. |
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. |
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. |
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. 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! |
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. |
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.
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. |
llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h | ||
---|---|---|
111–113 | Ah Sorry, I renamed them this way, to avoid confusion of naming while calling 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 ? |
llvm/lib/DebugInfo/DWARF/DWARFContext.cpp | ||
---|---|---|
300 | Is naming not conforming to standard are you referring here parseAndDump ? |
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. |
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; | |
llvm/lib/DebugInfo/DWARF/DWARFContext.cpp | ||
300 | Thanks! done locally, we'll wait for @ikudrin before revising this. |
llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h | ||
---|---|---|
287 |
| |
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(); } |
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.
I think it's OK - best I can tell at this point, given all the review iterations/comments/etc. *fingers crossed*