C++ PATCH for c++/69379 (ICE with PTRMEM_CST wrapped in NOP_EXPR)

C++ PATCH for c++/69379 (ICE with PTRMEM_CST wrapped in NOP_EXPR)

[Marek Polacek]

The problem in this PR is that we have a PTRMEM_CST wrapped in NOP_EXPR
and fold_convert can't digest that.

For the unreduced test in the PR, this occurs since rev 230508: we force
a NOP_EXPR when converting to the same type in build_static_cast_1:

          if (result == expr && SCALAR_TYPE_P (type))
            /* Leave some record of the cast.  */
            result = build_nop (type, expr);

For the reduced test in the PR, this happens since the C++ delayed folding
merge, and we wrap a PTRMEM_CST in NOP_EXPR in build_reinterpret_cast:

  else if ((TYPE_PTRFN_P (type) && TYPE_PTRFN_P (intype))
           || (TYPE_PTRMEMFUNC_P (type) && TYPE_PTRMEMFUNC_P (intype)))
    return build_nop (type, expr);

This patch <https://gcc.gnu.org/ml/gcc-patches/2015-04/msg00679.html> prevented
cp_fold_convert to wrap a PTRMEM_CST in NOP_EXPR, but in this case
cp_fold_convert gets a PTRMEM_CST already wrapped in NOP_EXPR.  I think we can
fix this by extending the fix to also handle nested PTRMEM_CSTs.  I've verified
that this fixes the ICE with the unreduced testcase too.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-01-21  Marek Polacek  <polacek@redhat.com>

	PR c++/69379
	* cvt.c (cp_fold_convert): Handle PTRMEM_CSTs wrapped in NOP_EXPRs.

	* g++.dg/pr69379.C: New test.

