[PATCH] D155818: [CloneFunction][DebugInfo] Clone DISubprogram's local types

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:281-301
+      // If we cloned the type, referenced by DICompileUnit, we must add a
+      // reference on it.
+      SmallVector<Metadata *> TypesToAdd;
+      for (DIScope *S : CU->getRetainedTypes()) {
+        if (DIType *Type = dyn_cast<DIType>(S)) {
+          auto It = VMap.MD().find(Type);
+
----------------
nickdesaulniers wrote:
> dblaikie wrote:
> > dexonsmith wrote:
> > > dzhidzhoev wrote:
> > > > dblaikie wrote:
> > > > > I guess this comes up, though I'd love it if it doesn't...
> > > > > 
> > > > > When does this situation arise? I would've hoped that function-local types wouldn't appear in the CU's retained types list. It'd simplify the model if they were only retained by either the CU or the subprogram, and not both.
> > > > ```
> > > > enum my_enum { AA, AB, AC } e;
> > > > int main() {
> > > >   enum my_local_enum { A, B, C };
> > > >   return 0;
> > > > }
> > > > ```
> > > > This example can be compiled with the flag `-fno-eliminate-unused-debug-types` :
> > > > ```
> > > > bin/clang local-retained.c -c -fno-eliminate-unused-debug-types -O0 -g -emit-llvm
> > > > ```
> > > > That's what metadata looks like:
> > > > ```
> > > > !2 = distinct !DICompileUnit(language: DW_LANG_C11, file: !3, retainedTypes: !11 ...
> > > > !5 = !DICompositeType(tag: DW_TAG_enumeration_type, name: "my_enum", ...
> > > > !11 = !{!5, !12}
> > > > !12 = !DICompositeType(tag: DW_TAG_enumeration_type, name: "my_local_enum", scope: !13, ...
> > > > !13 = distinct !DISubprogram(name: "main", retainedNodes: !17, ...
> > > > !17 = !{!12}
> > > > ```
> > > > 
> > > > Here `my_local_enum`, which is local to the subprogram, is referenced by both CU and Subprogram, via retainedTypes and retainedNodes correspondingly. I haven't encountered such a situation in real-world metadata since I haven't got much experience dealing with DebugInfo.
> > > I agree with @dblaikie that local types seem like a property of subprograms.
> > > 
> > > To replicate the current IRGen retention policy, we could make `-fno-eliminate-unused-debug-types` retain any subprogram that has a local type (dropping the explicit reference to the local types from compile-unit) and get the same effect.
> > > 
> > > But maybe `-fno-eliminate-unused-debug-types` is overreaching here? It's not obvious to me that it implies retaining function-local types. Feels like `-fno-eliminate-unused-debug-subprograms` (or similar) would be a better fit for that behaviour.
> > > 
> > > Either way, then we'd end up with a clean model here and this could wouldn't be necessary.
> > Yeah - @nickdesaulniers do you care about local types for optimized-out functions in `-fno-eliminate-unused-debug-types`? Looks like GCC is maybe slightly inconsistent. For a totally uncalled inline function with a local type, it doesn't generate any DWARF, but if it's called and then optimized away, it still manages to emit a `DW_TAG_inlined_subroutine` (though it seems to do this even without the local type - so the local type isn't what's causing the inlined subroutine description to be emitted)
> > 
> > I think it's fine to, yeah, change `-fno-eliminate-unused-debug-types` to attach local types to the subprogram - and if at some point we improve inlining (I'm not sure it's an improvement to include a `DW_TAG_inlined_subroutine` that describes a zero length range of instructions, though... ) then we'll keep these types too.
> > do you care about local types for optimized-out functions in -fno-eliminate-unused-debug-types?
> 
> I assume the answer is "yes" but let me check with the consumers of this data (in Android in the UK).
Direct feedback:

> I'm not sure what GCC's intent is. When I experimented with this (and the vaguely related -fstandalone-debug) it looked like that instead of doing some reachability analysis it was just emitting everything.
> In general we are completely uninterested in types that are defined wholly within functions (at any level of nesting if allowing nested functions) as the language semantics imply they cannot leak out. 
> There is the edge case of types defined as part of function prototypes. Whatever their actual usefulness, it makes sense for them to be considered "used" if the function itself is.
> The only bug we've seen with Clang is when there is a DIE outside a function referring to a type defined in a function.
> From the perspective of knowing "all types', I'd say it makes sense to emit them, if there's a DWARF context that is meaningful for them (like an unused out of line function definition or something).
> From ABI perspective, I don't think we care.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155818