Type source info for function template instances.

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Type source info for function template instances.

Enea Zaffanella
Hello.

In our client application, we need to visit all instantiations of
template declarations. While at it, we were surprised to notice that the
instantiations of function template declarations are provided with a
TypeSourceInfo object that seems to be an exact copy of the one found in
the corresponding templated decl.

In fact, method VisitFunctionDecl() in SemaTemplateInstantiationDecl.cpp
does the following:

   FunctionDecl *Function =
     FunctionDecl::Create(SemaRef.Context, DC, D->getLocation(),
                          D->getDeclName(), T, D->getTypeSourceInfo(),
                          D->getStorageClass(),
                          D->isInlineSpecified(),
                          D->hasWrittenPrototype());

so that in the instantiated function,
the function type T has been modified by instantiation
   QualType T = SubstFunctionType(D, Params);
but the type source info D->getTypeSourceInfo() is passed unmodified.

Apparently, this is different from what is done by other instantiating
visitors, such as those for VarDecl and FieldDecl. For instance, for the
case of VarDecl, we have the following:

   // Do substitution on the type of the declaration
   TypeSourceInfo *DI = SemaRef.SubstType(D->getTypeSourceInfo(),
                                          TemplateArgs,
                                          D->getTypeSpecStartLoc(),
                                          D->getDeclName());
   // Build the instantiated declaration
   VarDecl *Var = VarDecl::Create(SemaRef.Context, Owner,
                                  D->getLocation(), D->getIdentifier(),
                                  DI->getType(), DI,
                                  D->getStorageClass());

Are these different behaviors really meant?
If so, for what reasons?


We have tried a tentative implementation change (attached for your
convenience) where we also compute the instantiation for the type source
info of all kinds of functions (i.e., including CXXMethodDecl).
This passes all but 2 of the clang tests:

Failing Tests (2):
     Clang :: CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.elab/p3.cpp
     Clang :: SemaTemplate/instantiate-expr-2.cpp

   Expected Passes    : 2019
   Expected Failures  : 14
   Unexpected Failures: 2

The first failure is a difference in the expected output;
the second one is an assertion failing.

We would like to hear your opinion on the thing above.

Regards,
Enea Zaffanella.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

Subst-Function-TInfo.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Type source info for function template instances.

John McCall
On Feb 18, 2010, at 3:17 PM, Enea Zaffanella wrote:

> In our client application, we need to visit all instantiations of template declarations. While at it, we were surprised to notice that the instantiations of function template declarations are provided with a TypeSourceInfo object that seems to be an exact copy of the one found in the corresponding templated decl.
>
> In fact, method VisitFunctionDecl() in SemaTemplateInstantiationDecl.cpp does the following:
>
>  FunctionDecl *Function =
>    FunctionDecl::Create(SemaRef.Context, DC, D->getLocation(),
>                         D->getDeclName(), T, D->getTypeSourceInfo(),
>                         D->getStorageClass(),
>                         D->isInlineSpecified(),
>                         D->hasWrittenPrototype());
>
> so that in the instantiated function,
> the function type T has been modified by instantiation
>  QualType T = SubstFunctionType(D, Params);
> but the type source info D->getTypeSourceInfo() is passed unmodified.
>
> Apparently, this is different from what is done by other instantiating visitors, such as those for VarDecl and FieldDecl.

This is definitely just an oversight;  the fix required is more-or-less what you've proposed, except ideally we would only transform the type once.  If you'd like to look into the bugs this exposes, that would be great;  otherwise, please file a bug on this.

John.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Type source info for function template instances.

Enea Zaffanella
John McCall wrote:
> On Feb 18, 2010, at 3:17 PM, Enea Zaffanella wrote:
>> In our client application, we need to visit all instantiations of
 >> template declarations. While at it, we were surprised to notice that
 >> the instantiations of function template declarations are provided with
 >> a TypeSourceInfo object that seems to be an exact copy of the one found
 >> in the corresponding templated decl.

[...]

> This is definitely just an oversight; the fix required is
> more-or-less  what you've proposed, except ideally we would only
 > transform the type once. If you'd like to look into the bugs
 > this exposes, that would be great; otherwise, please file a bug on this.
>
> John.
>

Hello John.

One of the two errors was just due to the duplication of the expected
diagnostics (since we were transforming twice the function parameters).

We haven't found a simple way to avoid this double transformation:
we can not just transform the parameters found in the function TSI
(TypeSourceInfo), since this would disregard, e.g., default argument
expressions. On the other hand, we found no (simple) way to separately
transform the function declaration parameters and the function TSI
components and then rebuild the full function TSI from those.

So we adopted the following approach: we keep transforming the function
type and parameters as was done before, then we also transform the
function TSI but, when doing this, we temporarily suppress all
diagnostics, so that no repeated diagnostic is generated.
This implies a minor change in Diagnostic.h, where a Boolean flag
becomes an unsigned char counter to be used by a RAII object.

The patch is attached (based on r97215, it passes all tests).

Cheers,
Enea Zaffanella.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

Subst-Function-TInfo.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Type source info for function template instances.

John McCall

