diff mbox series

C++ PATCH for c++/91678 - wrong error with decltype and location wrapper

Message ID 20190906022455.GC14737@redhat.com
State New
Headers show
Series C++ PATCH for c++/91678 - wrong error with decltype and location wrapper | expand

Commit Message

Marek Polacek Sept. 6, 2019, 2:24 a.m. UTC 
Compiling this testcase results in a bogus "invalid cast" error; this occurs
since the introduction of location wrappers in finish_id_expression.

Here we are parsing the decltype expression via cp_parser_decltype_expr which
can lead to calling various fold_* and c-family routines.  They use
non_lvalue_loc, but that won't create a NON_LVALUE_EXPR wrapper around a location
wrapper.

So before the location wrappers addition cp_parser_decltype_expr would return
NON_LVALUE_EXPR <c>.  Now it returns VIEW_CONVERT_EXPR<float *>(c), but the
STRIP_ANY_LOCATION_WRAPPER immediately following it strips the location wrapper,
and suddenly we don't know whether we have an lvalue anymore.  And that's sad
because then decltype produces the wrong type, causing nonsense errors.

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

2019-09-05  Marek Polacek  <polacek@redhat.com>

	PR c++/91678 - wrong error with decltype and location wrapper.
	* parser.c (cp_parser_decltype): Use auto_suppress_location_wrappers
	sentinel.  Don't strip location wrappers.

	* g++.dg/cpp0x/decltype73.C: New test.

Comments

Marek Polacek Sept. 13, 2019, 2:26 a.m. UTC | #1 
Ping.

On Thu, Sep 05, 2019 at 10:24:55PM -0400, Marek Polacek wrote:
> Compiling this testcase results in a bogus "invalid cast" error; this occurs
> since the introduction of location wrappers in finish_id_expression.
> 
> Here we are parsing the decltype expression via cp_parser_decltype_expr which
> can lead to calling various fold_* and c-family routines.  They use
> non_lvalue_loc, but that won't create a NON_LVALUE_EXPR wrapper around a location
> wrapper.
> 
> So before the location wrappers addition cp_parser_decltype_expr would return
> NON_LVALUE_EXPR <c>.  Now it returns VIEW_CONVERT_EXPR<float *>(c), but the
> STRIP_ANY_LOCATION_WRAPPER immediately following it strips the location wrapper,
> and suddenly we don't know whether we have an lvalue anymore.  And that's sad
> because then decltype produces the wrong type, causing nonsense errors.
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk and 9?
> 
> 2019-09-05  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c++/91678 - wrong error with decltype and location wrapper.
> 	* parser.c (cp_parser_decltype): Use auto_suppress_location_wrappers
> 	sentinel.  Don't strip location wrappers.
> 
> 	* g++.dg/cpp0x/decltype73.C: New test.
> 
> diff --git gcc/cp/parser.c gcc/cp/parser.c
> index baa60b8834e..b3c7bff5988 100644
> --- gcc/cp/parser.c
> +++ gcc/cp/parser.c
> @@ -14729,8 +14729,13 @@ cp_parser_decltype (cp_parser *parser)
>        /* Do not warn about problems with the expression.  */
>        ++c_inhibit_evaluation_warnings;
>  
> +      /* Don't create wrapper nodes within decltype.  non_lvalue_loc won't
> +	 create a NON_LVALUE_EXPR wrapper around a location wrapper, and a
> +	 subsequent STRIP_ANY_LOCATION_WRAPPER would destroy the information
> +	 about lvalueness of the expression.  */
> +      auto_suppress_location_wrappers sentinel;
> +
>        expr = cp_parser_decltype_expr (parser, id_expression_or_member_access_p);
> -      STRIP_ANY_LOCATION_WRAPPER (expr);
>  
>        /* Go back to evaluating expressions.  */
>        --cp_unevaluated_operand;
> diff --git gcc/testsuite/g++.dg/cpp0x/decltype73.C gcc/testsuite/g++.dg/cpp0x/decltype73.C
> new file mode 100644
> index 00000000000..cbe94a898e3
> --- /dev/null
> +++ gcc/testsuite/g++.dg/cpp0x/decltype73.C
> @@ -0,0 +1,4 @@
> +// PR c++/91678 - wrong error with decltype and location wrapper.
> +// { dg-do compile { target c++11 } }
> +
> +float* test(float* c) { return (decltype(c + 0))(float*)c; }
Jason Merrill Sept. 15, 2019, 2:18 p.m. UTC | #2 
On 9/5/19 9:24 PM, Marek Polacek wrote:
> They use
> non_lvalue_loc, but that won't create a NON_LVALUE_EXPR wrapper around a location
> wrapper.

That seems like the bug. maybe_lvalue_p should be true for 
VIEW_CONVERT_EXPR.

