Patchwork PR c++/51462 - ICE in cx_check_missing_mem_inits

login
register
mail settings
Submitter Dodji Seketeli
Date Dec. 16, 2011, 4:40 p.m.
Message ID <m3aa6sxyfh.fsf@redhat.com>
Download mbox | patch
Permalink /patch/131865/
State New
Headers show

Comments

Dodji Seketeli - Dec. 16, 2011, 4:40 p.m.
Hello,

In the example of the patch below, the invalid initialization for the
'a' member in the context of the constructor of B (in
expand_default_init) yields an error_mark_node, and rightfully so.

But then expand_default_init doesn't generate any (expression)
statement to represent that invalid initialization.  So it's just like
if no initialization was written by the user.

Later, register_constexpr_fundef massages the body of the constexpr
constructor B and calls cx_check_missing_mem_inits to see if there are
some members that are missing initialization.  As expand_default_init
didn't generate any statement for the invalid initialization of 'a',
the assert below, from cx_check_missing_mem_inits is violated[1]:

	  if (type_has_constexpr_default_constructor (ftype))
	    {
	      /* It's OK to skip a member with a trivial constexpr ctor.
	         A constexpr ctor that isn't trivial should have been
	         added in by now.  */
	      gcc_checking_assert (!TYPE_HAS_COMPLEX_DFLT (ftype));

So I am thinking that maybe letting expand_default_init represent the
invalid initialization would prevent us from getting into this case.
For that to work I had to make
build_constexpr_constructor_member_initializers try a little bit
harder to represent invalid member initialization, as that code wasn't
expecting to have e.g, an EXPR_STMT for a member initialization.

If you think I am trying too hard, maybe I could just get out early
from register_constexpr_fundef if errorcount is non-zero?  Like, right
before:

  if (DECL_CONSTRUCTOR_P (fun)
      && cx_check_missing_mem_inits (fun, body, !DECL_GENERATED_P (fun)))
    return NULL;

?

The reason why I am not doing this straight away is that I am thinking
that maybe G++ could be missing some diagnostic in that case.  And
more generally, I think that representing invalid input (as much as we
can) in the semantic tree is consistent with what's done in the rest
of the FE and leads to better diagnostics.

FWIW, the patch below bootstraps and passes regression tests on
x86_64-unknown-linux-gnu against trunk.

[1]:  By the way, I am just curious, why using gcc_checking_assert
instead of just gcc_assert?

Thanks.

gcc/cp/

	PR c++/51462
	* init.c (expand_default_init): Pass even error_mark_node to
	finish_expr_stmt.
	* semantics.c (build_constexpr_constructor_member_initializers):
	call build_data_member_initialization even for invalid input.

gcc/testsuite/

	PR c++/51462
	* g++.dg/cpp0x/constexpr-99.C: New test.
---
 gcc/cp/init.c                             |    7 ++++++-
 gcc/cp/semantics.c                        |   13 ++++++++++++-
 gcc/testsuite/g++.dg/cpp0x/constexpr-99.C |   13 +++++++++++++
 3 files changed, 31 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-99.C
Gabriel Dos Reis - Dec. 16, 2011, 5:36 p.m.
On Fri, Dec 16, 2011 at 10:40 AM, Dodji Seketeli <dodji@redhat.com> wrote:
> Hello,

> So I am thinking that maybe letting expand_default_init represent the
> invalid initialization would prevent us from getting into this case.
> For that to work I had to make
> build_constexpr_constructor_member_initializers try a little bit
> harder to represent invalid member initialization, as that code wasn't
> expecting to have e.g, an EXPR_STMT for a member initialization.

Ideally, we should have erroneous member inits be initialized with
error_mark_node.

>
> If you think I am trying too hard, maybe I could just get out early
> from register_constexpr_fundef if errorcount is non-zero?  Like, right
> before:
>
>  if (DECL_CONSTRUCTOR_P (fun)
>      && cx_check_missing_mem_inits (fun, body, !DECL_GENERATED_P (fun)))
>    return NULL;
>
> ?
>
> The reason why I am not doing this straight away is that I am thinking
> that maybe G++ could be missing some diagnostic in that case.  And
> more generally, I think that representing invalid input (as much as we
> can) in the semantic tree is consistent with what's done in the rest
> of the FE and leads to better diagnostics.

We don't have a good track record at invalid input around for too long :-)
Plus, keeping invalid input around for too long means that other part
of the front-end gets messed up with hairy conditionals and control flows.
I would think that an erroneous member initialized already got a
 diagnostic and that should suffice and we shouldn't keep trying further...
Jason Merrill - Dec. 16, 2011, 6:26 p.m.
On 12/16/2011 11:40 AM, Dodji Seketeli wrote:
> 	      /* It's OK to skip a member with a trivial constexpr ctor.
> 	         A constexpr ctor that isn't trivial should have been
> 	         added in by now.  */
> 	      gcc_checking_assert (!TYPE_HAS_COMPLEX_DFLT (ftype));
>
> If you think I am trying too hard, maybe I could just get out early
> from register_constexpr_fundef if errorcount is non-zero?

Let's just check errorcount in this assert.

> [1]:  By the way, I am just curious, why using gcc_checking_assert
> instead of just gcc_assert?

In general, I think it makes sense to use gcc_checking_assert for checks 
that either are expensive, or check conditions that aren't really 
problematic to deal with if they do occur.  But I haven't been 
particularly methodical about using one or the other.

Jason
Dodji Seketeli - Dec. 16, 2011, 7:22 p.m.
Gabriel Dos Reis <gdr@integrable-solutions.net> writes:

> On Fri, Dec 16, 2011 at 10:40 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>> Hello,
>
>> So I am thinking that maybe letting expand_default_init represent the
>> invalid initialization would prevent us from getting into this case.
>> For that to work I had to make
>> build_constexpr_constructor_member_initializers try a little bit
>> harder to represent invalid member initialization, as that code wasn't
>> expecting to have e.g, an EXPR_STMT for a member initialization.
>
> Ideally, we should have erroneous member inits be initialized with
> error_mark_node.

With that patch, the EXPR_STMT wraps an error_mark_node.  But yes, I
agree.

> We don't have a good track record at invalid input around for too long :-)
> Plus, keeping invalid input around for too long means that other part
> of the front-end gets messed up with hairy conditionals and control flows.
> I would think that an erroneous member initialized already got a
>  diagnostic and that should suffice and we shouldn't keep trying further...

OK.  Thanks.

Patch

diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index e93e82c..c958c29 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -1671,7 +1671,12 @@  expand_default_init (tree binfo, tree true_exp, tree exp, tree init, int flags,
     }
 
   /* FIXME put back convert_to_void?  */
-  if (TREE_SIDE_EFFECTS (rval))
+  if (TREE_SIDE_EFFECTS (rval)
+      /* If the initialization failed, let's generate an EXPR_STMT
+	 containing error_mark_node, effectively telling other parts
+	 of the compiler that there actually was an initialization,
+	 even though it was invalid.  */
+      || rval == error_mark_node)
     finish_expr_stmt (rval);
 }
 
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index 2f2a26a..a8ea996 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -5900,7 +5900,18 @@  build_constexpr_constructor_member_initializers (tree type, tree body)
 	}
     }
   else
-    gcc_assert (errorcount > 0);
+    {
+      /* An error happened earlier, and some diagnostic has probably
+	 been emitted for it already.  */
+      gcc_assert (errorcount > 0);
+      /* What else but an expression could we have at this point?  */
+      gcc_assert (TREE_CODE_CLASS (TREE_CODE (body)) == tcc_expression);
+      /* Let's build the data member initialization representation,
+	 even if some initializers might actually contain
+	 error_mark_node.  */
+      if (ok)
+	ok = build_data_member_initialization (body, &vec);
+    }
   if (ok)
     return build_constructor (type, vec);
   else
diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-99.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-99.C
new file mode 100644
index 0000000..13a5ea3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-99.C
@@ -0,0 +1,13 @@ 
+// Origin PR c++/51462
+// { dg-options "-std=c++11" }
+
+struct A
+{
+  int i = 0;
+};
+
+struct B
+{
+  A a;
+    constexpr B() : a(0) {} // { dg-error "no matching function" }
+};