Page MenuHomePhabricator

[CUDA][HIP] Fix ShouldDeleteSpecialMember for inherited constructors
ClosedPublic

Authored by yaxunl on Sep 7 2018, 12:35 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

yaxunl created this revision.Sep 7 2018, 12:35 PM
tra added a reviewer: jlebar.Sep 7 2018, 1:32 PM

@jlebar Justin, can you take a look?

jlebar requested changes to this revision.Sep 21 2018, 5:49 PM
jlebar added subscribers: timshen, rsmith.

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

This revision now requires changes to proceed.Sep 21 2018, 5:49 PM
yaxunl updated this revision to Diff 168479.Oct 5 2018, 9:35 AM
yaxunl retitled this revision from [CUDA][HIP] Fix assertion in LookupSpecialMember to [CUDA][HIP] Fix ShouldDeleteSpecialMember for inherited constructors.
yaxunl edited the summary of this revision. (Show Details)

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 edited the summary of this revision. (Show Details)Oct 5 2018, 9:37 AM
yaxunl updated this revision to Diff 168500.Oct 5 2018, 12:01 PM

fix a typo.

jlebar accepted this revision.Oct 6 2018, 3:36 PM
jlebar added inline comments.
lib/Sema/SemaDeclCXX.cpp
7231 ↗(On Diff #168500)

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

This revision is now accepted and ready to land.Oct 6 2018, 3:36 PM
yaxunl added inline comments.Oct 9 2018, 8:25 AM
lib/Sema/SemaDeclCXX.cpp
7231 ↗(On Diff #168500)

will do when committing.

This revision was automatically updated to reflect the committed changes.