[CUDA][HIP] Fix ShouldDeleteSpecialMember for inherited constructors

[yaxunl]

Summary

ShouldDeleteSpecialMember is called upon inherited constructors.
It calls inferCUDATargetForImplicitSpecialMember.

Normally the special member enum passed to ShouldDeleteSpecialMember
matches the constructor. However this is not true when inherited
constructor is passed, where DefaultConstructor is passed to treat
the inherited constructor as DefaultConstructor. However
inferCUDATargetForImplicitSpecialMember expects the special
member enum argument to match the constructor, which results
in assertion when this expection is not satisfied.

This patch checks whether the constructor is inherited. If true it will
get the real special member enum for the constructor and pass it
to inferCUDATargetForImplicitSpecialMember.

[tra]

@jlebar Justin, can you take a look?

[yaxunl]

ping

[jlebar]

Sorry for missing tra's ping earlier, I get a lot of HIP email traffic that's 99% inactionable by me, so I didn't notice my username in tra's earlier email.

@tra, @timshen, and I debugged this IRL for a few hours this afternoon. The result of this is that we don't think the fix in this patch is correct.

Here's what we think is happening.

When clang sees using A::A inside of B, it has to check whether this constructor is legal in B. An example of where this constructor would *not* be legal is something like:

struct NoDefaultConstructor { NoDefaultConstructor() = delete; };
struct A { A(const int& x) {} }
struct B {
  using A::A;
  NoDefaultConstructor c;
};

The reason this using A::A is not legal here is because the using statement is equivalent to writing

B(const int& x) : A(x) {}

but this constructor is not legal, because NoDefaultConstructor is not default-constructible, and a constructor for B must explicitly initialize all non-default-initializable members.

Here is the code that checks whether the using statement is legal:

https://github.com/llvm-project/llvm-project-20170507/blob/51b65eeeab0d24268783d6246fd949d9a16e10e8/clang/lib/Sema/SemaDeclCXX.cpp#L11018

This code is kind of a lie! DerivedCtor is the constructor B(const int& x) : A(x) {} that we've created in response to the using declaration. Notice that it's not a default constructor! In fact, it's not even a special member (i.e. it's not a default constructor, copy constructor, move constructor, etc.). But notice that we pass CXXDefaultConstructor, and we call the function ShouldDeleteSpecialMember!

The reason we don't tell the truth here seems to be out of convenience. To determine whether we should delete the new constructor on B, it seems like we are trying to ask: Would a default constructor on B be legal, ignoring the fact that A has to be explicitly initialized? That is, the new constructor we're creating is just like a default constructor on B, except that first it initializes A. So we're trying to reuse the default constructor logic.

But eventually our tricks and dishonesty catch up to us, here in CUDA code. This patch fixes one instance where we do the wrong thing and hit an assertion, but who knows if the code is right in general; simply adding on another layer of hack does not seem like the right approach to us.

cc @rsmith

[yaxunl]

Revised by Justin's comments.

Handle the situation where an inherited constructor is faked to be a default constructor but it is really not.

[yaxunl]

fix a typo.

[jlebar]

lib/Sema/SemaDeclCXX.cpp

7231

LGTM, but perhaps we should use a new variable instead of modifying CSM in case someone adds code beneath this branch?

[yaxunl]

lib/Sema/SemaDeclCXX.cpp

7231

will do when committing.

lib/Sema/SemaDeclCXX.cpp

     return true;
 
   if (SMI.shouldDeleteForAllConstMembers())
     return true;
 
   if (getLangOpts().CUDA) {
     // We should delete the special member in CUDA mode if target inference
     // failed.
-    return inferCUDATargetForImplicitSpecialMember(RD, CSM, MD, SMI.ConstArg,
-                                                   Diagnose);
+    return inferCUDATargetForImplicitSpecialMember(
+        RD, CSM, MD, CSM == CXXDefaultConstructor ? false : SMI.ConstArg,
+        Diagnose);
   }
 
   return false;
 }
 
 /// Perform lookup for a special member of the specified kind, and determine
 /// whether it is trivial. If the triviality can be determined without the
 /// lookup, skip it. This is intended for use when determining whether a

     //   A default constructor is trivial if:
     //    - all the [direct subobjects] have trivial default constructors
     //
     // Note, no overload resolution is performed in this case.
     if (RD->hasTrivialDefaultConstructor())
       return true;
 
     if (Selected) {
       // If there's a default constructor which could have been trivial, dig it
       // out. Otherwise, if there's any user-provided default constructor, point
       // to that as an example of why there's not a trivial one.
       CXXConstructorDecl *DefCtor = nullptr;
       if (RD->needsImplicitDefaultConstructor())
         S.DeclareImplicitDefaultConstructor(RD);
       for (auto *CI : RD->ctors()) {
         if (!CI->isDefaultConstructor())
           continue;

test/SemaCUDA/inherited-ctor.cu

+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+struct A {
+  A(const int &x) {}
+};
+
+struct B : A {
+  using A::A;
+};
+
+struct C {
+  struct B b;
+  C() : b(0) {}
+};
+
+void test() {
+  B b(0);
+  C c;
+}