Jason
Marek Polacek Sept. 16, 2019, 6:12 p.m. UTC | #3 
On Sun, Sep 15, 2019 at 10:18:29AM -0400, Jason Merrill wrote:
> On 9/5/19 9:24 PM, Marek Polacek wrote:
> > They use
> > non_lvalue_loc, but that won't create a NON_LVALUE_EXPR wrapper around a location
> > wrapper.
> 
> That seems like the bug. maybe_lvalue_p should be true for
> VIEW_CONVERT_EXPR.

That makes sense but it breaks in tsubst_* which doesn't expect a
NON_LVALUE_EXPR wrapped around a location wrapper.  Perhaps we want to handle
it like this.

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

2019-09-16  Marek Polacek  <polacek@redhat.com>

	PR c++/91678 - wrong error with decltype and location wrapper.
	* pt.c (tsubst_copy): Handle NON_LVALUE_EXPRs wrapped around a location
	wrapper. 

	* fold-const.c (maybe_lvalue_p): Handle VIEW_CONVERT_EXPR.

	* g++.dg/cpp0x/decltype73.C: New test.

diff --git gcc/cp/pt.c gcc/cp/pt.c
index 9de1b8fec97..4bd77ac2578 100644
--- gcc/cp/pt.c
+++ gcc/cp/pt.c
@@ -15786,8 +15786,16 @@ tsubst_copy (tree t, tree args, tsubst_flags_t complain, tree in_decl)
 		  return op;
 		}
 	    }
-	  /* We shouldn't see any other uses of these in templates.  */
-	  gcc_unreachable ();
+	  /* We can get a NON_LVALUE_EXPR wrapped around a location wrapper.  */
+	  else if (code == NON_LVALUE_EXPR && location_wrapper_p (op))
+	    {
+	      tree type = tsubst (TREE_TYPE (t), args, complain, in_decl);
+	      op = tsubst_copy (TREE_OPERAND (t, 0), args, complain, in_decl);
+	      return build1 (code, type, op);
+	    }
+	  else
+	    /* We shouldn't see any other uses of these in templates.  */
+	    gcc_unreachable ();
 	}
 
     case CAST_EXPR:
diff --git gcc/fold-const.c gcc/fold-const.c
index a99dafec589..6d955a76f87 100644
--- gcc/fold-const.c
+++ gcc/fold-const.c
@@ -2594,6 +2594,7 @@ maybe_lvalue_p (const_tree x)
   case TARGET_EXPR:
   case COND_EXPR:
   case BIND_EXPR:
+  case VIEW_CONVERT_EXPR:
     break;
 
   default:
diff --git gcc/testsuite/g++.dg/cpp0x/decltype73.C gcc/testsuite/g++.dg/cpp0x/decltype73.C
new file mode 100644
index 00000000000..cbe94a898e3
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp0x/decltype73.C
@@ -0,0 +1,4 @@
+// PR c++/91678 - wrong error with decltype and location wrapper.
+// { dg-do compile { target c++11 } }
+
+float* test(float* c) { return (decltype(c + 0))(float*)c; }
Jason Merrill Sept. 18, 2019, 3:59 a.m. UTC | #4 
On 9/16/19 1:12 PM, Marek Polacek wrote:
> On Sun, Sep 15, 2019 at 10:18:29AM -0400, Jason Merrill wrote:
>> On 9/5/19 9:24 PM, Marek Polacek wrote:
>>> They use
>>> non_lvalue_loc, but that won't create a NON_LVALUE_EXPR wrapper around a location
>>> wrapper.
>>
>> That seems like the bug. maybe_lvalue_p should be true for
>> VIEW_CONVERT_EXPR.
> 
> That makes sense but it breaks in tsubst_* which doesn't expect a
> NON_LVALUE_EXPR wrapped around a location wrapper.

Hmm, why would we get that in a template when we don't get 
NON_LVALUE_EXPR wrapped around other lvalue nodes?

Jason
Marek Polacek Dec. 7, 2019, 12:18 a.m. UTC | #5 
[ Sorry for dropping the ball on this. ]

On Tue, Sep 17, 2019 at 11:59:02PM -0400, Jason Merrill wrote:
> On 9/16/19 1:12 PM, Marek Polacek wrote:
> > On Sun, Sep 15, 2019 at 10:18:29AM -0400, Jason Merrill wrote:
> > > On 9/5/19 9:24 PM, Marek Polacek wrote:
> > > > They use
> > > > non_lvalue_loc, but that won't create a NON_LVALUE_EXPR wrapper around a location
> > > > wrapper.
> > > 
> > > That seems like the bug. maybe_lvalue_p should be true for
> > > VIEW_CONVERT_EXPR.
> > 
> > That makes sense but it breaks in tsubst_* which doesn't expect a
> > NON_LVALUE_EXPR wrapped around a location wrapper.
> 
> Hmm, why would we get that in a template when we don't get NON_LVALUE_EXPR
> wrapped around other lvalue nodes?

