[cfe-commits] Possible fix for PR9685
Douglas Gregor
dgregor at apple.com
Wed Aug 22 21:50:09 PDT 2012
On Jul 30, 2012, at 5:35 AM, Erik Olofsson <Erik.Olofsson at hansoft.se> wrote:
> 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.
Right, blocks are BlockDecls. We don't have to deal with blocks in the first implement of this.
> 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.
Interesting. Do you have an example of this that we could poke at?
Very nice patch. A couple comments, most trivial and some more substantial,
@@ -352,6 +369,47 @@
unsigned *NumExplicitArgs = 0) const;
};
+ /// \brief Keeps track of overrides for local instantiation scopes
+ /// used to reset the scope when instantiating dependant types defined
+ /// in function local contexts.
Typo "dependant"; should be "dependent".
+ /// \brief Keeps track of linking permanent function scopes into the currently
+ /// instanciating scope. This is needed because the permanent scope might be
+ /// recursively overridden.
Typo "instanciating".
+void FunctionInInstantiationScope::functionInScope(
+ LocalInstantiationScope *Scope, FunctionDecl *D) {
+ assert(LinkedSavedFunctionScope == 0 && "Function already in scope");
+ Sema::SavedLocalScope &SavedScope = SemaRef.SavedLocalFunctionScopes[D];
+ if (!SavedScope.SaveScope)
+ SavedScope.SaveScope = new LocalInstantiationScope(SemaRef,
+ /*CombineWithOuterScope=*/true, /*IsSaved=*/true);
+ assert((!D->getTemplateInstantiationPattern()
+ || D->getTemplateInstantiationPattern() != D)
+ && "Can only be used on instantiations");
+
+ const FunctionDecl *ParentFunc = cast_or_null<const FunctionDecl>(
+ D->getParentFunctionOrMethod());
Shouldn't need the "const" in "const FunctionDecl".
Decl *Sema::SubstDecl(Decl *D, DeclContext *Owner,
const MultiLevelTemplateArgumentList &TemplateArgs) {
- TemplateDeclInstantiator Instantiator(*this, Owner, TemplateArgs);
- if (D->isInvalidDecl())
- return 0;
+
+ RestoreLocalInstantiationScope RestoreLocalScope(*this);
+ if (setupLocalScope(RestoreLocalScope, dyn_cast_or_null<Decl>(D),
+ dyn_cast_or_null<Decl>(Owner))) {
+ LocalInstantiationScope Scope(*this, /*CombineWithOuterScope=*/true);
- return Instantiator.Visit(D);
+ TemplateDeclInstantiator Instantiator(*this, Owner, TemplateArgs);
+ if (D->isInvalidDecl())
+ return 0;
+
+ return Instantiator.Visit(D);
+ }
+ else {
+ TemplateDeclInstantiator Instantiator(*this, Owner, TemplateArgs);
+ if (D->isInvalidDecl())
+ return 0;
+
+ return Instantiator.Visit(D);
+ }
}
Since the "if" always returns, please de-nest the "else".
+void LocalInstantiationScope::saveDeclInPermanentScope(Decl const *D,
+ Decl *Inst) {
+ FunctionDecl *ParentFunction = dyn_cast_or_null<FunctionDecl>(
+ Inst->getParentFunctionOrMethod());
+ if (ParentFunction) {
+
+ FunctionDecl *Pattern =
+ ParentFunction->getTemplateInstantiationPattern();
+ if (Pattern && Pattern != ParentFunction)
+ {
We tend to prefer hanging braces, e.g.,
if (Pattern && Pattern != ParentFunction) {
Index: lib/Parse/ParseStmt.cpp
===================================================================
--- lib/Parse/ParseStmt.cpp (revision 160944)
+++ lib/Parse/ParseStmt.cpp (working copy)
@@ -2043,7 +2043,14 @@
// Do not enter a scope for the brace, as the arguments are in the same scope
// (the function body) as the body itself. Instead, just read the statement
// list and put it into a CompoundStmt for safe keeping.
+
+ // Increase depth for any templates specified inside of local classes
+ unsigned OldDepth = TemplateParameterDepth;
+ if (Decl && Decl->isTemplateDecl())
+ ++TemplateParameterDepth;
+
StmtResult FnBody(ParseCompoundStatementBody());
+ TemplateParameterDepth = OldDepth;
// If the function body could not be parsed, make a bogus compoundstmt.
if (FnBody.isInvalid()) {
I don't quite understand this. Local classes cannot have member templates, so what exactly are we adjusting for here?
+
+ llvm::DenseMap<const FunctionDecl *, SavedLocalScope>
+ SavedLocalFunctionScopes;
+
This map isn't being persisted in the serialized AST format. In practice, that isn't going to be a problem, because precompiled headers don't instantiate function definitions [*], and people don't tend to use AST files for "complete" translation units in ways that trigger more instantiations. Still, a note here describing why we don't have to serialize this information would be helpful for our future selves.
My only other general comment is that this is a lot of information to save for template instantiations, but it's only used in the (relatively uncommon) case where there is a local class within a function template. Can we only save this information when the pattern from which the function is instantiated actually has a local class somewhere in it?
Thanks for working on this!
- Doug
[*] Except for constexpr function definitions, but those don't have local classes.
> // Erik
> <PR9685.patch>
>
> 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
>
More information about the cfe-commits
mailing list