PR22931 - When cloning scopes, reset the scope in Sema instead of using the cloned scope as the current scope

[rtrieu]

Summary

Fix for PR22931, assertion failure with attributes and templates.

When cloning a scope, the constructor of LocalInstantiationScope will set the current scope in Sema to be that new scope. However, cloning scopes doesn't change what the current scope should be. This manifests when multiple late parsed attributed are used. The first attribute has a properly cloned scope, but the other attributes get the wrong scope. The assertion in PR22931 is triggered when a Decl is looked up within an incorrect scope and is not found.

[delesley]

Nice catch! LGTM. BTW, I'm surprised it has taken so long for this bug
to manifest, since we should have lots of code with multiple
late parsed attributes. When did it show up?

[rtrieu]

Committed in r232675

As Richard Smith mentioned, this is an assertion failure which wouldn't show up in release builds. In addition, look at the test case on what is required to trigger this assertion:

2 instantiation scopes (templated class plus templated function)
2 late parsed attributes
second attribute requiring a lookup on the inner scope (The parameter decl F)

Missing any of these would avoid the assertion.

include/clang/Sema/Template.h

void Exit() {
       SemaRef.CurrentInstantiationScope = Outer;
       Exited = true;
     }
 
     /// \brief Clone this scope, and all outer scopes, down to the given
     /// outermost scope.
     LocalInstantiationScope *cloneScopes(LocalInstantiationScope *Outermost) {
       if (this == Outermost) return this;
+
+      // Save the current scope from SemaRef since the LocalInstantiationScope
+      // will overwrite it on construction
+      LocalInstantiationScope *oldScope = SemaRef.CurrentInstantiationScope;
+
       LocalInstantiationScope *newScope =
         new LocalInstantiationScope(SemaRef, CombineWithOuterScope);
 
       newScope->Outer = nullptr;
       if (Outer)
         newScope->Outer = Outer->cloneScopes(Outermost);
 
       newScope->PartiallySubstitutedPack = PartiallySubstitutedPack;
LocalInstantiationScope *cloneScopes(LocalInstantiationScope *Outermost) {
           Stored = I->second.get<Decl *>();
         } else {
           DeclArgumentPack *OldPack = I->second.get<DeclArgumentPack *>();
           DeclArgumentPack *NewPack = new DeclArgumentPack(*OldPack);
           Stored = NewPack;
           newScope->ArgumentPacks.push_back(NewPack);
         }
       }
+      // Restore the saved scope to SemaRef
+      SemaRef.CurrentInstantiationScope = oldScope;
       return newScope;
     }
 
     /// \brief deletes the given scope, and all otuer scopes, down to the
     /// given outermost scope.
     static void deleteScopes(LocalInstantiationScope *Scope,
                              LocalInstantiationScope *Outermost) {
       while (Scope && Scope != Outermost) {

test/SemaCXX/warn-thread-safety-negative.cpp

void test3() EXCLUSIVE_LOCKS_REQUIRED(!mu) {
     mu.Lock();
     a = 0;
     mu.Unlock();
     baz();       // no warning -- !mu in set.
   }
 };
 
 }  // end namespace SimpleTest
+
+namespace DoubleAttribute {
+
+struct Foo {
+  Mutex &mutex();
+};
+
+template <typename A>
+class TemplateClass {
+  template <typename B>
+  static void Function(Foo *F)
+      EXCLUSIVE_LOCKS_REQUIRED(F->mutex()) UNLOCK_FUNCTION(F->mutex()) {}
+};
+
+void test() { TemplateClass<int> TC; }
+
+}  // end namespace DoubleAttribute