Page MenuHomePhabricator

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

Authored by rtrieu on Mar 17 2015, 8:12 PM.

Details

Reviewers
rtrieu
delesley
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.

Diff Detail

Event Timeline

rtrieu updated this revision to Diff 22153.Mar 17 2015, 8:12 PM
rtrieu retitled this revision from to PR22931 - When cloning scopes, reset the scope in Sema instead of using the cloned scope as the current scope.
rtrieu updated this object.
rtrieu edited the test plan for this revision. (Show Details)
rtrieu added a reviewer: delesley.
rtrieu added a subscriber: Unknown Object (MLST).
delesley edited edge metadata.Mar 18 2015, 8:56 AM

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 accepted this revision.Mar 18 2015, 2:58 PM
rtrieu added a reviewer: rtrieu.
This revision is now accepted and ready to land.Mar 18 2015, 2:58 PM
rtrieu closed this revision.Mar 18 2015, 3:03 PM

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.