Instantiate incomplete class used in template method.

[sepavloff]

Summary

If class is absent from instantiation and is incomplete, instantiate it as
incomplete class thus avoiding compiler crash.
This change fixes PR18653.

[sepavloff]

Ping.

Thanks,
--Serge

2015-03-12 13:23 GMT+06:00 Serge Pavlov <[e-mail]>:

If class is absent from instantiation and is incomplete, instantiate it as
incomplete class thus avoiding compiler crash.
This change fixes PR18653.

http://reviews.llvm.org/D8281

Files:

lib/Sema/SemaTemplateInstantiate.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/SemaTemplate/instantiate-local-class.cpp

Index: lib/Sema/SemaTemplateInstantiate.cpp

• lib/Sema/SemaTemplateInstantiate.cpp

+++ lib/Sema/SemaTemplateInstantiate.cpp
@@ -2792,6 +2792,12 @@

  isa<TemplateTemplateParmDecl>(D))
return nullptr;

+ If the declaration we are looking for is an incomplete type
declaration, we
+ may not have a chance to instantiate it.
+ if (const TypeDecl *TD = dyn_cast<TypeDecl>(D))
+ if (TD->getTypeForDecl()->isIncompleteType())
+ return nullptr;
+

// If we didn't find the decl, then we either have a sema bug, or we

have a

// forward reference to a label declaration.  Return null to indicate

that

// we have an uninstantiated label.

Index: lib/Sema/SemaTemplateInstantiateDecl.cpp

• lib/Sema/SemaTemplateInstantiateDecl.cpp

+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -4399,6 +4399,15 @@

  isa<TemplateTemplateParmDecl>(D))
return D;

+ If the declaration we are looking for is an incomplete type
declaration,
+ we may not have a chance to instantiate it.
+ if (const TypeDecl *TD = dyn_cast<TypeDecl>(D))
+ if (TD->getTypeForDecl()->isIncompleteType()) {
+ Decl *Inst = SubstDecl(D, CurContext, TemplateArgs);
+ CurrentInstantiationScope->InstantiatedLocal(D, Inst);
+ return cast<TypeDecl>(Inst);
+ }
+

if (D->isInvalidDecl())
  return nullptr;

Index: test/SemaTemplate/instantiate-local-class.cpp

• test/SemaTemplate/instantiate-local-class.cpp

+++ test/SemaTemplate/instantiate-local-class.cpp
@@ -194,3 +194,13 @@

void f() { F<int>(); }

};
}
+
+namespace PR18653 {
+ template <class T> void f();
+ template <class T> struct str {
+ void m() {
+ f<class newclass>();
+ }
+ };
+ template struct str<int>;
+}

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/

[rsmith]

This looks like a good approach to me.

lib/Sema/SemaTemplateInstantiate.cpp

2797–2798 ↗

