[PATCH] D93441: [DebugInfo] Fix crash with -fdebug-info-for-profiling and split dwarf

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:985-987
+  // -fdebug-info-for-profiling requires a subprogram DIE.
+  if (includeMinimalInlineScopes() && !getCUNode()->getDebugInfoForProfiling())
     ContextDIE = &getUnitDie();
----------------
dblaikie wrote:
> dblaikie wrote:
> > Idle thoughts, not especially related to this patch:
> > 
> > This may highlight an interesting situation:
> > 
> > Currently gmlt + split-dwarf-inlining + fission => non-fission gmlt, because split-dwarf-inlining is essentially the gmlt-like data, kept in the object file. So if the only thing that's being fissioned into the dwo is gmlt data anyway, and the gmlt-like data is also going in the object file, why bother with the dwo gmlt data? But if debug-info-for-profiling makes for particularly larger debug info, then it could be reasonable to have gmlt + split-dwarf-inlining + fission + debug-info-for-profiling all active at once. (though, perhaps now we have a good way to turn fission off (-gno-split-dwarf) perhaps we could remove that quirk I added that caused gmlt + split-dwarf-inlining + fission not to compos, but overriding/disabling fission) 
> > 
> > Might be interesting to measure object/executable size metrics for debug-info-for-profiling + gmlt and debug-info-for-profiling + gmlt + fission + split-dwarf-inlining. See how much obj/exe space is saved by that and whether it's worth supporting.
> Hmm, now that I look at it, I'm not so sure this is an idle thought.
> 
> This proposed patch looks like it might be doing the thing I'm not certain about - it seems like this patch is expanding -fdebug-info-for-profiling to apply to split-dwarf-inlining info, which I'm not sure is accurate/useful. At least given Google implemented -fdebug-info-for-profiling and our use case for it doesn't include needing it "online" (it's processed offline, with access to dwo/dwp files when using Split DWARF) we don't need this info in .o/executable files, so perhaps we shouldn't add it there?
> 
> (hmm, well - debug-info-for-profiling is already somewhat used (I see the extra function name data and function decl file/line numbers) in the split-dwarf-inlining data, so I'm suggesting rolling that back/"fixing" that bug too/going more in that direction, rather than in the direction of adding more debug-info-for-profiling data into the split-dwarf-inlining data)
Ping for thoughts on this ^.

If even the most basic case of split-dwarf+split-dwarf-inlining+debug-info-for-profiling is/was broken I'm sort of inclined to suggest this should move away from the split-dwarf-inlining having the debug-info-for-profiling data in it.


================
Comment at: llvm/test/DebugInfo/X86/debug-info-for-profiling-crash.ll:4-8
+; struct b {
+;   template <typename c>
+;   void d(c e) { d(e); }
+; };
+; void f() { b a; a.d(0); }
----------------
rupprecht wrote:
> dblaikie wrote:
> > rupprecht wrote:
> > > dblaikie wrote:
> > > > A slightly simpler/more canonical test that seems to reproduce the crash would be:
> > > > ```
> > > > void f1();
> > > > __attribute__((always_inline)) inline void f2() { f1(); }
> > > > void f3() { f2(); }
> > > > ```
> > > > This is the smallest/simplest example of inlining, reproduces without extra instructions at -O0, etc.
> > > Looks like that runs into the second crash I'm looking at, but isn't dwarf-5 specific, so I'll take a look at that :)
> > ah, fair enough - maybe there's a common fix, especially if we move in the direction of debug-info-for-profiling not applying to split-dwarf-inlining info.
> Managed to fix that crash too, but it looks like the original repro (with dwarf 5) still has a crash, so I replaced the IR with that example.
Ah - so this is now two fixes and tests in one patch? Sorry for the runaround, but they should generally be separate patches. Is one a subset of the other? (eg: with one fix one of the tests passes, but the other test doesn't pass unless both fixes are applied?)


================
Comment at: llvm/test/DebugInfo/X86/debug-info-for-profiling-crash.ll:26-40
+;; Note: .debug_info.dwo has the same basic structure as .debug_info
+
+; CHECK: .debug_info.dwo contents:
+; CHECK:    DW_TAG_compile_unit
+; CHECK:      DW_TAG_structure_type
+; CHECK:        DW_TAG_subprogram
+; CHECK:          DW_AT_linkage_name  ("_ZN1b1dIiEEvT_")
----------------
rupprecht wrote:
> rupprecht wrote:
> > dblaikie wrote:
> > > I'd probably skip this bit - if anything, maybe a comparison with -gmlt -fdebug-info-for-profiling would be a more Apples to Apples take (the split-dwarf-inlining is meant to, roughly, give the same experience as -gmlt in the object file).
> > Removed.
> > 
> > I do still need the `; CHECK: .debug_info.dwo contents:`line, because otherwise the section above may match the .debug_info.dwo section. (Hmm, I guess dwarfdump can probably filter on just a single section instead...)
> I didn't find a dwarfdump option to do that, so I left in the `; CHECK: .debug_info.dwo contents:` with a comment on why.
Yeah - when we added dwo sections we didn't end up adding a whole separate set of flags for dumping only a dwo or non-dwo variant of a section. So you can select, say -debug-info, but you get debug_info and debug_info.dwo sections.

That `CHECK: * contents:` is about right/as good as we can do currently.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93441