[PATCH] D87808: [DebugInfo] Fix bug in constructor homing where it would use ctor homing when a class only has copy/move constructors

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D87808#2280197 <https://reviews.llvm.org/D87808#2280197>, @dblaikie wrote:

> @rsmith What's the deal with these anonymous structs/unions? Why do they have copy/move constructors (are those technically called from the enclosing class's copy/move constructors?) but no default constructor to be called from the other ctors of the enclosing class?

Let's use the class `G` in the example. `G::G` directly initializes the field `g_`, so it can't call any kind of constructor for the anonymous struct member. Instead, anonymous structs and unions are "expanded" when building the constructor initializer lists for all cases other than a copy or move constructor (no initialization action is taken for union members by default, though -- not unless they have a default member initializer or an explicit mem-initializer). So the default constructor of an anonymous struct or union is never really used for anything[*].

But now consider the copy or move constructor for a type like this:

  struct X {
    Y y;
    union { ... };
  };

That copy or move constructor needs to build a member initializer list that (a) calls the `Y` copy/move constructor for `y`, and (b) performs a bytewise copy for the anonymous union. We can't express this in the "expanded" form, because we wouldn't know which union member to initialize.

So for the non-copy/move case, we build `CXXCtorInitializers` for expanded members, and in the copy/move case, we build `CXXCtorInitializers` calling the copy/move constructors for the anonymous structs/unions themselves.

[*]: It's not entirely true that the default constructor is never used for anything. When we look up special members for the anonymous struct / union (looking for the copy or move constructor) we can trigger the implicit declaration of the default constructor. And it's actually even possible to *call* that default constructor, if you're tricksy enough: https://godbolt.org/z/Tq56bz

In D87808#2282223 <https://reviews.llvm.org/D87808#2282223>, @rnk wrote:

> Maybe the issue is that this code is running into the lazy implicit special member declaration optimization. Maybe the class in question has an implicit, trivial, default constructor, but we there is no CXXConstructorDecl present in the ctors list for the loop to find.

That seems very likely. The existing check:

  if (Ctor->isTrivial() && !Ctor->isCopyOrMoveConstructor())
    return false;

is skating on thin ice in this regard: a class with an implicit default constructor might or might not have had that default constructor implicitly declared. But I think this code should give the same outcome either way, because a class with any constructor other than a default/copy/move constructor must have a user-declared constructor of some kind, and so will never have an implicit default constructor. (Inherited constructors introduce some complications here, but we don't do lazy constructor declaration for classes with inherited constructors, so I think that's OK too.)



================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2292-2300
+  bool hasCtor = false;
+  for (const auto *Ctor : RD->ctors()) {
     if (Ctor->isTrivial() && !Ctor->isCopyOrMoveConstructor())
       return false;
+    if (!Ctor->isCopyOrMoveConstructor())
+      hasCtor = true;
+  }
----------------
This looks pretty similar to:

```
return RD->hasUserDeclaredConstructor() && !RD->hasTrivialDefaultConstructor();
```

(other than its treatment of user-declared copy or move constructors), but I have to admit I don't really understand what the "at least one constructor and no trivial or constexpr constructors" rule aims to achieve, so it's hard to know if this is the right interpretation. The rule as written in the comment above is presumably not exactly right -- all classes have at least one constructor, and we're not excluding classes with trivial copy or move constructors, only those with trivial default constructors.

I wonder if the intent would be better captured by "at least one non-inline constructor" (that is, assuming all declared functions are defined, there is at least one translation unit in which a constructor is defined that can be used as a home for the class info).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87808