(On Diff #21801)

I think this should only apply to TagDecls. You should then instantiate if !TD->isThisDeclarationADefiinition() && !TD->isFreeStanding(). Ideally, the freestanding check would instead check isEmbeddedInDeclarator, but it looks like we don't instantiate the tag early enough for that to work in cases like

template<typename T> void f() { void g(struct x); }
template void f<int>();

[sepavloff]

Updated patch

Thank you for advice and explanation! Changed the patch accordingly.

[rsmith]

Please add a testcase that uses a forward declaration and then defines it, such as:

template<typename T> void f() {
  void g(struct x);
  struct x {};
}
template void f<int>();

I think a case like that will still assert with your current patch (because x is not an incomplete type, but it won't have had a declaration instantiated when we instantiate the declaration of g).

lib/Sema/SemaTemplateInstantiate.cpp

2802 ↗

(On Diff #23254)

This should use the same condition as the other change.

[sepavloff]

Hi Richard,

2015-04-06 6:16 GMT+06:00 Richard Smith <[e-mail]>:

Please add a testcase that uses a forward declaration and then defines it,
such as:

template<typename T> void f() {
  void g(struct x);
  struct x {};
}
template void f<int>();

I think a case like that will still assert with your current patch
(because x is not an incomplete type, but it won't have had a declaration
instantiated when we instantiate the declaration of g).

Yes, you are right, this case was not handled. Added instantiation of the
type for which we see forward declaration.

Comment at: lib/Sema/SemaTemplateInstantiate.cpp:2802
@@ +2801,3 @@
+ if (const TagDecl *TD = dyn_cast<TagDecl>(D))
+ if (TD->getTypeForDecl()->isIncompleteType())

+ return nullptr;

This should use the same condition as the other change.

Fixed.

Thanks,
--Serge

[sepavloff]

Updated patch.

[rsmith]

lib/Sema/SemaTemplateInstantiateDecl.cpp

4406–4407 ↗

(On Diff #23288)

What's the purpose of this change? If we have a reference to a particular TagDecl, why don't we always instantiate that declaration?

[sepavloff]

2015-04-07 2:23 GMT+06:00 Richard Smith <[e-mail]>:

Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:4406-4407
@@ +4405,4 @@
+ const TagDecl *DeclToInstantiate = nullptr;
+ if (TagDecl *Def = TD->getDefinition())
+ DeclToInstantiate = Def;
+ else if (!TD->isThisDeclarationADefinition() &&

!TD->isFreeStanding())

What's the purpose of this change? If we have a reference to a particular
TagDecl, why don't we always instantiate that declaration?

If check for invalid declaration is made prior to this one, it seems that

we can always instantiate the type.
This fix however allows to compile the following code:

template<typename T> void f3() {
  struct AA {
    struct x3 vvv;
  } xx;
  return xx.aaa;
  struct x3 { int aaa; };
}

It is because at the point where incomplete type is used we instantiate its
definition, which is defined later.Strange, but gcc also compiles this
code. Not sure why this code should be accepted.

Thanks,
--Serge

[sepavloff]

Simplified patch.

[sepavloff]

Rephased comments, added more tests.

[rsmith]

OK, this seems like a clear step forward.

lib/Sema/SemaTemplateInstantiateDecl.cpp

4440 ↗

(On Diff #24457)

Is there anything we can assert here to check that we're not accidentally taking this codepath for a tag decl that should be separately instantiated?

cfe/trunk/lib/Sema/SemaTemplateInstantiate.cpp

LocalInstantiationScope::findInstantiationOf(const Decl *D) {
   }
 
   // If we're performing a partial substitution during template argument
   // deduction, we may not have values for template parameters yet.
   if (isa<NonTypeTemplateParmDecl>(D) || isa<TemplateTypeParmDecl>(D) ||
       isa<TemplateTemplateParmDecl>(D))
     return nullptr;
 
+  // Tag type may be referenced prior to definition, in this case it does not
+  // have instantiation yet.
+  if (isa<TagDecl>(D))
+    return nullptr;
+
   // If we didn't find the decl, then we either have a sema bug, or we have a
   // forward reference to a label declaration.  Return null to indicate that
   // we have an uninstantiated label.
   assert(isa<LabelDecl>(D) && "declaration not instantiated in this scope");
   return nullptr;
 }
 
 void LocalInstantiationScope::InstantiatedLocal(const Decl *D, Decl *Inst) {

cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp

if (isa<ParmVarDecl>(D) || isa<NonTypeTemplateParmDecl>(D) ||
     // just map to themselves.
     if (isa<NonTypeTemplateParmDecl>(D) || isa<TemplateTypeParmDecl>(D) ||
         isa<TemplateTemplateParmDecl>(D))
       return D;
 
     if (D->isInvalidDecl())
       return nullptr;
 
+    // Tag type may be referenced prior to definition, in this case it must be
+    // instantiated now.
+    if (const TagDecl *TD = dyn_cast<TagDecl>(D)) {
+      Decl *Inst = SubstDecl(D, CurContext, TemplateArgs);
+      CurrentInstantiationScope->InstantiatedLocal(D, Inst);
+      return cast<TypeDecl>(Inst);
+    }
+
     // If we didn't find the decl, then we must have a label decl that hasn't
     // been found yet.  Lazily instantiate it and return it now.
     assert(isa<LabelDecl>(D));
 
     Decl *Inst = SubstDecl(D, CurContext, TemplateArgs);
     assert(Inst && "Failed to instantiate label??");
 
     CurrentInstantiationScope->InstantiatedLocal(D, Inst);

cfe/trunk/test/SemaTemplate/instantiate-local-class.cpp

struct state_t {
       Y y{x()};
     };
     return 0;
   }
   int func() {
     return make_seed_pair();
   }
 }
+
+namespace PR18653 {
+  // Forward declarations
+
+  template<typename T> void f1() {
+    void g1(struct x1);
+    struct x1 {};
+  }
+  template void f1<int>();
+
+  template<typename T> void f2() {
+    void g2(enum x2);  // expected-error{{ISO C++ forbids forward references to 'enum' types}}
+    enum x2 { nothing };
+  }
+  template void f2<int>();
+
+  template<typename T> void f3() {
+    void g3(enum class x3);
+    enum class x3 { nothing };
+  }
+  template void f3<int>();
+
+
+  template<typename T> void f4() {
+    void g4(struct x4 {} x);  // expected-error{{'x4' cannot be defined in a parameter type}}
+  }
+  template void f4<int>();
+
+
+  template <class T> void f();
+  template <class T> struct S1 {
+    void m() {
+      f<class newclass>();
+    }
+  };
+  template struct S1<int>;
+
+  template <class T> struct S2 {
+    void m() {
+      f<enum new_enum>();  // expected-error{{ISO C++ forbids forward references to 'enum' types}}
+    }
+  };
+  template struct S2<int>;
+
+  template <class T> struct S3 {
+    void m() {
+      f<enum class new_enum>();
+    }
+  };
+  template struct S3<int>;
+
+  template <class T> struct S4 {
+    struct local {};
+    void m() {
+      f<local>();
+    }
+  };
+  template struct S4<int>;
+
+  template <class T> struct S5 {
+    enum local { nothing };
+    void m() {
+      f<local>();
+    }
+  };
+  template struct S5<int>;
+
+  template <class T> struct S7 {
+    enum class local { nothing };
+    void m() {
+      f<local>();
+    }
+  };
+  template struct S7<int>;
+
+
+  template <class T> void fff(T *x);
+  template <class T> struct S01 {
+    struct local { };
+    void m() {
+      local x;
+      fff(&x);
+    }
+  };
+  template struct S01<int>;
+
+  template <class T> struct S02 {
+    enum local { nothing };
+    void m() {
+      local x;
+      fff(&x);
+    }
+  };
+  template struct S02<int>;
+
+  template <class T> struct S03 {
+    enum class local { nothing };
+    void m() {
+      local x;
+      fff(&x);
+    }
+  };
+  template struct S03<int>;
+
+
+  template <class T> struct S04 {
+    void m() {
+      struct { } x;
+      fff(&x);
+    }
+  };
+  template struct S04<int>;
+
+  template <class T> struct S05 {
+    void m() {
+      enum { nothing } x;
+      fff(&x);
+    }
+  };
+  template struct S05<int>;
+
+  template <class T> struct S06 {
+    void m() {
+      class { virtual void mmm() {} } x;
+      fff(&x);
+    }
+  };
+  template struct S06<int>;
+}