Re: [C++ Patch/RFC] PR 71569 ("[6/7/8 regression] Crash: External definition of template member from template struct")

[Jason Merrill]

On Thu, Mar 1, 2018 at 11:00 AM, Jason Merrill  wrote:
> On Thu, Mar 1, 2018 at 9:41 AM, Jason Merrill  wrote:
>> On Wed, Feb 28, 2018 at 4:04 PM, Paolo Carlini  
>> wrote:
>>> On 28/02/2018 20:20, Jason Merrill wrote:

 Hmm, the test seems wrong; the diagnostic talks about specializing the
 arguments, but the actual test is checking whether the enclosing scope
 is fully specialized.  It looks like you'll give the same error for

 template 
 struct A {
template 
static const U u;
 };

 template 
 template 
 const U* A::u = nullptr;

 which does specialize the argument; since we accept

 template 
 struct A {
template 
struct B;
 };

 template 
 template 
 struct A::B { };

 we ought to accept the above as well.

 So, we could reject the testcase with this error, but we would need to
 test for it using the same check as in process_partial_specialization.
>>>
>>> I see. Doing that seems relatively easy - I have a draft which appears to
>>> work - but then we have immediately to change the gcc_assert at the
>>> beginning of determine_specialization to let such specializations through
>>> (of course it still uses the wrong test!!).  I'm not sure about the best way
>>> to do that... But that seems also doable.
>>
>> That test is correct for functions, I think we just want to restrict
>> that block to functions.
>>
>>> The next problem is
>>> duplicate_decls which apparently misses a bit of code to understand that the
>>> new declaration does not conflict with the old one near line #1650... So,
>>> frankly, I think that even with your great guidance I would need a few
>>> iterations to get right the whole package and we are so damn close to the
>>> release. What do you think? Do you want to take this over, or maybe you see
>>> us restricting a bit what we'll have working in this area for 8.1.0?
>>
>> Yeah, I'll take it.
>
> Ah, needed to fix the code in start_decl that checks for a variable
> template specialization to consider that a specialization can be a
> template.

And this fixes the other testcase in comment 6.
commit 530608b73c7fc0345f8150ad124b57e180abfa11
Author: Jason Merrill 
Date:   Thu Mar 1 12:24:39 2018 -0500

PR c++/71569 - decltype of template.

