[DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types

[akhuang]

Summary

There are some types in libcxx that are used but their constructors
are not called (hash_node, hash_value_type, tree_node, value_type),
which means that with constructor homing, the types are not complete.

This patch avoids using ctor homing if there are no constructors in
the class definition. So it'll mean we emit some extra debug info in places.

I also re-measured the size of object files in a clang build
-debug-info-kind=limited: 5568746k
-debug-info-kind=constructor: 2695028k

after this patch: 2685607k

So they're ~10mb larger after this change, which is not too bad.

In terms of testing, in chromium (on windows) there's visualization for libcxx
types and all of those types are displayed correctly after this patch, so I
think that means all the STL types are getting emitted now.

[dblaikie]

Does Chromium need this fixed in clang? Or if it were fixed in libc++ would that be adequate? (does Chromium's build need to work with old libc++s, or does it always build with a libc++ that matches the compiler? (in the latter case, a fix in libc++ would be as good as a fix in clang))

[akhuang]

No, chromium doesn't need this fixed in clang, but I didn't see a clear way to fix this in libc++. Do you think it should be fixed as a bug in libc++?

[rnk]

In D90719#2372463, @dblaikie wrote:

Does Chromium need this fixed in clang? Or if it were fixed in libc++ would that be adequate? (does Chromium's build need to work with old libc++s, or does it always build with a libc++ that matches the compiler? (in the latter case, a fix in libc++ would be as good as a fix in clang))

Well, we'd like to make this new type homing behavior the default, and it wouldn't work with old libc++ versions, and there is the general possibility that there is code out there like this libc++ code that has implicit nontrivial constructors that are not used. So, I wouldn't think so much about what's right for Chrome, and more about what's right for Clang.

If we do want to fix libc++, I'd warn you that the code is subtle. It takes care to construct the node value in a particular way, and it sets a flag after construction succeeds, which perhaps affects exception safety:
https://github.com/llvm/llvm-project/blob/master/libcxx/include/__hash_table#L2456
+ @EricWF

[dblaikie]

In D90719#2372554, @rnk wrote:

In D90719#2372463, @dblaikie wrote:

Does Chromium need this fixed in clang? Or if it were fixed in libc++ would that be adequate? (does Chromium's build need to work with old libc++s, or does it always build with a libc++ that matches the compiler? (in the latter case, a fix in libc++ would be as good as a fix in clang))

Well, we'd like to make this new type homing behavior the default, and it wouldn't work with old libc++ versions, and there is the general possibility that there is code out there like this libc++ code that has implicit nontrivial constructors that are not used. So, I wouldn't think so much about what's right for Chrome, and more about what's right for Clang.

My understanding is that such code is UB, is that right? - if libc++ was threading some needle/depending on some agreed upon Clang guarantee (even if it was a secret handshake only for libc++) then I'd be more in favor of the "we have to/should support it" but as it seems to stand, I'm inclined towards addressing this by fixing libc++ unless there's evidence of a pervasive dependence on this UB.

If we do want to fix libc++, I'd warn you that the code is subtle. It takes care to construct the node value in a particular way, and it sets a flag after construction succeeds, which perhaps affects exception safety:
https://github.com/llvm/llvm-project/blob/master/libcxx/include/__hash_table#L2456
+ @EricWF

Yeah, +1 on getting @EricWF's take on this for sure.

[rnk]

In D90719#2372656, @dblaikie wrote:

My understanding is that such code is UB, is that right?

I guess I'm not convinced it's UB, and need some language lawyering help to decide.

[dblaikie]

In D90719#2376388, @rnk wrote:

In D90719#2372656, @dblaikie wrote:

My understanding is that such code is UB, is that right?

I guess I'm not convinced it's UB, and need some language lawyering help to decide.

Fair enough. The code we're talking about essentially boils down to this, right:

struct non_trivially_constructible {
  non_trivially_constructible();
  int i;
};
struct implicitly_non_trivially_constructible : non_trivially_constructible {
};
void f1() {
  using T = implicitly_non_trivially_constructible;
  alignas(T) unsigned char data[sizeof(T)];
  T* t = static_cast<T*>(&data);
  t->i = 3;
  ...
}

Yeah? My understanding is that the lifetime of the T object hasn't started, because its ctor hasn't been run. For trivial types that's a bit fuzzier (eg: int *i = (int*)malloc(sizeof(int)); *i = 3; - we don't usually bother to call the pseudodestructor on this type) but for a non-trivially constructible thing, I'd think that was pretty well required/guaranteed by the language? (sort of by definition of non-trivially constructible)

[rnk]

I had another thought, which is that even if it is UB, perhaps we really shouldn't be using UB as the basis for debug info emission. All programs have bugs, and most bugs invoke some form of UB. If we don't provide sufficient info when UB is involved, it can become harder to find the UB. The vtable type homing heuristic works because violating the heuristic assumptions typically results in a link error. Creating an object without calling the class's constructor is the kind of UB that is likely to manifest as runtime errors.

Which is to say, I'm in favor of Amy's change as written.

[dblaikie]

In D90719#2377324, @rnk wrote:

I had another thought, which is that even if it is UB, perhaps we really shouldn't be using UB as the basis for debug info emission. All programs have bugs, and most bugs invoke some form of UB. If we don't provide sufficient info when UB is involved, it can become harder to find the UB. The vtable type homing heuristic works because violating the heuristic assumptions typically results in a link error. Creating an object without calling the class's constructor is the kind of UB that is likely to manifest as runtime errors.

Which is to say, I'm in favor of Amy's change as written.

The same would be true for the ctor homing generally though, I think? If there was a user-defined ctor and someone chose not to call it, that'd be UB too. I'm not sure there's especially more cases of not calling implicit non-trivial ctors versus non-trivial explicit non-trivial ctors to diverge on this criteria?

[akhuang]

In D90719#2377485, @dblaikie wrote:

In D90719#2377324, @rnk wrote:

I had another thought, which is that even if it is UB, perhaps we really shouldn't be using UB as the basis for debug info emission. All programs have bugs, and most bugs invoke some form of UB. If we don't provide sufficient info when UB is involved, it can become harder to find the UB. The vtable type homing heuristic works because violating the heuristic assumptions typically results in a link error. Creating an object without calling the class's constructor is the kind of UB that is likely to manifest as runtime errors.

Which is to say, I'm in favor of Amy's change as written.

The same would be true for the ctor homing generally though, I think? If there was a user-defined ctor and someone chose not to call it, that'd be UB too. I'm not sure there's especially more cases of not calling implicit non-trivial ctors versus non-trivial explicit non-trivial ctors to diverge on this criteria?

That's true. I was kind of thinking of this as a workaround for this particular libcxx issue, and if it becomes no longer an issue in libcxx, we could revert it.

Anyway, guess it would be good to know why libcxx does things this way and whether it could be changed.

[rsmith]

Perhaps we could address both the UB and the debug info homing issue by adding a new attribute on the libc++ types that says it's valid to create an instance of the type without calling a constructor? (If we want to support old libc++ with trunk clang, we could detect those types and automatically synthesize the attribute, but libc++ is moving towards being version-locked to clang anyway, so I don't think we need to worry about that too much.)

[dblaikie]

In D90719#2377883, @rsmith wrote:

Perhaps we could address both the UB and the debug info homing issue by adding a new attribute on the libc++ types that says it's valid to create an instance of the type without calling a constructor? (If we want to support old libc++ with trunk clang, we could detect those types and automatically synthesize the attribute, but libc++ is moving towards being version-locked to clang anyway, so I don't think we need to worry about that too much.)

My guess would be that this doesn't come up often enough to merit an attribute, etc, and that libc++ is fixable. (if the code really wants to do no work when constructing, changing the type to have a trivial ctor and the places that want non-trivial construction can initialize the members as needed seems like it'd be viable)