diff --git gcc/cp/cvt.c gcc/cp/cvt.c
index f381f9b..667307a 100644
--- gcc/cp/cvt.c
+++ gcc/cp/cvt.c
@@ -592,12 +592,14 @@ tree
 cp_fold_convert (tree type, tree expr)
 {
   tree conv;
+  tree sexpr = tree_strip_nop_conversions (expr);
+
   if (TREE_TYPE (expr) == type)
     conv = expr;
-  else if (TREE_CODE (expr) == PTRMEM_CST)
+  else if (TREE_CODE (sexpr) == PTRMEM_CST)
     {
       /* Avoid wrapping a PTRMEM_CST in NOP_EXPR.  */
-      conv = copy_node (expr);
+      conv = copy_node (sexpr);
       TREE_TYPE (conv) = type;
     }
   else
diff --git gcc/testsuite/g++.dg/pr69379.C gcc/testsuite/g++.dg/pr69379.C
index e69de29..249ad00 100644
--- gcc/testsuite/g++.dg/pr69379.C
+++ gcc/testsuite/g++.dg/pr69379.C
@@ -0,0 +1,20 @@
+// PR c++/69379
+// { dg-do compile }
+// { dg-options "-Wformat" }
+
+typedef int T;
+class A {
+public:
+  template <class D> A(const char *, D);
+  template <class Fn, class A1, class A2>
+  void m_fn1(const char *, Fn, A1 const &, A2);
+};
+struct Dict {
+  void m_fn2();
+};
+void fn1() {
+  A a("", "");
+  typedef void *Get;
+  typedef void (Dict::*d)(T);
+  a.m_fn1("", Get(), d(&Dict::m_fn2), "");
+}

	Marek

Re: C++ PATCH for c++/69379 (ICE with PTRMEM_CST wrapped in NOP_EXPR)

[Jason Merrill]

On 01/21/2016 01:25 PM, Marek Polacek wrote:
> The problem in this PR is that we have a PTRMEM_CST wrapped in NOP_EXPR
> and fold_convert can't digest that.

Why didn't we fold away the NOP_EXPR before calling fold_convert?  I 
guess we shouldn't call fold_convert on an un-folded operand.

Jason

Re: C++ PATCH for c++/69379 (ICE with PTRMEM_CST wrapped in NOP_EXPR)

[Marek Polacek]

On Thu, Jan 21, 2016 at 01:49:14PM -0500, Jason Merrill wrote:
> On 01/21/2016 01:25 PM, Marek Polacek wrote:
> >The problem in this PR is that we have a PTRMEM_CST wrapped in NOP_EXPR
> >and fold_convert can't digest that.
> 
> Why didn't we fold away the NOP_EXPR before calling fold_convert?  I guess
> we shouldn't call fold_convert on an un-folded operand.

So we start with fargs[j] = maybe_constant_value (argarray[j]); in
build_over_call, where argarray[j] is
(const struct 
{
  void Dict::<T462> (struct Dict *, int) * __pfn;
  long int __delta;
} &) &TARGET_EXPR <D.2347, (struct d) <<< Unknown tree: ptrmem_cst >>>>

Later on, we end up in cxx_eval_constant_expression with
t == (struct d) <<< Unknown tree: ptrmem_cst >>>
so we go to the
3607     case NOP_EXPR:
case.  Here cxx_eval_constant_expression evaluates the inner ptrmem_cst,
then there's
3619         if (TREE_CODE (op) == PTRMEM_CST
3620             && !TYPE_PTRMEM_P (type))
3621           op = cplus_expand_constant (op);
but that doesn't trigger, because type is TYPE_PTRMEM_P.
Then we fold () the whole expression but that doesn't change the expression
(and I don't think it should do anything with C++-specific PTRMEM_CST) so we're
stuck with NOP_EXPR around PTRMEM_CST.

So maybe cxx_eval_constant_expression should handle PTRMEM_CSTs wrapped in
NOP_EXPR specially, but I don't know how.

	Marek

Re: C++ PATCH for c++/69379 (ICE with PTRMEM_CST wrapped in NOP_EXPR)

[Jason Merrill]

On 01/22/2016 12:20 PM, Marek Polacek wrote:
> On Thu, Jan 21, 2016 at 01:49:14PM -0500, Jason Merrill wrote:
>> On 01/21/2016 01:25 PM, Marek Polacek wrote:
>>> The problem in this PR is that we have a PTRMEM_CST wrapped in NOP_EXPR
>>> and fold_convert can't digest that.
>>
>> Why didn't we fold away the NOP_EXPR before calling fold_convert?  I guess
>> we shouldn't call fold_convert on an un-folded operand.
>
> So we start with fargs[j] = maybe_constant_value (argarray[j]); in
> build_over_call, where argarray[j] is
> (const struct
> {
>    void Dict::<T462> (struct Dict *, int) * __pfn;
>    long int __delta;
> } &) &TARGET_EXPR <D.2347, (struct d) <<< Unknown tree: ptrmem_cst >>>>
>
> Later on, we end up in cxx_eval_constant_expression with
> t == (struct d) <<< Unknown tree: ptrmem_cst >>>
> so we go to the
> 3607     case NOP_EXPR:
> case.  Here cxx_eval_constant_expression evaluates the inner ptrmem_cst,
> then there's
> 3619         if (TREE_CODE (op) == PTRMEM_CST
> 3620             && !TYPE_PTRMEM_P (type))
> 3621           op = cplus_expand_constant (op);
> but that doesn't trigger, because type is TYPE_PTRMEM_P.
> Then we fold () the whole expression but that doesn't change the expression
> (and I don't think it should do anything with C++-specific PTRMEM_CST) so we're
> stuck with NOP_EXPR around PTRMEM_CST.
>
> So maybe cxx_eval_constant_expression should handle PTRMEM_CSTs wrapped in
> NOP_EXPR specially, but I don't know how.

If we have a NOP_EXPR to the same type, we should strip it here.

Jason

Re: C++ PATCH for c++/69379 (ICE with PTRMEM_CST wrapped in NOP_EXPR)

[Marek Polacek]

On Fri, Jan 22, 2016 at 03:38:26PM -0500, Jason Merrill wrote:
> If we have a NOP_EXPR to the same type, we should strip it here.

This helps for the unreduced testcases in the PR, but not for the reduced one,
because for the reduced one, the types are not the same.  One type is
struct 
{
  void Dict::<T461> (struct Dict *, T) * __pfn;
  long int __delta;
}
and the second one
struct 
{
  void Dict::<T442> (struct Dict *) * __pfn;
  long int __delta;
}

The NOP_EXPR in this case originated in build_reinterpret_cast_1:
7070   else if ((TYPE_PTRFN_P (type) && TYPE_PTRFN_P (intype))
7071            || (TYPE_PTRMEMFUNC_P (type) && TYPE_PTRMEMFUNC_P (intype)))
7072     return build_nop (type, expr);

So maybe the following patch is desirable, but it's not a complete fix :(.
Thanks,

2016-01-22  Marek Polacek  <polacek@redhat.com>

	PR c++/69379
	* constexpr.c (cxx_eval_constant_expression) [NOP_EXPR]: When
	converting PTRMEM_CST to the same type, strip nops.

diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c
index 6b0e5a8..6fe5cbe 100644
--- gcc/cp/constexpr.c
+++ gcc/cp/constexpr.c
@@ -3619,6 +3619,11 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
 	if (TREE_CODE (op) == PTRMEM_CST
 	    && !TYPE_PTRMEM_P (type))
 	  op = cplus_expand_constant (op);
+	if (TREE_CODE (op) == PTRMEM_CST
+	    && tcode == NOP_EXPR
+	    && same_type_ignoring_top_level_qualifiers_p (type,
+							  TREE_TYPE (op)))
+	  STRIP_NOPS (t);
 	if (POINTER_TYPE_P (type)
 	    && TREE_CODE (op) == INTEGER_CST
 	    && !integer_zerop (op))

	Marek

Re: C++ PATCH for c++/69379 (ICE with PTRMEM_CST wrapped in NOP_EXPR)

[Jason Merrill]

On 01/22/2016 05:07 PM, Marek Polacek wrote:
> On Fri, Jan 22, 2016 at 03:38:26PM -0500, Jason Merrill wrote:
>> If we have a NOP_EXPR to the same type, we should strip it here.
>
> This helps for the unreduced testcases in the PR, but not for the reduced one,
> because for the reduced one, the types are not the same.  One type is
> struct
> {
>    void Dict::<T461> (struct Dict *, T) * __pfn;
>    long int __delta;
> }
> and the second one
> struct
> {
>    void Dict::<T442> (struct Dict *) * __pfn;
>    long int __delta;
> }
>
> The NOP_EXPR in this case originated in build_reinterpret_cast_1:
> 7070   else if ((TYPE_PTRFN_P (type) && TYPE_PTRFN_P (intype))
> 7071            || (TYPE_PTRMEMFUNC_P (type) && TYPE_PTRMEMFUNC_P (intype)))
> 7072     return build_nop (type, expr);

Well, a reinterpret_cast makes the expression non-constant, so we can 
recognize that case (when the types are unrelated) and bail out.  After 
that we probably still need to deal with the case of conversion to a 
pointer-to-member-of-base type; for functions it looks like we can just 
copy the PTRMEM_CST and give it a different type, but for data members I 
think we'll need to add support for the type not matching the member in 
expand_ptrmem_cst.

Jason

Re: C++ PATCH for c++/69379 (ICE with PTRMEM_CST wrapped in NOP_EXPR)

[Marek Polacek]

On Mon, Jan 25, 2016 at 10:08:34AM -0500, Jason Merrill wrote:
> On 01/22/2016 05:07 PM, Marek Polacek wrote:
> >On Fri, Jan 22, 2016 at 03:38:26PM -0500, Jason Merrill wrote:
> >>If we have a NOP_EXPR to the same type, we should strip it here.
> >
> >This helps for the unreduced testcases in the PR, but not for the reduced one,
> >because for the reduced one, the types are not the same.  One type is
> >struct
> >{
> >   void Dict::<T461> (struct Dict *, T) * __pfn;
> >   long int __delta;
> >}
> >and the second one
> >struct
> >{
> >   void Dict::<T442> (struct Dict *) * __pfn;
> >   long int __delta;
> >}
> >
> >The NOP_EXPR in this case originated in build_reinterpret_cast_1:
> >7070   else if ((TYPE_PTRFN_P (type) && TYPE_PTRFN_P (intype))
> >7071            || (TYPE_PTRMEMFUNC_P (type) && TYPE_PTRMEMFUNC_P (intype)))
> >7072     return build_nop (type, expr);
> 
> Well, a reinterpret_cast makes the expression non-constant, so we can
> recognize that case (when the types are unrelated) and bail out.  After that
> we probably still need to deal with the case of conversion to a
> pointer-to-member-of-base type; for functions it looks like we can just copy
> the PTRMEM_CST and give it a different type, but for data members I think
> we'll need to add support for the type not matching the member in
> expand_ptrmem_cst.

It appears that handling the case when the types don't match is sufficient, at
least all the tests pass, thus the following should be enough.

If you want me to take care of the rest then please let me know, though without
a testcase it might be harder to get right.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-01-25  Marek Polacek  <polacek@redhat.com>

	PR c++/69379
	* constexpr.c (cxx_eval_constant_expression): Handle PTRMEM_CSTs
	wrapped in NOP_EXPRs.

	* g++.dg/pr69379.C: New test.

diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c
index 6b0e5a8..4b952d1 100644
--- gcc/cp/constexpr.c
+++ gcc/cp/constexpr.c
@@ -3619,6 +3619,20 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
 	if (TREE_CODE (op) == PTRMEM_CST
 	    && !TYPE_PTRMEM_P (type))
 	  op = cplus_expand_constant (op);
+	if (TREE_CODE (op) == PTRMEM_CST && tcode == NOP_EXPR)
+	  {
+	    if (same_type_ignoring_top_level_qualifiers_p (type,
+							   TREE_TYPE (op)))
+	      STRIP_NOPS (t);
+	    else
+	      {
+		if (!ctx->quiet)
+		  error_at (EXPR_LOC_OR_LOC (t, input_location),
+			    "reinterpret_cast has different types");
+		*non_constant_p = true;
+		return t;
+	      }
+	  }
 	if (POINTER_TYPE_P (type)
 	    && TREE_CODE (op) == INTEGER_CST
 	    && !integer_zerop (op))
diff --git gcc/testsuite/g++.dg/pr69379.C gcc/testsuite/g++.dg/pr69379.C
index e69de29..249ad00 100644
--- gcc/testsuite/g++.dg/pr69379.C
+++ gcc/testsuite/g++.dg/pr69379.C
@@ -0,0 +1,20 @@
+// PR c++/69379
+// { dg-do compile }
+// { dg-options "-Wformat" }
+
+typedef int T;
+class A {
+public:
+  template <class D> A(const char *, D);
+  template <class Fn, class A1, class A2>
+  void m_fn1(const char *, Fn, A1 const &, A2);
+};
+struct Dict {
+  void m_fn2();
+};
+void fn1() {
+  A a("", "");
+  typedef void *Get;
+  typedef void (Dict::*d)(T);
+  a.m_fn1("", Get(), d(&Dict::m_fn2), "");
+}


	Marek

Re: C++ PATCH for c++/69379 (ICE with PTRMEM_CST wrapped in NOP_EXPR)

[Jason Merrill]

On 01/25/2016 03:00 PM, Marek Polacek wrote:
> It appears that handling the case when the types don't match is sufficient, at
> least all the tests pass, thus the following should be enough.

OK.

> +			    "reinterpret_cast has different types");

Let's say "a reinterpret_cast is not a constant-expression" here.

OK with that change.

Jason