[PATCH] D97411: [DebugInfo] Add an attribute to force type info to be emitted for types that are required to be complete.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Generally OK with this, as it seemed the libc++ issue might be a bit thorny to solve. Though I'd like to hear from libc++ maintainers about how they feel to ensure they're comfortable adding this attribute in the interim (or if this might be a motivation to fix libc++ instead of adding the attribute)

As for the name - I take it the "if required" is meant to connote the fact that this attribute only has an effect if the type is used/reachable from the DWARF, and doesn't force the type to be emitted even when unused/unreferenced?

Maybe "full_debug_if_used"? or "debug_full_if_used" (or maybe even reuse the equivalent compiler flag: standalone_debug?)

In D97411#2585910 <https://reviews.llvm.org/D97411#2585910>, @rnk wrote:

> To help bikeshed the name, I can imagine a few other use cases:
>
> - an attribute to suppress type info emission, never emit full type info for this type (similar to nodebug / artificial attributes for functions) -- this could help optimize debug info size

nodebug is already supported on types for this purpose - we used it in libc++ to remove some type trait debug info to trim it down.

> - an attribute to always emit type information, whether it is required to be complete or not (similar to -fno-eliminate-unused-types behavior)
> - non-use-case: It's not clear if it's useful to have an attribute to explicitly indicate that a type should use the vptr, constructor, or extern template heuristics.
>
> I thought maybe we could have a single attribute that takes a mode, something like `emit_debug_type_info("never/always/when_required_complete/with_ctor/with_vtable")`. Or maybe shorter is better: `emit_typeinfo("when_required_complete")`. But that sounds like we're talking about RTTI, not debug info.

Yeah, might be useful to have something that supports the different type homing strategies separately, though it is a more complicated user facing feature. The "always" mode could be handy, though I hesitate to build more features/surface area without users in mind, especially since these sort of things may paper over debug info issues we might be better off fixing generally.

I'd like to keep "debug" or "debuginfo" in the name, as you say, to avoid some confusion with RTTI for instance.



================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2402-2405
+  // Don't omit definition if marked with attribute.
+  if (RD->hasAttr<ForceDebugIfRequiredTypeAttr>())
+    return false;
+
----------------
Perahps this should be a bit further up in this function? For instance the template specialization homing seems like it'd override this behavior. Perhaps someone has a template specialization that shouldn't be homed for some reason? & similarly with modular debug info (both the "isDefinediInClangModule" and "hasExternalDefinitions" cases are different kinds of modular debug info homing)

Hmm, I guess the modular debug info is less likely to have these sort of problems - it's more explicit about what is homed, both explicitly making a home, and explicitly communicating the presence of a home to other compilations that rely on that data. So it might be unfortunate to pessimize that scenario unnecessarily.

@rnk - any thoughts on the tradeoff of uniformity of the attribute (ie: applies to all type homing strategies) V applying the attribute to address shortcomings in the source that only affect some homing strategies and not others?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97411