[C++ Patch] PR 52487

[C++ Patch] PR 52487

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]

Hi,

this regression is about literal_type_p ICEing for types which cannot be 
completed. Indeed, for the testcase, complete_type cannot complete the 
type but doesn't error out either, just returns the type as-is, and the 
gcc_assert triggers. We could imagine handling such types in the caller 
- check_field_decls - but in my opinion makes more sense to just allow 
such types and return false. I also considered changing literal_type_p 
to use complete_type_or_else but then it's easy to produce duplicate 
diagnostics, for example. What do you think?

Tested x86_64-linux.

Thanks,
Paolo.

PS: eventually I guess we want to fix this in mainline and 4.7.1.

///////////////////////////

[-- Attachment #2: CL_52487 --]
[-- Type: text/plain, Size: 288 bytes --]

/cp
2012-03-20  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/52487
	* semantics.c (literal_type_p): Simply return false for types
	which cannot be completed.

/testsuite
2012-03-20  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/52487
	* g++.dg/cpp0x/lambda/lambda-ice7.C: New.

[-- Attachment #3: patch_52487 --]
[-- Type: text/plain, Size: 977 bytes --]

Index: testsuite/g++.dg/cpp0x/lambda/lambda-ice7.C
===================================================================
--- testsuite/g++.dg/cpp0x/lambda/lambda-ice7.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/lambda/lambda-ice7.C	(revision 0)
@@ -0,0 +1,9 @@
+// PR c++/52487
+// { dg-options "-std=c++0x" }
+
+struct A;         // { dg-error "forward declaration" }
+
+void foo(A& a)
+{
+  [=](){a;};      // { dg-error "invalid use of incomplete type" }
+}
Index: cp/semantics.c
===================================================================
--- cp/semantics.c	(revision 185571)
+++ cp/semantics.c	(working copy)
@@ -5610,8 +5610,7 @@ literal_type_p (tree t)
   if (CLASS_TYPE_P (t))
     {
       t = complete_type (t);
-      gcc_assert (COMPLETE_TYPE_P (t) || errorcount);
-      return CLASSTYPE_LITERAL_P (t);
+      return COMPLETE_TYPE_P (t) && CLASSTYPE_LITERAL_P (t);
     }
   if (TREE_CODE (t) == ARRAY_TYPE)
     return literal_type_p (strip_array_types (t));

Re: [C++ Patch] PR 52487

[Jason Merrill]

That assert is there to make sure that we don't try to test for 
literality of an incomplete type.  We should check for completeness 
before trying to check for literality.

Jason

Re: [C++ Patch] PR 52487

[Paolo Carlini]

On 03/20/2012 08:22 PM, Jason Merrill wrote:
> That assert is there to make sure that we don't try to test for 
> literality of an incomplete type.  We should check for completeness 
> before trying to check for literality.
You mean, in the relevant caller, here in check_field_decls:

       /* If at least one non-static data member is non-literal, the whole
          class becomes non-literal.  */
       if (!literal_type_p (type))
         CLASSTYPE_LITERAL_P (t) = false;

essentially setting CLASSTYPE_LITERAL_P (t) = false; also when 
CLASS_TYPE_P (type) && !COMPLETE_TYPE_P (complete_type (type) or maybe 
just CLASS_TYPE_P (type) && !COMPLETE_TYPE_P (type) ?

Thanks,
Paolo.

Re: [C++ Patch] PR 52487

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 67 bytes --]

... this simple also passes testing.

Paolo.

////////////////////

[-- Attachment #2: CL_52487_2 --]
[-- Type: text/plain, Size: 274 bytes --]

/cp
2012-03-20  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/52487
	* class.c (check_field_decls): Call literal_type_p only
	on complete types.

/testsuite
2012-03-20  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/52487
	* g++.dg/cpp0x/lambda/lambda-ice7.C: New.

[-- Attachment #3: patch_52487_2 --]
[-- Type: text/plain, Size: 984 bytes --]

Index: testsuite/g++.dg/cpp0x/lambda/lambda-ice7.C
===================================================================
--- testsuite/g++.dg/cpp0x/lambda/lambda-ice7.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/lambda/lambda-ice7.C	(revision 0)
@@ -0,0 +1,9 @@
+// PR c++/52487
+// { dg-options "-std=c++0x" }
+
+struct A;         // { dg-error "forward declaration" }
+
+void foo(A& a)
+{
+  [=](){a;};      // { dg-error "invalid use of incomplete type" }
+}
Index: cp/class.c
===================================================================
--- cp/class.c	(revision 185588)
+++ cp/class.c	(working copy)
@@ -3150,7 +3150,7 @@ check_field_decls (tree t, tree *access_decls,
 
       /* If at least one non-static data member is non-literal, the whole
          class becomes non-literal.  */
-      if (!literal_type_p (type))
+      if (COMPLETE_TYPE_P (type) && !literal_type_p (type))
         CLASSTYPE_LITERAL_P (t) = false;
 
       /* A standard-layout class is a class that:

Re: [C++ Patch] PR 52487

[Jason Merrill]

It's ill-formed to have a field with incomplete type.  The best thing 
would be to complain about that before we get to literal_type_p so that 
errorcount is set, if that's not too complicated.

Jason

Re: [C++ Patch] PR 52487

[Paolo Carlini]

On 03/22/2012 03:49 PM, Jason Merrill wrote:
> It's ill-formed to have a field with incomplete type.  The best thing 
> would be to complain about that before we get to literal_type_p so 
> that errorcount is set, if that's not too complicated.
Agreed. The problem is that if we just change check_field_decls to 
produce an error about the incomplete field, we produce also another 
later: that is, considering cp_parser_lambda_expression, we get to 
check_field_decls from finish_struct, but we eventually also produce an 
error with cxx_incomplete_type_diagnostic from build_lambda_object (-> 
force_rvalue -> build_special_member_call -> 
complete_type_or_maybe_complain)

Anyway, I also think not calling literal_type_p from check_field_decls 
if the type is incomplete is pretty ugly, but I'm not sure which is the 
best way to make progress: I could try returning a boolean from 
check_field_decls if something goes wrong in order to bail out early 
from cp_parser_lambda_expression (at the moment, finish_struct_1, 
check_bases_and_members, all return void). Or I could try to catch the 
incomplete field even *before* check_field_decls.

What do you suggest?

Thanks,
Paolo.

Re: [C++ Patch] PR 52487

[Gabriel Dos Reis]

On Thu, Mar 22, 2012 at 11:58 AM, Paolo Carlini
<paolo.carlini@oracle.com> wrote:
> On 03/22/2012 03:49 PM, Jason Merrill wrote:
>>
>> It's ill-formed to have a field with incomplete type.  The best thing
>> would be to complain about that before we get to literal_type_p so that
>> errorcount is set, if that's not too complicated.
>
> Agreed. The problem is that if we just change check_field_decls to produce
> an error about the incomplete field, we produce also another later: that is,

Can't we set a bit saying that the field has already gone through
diagnostics, just
like we do when trying to avoid duplicate warnings?

> considering cp_parser_lambda_expression, we get to check_field_decls from
> finish_struct, but we eventually also produce an error with
> cxx_incomplete_type_diagnostic from build_lambda_object (-> force_rvalue ->
> build_special_member_call -> complete_type_or_maybe_complain)
>
> Anyway, I also think not calling literal_type_p from check_field_decls if
> the type is incomplete is pretty ugly, but I'm not sure which is the best
> way to make progress: I could try returning a boolean from check_field_decls
> if something goes wrong in order to bail out early from
> cp_parser_lambda_expression (at the moment, finish_struct_1,
> check_bases_and_members, all return void). Or I could try to catch the
> incomplete field even *before* check_field_decls.
>
> What do you suggest?
>
> Thanks,
> Paolo.
>

Re: [C++ Patch] PR 52487

[Jason Merrill]

On 03/22/2012 12:58 PM, Paolo Carlini wrote:
> Anyway, I also think not calling literal_type_p from check_field_decls
> if the type is incomplete is pretty ugly, but I'm not sure which is the
> best way to make progress

I guess that's OK.  Just add a comment explaining that we'll get an 
error later.

Jason

Re: [C++ Patch] PR 52487

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 447 bytes --]

On 03/22/2012 07:28 PM, Jason Merrill wrote:
> On 03/22/2012 12:58 PM, Paolo Carlini wrote:
>> Anyway, I also think not calling literal_type_p from check_field_decls
>> if the type is incomplete is pretty ugly, but I'm not sure which is the
>> best way to make progress
> I guess that's OK.  Just add a comment explaining that we'll get an 
> error later.
Done: I applied the below to mainline and 4_7-branch.

Thanks!
Paolo.

///////////////////

[-- Attachment #2: CL_52487_2 --]
[-- Type: text/plain, Size: 274 bytes --]

/cp
2012-03-22  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/52487
	* class.c (check_field_decls): Call literal_type_p only
	on complete types.

/testsuite
2012-03-22  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/52487
	* g++.dg/cpp0x/lambda/lambda-ice7.C: New.

[-- Attachment #3: patch_52487_2b --]
[-- Type: text/plain, Size: 1123 bytes --]

Index: testsuite/g++.dg/cpp0x/lambda/lambda-ice7.C
===================================================================
--- testsuite/g++.dg/cpp0x/lambda/lambda-ice7.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/lambda/lambda-ice7.C	(revision 0)
@@ -0,0 +1,9 @@
+// PR c++/52487
+// { dg-options "-std=c++0x" }
+
+struct A;         // { dg-error "forward declaration" }
+
+void foo(A& a)
+{
+  [=](){a;};      // { dg-error "invalid use of incomplete type" }
+}
Index: cp/class.c
===================================================================
--- cp/class.c	(revision 185715)
+++ cp/class.c	(working copy)
@@ -3149,8 +3149,9 @@ check_field_decls (tree t, tree *access_decls,
 	CLASSTYPE_NON_AGGREGATE (t) = 1;
 
       /* If at least one non-static data member is non-literal, the whole
-         class becomes non-literal.  */
-      if (!literal_type_p (type))
+         class becomes non-literal.  Note: if the type is incomplete we
+	 will complain later on.  */
+      if (COMPLETE_TYPE_P (type) && !literal_type_p (type))
         CLASSTYPE_LITERAL_P (t) = false;
 
       /* A standard-layout class is a class that: