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

4403

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

4407

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

4409

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

4415–4416

Put else on the same line as }.

4449

Use auto here.

4451

The assert is excessive.

4457–4458

Put else on the same line as }.

[twoh]

Addressing comments from @sepavloff. Thanks!

[twoh]

Ping

[rsmith]

lib/Sema/SemaExprCXX.cpp

4401–4402

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

4406–4408

This is better written as

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

4409–4412

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.

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.
           // For now, we'll be conservative and assume that they can throw.
           if (!CPT->isNothrow(C) || CPT->getNumParams() > 1)
             return false;
         }
       }
 
       return FoundConstructor;
     }
     return false;
   case UTT_HasNothrowConstructor:
     // http://gcc.gnu.org/onlinedocs/gcc/Type-Traits.html
     //   If __has_trivial_constructor (type) is true then the trait is
     //   true, else if type is a cv class or union type (or array
     //   thereof) with a default constructor that is known not to
     //   throw an exception then the trait is true, else it is false.
     if (T.isPODType(C) || T->isObjCLifetimeType())
       return true;
     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.
           // For now, we'll be conservative and assume that they can throw.
           if (!CPT->isNothrow(C) || CPT->getNumParams() > 0)
             return false;
         }
       }
       return FoundConstructor;
     }
     return false;
   case UTT_HasVirtualDestructor:
     // http://gcc.gnu.org/onlinedocs/gcc/Type-Traits.html:
     //   If type is a class type with a virtual destructor ([class.dtor])
     //   then the trait is true, else it is false.
     if (CXXRecordDecl *RD = T->getAsCXXRecordDecl())

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); }
+}