Fix for clang PR 29087

[twoh]

Summary

Since rL274049, for an inheriting constructor declaration, the name of the using declaration (and using shadow declaration comes from the using declaration) is the name of a derived class, not the base class (line 8225-8232 of lib/Sema/SemaDeclCXX.cpp in https://reviews.llvm.org/rL274049). Because of this, name-based lookup performed inside Sema::LookupConstructors returns not only CXXConstructorDecls but also Using(Shadow)Decls, which results assertion failure reported in PR 29087.

[twoh]

ping

[twoh]

Ping.

[sepavloff]

You need to provide a test for the fix.

[twoh]

Tests added

[rsmith]

lib/Sema/SemaExprCXX.cpp

4227 ↗

(On Diff #71560)

This isn't really right: a UsingShadowDecl whose underlying declaration is a constructor should itself act like a constructor. This could matter in some obscure cases:

struct B;
struct A { A(B&); };
struct B : A { using A::A; };

What does __has_nothrow_copy(B) return? It should probably be false, since copying a non-const B object will invoke the A(B&) constructor, which may throw, even though the B(const B&) constructor does not.

However, these __has_* traits should be considered deprecated and are essentially useless, so perhaps it doesn't matter too much whether we get these corner cases right. We also get the constructor template case "wrong" here, which I would imagine comes up a lot more frequently.

test/SemaCXX/crash-has-nothrow-constructor.cpp

1 ↗

(On Diff #71560)

Please add these tests into some existing test file for the type traits rather than adding two new files.

[twoh]

Updated diff. For ConstructorUsingShadowDecl, test with its target CXXConstructorDecl, but only when it is not a default/copy/move constructor.

[twoh]

@rsmith Thank you for your review! I added tests to cxx11-crashes.cpp, as the goal of this patch is not handling __has_* traits right but preventing ICE. Also, I tried to use ConstructorUsingShadowDecl::getConstructor instead of ConstructorUsingShadowDecl::getTargetDecl followed by casting, but clang build was failed with undefined references. I wonder if the definition of ConstructorUsingShadowDecl::getConstructor is missed in https://reviews.llvm.org/rL274049.

[twoh]

ping

[twoh]

ping

[twoh]

ping

[twoh]

ping

[sepavloff]

lib/Sema/SemaExprCXX.cpp

4231 ↗

(On Diff #71711)

Use auto here. Type of CSD is clear from dyn_cast.

4233 ↗

(On Diff #71711)

This assert is excessive. The subsequent cast makes this check.

4239–4240 ↗

(On Diff #71711)

Put else on the same line as }.

4281 ↗

(On Diff #71711)

Use auto here.

4283 ↗

(On Diff #71711)

The assert is excessive.

4289–4290 ↗

(On Diff #71711)

Put else on the same line as }.

[twoh]

Addressing comments from @sepavloff. Thanks!

[twoh]

Ping

[rsmith]

lib/Sema/SemaExprCXX.cpp

4390–4391 ↗

(On Diff #75441)

You also need to handle the case of an inherited constructor template (isa<FunctionTemplateDecl>(ND->getUnderlyingDecl()) will do the right thing in both cases).

4395–4397 ↗

(On Diff #75441)

This is better written as

auto *Constructor = cast<CXXConstructorDecl>(ND->getUnderlyingDecl());

4398–4401 ↗

(On Diff #75441)

This is not necessary: Sema has already filtered out the ones that aren't inherited.

[twoh]

Addressing comments from @rsmith. Thanks!

[• gnzlbg]

ping

[rsmith]

Looks good for trunk and 3.9 branch.

cfe/trunk/lib/Sema/SemaExprCXX.cpp

if (CXXRecordDecl *RD = T->getAsCXXRecordDecl()) {
         return true;
 
       bool FoundConstructor = false;
       unsigned FoundTQs;
       for (const auto *ND : Self.LookupConstructors(RD)) {
         // A template constructor is never a copy constructor.
         // FIXME: However, it may actually be selected at the actual overload
         // resolution point.
-        if (isa<FunctionTemplateDecl>(ND))
+        if (isa<FunctionTemplateDecl>(ND->getUnderlyingDecl()))
           continue;
-        const CXXConstructorDecl *Constructor = cast<CXXConstructorDecl>(ND);
+        // UsingDecl itself is not a constructor
+        if (isa<UsingDecl>(ND))
+          continue;
+        auto *Constructor = cast<CXXConstructorDecl>(ND->getUnderlyingDecl());
         if (Constructor->isCopyConstructor(FoundTQs)) {
           FoundConstructor = true;
           const FunctionProtoType *CPT
               = Constructor->getType()->getAs<FunctionProtoType>();
           CPT = Self.ResolveExceptionSpec(KeyLoc, CPT);
           if (!CPT)
             return false;
           // TODO: check whether evaluating default arguments can throw.
case UTT_HasNothrowConstructor:
     if (CXXRecordDecl *RD = C.getBaseElementType(T)->getAsCXXRecordDecl()) {
       if (RD->hasTrivialDefaultConstructor() &&
           !RD->hasNonTrivialDefaultConstructor())
         return true;
 
       bool FoundConstructor = false;
       for (const auto *ND : Self.LookupConstructors(RD)) {
         // FIXME: In C++0x, a constructor template can be a default constructor.
-        if (isa<FunctionTemplateDecl>(ND))
+        if (isa<FunctionTemplateDecl>(ND->getUnderlyingDecl()))
+          continue;
+        // UsingDecl itself is not a constructor
+        if (isa<UsingDecl>(ND))
           continue;
-        const CXXConstructorDecl *Constructor = cast<CXXConstructorDecl>(ND);
+        auto *Constructor = cast<CXXConstructorDecl>(ND->getUnderlyingDecl());
         if (Constructor->isDefaultConstructor()) {
           FoundConstructor = true;
           const FunctionProtoType *CPT
               = Constructor->getType()->getAs<FunctionProtoType>();
           CPT = Self.ResolveExceptionSpec(KeyLoc, CPT);
           if (!CPT)
             return false;
           // FIXME: check whether evaluating default arguments can throw.

cfe/trunk/test/SemaCXX/cxx11-crashes.cpp

 void Foo(S);
 
 void test(int some_number) {  // expected-note {{'some_number' declared here}}
   auto x = sum_number;  // expected-error {{use of undeclared identifier 'sum_number'; did you mean 'some_number'?}}
   auto lambda = [x] {};
   Foo(lambda);
 }
 }
+
+namespace pr29091 {
+  struct X{ X(const X &x); };
+  struct Y: X { using X::X; };
+  bool foo() { return __has_nothrow_constructor(Y); }
+  bool bar() { return __has_nothrow_copy(Y); }
+
+  struct A { template <typename T> A(); };
+  struct B : A { using A::A; };
+  bool baz() { return __has_nothrow_constructor(B); }
+  bool qux() { return __has_nothrow_copy(B); }
+}