On Mar 4, 2010, at 5:49 AM, Enea Zaffanella wrote:

> John McCall wrote:
>> On Feb 18, 2010, at 3:17 PM, Enea Zaffanella wrote:
>>> In our client application, we need to visit all instantiations of
> >> template declarations. While at it, we were surprised to notice that
> >> the instantiations of function template declarations are provided with
> >> a TypeSourceInfo object that seems to be an exact copy of the one found
> >> in the corresponding templated decl.
>
> [...]
>
>> This is definitely just an oversight; the fix required is
>> more-or-less  what you've proposed, except ideally we would only
> > transform the type once. If you'd like to look into the bugs
> > this exposes, that would be great; otherwise, please file a bug on this.
>> John.
>
> Hello John.
>
> One of the two errors was just due to the duplication of the expected diagnostics (since we were transforming twice the function parameters).
>
> We haven't found a simple way to avoid this double transformation:
> we can not just transform the parameters found in the function TSI (TypeSourceInfo), since this would disregard, e.g., default argument expressions. On the other hand, we found no (simple) way to separately transform the function declaration parameters and the function TSI components and then rebuild the full function TSI from those.

Can you not "reattach" default argument expressions during TemplateDeclInstantiator::SubstFunctionType()?  Or even better, transform them appropriately in TreeTransform.  I would be much more comfortable doing either of these than suppressing diagnostics assumed to be redundant.

John.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Type source info for function template instances.

Enea Zaffanella
John McCall wrote:
> Can you not "reattach" default argument expressions during
> TemplateDeclInstantiator::SubstFunctionType()?  Or even better,
> transform them appropriately in TreeTransform.  I would be much more
> comfortable doing either of these than suppressing diagnostics
> assumed to be redundant.
>
> John.
>

Hello John.

Please find attached a revised patch.

In this version, we no longer visit parameters twice,
so we no longer need to suppress diagnostics.

We (re-)discovered that we are NOT allowed to substitute the default
argument expressions when instantiating the function decl, because this
could eagerly generate diagnostics that should only be produced if and
when the default argument is actually used.

We added a new helper named
    SubstFunctionTypeSourceInfo
which mimics the behavior of the existing
    SubstFunctionType
but produces a TypeSourceInfo instead of a QualType.
We factored the common code that checks parameters in
    CheckInstantiatedParams.


The patch passes all but a single test:

Failing Tests (1):
     Clang :: SemaTemplate/instantiate-expr-2.cpp

An assertion is failing:

clang: Sema.h:3416: clang::Decl*
clang::Sema::LocalInstantiationScope::getInstantiationOf(const
clang::Decl*): Assertion `Result && "declaration was not instantiated in
this scope!"' failed.

The relevant code in the example is the following:
================
template<typename T, unsigned long N> struct IntegralConstant { };

template<typename T>
struct X0 {
   void f(T x, IntegralConstant<T, sizeof(x)>);
};

void test_X0(X0<int> x, IntegralConstant<int, sizeof(int)> ic) {
   x.f(5,ic);
}
================

The assertion crash disappears if you replace sizeof(x) with sizeof(T)
in the declaration of method X0<T>::f.

Could this be an already existing, but hidden bug that was uncovered by
our patch?

Cheers,
Enea Zaffanella.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

Subst-Function-TInfo-2.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Type source info for function template instances.

John McCall

On Mar 5, 2010, at 12:23 PM, Enea Zaffanella wrote:

> John McCall wrote:
>> Can you not "reattach" default argument expressions during
>> TemplateDeclInstantiator::SubstFunctionType()?  Or even better,
>> transform them appropriately in TreeTransform.  I would be much more
>> comfortable doing either of these than suppressing diagnostics
>> assumed to be redundant.
>> John.
>
> Hello John.
>
> Please find attached a revised patch.
>
> In this version, we no longer visit parameters twice,
> so we no longer need to suppress diagnostics.
>
> We (re-)discovered that we are NOT allowed to substitute the default argument expressions when instantiating the function decl, because this could eagerly generate diagnostics that should only be produced if and when the default argument is actually used.
>
> We added a new helper named
>   SubstFunctionTypeSourceInfo
> which mimics the behavior of the existing
>   SubstFunctionType
> but produces a TypeSourceInfo instead of a QualType.
> We factored the common code that checks parameters in
>   CheckInstantiatedParams.
>
>
> The patch passes all but a single test:
>
> Failing Tests (1):
>    Clang :: SemaTemplate/instantiate-expr-2.cpp
>
> An assertion is failing:
>
> clang: Sema.h:3416: clang::Decl* clang::Sema::LocalInstantiationScope::getInstantiationOf(const clang::Decl*): Assertion `Result && "declaration was not instantiated in this scope!"' failed.
>
> The relevant code in the example is the following:
> ================
> template<typename T, unsigned long N> struct IntegralConstant { };
>
> template<typename T>
> struct X0 {
>  void f(T x, IntegralConstant<T, sizeof(x)>);
> };
>
> void test_X0(X0<int> x, IntegralConstant<int, sizeof(int)> ic) {
>  x.f(5,ic);
> }
> ================
>
> The assertion crash disappears if you replace sizeof(x) with sizeof(T) in the declaration of method X0<T>::f.
>
> Could this be an already existing, but hidden bug that was uncovered by our patch?