I just retested the patch without the pt.c hunk and no longer see that problem,
and the fold-const.c hunk still fixes the bogus error.  Yay?

So I think the following should be good to go:

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

2019-12-06  Marek Polacek  <polacek@redhat.com>

	PR c++/91678 - wrong error with decltype and location wrapper.
	* fold-const.c (maybe_lvalue_p): Handle VIEW_CONVERT_EXPR.

	* g++.dg/cpp0x/decltype73.C: New test.

--- gcc/fold-const.c
+++ gcc/fold-const.c
@@ -2594,6 +2594,7 @@ maybe_lvalue_p (const_tree x)
   case TARGET_EXPR:
   case COND_EXPR:
   case BIND_EXPR:
+  case VIEW_CONVERT_EXPR:
     break;
 
   default:
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp0x/decltype73.C
@@ -0,0 +1,4 @@
+// PR c++/91678 - wrong error with decltype and location wrapper.
+// { dg-do compile { target c++11 } }
+
+float* test(float* c) { return (decltype(c + 0))(float*)c; }
Jason Merrill Dec. 7, 2019, 4:03 a.m. UTC | #6 
On 12/6/19 7:18 PM, Marek Polacek wrote:
> [ Sorry for dropping the ball on this. ]
> 
> On Tue, Sep 17, 2019 at 11:59:02PM -0400, Jason Merrill wrote:
>> On 9/16/19 1:12 PM, Marek Polacek wrote:
>>> On Sun, Sep 15, 2019 at 10:18:29AM -0400, Jason Merrill wrote:
>>>> On 9/5/19 9:24 PM, Marek Polacek wrote:
>>>>> They use
>>>>> non_lvalue_loc, but that won't create a NON_LVALUE_EXPR wrapper around a location
>>>>> wrapper.
>>>>
>>>> That seems like the bug. maybe_lvalue_p should be true for
>>>> VIEW_CONVERT_EXPR.
>>>
>>> That makes sense but it breaks in tsubst_* which doesn't expect a
>>> NON_LVALUE_EXPR wrapped around a location wrapper.
>>
>> Hmm, why would we get that in a template when we don't get NON_LVALUE_EXPR
>> wrapped around other lvalue nodes?
> 
> I just retested the patch without the pt.c hunk and no longer see that problem,
> and the fold-const.c hunk still fixes the bogus error.  Yay?
> 
> So I think the following should be good to go:
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?

OK.

> 2019-12-06  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c++/91678 - wrong error with decltype and location wrapper.
> 	* fold-const.c (maybe_lvalue_p): Handle VIEW_CONVERT_EXPR.
> 
> 	* g++.dg/cpp0x/decltype73.C: New test.
> 
> --- gcc/fold-const.c
> +++ gcc/fold-const.c
> @@ -2594,6 +2594,7 @@ maybe_lvalue_p (const_tree x)
>     case TARGET_EXPR:
>     case COND_EXPR:
>     case BIND_EXPR:
> +  case VIEW_CONVERT_EXPR:
>       break;
>   
>     default:
> --- /dev/null
> +++ gcc/testsuite/g++.dg/cpp0x/decltype73.C
> @@ -0,0 +1,4 @@
> +// PR c++/91678 - wrong error with decltype and location wrapper.
> +// { dg-do compile { target c++11 } }
> +
> +float* test(float* c) { return (decltype(c + 0))(float*)c; }
>
diff mbox series

Patch

diff --git gcc/cp/parser.c gcc/cp/parser.c
index baa60b8834e..b3c7bff5988 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -14729,8 +14729,13 @@  cp_parser_decltype (cp_parser *parser)
       /* Do not warn about problems with the expression.  */
       ++c_inhibit_evaluation_warnings;
 
+      /* Don't create wrapper nodes within decltype.  non_lvalue_loc won't
+	 create a NON_LVALUE_EXPR wrapper around a location wrapper, and a
+	 subsequent STRIP_ANY_LOCATION_WRAPPER would destroy the information
+	 about lvalueness of the expression.  */
+      auto_suppress_location_wrappers sentinel;
+
       expr = cp_parser_decltype_expr (parser, id_expression_or_member_access_p);
-      STRIP_ANY_LOCATION_WRAPPER (expr);
 
       /* Go back to evaluating expressions.  */
       --cp_unevaluated_operand;
diff --git gcc/testsuite/g++.dg/cpp0x/decltype73.C gcc/testsuite/g++.dg/cpp0x/decltype73.C
new file mode 100644
index 00000000000..cbe94a898e3
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp0x/decltype73.C
@@ -0,0 +1,4 @@ 
+// PR c++/91678 - wrong error with decltype and location wrapper.
+// { dg-do compile { target c++11 } }
+
+float* test(float* c) { return (decltype(c + 0))(float*)c; }