[clang] Don't segfault on incorrect using directive (PR41400)

[Tyker]

Summary

this is a bugfixe for PR41400

added nullptr check at the relevent place and test

[Tyker]

awaiting feedback on this

[riccibruno]

I will take a look at this tomorrow, I know that it is annoying to get no feedback!

[riccibruno]

In D60523#1473867, @riccibruno wrote:

I will take a look at this tomorrow, I know that it is annoying to get no feedback!

Sorry, I don't think I can judge whether this is the correct solution. I don't know if returning a type for the injected-class-name is the correct thing to do when there is no nested-name-specifier.

[jkorous]

I can't really comment on correctness of your fix but had been willing to do the work I'd suggest making ASTContext::getDependentNameType and DependentNameType::DependentNameType interface more robust.

With current master (95c18c7beec) the crash is here (with your test):

thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x10)

  frame #0: 0x0000000105f0c2e0 clang` clang::NestedNameSpecifier::getKind(this=0x0000000000000000) const  + 16 at NestedNameSpecifier.cpp:143
  frame #1: 0x0000000105f0c659 clang` clang::NestedNameSpecifier::containsUnexpandedParameterPack(this=0x0000000000000000) const  + 25 at NestedNameSpecifier.cpp:253
* frame #2: 0x00000001059f2dc1 clang` clang::DependentNameType::DependentNameType(this=0x0000000116852b30, Keyword=ETK_None, NNS=0x0000000000000000, Name=0x000000011686eff8, CanonType=QualType @ 0x00007ffeefbf7818)  + 65 at Type.h:5238
  frame #3: 0x00000001059c1113 clang` clang::DependentNameType::DependentNameType(this=0x0000000116852b30, Keyword=ETK_None, NNS=0x0000000000000000, Name=0x000000011686eff8, CanonType=QualType @ 0x00007ffeefbf7858)  + 51 at Type.h:5239
  frame #4: 0x00000001059c0d9f clang` clang::ASTContext::getDependentNameType(this=0x0000000116815000, Keyword=ETK_None, NNS=0x0000000000000000, Name=0x000000011686eff8, Canon=QualType @ 0x00007ffeefbf7990) const  + 431 at ASTContext.cpp:4247
  frame #5: 0x000000010509f539 clang` clang::Sema::getConstructorName(this=0x0000000116846600, II=0x000000011686eff8, NameLoc=(ID = 35), S=0x000000011530bc40, SS=0x00007ffeefbf8730, EnteringContext=false)  + 377 at SemaExprCXX.cpp:94

The problem starts in frame #5 in getConstructorName where your fix is but

• DependentNameType should also have a tighter interface and not accept NSS = nullptr in constructor since it obviously expects some value to be present (calling a method on it) - the parameter should be a reference.
• ASTContext::getDependentNameType also seems to expect some non-null value - it should either make this obvious in the interface by using a reference or deal with absence of the value in a reasonable manner.

[Tyker]

@jkooker
I don't think it is possible for ASTContext::getDependentNameType to deal with NSS = nullptr except by reporting the error. we probably don't want to just report the error because the error could have been handled before invoking ASTContext::getDependentNameType. I agree that it should optimally be reflected in the interface of those functions because pointer are often used a optional references. but wouldn't an assert + doxygen comment suffice ? instead of changing the interface, implementation and all callsites. Pointers as argument that are required not to be null are used in a lot of places in LLVM. For example DependentNameType::DependentNameType takes NNS as a pointer even thought it requires it to be non-null(calls a method on it). Mixing pointers and references adds a lot of & and * which makes the code harder to read.

cfe/trunk/lib/Sema/SemaExprCXX.cpp

ParsedType Sema::getConstructorName(IdentifierInfo &II,
                                     bool EnteringContext) {
   CXXRecordDecl *CurClass = getCurrentClass(S, &SS);
   assert(CurClass && &II == CurClass->getIdentifier() &&
          "not a constructor name");
 
   // When naming a constructor as a member of a dependent context (eg, in a
   // friend declaration or an inherited constructor declaration), form an
   // unresolved "typename" type.
-  if (CurClass->isDependentContext() && !EnteringContext) {
+  if (CurClass->isDependentContext() && !EnteringContext && SS.getScopeRep()) {
     QualType T = Context.getDependentNameType(ETK_None, SS.getScopeRep(), &II);
     return ParsedType::make(T);
   }
 
   if (SS.isNotEmpty() && RequireCompleteDeclContext(SS, CurClass))
     return ParsedType();
 
   // Find the injected-class-name declaration. Note that we make no attempt to

cfe/trunk/test/SemaCXX/using-decl-1.cpp

namespace N {
     int X;
 
     struct Z {};
   }
   using N::X;
   using N::Y;
   using N::Z;
 }
+
+// expected-error@+5 {{requires a qualified name}}
+// expected-error@+4 {{expected ';'}}
+// expected-error@+3 {{expected '}'}}
+// expected-note@+2 {{to match this '{'}}
+// expected-error@+1 {{expected ';'}}
+template<class> struct S { using S
 No newline at end of file