But it does present significant challenges/hamstring code optimization opportunities, sure, an attribute could be handy/a way forward.

[akhuang]

+ @ldionne for libc++ input?

To summarize, this constructor homing debug info optimization makes the assumption that if a class is being used then its constructor must have been called at some point. We noticed that some libc++ types (such as __hash_node) are nontrivial but the constructor is never called (instead there's some allocation and then the members are constructed separately).

Basically we're not sure if this can be fixed from the libc++ side but it would be nice to have some more input about this. For example would it be possible to call the constructor when __hash_node is created?

My guess would be that this doesn't come up often enough to merit an attribute, etc, and that libc++ is fixable. (if the code really wants to do no work when constructing, changing the type to have a trivial ctor and the places that want non-trivial construction can initialize the members as needed seems like it'd be viable)

I looked at the code again and __hash_node also has nontrivial members, so maybe it's not as feasible to make it a nontrivial constructor.

[dblaikie]

In D90719#2387379, @akhuang wrote:

+ @ldionne for libc++ input?

To summarize, this constructor homing debug info optimization makes the assumption that if a class is being used then its constructor must have been called at some point. We noticed that some libc++ types (such as __hash_node) are nontrivial but the constructor is never called (instead there's some allocation and then the members are constructed separately).

Basically we're not sure if this can be fixed from the libc++ side but it would be nice to have some more input about this. For example would it be possible to call the constructor when __hash_node is created?

My guess would be that this doesn't come up often enough to merit an attribute, etc, and that libc++ is fixable. (if the code really wants to do no work when constructing, changing the type to have a trivial ctor and the places that want non-trivial construction can initialize the members as needed seems like it'd be viable)

