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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Yeah - would you be able to see if rather than making changes to support debug-info-for-profiling further in split-dwarf-inlining, we could go the other way and support it less/not at all? (it's going to be a bit annoying, now splitting hairs of having gmlt-without-profiling and gmlt-with-profiling, but hopefully not too drastic *fingers crossed*)

Though given this seems pretty generically broken (even the simplest examples assert when all 3 features (split-dwarf-inlining, split-dwarf, and debug-info-for-profiling) are combined) I'd probably be OK with this patch landing first, but then revisiting the general direction separately. This could reduce sanitizer object/executable size  (in builds where debug-info-for-profiling and split-dwarf are enabled), for instance - since split-dwarf-inlining is used there. (though perhaps we restrict debug-info-for-profiling more to only the places we really need it (release builds that get sample based profiling) and so the overlap doesn't come up as often?)



================
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:
> 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)


================
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); }
----------------
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.


================
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_")
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93441