Page MenuHomePhabricator

[DebugInfo] Fix for stopping emission of debug_macinfo section in normal case and -fno-debug-macro switch enabled case.
Needs ReviewPublic

Authored by SouraVX on Tue, Nov 5, 3:19 AM.

Diff Detail

Event Timeline

SouraVX created this revision.Tue, Nov 5, 3:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Nov 5, 3:19 AM
aprantl added inline comments.Tue, Nov 5, 8:56 AM
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.

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)

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.

SouraVX marked an inline comment as done.Tue, Nov 5, 11:04 AM
SouraVX added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2734–2735

Thanks for reviewing this.
Yes, I'm considering fusing these, loops. I'll address this in my next revision.
BTW, do you have any suggestion / doubt regarding this approach I have chosen here ?

SouraVX marked an inline comment as done.Tue, Nov 5, 11:09 AM
SouraVX added inline comments.
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.
Doing this while entering / preparing for emission seems fine to me ??

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)

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().

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 ??

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?

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.

SouraVX updated this revision to Diff 228029.Wed, Nov 6, 2:59 AM

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.

SouraVX marked 2 inline comments as done.Wed, Nov 6, 3:04 AM
SouraVX added inline comments.
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