[cfe-commits] Possible fix for PR9685

[Erik Olofsson]

I have taken a stab at this with the attached patch.

At the exit of each LocalInstantiationScope we save all instantiations to a permanent instantiation scope for their parent function.

When an instantiation of a function is on the stack we create the permanent instantiation scope for this function and link this permanent scope to it's local LocalInstantiationScope on the stack. We also look for a parent function of this function and link it into the permanent scope stack. We also take care to link the current stack scope for the function because we may need to restore this scope recursively and then we need the local stack scope in the chain as well. This is handled with FunctionInInstantiationScope helper.

We then use RestoreLocalInstantiationScope together with setupLocalScope to restore the function local scope chain for the instantiations that require it.

I also fixed bugs in the parser that got template depth wrong with recursive function-local scopes.

Test cases were added that includes the cases of local scopes not working I found during the development of this.

Blocks don't seem to be functions internally so they will probably not work correctly with function local contexts.

While I was developing this I sometimes ended up with a static_assert that was still dependant and the compiler didn't complain about this just silently ignored it, I was not sure how to fix this but just thought I would mention it.

// Erik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PR9685.patch
Type: application/octet-stream
Size: 41717 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120730/dacc2d4e/attachment.obj>
-------------- next part --------------


On 2011-10-19, at 07:20, Douglas Gregor <dgregor at apple.com> wrote:

> 
> On Oct 17, 2011, at 10:45 PM, David Blaikie wrote:
> 
>> I'll be the first to admit I'm not entirely confident that this fix is the right one - but at the very least it's a starting point. It fixes the bug and doesn't regress any other clang tests.
>> 
>> If there are things this change breaks, I'll be happy to add tests for those cases & work on a fix that accommodates them, or if there's just a more appropriate way to express the required semantics, I'm all ears.
>> 
>> [basically, members of local structs were being instantiated in the context of their reference, not their definition, and failing because the Decl corresponding to the template of the member wasn't in the context of the instantiation scope (forEach in the attached example). By extending the lifetime of the LocalInstantiationScope and walking up past uncombined scopes in LocalInstantiationScope::findInstantiationOf it's now able to find the relevant Decl & instantiate correctly]
> 
> I see why this appears to be working, but it's not actually the right fix. Let's walk into what it's actually doing:
> 
> Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
> ===================================================================
> --- lib/Sema/SemaTemplateInstantiateDecl.cpp    (revision 142336)
> +++ lib/Sema/SemaTemplateInstantiateDecl.cpp    (working copy)
> @@ -2540,7 +2540,6 @@
>   // This class may have local implicit instantiations that need to be
>   // instantiation within this scope.
>   PerformPendingInstantiations(/*LocalOnly=*/true);
> -  Scope.Exit();
> 
> This tells Clang not to exit the instantiation scope, i.e., the thing that maps from entities in the template to entities that map into the current instantiation. So, we'll end up leaving the mapping for this instantiation (doIt<int>) available while we instantiate other templates (including completely unrelated templates whose instantiation happens to have shown up in the list of instantiations).
> 
> Now let's look at:
> 
> Index: lib/Sema/SemaTemplateInstantiate.cpp
> ===================================================================
> --- lib/Sema/SemaTemplateInstantiate.cpp        (revision 142336)
> +++ lib/Sema/SemaTemplateInstantiate.cpp        (working copy)
> @@ -2291,10 +2291,6 @@
>       else
>         CheckD = 0;
>     } while (CheckD);
> -
> -    // If we aren't combined with our outer scope, we're done.
> -    if (!Current->CombineWithOuterScope)
> -      break;
>   }
> 
> This change tells Clang to keep looking into outer instantiation scopes when it can't find what it's looking for in the current instantiation scope. That CombineWithOuterScope barrier is important, however, to make sure that we don't ever end up using bindings from other instantiations of the same template that are further up on the stack. This can become problematic with, e.g., label declarations, which are instantiated on demand.
> 
> I don't have a killer test case for you that breaks when you make this change; I suspect that that test case is out there, but that it is going to be very hard to write. However, let's see how things went wrong:
> 
>  - In Sema::MarkDeclarationReferenced, not how around line 9365 we check whether the context of an instantiable function is a local class and, if so, push it into the set of pending *local* instantiations rather than the set of pending (global) instantiations).
>  - In Sema::InstantiateFunctionDefinition, we create a LocalInstantiationScope and again check for a local class. If we're instantiating something in a local class, then we merge our current instantiation scope with its parent scope, which turns on that CombineWithOuterScope flag (i.e., skipping the break in the second patch snippet above).
>  - Late in Sema::InstantiateFunctionDefinition, we instantiate all of the pending local instantiations while we're still inside the instantiation scope of the original function. So, in combination with the previous bullet, this means that those instantiations can access our mappings from local names (e..g, Functor) to their instantiations (the instantiated Functor).
> 
> Now, things break in your test case because we're forEach<Functor> is not classified as a local instantiation, so it doesn't get instantiated while we're still inside doIt<int>'s scope. Hence, when forEach<Functor> then triggers an instantiation of doIt<int>::Functor::operator(), we're not longer
> 
> What your patch is doing is essentially treating *all* instantiations like they are local instantiations, so everything gets instantiated while doIt<int> is on the current instantiation stack, and all of the instantiation scopes are effectively merged.
> 
> I *think* the right answer is that, when there are local classes involved in the instantiation, we're going to want to keep around the contents of the LocalInstantiationScope in some side data structure. Then, when we're instantiating something that somehow refers to the local class (or a member of it), we'll have to re-create a LocalInstantiationScope as a parent LocalInstantiationScope, so that the right set of bindings are available.
> 
>        - Doug
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits