[PATCH] D51809: [CUDA][HIP] Fix assertion in LookupSpecialMember
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51809: [CUDA][HIP] Fix assertion in LookupSpecialMember
yaxunl added a comment. ping https://reviews.llvm.org/D51809 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51809: [CUDA][HIP] Fix assertion in LookupSpecialMember
tra added a comment. @jlebar Justin, can you take a look? https://reviews.llvm.org/D51809 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51809: [CUDA][HIP] Fix assertion in LookupSpecialMember
yaxunl created this revision.
yaxunl added a reviewer: tra.
ShouldDeleteSpecialMember is called upon inherited constructors.
It calls inferCUDATargetForImplicitSpecialMember, which in turn
calls LookupSpecialMember.
LookupSpecialMember expects ConstArg==false for
CXXDefaultConstructor. For inherited constructor with
const arguments, this causes assertion.
This patch fixes that by passing false as ConstArg argument
for CXXDefaultConstructor in ShouldDeleteSpecialMember.
https://reviews.llvm.org/D51809
Files:
lib/Sema/SemaDeclCXX.cpp
test/SemaCUDA/inherited-ctor.cu
Index: test/SemaCUDA/inherited-ctor.cu
===
--- /dev/null
+++ test/SemaCUDA/inherited-ctor.cu
@@ -0,0 +1,20 @@
+// 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;
+}
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -7187,8 +7187,9 @@
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;
Index: test/SemaCUDA/inherited-ctor.cu
===
--- /dev/null
+++ test/SemaCUDA/inherited-ctor.cu
@@ -0,0 +1,20 @@
+// 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;
+}
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -7187,8 +7187,9 @@
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;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
