DWARF forward template parameters + type units

@pogo59

So some overlap with simplified template names brought me across one incompleteness with type units and forward template parameters. Do you/Sony use type units?

So it comes up with code something like this:

template<typename T> struct t1;
template<> struct t1<int> {
  virtual void f1();
};
void t1<int>::f1() {}
struct t2 { t1<int> v1; };
t2 v1;

This produces DWARF like this:

0x00000000: Type Unit: length = 0x00000036, format = DWARF32, version = 0x0004, abbr_offset = 0x0000, addr_size = 0x08, name = 't2', type_signature = 0x4e6669a661523280, type_offset = 0x001e (next unit at 0x0000003a)

0x00000017: DW_TAG_type_unit
              DW_AT_language    (DW_LANG_C_plus_plus_14)
              DW_AT_stmt_list   (0x00000000)

0x0000001e:   DW_TAG_structure_type
                DW_AT_calling_convention        (DW_CC_pass_by_reference)
                DW_AT_name      ("t2")
                DW_AT_byte_size (0x08)
                DW_AT_decl_file ("decl.cpp")
                DW_AT_decl_line (6)

0x00000027:     DW_TAG_member
                  DW_AT_name    ("v1")
                  DW_AT_type    (0x00000034 "t1<int>")
                  DW_AT_decl_file       ("decl.cpp")
                  DW_AT_decl_line       (6)
                  DW_AT_data_member_location    (0x00)

0x00000033:     NULL

0x00000034:   DW_TAG_structure_type
                DW_AT_declaration       (true)
                DW_AT_name      ("t1<int>")

0x00000039:   NULL

This is due to the code here: llvm-project/DwarfUnit.cpp at 448d0dfab701ab00d081136d8373304fd91693a0 · llvm/llvm-project · GitHub That builds a declaration from a definition - but it doesn’t know whether that declaration should include type parameters.

I can add a bit of a hack around that for simplified template names (checking whether the name has “<” in it - if it does, skip the template parameters, if it doesn’t, include them) - but that wouldn’t address Sony’s current need, if it has a need for this behavior.

Do you? What should we do about that? Could include a flag on the DICompositeType, or on the DICompileUnit more generally? Something else?

Sony does support type units. I see the same output you do in our compiler, although it’s not what I’d have expected. SCE tuning passes -debug-forward-template-params to cc1, and I’d have expected the forward declaration of t1 in the type unit to include its template parameters. It doesn’t, and I think that’s a bug.
It looks like that flag doesn’t get forwarded to LLVM, and that’s really what you’d want to base it on, I think?

It looks like that flag doesn’t get forwarded to LLVM, and that’s really what you’d want to base it on, I think?

Right, the flag’s currently implemented purely in the frontend - so these declarations that are synthesized in the backend don’t get the desired effect.

We’d need to pass down a backend flag (the “hacky” way using just a cl::opt in DwarfDebug, for instance - this doesn’t support LTO-acts-like-non-LTO (the flag you’d pass to the compilation in non-LTO then has to be explicitly passed to the link step to get the behavior in an LTO build, and you can’t get the behavior on a per-file basis like you could with non-LTO) - or the “fancy”/LTO-full-compatibility way of using a flag either on the DICompositeType (probably overkill) or on the DICompileUnit)

I think DICompileUnit would make more sense, as all the places that would need it should be able to find the CU pretty directly, and it’s easier to verify that we’ve set it on the CU than on all the right CompositeTypes. It flows into LTO automatically. Could also make it a module flag, but then that’s intruding something debug-info specific into the regular IR when it doesn’t need to be.

Huh, seems we don’t actually have a flags bag in DICompileUnit - we just use wholely separate bitcode elements/fields for even boolean DICompileUnit flags (though looks like I’m responsible for most/all of them - SplitDebugInlining, RangesBaseAddress, and DebugInfoForProfiling).

Anyone have thoughts on whether we should add another boolean field, or start something more flag-like (migrate existing booleans to flags? Leave them alone and just use flags for newer things?)? @dexonsmith @adrian.prantl

Could also make it a module flag, but then that’s intruding something debug-info specific into the regular IR when it doesn’t need to be.

IMO, the problem with using a module flag is not that it should be reserved for “regular” IR— just that module flags don’t work well with LTO. In LTO (at least full LTO), you need to decide what to do when two modules are merged that have different flags. I think it’s better to have the flag on something that survives intact.

Anyone have thoughts on whether we should add another boolean field, or start something more flag-like (migrate existing booleans to flags? Leave them alone and just use flags for newer things?)?

No strong feelings on that.

FWIW, I feel like putting the flag on the composite type is a bit more flexible— avoids needing to know which compile unit a composite type comes from to know what to do with it.

At some point the number of individual flags become too many and we should add a DIFlags-style field to DICompileUnit. The cutoff for when that time comes is arbitrary. I’m supportive and happy to review the patch that’s going to implement the bitcode upgrade.

FWIW, I feel like putting the flag on the composite type is a bit more flexible— avoids needing to know which compile unit a composite type comes from to know what to do with it.

If finding the containing CU from a type is tricky/expensive, sure put the flag on the type. I suppose finding all the places we conjure up a composite type isn’t super hard.

Given that for the Simplified Template Names not all templates will be simplified (since some aren’t rebuildable from the DW_TAG template parameters) - having it on the composite type will mean only those that are missing the template parameters in the name will get the DW_TAGs emitted, which will be marginally less verbose. (though, thinking about it, I haven’t implemented it that way currently - the Simplified Template Names driver flag just lowers to that flag + forward decl template args… but at least it’d be plausibly nice to be able to continue with that - maybe not worth much, but might be a tie breaker for where to put the flag for this patch… though that then starts to run up against the DICompositeType flag limits between this change & this other one: DWARF Type Units and single definition types - #2 by pogo59 )

Added a workaround for simplified template names for now here: DebugInfo: Include template parameters for simplified template decls … · llvm/llvm-project@2e58a18 · GitHub

But yeah, in the long term, a composite type flag would probably be a good idea.

It does hinge a bit on whether this other issue is addressed ( DWARF Type Units and single definition types - #2 by pogo59 ) - without that addressed, I’m leaning towards never producing a synthetic declaration in a type unit - if something is in the CU I’ll just assume it can’t be forward declared (because it might contain references to internal-linkage types) and skip using a type unit for the referencing type too anyway. But if we address that issue by flagging type-unit-able types distinct from decl+def merging types (currently using the mangled name identifier) - then we would need to address this again, because we could have an identifiable type that’s still not in a type unit and so needs a declaration to be synthesized…