I looked at the code again and __hash_node also has nontrivial members, so maybe it's not as feasible to make it a nontrivial constructor.

At least my thinking is: If this type is being constructed without executing a constructor and the code is correct (modulo UB, but correct for the behavior it happens to be getting) then it doesn't really need those ctors to run - so perhpas they shouldn't be there either/should be trivial, because they're not being used, at least here. That's my theory, at least.

[akhuang]

Ok, it seems like the general opinion here is that libc++ should be changed in some way and not ctor homing. I don't know who'd be responsible for changing the libc++ code, though.

Maybe I should file a bug for libc++?

[ldionne]

Sorry, I read this last week and probably forgot to reply.

I think the right thing to do is to fix the UB, which appears to mean fixing libc++. However, can you take a look at whether defining these macros help?

// Fix undefined behavior in  how __tree stores its end and parent nodes.
#  define _LIBCPP_ABI_TREE_REMOVE_NODE_POINTER_UB
// Fix undefined behavior in how __hash_table stores its pointer types.
#  define _LIBCPP_ABI_FIX_UNORDERED_NODE_POINTER_UB
#  define _LIBCPP_ABI_FORWARD_LIST_REMOVE_NODE_POINTER_UB
#  define _LIBCPP_ABI_FIX_UNORDERED_CONTAINER_SIZE_TYPE

We have some known UB in our container types, but fixing that UB is an ABI break, so it's only enabled when some specific macros are turned on. I think you might be hitting that.

[akhuang]

Thanks, looks like I'll try to see if libcxx code can be changed here.

clang/lib/CodeGen/CGDebugInfo.cpp

static bool canUseCtorHoming(const CXXRecordDecl *RD) {
   // aggregate initialization. Also skip lambda objects because they don't call
   // constructors.
 
   // Skip this optimization if the class or any of its methods are marked
   // dllimport.
   if (isClassOrMethodDLLImport(RD))
     return false;
 
-  return !RD->isLambda() && !RD->isAggregate() &&
+  // Attempt to avoid using constructor homing if a class is used but for
+  // whatever reason is not constructed. Constructors are added to the class
+  // definition when used, so use the existence of constructors here as a
+  // heuristic.
+  bool HasCtor = false;
+  for (auto *Ctor : RD->ctors())
+    if (!Ctor->isCopyOrMoveConstructor() && !Ctor->isDeleted())
+      HasCtor = true;
+
+  return HasCtor && !RD->isLambda() && !RD->isAggregate() &&
          !RD->hasTrivialDefaultConstructor() &&
          !RD->hasConstexprNonCopyMoveConstructor();
 }
 
 static bool shouldOmitDefinition(codegenoptions::DebugInfoKind DebugKind,
                                  bool DebugTypeExtRefs, const RecordDecl *RD,
                                  const LangOptions &LangOpts) {
   if (DebugTypeExtRefs && isDefinedInClangModule(RD->getDefinition()))

clang/test/CodeGenCXX/debug-info-limited-ctor.cpp

 
 // Test that we don't use constructor homing on lambdas.
 // CHECK-DAG: ![[L:.*]] ={{.*}}!DISubprogram({{.*}}name: "L"
 // CHECK-DAG: !DICompositeType({{.*}}scope: ![[L]], {{.*}}DIFlagTypePassByValue
 void L() {
   auto func = [&]() {};
 }
 
+// Test that a class that otherwise should be subject to constructor homing
+// is not because its constructor isn't used in this translation unit.
+// CHECK-DAG: !DICompositeType({{.*}}name: "M",{{.*}}DIFlagTypePassByValue
+class M {
+  B b;
+};
+void f(M m) {}
+
 // Check that types are being added to retained types list.
 // CHECK-DAG: !DICompileUnit{{.*}}retainedTypes: ![[RETAINED:[0-9]+]]
 // CHECK-DAG: ![[RETAINED]] = {{.*}}![[C]]