Details
Diff Detail
Event Timeline
| llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp | ||
|---|---|---|
| 2734–2735 | You might want to fuse these two loops for performance reasons and add a comment on what is being filtered out here. | |
I think the right fix for this would be to move the section change inside the if inside the loop (only changing section when we're about to emit macros - it's cheap to change the section to the section you're already in, so there's not much to be gained by checking early/separately)
But that would break the tail (end of macro list mark) - but that's broken. There should be an end of list for each CU's macro contribution - currently the macro output for LTO would be pretty weird - one big macro list, which each CU being told to start at different points on the list...
So, the bug should be fixed & then the section change can be sunk into the loop/condition.
Thanks for reviewing this.
The Intention here is stop the emission{even switching of the section}, if the CUNode macro list is empty, just like first and foremost check as we prepare / enter for emission CUMap.empty(). Although that's a loop{CUNode check for macro list} we can't avoid.
BTW is their I'm missing something, I mean in your approach. If we do this check{CUNode macro list} before / after switching section ?? does this affect{unintentionally leave some void} anything ?
But that would break the tail (end of macro list mark) - but that's broken. There should be an end of list for each CU's macro contribution - currently the macro output for LTO would be pretty weird - one big macro list, which each CU being told to start at different points on the list...
So, the bug should be fixed & then the section change can be sunk into the loop/condition.
| llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp | ||
|---|---|---|
| 2734–2735 | Thanks for reviewing this. | |
| llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp | ||
|---|---|---|
| 2743–2744 | I also considered, inserting a return statement here, that also solves our problem,But code becomes a less let say "readable". because we have emit{handleMacrosNodes} in else case. | |
Yep, I'm with you there - I /think/ all these cases can be simplified, though, by taking a slightly different approach than the one already started/that your patch is continuing (& also fixing a more significant bug in the emission of macro lists)
Although that's a loop{CUNode check for macro list} we can't avoid.
BTW is their I'm missing something, I mean in your approach. If we do this check{CUNode macro list} before / after switching section ?? does this affect{unintentionally leave some void} anything ?
Here's what I'm suggesting:
/// Emit macros into a debug macinfo section.
void DwarfDebug::emitDebugMacinfo() {
for (const auto &P : CUMap) {
auto &TheCU = *P.second;
auto *SkCU = TheCU.getSkeleton();
DwarfCompileUnit &U = SkCU ? *SkCU : TheCU;
auto *CUNode = cast<DICompileUnit>(P.first);
DIMacroNodeArray Macros = CUNode->getMacros();
if (Macros.empty())
continue;
Asm->OutStreamer->SwitchSection(
Asm->getObjFileLowering().getDwarfMacinfoSection());
Asm->OutStreamer->EmitLabel(U.getMacroLabelBegin());
handleMacroNodes(Macros, U);
Asm->OutStreamer->AddComment("End Of Macro List Mark");
Asm->emitInt8(0);
}
}Though this should be split into at least two patches - one to fix the "End Of Macro List Marker" to be correct (by moving it into the loop/next to the "handleMacroNodes", so it ends each list separately - including testing this correction)
and then another patch (or a few) to remove the redundant checks and move the section switching into the loop so it's only done when needed.
(it's possible I've oversimplified the code above - the original code has a "if debug directives only" bail out early in the loop - but I /think/ that should naturally bail out at the "macros empty" case and not be a problem, but I could be wrong)
Hi, Sorry, I just missed out the key part. This patch fixes / stop emission of ...
When "-fno-debug-macro' switch is enabled, which is by default.
So all CUNode->getMacro() should be empty in first place, since no debug information of macro is cooked by clang ?? Right?
So for this case @dblaikie your concerns seems reasonable, but this patch, doesn't touches those things. I may be wrong here ??
Yes, when -fno-debug-macro is passed, CUNode->getMacros() would return an empty list. (both your proposed change and my proposed change rely on this property - yours checks emptiness separately/ahead of the main loop, mine relies on it already being checked in the loop)
So for this case @dblaikie your concerns seems reasonable, but this patch, doesn't touches those things. I may be wrong here ??
I'm suggesting that fixing the end of list marker bug, so that the bug you're trying to fix can be fixed in a less verbose/tidier way, would be the path I would prefer - rather than adding more code, we can fix this by (fixing a bug and) removing code, which is a better path forward because generally less code is easier to understand, less likely to have other bugs, easier to maintain, etc.
I've tried to streamline the code, based on review comments.
While working on this, I discovered one more caveat -- clang doesn't generate debug_macinfo.dwo section when "-fdebuf-macro -gsplit-dwarf" is specified.
But, This require another set of patches to dealt with separately.
| llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp | ||
|---|---|---|
| 2735 | This seems redundant to me, since eventually it's checked again in the main loop{emission}. | |
| 2741 | Moved this main emission loop, switch only when it's needed. | |
| llvm/test/DebugInfo/X86/debug-macro.ll | ||
| 27 | I'm not sure of this, but these are causing this test case failure ?? | |
I went through this & ended up committing the patch series I had in mind:
Git hashes:
39c308f6b8f06710b2b98d0b126c9175e4bafc20 DebugInfo: Use separate macinfo contributions for each CU
3951245c38ce2bcb4173a99d00278d704fcdeac1 NVPTX: Don't insert an extra empty line at the end of the last section.
736273c7fe3e88baf548cd555f21eb123f81381d DebugInfo: Do not create a debug_macinfo section if no CUs have associated macros
db797bfb2bd24e40d8f0ed422fd4087894ed0eab DebugInfo: Remove redundant conditionals/checks from macro info emission
This seems redundant to me, since eventually it's checked again in the main loop{emission}.