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

[Richard Smith]

On Wed, Mar 18, 2015 at 8:56 AM, Delesley Hutchins <delesley at google.com>
wrote:

>
> 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?
>

It caused an assert to fire, which doesn't show up for release Clang builds.


> On Tue, Mar 17, 2015 at 8:12 PM, Richard Trieu <rtrieu at google.com> wrote:
>
>> Hi delesley,
>>
>> 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.
>>
>> http://reviews.llvm.org/D8407
>>
>> Files:
>>   include/clang/Sema/Template.h
>>   test/SemaCXX/warn-thread-safety-negative.cpp
>>
>> Index: include/clang/Sema/Template.h
>> ===================================================================
>> --- include/clang/Sema/Template.h
>> +++ include/clang/Sema/Template.h
>> @@ -273,6 +273,11 @@
>>      /// 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);
>>
>> @@ -299,6 +304,8 @@
>>            newScope->ArgumentPacks.push_back(NewPack);
>>          }
>>        }
>> +      // Restore the saved scope to SemaRef
>> +      SemaRef.CurrentInstantiationScope = oldScope;
>>        return newScope;
>>      }
>>
>> Index: test/SemaCXX/warn-thread-safety-negative.cpp
>> ===================================================================
>> --- test/SemaCXX/warn-thread-safety-negative.cpp
>> +++ test/SemaCXX/warn-thread-safety-negative.cpp
>> @@ -102,3 +102,20 @@
>>  };
>>
>>  }  // 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
>>
>> EMAIL PREFERENCES
>>   http://reviews.llvm.org/settings/panel/emailpreferences/
>>
>
>
>
> --
> DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150318/a4b5d33f/attachment.html>