[PATCH] D51809: [CUDA][HIP] Fix assertion in LookupSpecialMember

[Justin Lebar via Phabricator via cfe-commits]

jlebar requested changes to this revision.
jlebar added subscribers: timshen, rsmith.
jlebar added a comment.
This revision now requires changes to proceed.

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


https://reviews.llvm.org/D51809