Page MenuHomePhabricator

[DebugInfo]Support for debug_macinfo.dwo section in llvm and llvm-dwarfdump.
ClosedPublic

Authored by SouraVX on Nov 25 2019, 11:03 PM.

Details

Summary

This patch adds support for debug_macinfo.dwo section[pre-standardized] to llvm and llvm-dwarfdump.

This is the first patch in direction of debug info WRT to macros.

  • This was necessary step forward in direction of debug info WRT to macros. macros section could be huge, so transferring this completely to dwo file reduce the size of executable + GDB also supports this style[loading macinfo.dwo section from dwo file].
  • This contains redundancies and require a lot cleanup, So I would like to have some initial thoughts on this.
  • Furthermore, once both macinfo /macinfo.dwo are set up, will try construct DWARF5 macro/macro.dwo on top of this infra, perhaps share/Reuse some common code.

Diff Detail

Event Timeline

SouraVX created this revision.Nov 25 2019, 11:03 PM

Build result: fail - 60307 tests passed, 1 failed and 732 were skipped.

failed: LLVM.DebugInfo/debugmacinfo-dwo.test

Log files: console-log.txt, CMakeCache.txt

Build result: fail - 60307 tests passed, 1 failed and 732 were skipped.

failed: LLVM.DebugInfo/debugmacinfo-dwo.test

Log files: console-log.txt, CMakeCache.txt

Log -- from BOT

Is this failure could be spurious,By any chance ??
Log shows the binary is not found by filecheck. -- Running fine on my System. Passing as well, no warning

 'RUN: at line 1';   /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/llvm-dwarfdump -debug-macro /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/DebugInfo/Inputs/dwarfdump-macro.dwo    | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/DebugInfo/debugmacinfo-dwo.test -check-prefix TEST_MACINFODWO
--
Exit Code: 2

Command Output (stderr):
--
error: /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/DebugInfo/Inputs/dwarfdump-macro.dwo: The file was not recognized as a valid object file
FileCheck error: '-' is empty.
FileCheck command line:  /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/DebugInfo/debugmacinfo-dwo.test -check-prefix TEST_MACINFODWO
aprantl accepted this revision.Dec 2 2019, 10:27 AM

Seems reasonable.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1237

that comment looks misplaced

This revision is now accepted and ready to land.Dec 2 2019, 10:27 AM

Thanks @aprantl for reviewing this!
Hi @dblaikie , @probinson , would you guys also want to share some final thoughts before I move forward and commit this.

This revision was automatically updated to reflect the committed changes.

This looks to be missing test coverage - this one patch adds both parsing and emission support for macinfo.dwo, but only tests the parsing support (there's no llc test that tests the emission)

Please separate patches like this into, first, a parsing patch (adds libDebugInfoDWARF functionality and test(s) like the one you have here) and then in a separate patch the emission support & tests for that.

(to be fair, I'm sort of OK (& have done myself in the past) with /only/ testing the emission, and getting test coverage for the parsing as a consequence of the emission testing, without separate parsing test coverage - but the opposite (only testing the parsing, without the emission) isn't sufficient)

dblaikie reopened this revision.Dec 3 2019, 7:56 AM

This seems like the wrong direction since the macinfo section was replaced (like loc/ranges) with a new format in DWARFv5: "Replace the .debug_macinfo macro information representation with with a .debug_macro representation that can potentially be much more compact."

Seems the thing to do would be to correct the DWARFv5 non-split-DWARF support by using the new format there, then expand that support to cover the split DWARF case as well.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2771–2807

Please refactor these functions to use a common implementation.

eg:

void DwarfDebug::emitDebugMacinfoDWO() {
  emitDebugMacinfoImpl(Asm->getObjFileLowering().getDwarfMacinfoDWOSection());
}
void DwarfDebug::emitDebugMacinfo() {
  emitDebugMacinfoImpl(Asm->getObjFileLowering().getDwarfMacinfoSection());
}
void DwarfDebug::emitDebugMacinfoImpl(MCSection *Section) {
  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(Section);
    Asm->OutStreamer->EmitLabel(U.getMacroLabelBegin());
    handleMacroNodes(Macros, U);
    Asm->OutStreamer->AddComment("End Of Macro List Mark");
    Asm->emitInt8(0);
  }
}
This revision is now accepted and ready to land.Dec 3 2019, 7:56 AM

(you can ignore my other feedback, since this is just not the right direction in general - but the feedback might be useful for ideas on future patches for this and other features)

This looks to be missing test coverage - this one patch adds both parsing and emission support for macinfo.dwo, but only tests the parsing support (there's no llc test that tests the emission)

Please separate patches like this into, first, a parsing patch (adds libDebugInfoDWARF functionality and test(s) like the one you have here) and then in a separate patch the emission support & tests for that.

(to be fair, I'm sort of OK (& have done myself in the past) with /only/ testing the emission, and getting test coverage for the parsing as a consequence of the emission testing, without separate parsing test coverage - but the opposite (only testing the parsing, without the emission) isn't sufficient)

(you can ignore my other feedback, since this is just not the right direction in general - but the feedback might be useful for ideas on future patches for this and other features)

Thanks David, I understand a lot cleanup has to be done for unification and to build DWARFv5 macro on top of this. I'll add emission test case soon. This macinfo.dwo emission and loclist parsing test case were in my TODO list, but I was carried away by other stuff. Will do those in separate patches/reviews.
Thanks again for sharing the implementation feedback also -- will work your inputs.

SouraVX closed this revision.Dec 17 2019, 11:03 PM