Hmm, yes.  Test case which crashes on ToT (doesn't work in gcc at all, which doesn't do parameter scopes properly):

  template <class T> class Foo {
    void (*ptr)(T x, char (&array)[sizeof(x)]);
  };
  template class Foo<int>;

We need to override TreeTransform::TransformFunctionProtoType in TemplateInstantiator;  it can just call your new method.

John.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Type source info for function template instances.

Enea Zaffanella
John McCall wrote:
> On Mar 5, 2010, at 12:23 PM, Enea Zaffanella wrote:
[...]

>> The patch passes all but a single test:
>>
>> Failing Tests (1):
>>    Clang :: SemaTemplate/instantiate-expr-2.cpp
>>
>> An assertion is failing:
>>
>> clang: Sema.h:3416: clang::Decl* clang::Sema::LocalInstantiationScope::getInstantiationOf(const clang::Decl*): Assertion `Result && "declaration was not instantiated in this scope!"' failed.
>>
>> The relevant code in the example is the following:
>> ================
>> template<typename T, unsigned long N> struct IntegralConstant { };
>>
>> template<typename T>
>> struct X0 {
>>  void f(T x, IntegralConstant<T, sizeof(x)>);
>> };
>>
>> void test_X0(X0<int> x, IntegralConstant<int, sizeof(int)> ic) {
>>  x.f(5,ic);
>> }
>> ================
>>
>> The assertion crash disappears if you replace sizeof(x) with sizeof(T) in the declaration of method X0<T>::f.
>>
>> Could this be an already existing, but hidden bug that was uncovered by our patch?
>
> Hmm, yes.  Test case which crashes on ToT (doesn't work in gcc at all, which doesn't do parameter scopes properly):
>
>   template <class T> class Foo {
>     void (*ptr)(T x, char (&array)[sizeof(x)]);
>   };
>   template class Foo<int>;
>
> We need to override TreeTransform::TransformFunctionProtoType in TemplateInstantiator;  it can just call your new method.
>
> John.
>
I think we managed to do what was required.

We override TransformFunctionProtoType as suggested: we have moved here
most of the code from our helper function SubstFunctionTypeSourceInfo,
so that the latter has been simplified.

We had a few tests that were failing because, due to overriding, we were
ending up substituting parameter declarations using a Sema object having
no active instantiation scope at all. Hence, we added a few calls to
   LocalInstantiationScope Scope(*this, CurrentInstantiationScope != 0);
where needed (please double check them).

The revised patch was computed against r97917 and passes all tests, here
included the one you were proposing here above.

Cheers,
Enea Zaffanella.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

Subst-Function-TInfo-3.patch (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Type source info for function template instances.

John McCall
On Mar 7, 2010, at 11:29 AM, Enea Zaffanella wrote:
> <Subst-Function-TInfo-3.patch>

Sorry for the delay in reviewing.

By-and-large this looks good.  You shouldn't need to enter a local instantiation scope at every conceivable call site, though;  it should be sufficient to do it within your new TransformFunctionProtoType (outside the parameter-transformation loop, of course), which, among other advantages, follows the usual scope rules more precisely.

John.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Type source info for function template instances.

Enea Zaffanella
John McCall wrote:
> On Mar 7, 2010, at 11:29 AM, Enea Zaffanella wrote:
>> <Subst-Function-TInfo-3.patch>
>
> Sorry for the delay in reviewing.
>
> By-and-large this looks good.  You shouldn't need to enter a local instantiation scope at every conceivable call site, though;  it should be sufficient to do it within your new TransformFunctionProtoType (outside the parameter-transformation loop, of course), which, among other advantages, follows the usual scope rules more precisely.
>
> John.
>

OK.

Do you want me to send a revised patch or are you going to correct this
yourself?

Cheers,
Enea.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Type source info for function template instances.

John McCall

On Mar 10, 2010, at 11:24 AM, Enea Zaffanella wrote:

> John McCall wrote:
>> On Mar 7, 2010, at 11:29 AM, Enea Zaffanella wrote:
>>> <Subst-Function-TInfo-3.patch>
>> Sorry for the delay in reviewing.
>> By-and-large this looks good.  You shouldn't need to enter a local instantiation scope at every conceivable call site, though;  it should be sufficient to do it within your new TransformFunctionProtoType (outside the parameter-transformation loop, of course), which, among other advantages, follows the usual scope rules more precisely.
>> John.
>
> OK.
>
> Do you want me to send a revised patch or are you going to correct this yourself?

A revised patch, please.

John.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Type source info for function template instances.

Enea Zaffanella
John McCall wrote:
> On Mar 10, 2010, at 11:24 AM, Enea Zaffanella wrote:
[...]
>> Do you want me to send a revised patch or are you going to correct this yourself?
>
> A revised patch, please.
>
> John.
>

Here it is.

Enea.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

Subst-Function-TInfo-4.patch (12K) Download Attachment