Type source info for function template instances.

Clang Frontend
1

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.

Subst-Function-TInfo.patch (3.24 KB)

2

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.

3

John McCall 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.

Subst-Function-TInfo.patch (7.88 KB)

4

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.

5

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:

Subst-Function-TInfo-2.patch (8.43 KB)

6

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.

7

John McCall 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.

Subst-Function-TInfo-3.patch (14.6 KB)

8

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.

9

John McCall wrote:

10

A revised patch, please.

John.

11

John McCall 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.

Subst-Function-TInfo-4.patch (12.3 KB)