[PATCH] D108261: [DebugInfo] Fix end_sequence of debug_line in LTO Object

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Would it be possible and easier to test this issue without any LTO concerns, with code like this:

  void f1() { }
  void __attribute__((nodebug)) void f2() { }

Looks like the bug manifests here by having the line table cover f2, when it should end at the end of `f1` instead?

(admittedly there's still a problem, maybe this isn't sufficiently representative, if there was `void f3() { }` at the end - we don't end the sequence around `f2` and restart it afterwards... )

Well, if you want to keep the existing LTO-related testing, you could link two IR files together with simple empty functions, rather than interesting functions like you have in your examples. Just `void f1() { }` and `void f2() { }` in two files, `llvm-link`d together would suffice, I think?



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2158-2160
+unsigned DwarfDebug::getDwarfCompileUnitID(const MachineFunction *MF) {
+  auto *SP = MF->getFunction().getSubprogram();
+  DwarfCompileUnit &CU = getOrCreateDwarfCompileUnit(SP->getUnit());
----------------
What's this refactoring for? Looks like it causes duplicate lookups of the DwarfCompileUnit, that might be nice to avoid/not introduce? (if it is necessary/preferred for some reason, might be worth doing separately from the rest of this patch?)

(both callers already have the DwarfCompileUnit available - perhaps this function should take that instead of the MachineFunction? that way it could avoid the extra lookups)


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2207-2211
+    // Update the end label in place to the CU's line table.
+    Asm->OutStreamer->getContext()
+        .getMCDwarfLineTable(getDwarfCompileUnitID(MF))
+        .getMCLineSections()
+        .updateEndLabel(R.second.EndLabel);
----------------
Perhaps this could be done once outside the loop?
```
// MBBSectionRanges is always non-empty at this point, right? (could assert just in case, but I think it's a reasonable invariant)
Asm->OutStreamer->getContext()
        .getMCDwarfLineTable(getDwarfCompileUnitID(MF))
        .getMCLineSections()
        .updateEndLabel(Asm->MBBSectionRanges.back().second.EndLabel);

```


================
Comment at: llvm/lib/MC/MCObjectStreamer.cpp:517-518
+    auto &Writer = Assembler.getWriter();
+    if (Writer.isSymbolRefDifferenceFullyResolvedImpl(Assembler, *LastLabel,
+                                                      *EndLabel, false))
+      EndSequence = EndLabel;
----------------
When is this condition false? (is it tested?)


================
Comment at: llvm/test/DebugInfo/lto-debugline-main.ll:1
+; RUN: opt -o %t1.bc < %s
+; RUN: opt -o %t2.bc < %p/Inputs/lto-debugline-foo.ll
----------------
This patch only applies to llvm's codegen - so the test probably shouldn't include opt or LTO tools - the post-LTO'd IR should be checked in directly & then fed into llc in the test to exercise the codegen codepath, most likely?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108261/new/

https://reviews.llvm.org/D108261