* parser.c (cp_parser_decltype_expr): Handle missing template args.

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 359460cd4d8..e1acb07d29e 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -13983,6 +13983,10 @@ cp_parser_decltype_expr (cp_parser *parser,
expr = cp_parser_lookup_name_simple (parser, expr,
 id_expr_start_token->location);
 
+  if (expr && TREE_CODE (expr) == TEMPLATE_DECL)
+   /* A template without args is not a complete id-expression.  */
+   expr = error_mark_node;
+
   if (expr
   && expr != error_mark_node
   && TREE_CODE (expr) != TYPE_DECL
@@ -14048,6 +14052,9 @@ cp_parser_decltype_expr (cp_parser *parser,
  expression.  */
   cp_parser_abort_tentative_parse (parser);
 
+  /* Commit to the tentative_firewall so we get syntax errors.  */
+  cp_parser_commit_to_tentative_parse (parser);
+
   /* Parse a full expression.  */
   expr = cp_parser_expression (parser, /*pidk=*/NULL, /*cast_p=*/false,
   /*decltype_p=*/true);
diff --git a/gcc/testsuite/g++.dg/cpp0x/decltype-33837.C 
b/gcc/testsuite/g++.dg/cpp0x/decltype-33837.C
index fbbc6a14972..4a8f053234e 100644
--- a/gcc/testsuite/g++.dg/cpp0x/decltype-33837.C
+++ b/gcc/testsuite/g++.dg/cpp0x/decltype-33837.C
@@ -2,6 +2,6 @@
 // PR c++/33837
 void foo()
 {
-  __decltype (A::foo()); // { dg-error "was not declared|expected" }
-  __decltype (B); // { dg-error "was not declared" }
+  __decltype (A::foo()); // { dg-error "A" }
+  __decltype (B); // { dg-error "B" }
 }
diff --git a/gcc/testsuite/g++.dg/cpp0x/decltype43.C 
b/gcc/testsuite/g++.dg/cpp0x/decltype43.C
index 4df95a1047c..7a1dcbf8744 100644
--- a/gcc/testsuite/g++.dg/cpp0x/decltype43.C
+++ b/gcc/testsuite/g++.dg/cpp0x/decltype43.C
@@ -22,6 +22,6 @@ struct B
 int main()
 {
   int x = B::b(A::a(1));
-  int y = B::b(A::a(2)); // { dg-error "template 
argument" }
+  int y = B::b(A::a(2)); // { dg-error "template" 
}
   return x + y;
 }
diff --git a/gcc/testsuite/g++.dg/cpp1y/var-templ59.C 
b/gcc/testsuite/g++.dg/cpp1y/var-templ59.C
new file mode 100644
index 000..da9710e1ce4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/var-templ59.C
@@ -0,0 +1,14 @@
+// PR c++/71569
+// { dg-do compile { target c++14 } }
+
+template 
+struct A {
+  template 
+  static U u;
+};
+
+int main()
+{
+  decltype(A::u) a;   // { dg-error "missing template arguments" }
+  

Re: [C++ Patch/RFC] PR 71569 ("[6/7/8 regression] Crash: External definition of template member from template struct")

[Jason Merrill]

On Thu, Mar 1, 2018 at 9:41 AM, Jason Merrill  wrote:
> On Wed, Feb 28, 2018 at 4:04 PM, Paolo Carlini  
> wrote:
>> On 28/02/2018 20:20, Jason Merrill wrote:
>>>
>>> Hmm, the test seems wrong; the diagnostic talks about specializing the
>>> arguments, but the actual test is checking whether the enclosing scope
>>> is fully specialized.  It looks like you'll give the same error for
>>>
>>> template 
>>> struct A {
>>>template 
>>>static const U u;
>>> };
>>>
>>> template 
>>> template 
>>> const U* A::u = nullptr;
>>>
>>> which does specialize the argument; since we accept
>>>
>>> template 
>>> struct A {
>>>template 
>>>struct B;
>>> };
>>>
>>> template 
>>> template 
>>> struct A::B { };
>>>
>>> we ought to accept the above as well.
>>>
>>> So, we could reject the testcase with this error, but we would need to
>>> test for it using the same check as in process_partial_specialization.
>>
>> I see. Doing that seems relatively easy - I have a draft which appears to
>> work - but then we have immediately to change the gcc_assert at the
>> beginning of determine_specialization to let such specializations through
>> (of course it still uses the wrong test!!).  I'm not sure about the best way
>> to do that... But that seems also doable.
>
> That test is correct for functions, I think we just want to restrict
> that block to functions.
>
>> The next problem is
>> duplicate_decls which apparently misses a bit of code to understand that the
>> new declaration does not conflict with the old one near line #1650... So,
>> frankly, I think that even with your great guidance I would need a few
>> iterations to get right the whole package and we are so damn close to the
>> release. What do you think? Do you want to take this over, or maybe you see
>> us restricting a bit what we'll have working in this area for 8.1.0?
>
> Yeah, I'll take it.

Ah, needed to fix the code in start_decl that checks for a variable
template specialization to consider that a specialization can be a
template.

Tested x86_64-pc-linux-gnu, applying to trunk.
commit 1c44723705fb34f8b3ccc0493a2a79230bb5a7d9
Author: Jason Merrill 
Date:   Thu Mar 1 10:16:13 2018 -0500

PR c++/71569 - ICE with redundant args on member variable template.

* decl.c (start_decl): Handle partial specialization of member
variable template.
* pt.c (determine_specialization): Allow partial specialization
of member variable template without specializing enclosing class.
(process_partial_specialization): Improve error message.

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index f1be2292c59..db64d12c95a 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -5080,19 +5080,17 @@ start_decl (const cp_declarator *declarator,
  if (field == NULL_TREE
  || !(VAR_P (field) || variable_template_p (field)))
error ("%q+#D is not a static data member of %q#T", decl, context);
+ else if (variable_template_p (field)
+  && (DECL_LANG_SPECIFIC (decl)
+  && DECL_TEMPLATE_SPECIALIZATION (decl)))
+   /* OK, specialization was already checked.  */;
  else if (variable_template_p (field) && !this_tmpl)
{
- if (DECL_LANG_SPECIFIC (decl)
- && DECL_TEMPLATE_SPECIALIZATION (decl))
-   /* OK, specialization was already checked.  */;
- else
-   {
- error_at (DECL_SOURCE_LOCATION (decl),
-   "non-member-template declaration of %qD", decl);
- inform (DECL_SOURCE_LOCATION (field), "does not match "
- "member template declaration here");
- return error_mark_node;
-   }
+ error_at (DECL_SOURCE_LOCATION (decl),
+   "non-member-template declaration of %qD", decl);
+ inform (DECL_SOURCE_LOCATION (field), "does not match "
+ "member template declaration here");
+ return error_mark_node;
}
  else
{
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 73451195cd0..e07d77bb87e 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -2060,7 +2060,8 @@ determine_specialization (tree template_id,
   /* We shouldn't be specializing a member template of an
  unspecialized class template; we already gave an error in
  check_specialization_scope, now avoid crashing.  */
-  if (template_count && DECL_CLASS_SCOPE_P (decl)
+  if (!VAR_P (decl)
+  && template_count && DECL_CLASS_SCOPE_P (decl)
   && template_class_depth (DECL_CONTEXT (decl)) > 0)
 {
   gcc_assert (errorcount);
@@ -4840,10 +4841,13 @@ process_partial_specialization (tree decl)
 {
   if (!flag_concepts)
 error ("partial specialization %q+D does not specialize "
-  "any template arguments", decl);

Re: [C++ Patch/RFC] PR 71569 ("[6/7/8 regression] Crash: External definition of template member from template struct")

[Jason Merrill]

On Wed, Feb 28, 2018 at 4:04 PM, Paolo Carlini  wrote:
> On 28/02/2018 20:20, Jason Merrill wrote:
>>
>> Hmm, the test seems wrong; the diagnostic talks about specializing the
>> arguments, but the actual test is checking whether the enclosing scope
>> is fully specialized.  It looks like you'll give the same error for
>>
>> template 
>> struct A {
>>template 
>>static const U u;
>> };
>>
>> template 
>> template 
>> const U* A::u = nullptr;
>>
>> which does specialize the argument; since we accept
>>
>> template 
>> struct A {
>>template 
>>struct B;
>> };
>>
>> template 
>> template 
>> struct A::B { };
>>
>> we ought to accept the above as well.
>>
>> So, we could reject the testcase with this error, but we would need to
>> test for it using the same check as in process_partial_specialization.
>
> I see. Doing that seems relatively easy - I have a draft which appears to
> work - but then we have immediately to change the gcc_assert at the
> beginning of determine_specialization to let such specializations through
> (of course it still uses the wrong test!!).  I'm not sure about the best way
> to do that... But that seems also doable.

That test is correct for functions, I think we just want to restrict
that block to functions.

> The next problem is
> duplicate_decls which apparently misses a bit of code to understand that the
> new declaration does not conflict with the old one near line #1650... So,
> frankly, I think that even with your great guidance I would need a few
> iterations to get right the whole package and we are so damn close to the
> release. What do you think? Do you want to take this over, or maybe you see
> us restricting a bit what we'll have working in this area for 8.1.0?

Yeah, I'll take it.

Jason

Re: [C++ Patch/RFC] PR 71569 ("[6/7/8 regression] Crash: External definition of template member from template struct")

[Paolo Carlini]

Hi,

On 28/02/2018 20:20, Jason Merrill wrote:

Hmm, the test seems wrong; the diagnostic talks about specializing the
arguments, but the actual test is checking whether the enclosing scope
is fully specialized.  It looks like you'll give the same error for

template 
struct A {
   template 
   static const U u;
};

template 
template 
const U* A::u = nullptr;

which does specialize the argument; since we accept

template 
struct A {
   template 
   struct B;
};

template 
template 
struct A::B { };

we ought to accept the above as well.

So, we could reject the testcase with this error, but we would need to
test for it using the same check as in process_partial_specialization.
I see. Doing that seems relatively easy - I have a draft which appears 
to work - but then we have immediately to change the gcc_assert at the 
beginning of determine_specialization to let such specializations 
through (of course it still uses the wrong test!!). I'm not sure about 
the best way to do that... But that seems also doable. The next problem 
is duplicate_decls which apparently misses a bit of code to understand 
that the new declaration does not conflict with the old one near line 
#1650... So, frankly, I think that even with your great guidance I would 
need a few iterations to get right the whole package and we are so damn 
close to the release. What do you think? Do you want to take this over, 
or maybe you see us restricting a bit what we'll have working in this 
area for 8.1.0?


Thanks,
Paolo.

Re: [C++ Patch/RFC] PR 71569 ("[6/7/8 regression] Crash: External definition of template member from template struct")

[Jason Merrill]

On Wed, Feb 28, 2018 at 1:42 PM, Paolo Carlini  wrote:
> Hi,
>
> On 28/02/2018 17:24, Jason Merrill wrote:
>>>
>>> What do you think, should we do something similar?
>>
>> Sounds good.
>>
>>> (by the way, yesterday
>>> got a bit confused because in other vaguely related cases clang also has
>>> this kind of two-part error message and we don't. Eg, I cooked up:
>>>
>>> template 
>>> class Foo
>>> {
>>>template 
>>>class Bar;
>>> };
>>>
>>> template
>>> template
>>> class Foo::Bar;
>>
>> And the same would be good for this example.
>
> Great. I'm finishing testing the below, looks Ok?

Hmm, the test seems wrong; the diagnostic talks about specializing the
arguments, but the actual test is checking whether the enclosing scope
is fully specialized.  It looks like you'll give the same error for

template 
struct A {
  template 
  static const U u;
};

template 
template 
const U* A::u = nullptr;

which does specialize the argument; since we accept

template 
struct A {
  template 
  struct B;
};

template 
template 
struct A::B { };

we ought to accept the above as well.

So, we could reject the testcase with this error, but we would need to
test for it using the same check as in process_partial_specialization.

Jason

Re: [C++ Patch/RFC] PR 71569 ("[6/7/8 regression] Crash: External definition of template member from template struct")

[Paolo Carlini]

Hi,

On 28/02/2018 17:24, Jason Merrill wrote:

What do you think, should we do something similar?

Sounds good.


(by the way, yesterday
got a bit confused because in other vaguely related cases clang also has
this kind of two-part error message and we don't. Eg, I cooked up:

template 
class Foo
{
   template 
   class Bar;
};

template
template
class Foo::Bar;

And the same would be good for this example.

Great. I'm finishing testing the below, looks Ok?

Thanks,
Paolo.

///
Index: cp/pt.c
===
--- cp/pt.c (revision 258059)
+++ cp/pt.c (working copy)
@@ -2873,7 +2873,18 @@ check_explicit_specialization (tree declarator,
  /* Partial specialization of variable template.  */
  SET_DECL_TEMPLATE_SPECIALIZATION (decl);
  specialization = 1;
- goto ok;
+ if (template_count && DECL_CLASS_SCOPE_P (decl)
+ && template_class_depth (DECL_CONTEXT (decl)) > 0)
+   {
+ error ("variable template partial specialization %qD does "
+"not specialize any template arguments; to define the "
+"primary template, remove the template argument list",
+declarator);
+ inform (DECL_SOURCE_LOCATION (TREE_OPERAND (declarator, 0)),
+ "primary template here");
+   }
+ else
+   goto ok;
}
  else if (cxx_dialect < cxx14)
error ("non-type partial specialization %qD "
@@ -4832,11 +4843,14 @@ process_partial_specialization (tree decl)
 get_constraints (maintmpl
 {
   if (!flag_concepts)
-error ("partial specialization %q+D does not specialize "
-  "any template arguments", decl);
+error ("partial specialization %q+D does not specialize any "
+  "template arguments; to define the primary template, "
+  "remove the template argument list", decl);
   else
 error ("partial specialization %q+D does not specialize any "
-  "template arguments and is not more constrained than", decl);
+  "template arguments and is not more constrained than "
+  "the primary template; to define the primary template, "
+  "remove the template argument list", decl);
   inform (DECL_SOURCE_LOCATION (maintmpl), "primary template here");
 }
 
Index: testsuite/g++.dg/cpp1y/var-templ58.C
===
--- testsuite/g++.dg/cpp1y/var-templ58.C(nonexistent)
+++ testsuite/g++.dg/cpp1y/var-templ58.C(working copy)
@@ -0,0 +1,13 @@
+// PR c++/71569
+// { dg-do compile { target c++14 } }
+
+template 
+class Foo
+{
+  template 
+  static bool Bar;  // { dg-message "primary template" }
+};
+
+template
+template
+bool Foo::Bar;  // { dg-error "variable template partial" }
Index: testsuite/g++.dg/template/partial-specialization9.C
===
--- testsuite/g++.dg/template/partial-specialization9.C (nonexistent)
+++ testsuite/g++.dg/template/partial-specialization9.C (working copy)
@@ -0,0 +1,12 @@
+// PR c++/71569
+
+template 
+class Foo
+{
+  template 
+  class Bar;  // { dg-message "primary template" }
+};
+
+template
+template
+class Foo::Bar {};  // { dg-error "partial specialization" }

Re: [C++ Patch/RFC] PR 71569 ("[6/7/8 regression] Crash: External definition of template member from template struct")

[Jason Merrill]

On Wed, Feb 28, 2018 at 11:18 AM, Paolo Carlini
 wrote:
> On 28/02/2018 16:53, Jason Merrill wrote:
>> On Wed, Feb 28, 2018 at 9:36 AM, Paolo Carlini 
>> wrote:
>>>
>>> in this P2 ICE on invalid we fail to diagnose an ill-formed variable
>>> template partial specialization and the assert at the beginning of
>>> determine_specialization triggers. Indeed, we didn't diagnose the problem
>>> via check_specialization_scope (there are no template <>, thus
>>> begin_specialization isn't involved). Simply adding a specific check to
>>> check_explicit_specialization exactly when we recognize a variable template
>>> partial specialization seems a safe thing to do and passes testing. Not sure
>>> if the test itself is as simple as possible or is better done somewhere
>>> else...
>>
>> Hmm, this doesn't look like the user intended to write a partial
>> specialization, but rather that they were trying to define the member
>> template and wrote the template arguments out by mistake.
>
> I see your point. If I understand correctly, in such cases clang somehow
> talks about both possibilities:
>
> error: variable template partial specialization does not specialize any
> template argument; to define the primary template, remove the template
> argument list
>
> What do you think, should we do something similar?

Sounds good.

> (by the way, yesterday
> got a bit confused because in other vaguely related cases clang also has
> this kind of two-part error message and we don't. Eg, I cooked up:
>
> template 
> class Foo
> {
>   template 
>   class Bar;
> };
>
> template
> template
> class Foo::Bar;

And the same would be good for this example.

Jason

Re: [C++ Patch/RFC] PR 71569 ("[6/7/8 regression] Crash: External definition of template member from template struct")

[Paolo Carlini]

Hi,

On 28/02/2018 16:53, Jason Merrill wrote:

On Wed, Feb 28, 2018 at 9:36 AM, Paolo Carlini  wrote:

in this P2 ICE on invalid we fail to diagnose an ill-formed variable
template partial specialization and the assert at the beginning of
determine_specialization triggers. Indeed, we didn't diagnose the problem
via check_specialization_scope (there are no template <>, thus
begin_specialization isn't involved). Simply adding a specific check to
check_explicit_specialization exactly when we recognize a variable template
partial specialization seems a safe thing to do and passes testing. Not sure
if the test itself is as simple as possible or is better done somewhere
else...

Hmm, this doesn't look like the user intended to write a partial
specialization, but rather that they were trying to define the member
template and wrote the template arguments out by mistake.
I see your point. If I understand correctly, in such cases clang somehow 
talks about both possibilities:


error: variable template partial specialization does not specialize any 
template argument; to define the primary template, remove the template 
argument list


What do you think, should we do something similar? (by the way, 
yesterday got a bit confused because in other vaguely related cases 
clang also has this kind of two-part error message and we don't. Eg, I 
cooked up:


template 
class Foo
{
  template 
  class Bar;
};

template
template
class Foo::Bar;

Thanks,
Paolo.

Re: [C++ Patch/RFC] PR 71569 ("[6/7/8 regression] Crash: External definition of template member from template struct")

[Jason Merrill]

On Wed, Feb 28, 2018 at 9:36 AM, Paolo Carlini  wrote:
> in this P2 ICE on invalid we fail to diagnose an ill-formed variable
> template partial specialization and the assert at the beginning of
> determine_specialization triggers. Indeed, we didn't diagnose the problem
> via check_specialization_scope (there are no template <>, thus
> begin_specialization isn't involved). Simply adding a specific check to
> check_explicit_specialization exactly when we recognize a variable template
> partial specialization seems a safe thing to do and passes testing. Not sure
> if the test itself is as simple as possible or is better done somewhere
> else...

Hmm, this doesn't look like the user intended to write a partial
specialization, but rather that they were trying to define the member
template and wrote the template arguments out by mistake.

Jason

[C++ Patch/RFC] PR 71569 ("[6/7/8 regression] Crash: External definition of template member from template struct")

[Paolo Carlini]

Hi,

in this P2 ICE on invalid we fail to diagnose an ill-formed variable 
template partial specialization and the assert at the beginning of 
determine_specialization triggers. Indeed, we didn't diagnose the 
problem via check_specialization_scope (there are no template <>, thus 
begin_specialization isn't involved). Simply adding a specific check to 
check_explicit_specialization exactly when we recognize a variable 
template partial specialization seems a safe thing to do and passes 
testing. Not sure if the test itself is as simple as possible or is 
better done somewhere else...


Thanks, Paolo.

//

Index: cp/pt.c
===
--- cp/pt.c (revision 258059)
+++ cp/pt.c (working copy)
@@ -2873,7 +2873,12 @@ check_explicit_specialization (tree declarator,
  /* Partial specialization of variable template.  */
  SET_DECL_TEMPLATE_SPECIALIZATION (decl);
  specialization = 1;
- goto ok;
+ if (template_count && DECL_CLASS_SCOPE_P (decl)
+ && template_class_depth (DECL_CONTEXT (decl)) > 0)
+   error ("variable template partial specialization %qD does "
+  "not specialize any template arguments", declarator);
+ else
+   goto ok;
}
  else if (cxx_dialect < cxx14)
error ("non-type partial specialization %qD "
Index: testsuite/g++.dg/cpp1y/var-templ58.C
===
--- testsuite/g++.dg/cpp1y/var-templ58.C(nonexistent)
+++ testsuite/g++.dg/cpp1y/var-templ58.C(working copy)
@@ -0,0 +1,13 @@
+// PR c++/71569
+// { dg-do compile { target c++14 } }
+
+template 
+class Foo
+{
+template 
+static bool Bar;
+};
+
+template
+template
+bool Foo::Bar;  // { dg-error "variable template partial" }