[C++] Fix ICE in potential_constant_expression_1 (PR c++/83918)

Message ID 20180118231315.GM2063@tucnak
State New
Headers show
Series
  • [C++] Fix ICE in potential_constant_expression_1 (PR c++/83918)
Related show

Commit Message

Jakub Jelinek Jan. 18, 2018, 11:13 p.m.
Hi!

Before location wrappers were introduced, the potential_constant_expression_1
assumption that a VIEW_CONVERT_EXPR must have non-NULL type was right,
if it is a type dependent reinterpret cast, it would be
REINTERPRET_CAST_EXPR instead.

Location wrappers around decls can have NULL type, if the decl they wrap
also has NULL type.

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?  Alternatively it could if (TREE_TYPE (t) && ...

On a related note, I wonder if the
	return (RECUR (from, TREE_CODE (t) != VIEW_CONVERT_EXPR));
a few lines below is correct, VCE isn't always an lvalue, only if it is used
in non-rvalue contexts.  So, shouldn't it be instead:
	return (RECUR (from, (TREE_CODE (t) == VIEW_CONVERT_EXPR
			      ? want_rval : rval)));
Don't have a testcase where it would matter though.

2018-01-18  Jakub Jelinek  <jakub@redhat.com>

	PR c++/83918
	* constexpr.c (potential_constant_expression_1)
	<case VIEW_CONVERT_EXPR>: Don't check for INTEGER_CST to pointer
	type conversion for location wrappers.

	* g++.dg/cpp1z/pr83918.C: New test.


 
	Jakub

Comments

Jason Merrill Jan. 22, 2018, 9:49 p.m. | #1
On Thu, Jan 18, 2018 at 6:13 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Before location wrappers were introduced, the potential_constant_expression_1

> assumption that a VIEW_CONVERT_EXPR must have non-NULL type was right,

> if it is a type dependent reinterpret cast, it would be

> REINTERPRET_CAST_EXPR instead.

>

> Location wrappers around decls can have NULL type, if the decl they wrap

> also has NULL type.


Hmm, why would a decl have NULL type?  Are we wrapping a CONST_DECL in
VIEW_CONVERT_EXPR?  They should get NON_LVALUE_EXPR, since they aren't
lvalues (except the objc++ ones that have TREE_STATIC set).

> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for

> trunk?  Alternatively it could if (TREE_TYPE (t) && ...

>

> On a related note, I wonder if the

>         return (RECUR (from, TREE_CODE (t) != VIEW_CONVERT_EXPR));

> a few lines below is correct, VCE isn't always an lvalue, only if it is used

> in non-rvalue contexts.  So, shouldn't it be instead:

>         return (RECUR (from, (TREE_CODE (t) == VIEW_CONVERT_EXPR

>                               ? want_rval : rval)));


A VCE should always be an lvalue.  If it's used in an rvalue context,
then it's an lvalue used in an rvalue context and so the larger
expression is an rvalue, but the VCE is still an lvalue.

Jason
Jakub Jelinek Jan. 22, 2018, 11:14 p.m. | #2
On Mon, Jan 22, 2018 at 04:49:34PM -0500, Jason Merrill wrote:
> On Thu, Jan 18, 2018 at 6:13 PM, Jakub Jelinek <jakub@redhat.com> wrote:

> > Before location wrappers were introduced, the potential_constant_expression_1

> > assumption that a VIEW_CONVERT_EXPR must have non-NULL type was right,

> > if it is a type dependent reinterpret cast, it would be

> > REINTERPRET_CAST_EXPR instead.

> >

> > Location wrappers around decls can have NULL type, if the decl they wrap

> > also has NULL type.

> 

> Hmm, why would a decl have NULL type?  Are we wrapping a CONST_DECL in

> VIEW_CONVERT_EXPR?  They should get NON_LVALUE_EXPR, since they aren't

> lvalues (except the objc++ ones that have TREE_STATIC set).


So, do you want following instead, if it passes bootstrap/regtest?

2018-01-22  Jakub Jelinek  <jakub@redhat.com>

	PR c++/83918
	* tree.c (maybe_wrap_with_location): Use NON_LVALUE_EXPR rather than
	VIEW_CONVERT_EXPR to wrap CONST_DECLs.

	* g++.dg/cpp1z/pr83918.C: New test.

--- gcc/tree.c.jj	2018-01-16 16:16:37.124694238 +0100
+++ gcc/tree.c	2018-01-22 23:53:58.171837029 +0100
@@ -14085,8 +14085,10 @@ maybe_wrap_with_location (tree expr, loc
   if (EXCEPTIONAL_CLASS_P (expr))
     return expr;
 
-  tree_code code = (CONSTANT_CLASS_P (expr) && TREE_CODE (expr) != STRING_CST
-		    ? NON_LVALUE_EXPR : VIEW_CONVERT_EXPR);
+  tree_code code
+    = (((CONSTANT_CLASS_P (expr) && TREE_CODE (expr) != STRING_CST)
+	|| (TREE_CODE (expr) == CONST_DECL && !TREE_STATIC (expr)))
+       ? NON_LVALUE_EXPR : VIEW_CONVERT_EXPR);
   tree wrapper = build1_loc (loc, code, TREE_TYPE (expr), expr);
   /* Mark this node as being a wrapper.  */
   EXPR_LOCATION_WRAPPER_P (wrapper) = 1;
--- gcc/testsuite/g++.dg/cpp1z/pr83918.C.jj	2018-01-22 23:54:40.824824960 +0100
+++ gcc/testsuite/g++.dg/cpp1z/pr83918.C	2018-01-22 23:54:40.824824960 +0100
@@ -0,0 +1,32 @@
+// PR c++/83918
+// { dg-do compile }
+// { dg-options "-std=c++17" }
+
+constexpr unsigned
+foo (unsigned x, unsigned y)
+{
+  return x > y ? x : y;
+}
+
+template <typename, typename> struct A;
+template <auto ...> struct B;
+template <auto S, auto ... T, auto U, auto ... V>
+struct A <B <S, T...>, B <U, V...>>
+{
+  enum : unsigned
+  {
+    u = foo (sizeof (S), sizeof (U)),
+    v = A <B <T...>, B <V...>>::w,
+    w = foo (u, v)
+  };
+};
+
+template <>
+struct A <B <>, B <>>
+{
+  enum : unsigned { w = 0 };
+};
+
+constexpr static const auto v { A <B <1,2,3,4,5,6,7,8,9>,
+				   B <9,8,7,6,5,4,3,2,1>>::w };
+static_assert (v == sizeof (int));


	Jakub
Jason Merrill Jan. 23, 2018, 2:42 a.m. | #3
On Mon, Jan 22, 2018 at 6:14 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Jan 22, 2018 at 04:49:34PM -0500, Jason Merrill wrote:

>> On Thu, Jan 18, 2018 at 6:13 PM, Jakub Jelinek <jakub@redhat.com> wrote:

>> > Before location wrappers were introduced, the potential_constant_expression_1

>> > assumption that a VIEW_CONVERT_EXPR must have non-NULL type was right,

>> > if it is a type dependent reinterpret cast, it would be

>> > REINTERPRET_CAST_EXPR instead.

>> >

>> > Location wrappers around decls can have NULL type, if the decl they wrap

>> > also has NULL type.

>>

>> Hmm, why would a decl have NULL type?  Are we wrapping a CONST_DECL in

>> VIEW_CONVERT_EXPR?  They should get NON_LVALUE_EXPR, since they aren't

>> lvalues (except the objc++ ones that have TREE_STATIC set).

>

> So, do you want following instead, if it passes bootstrap/regtest?


Looks good.

Jason

Patch

--- gcc/cp/constexpr.c.jj	2018-01-17 22:00:13.624228171 +0100
+++ gcc/cp/constexpr.c	2018-01-18 10:38:07.357469094 +0100
@@ -5385,7 +5385,8 @@  potential_constant_expression_1 (tree t,
 	 may change to something more specific to type-punning (DR 1312).  */
       {
         tree from = TREE_OPERAND (t, 0);
-	if (POINTER_TYPE_P (TREE_TYPE (t))
+	if (!location_wrapper_p (t)
+	    && POINTER_TYPE_P (TREE_TYPE (t))
 	    && TREE_CODE (from) == INTEGER_CST
 	    && !integer_zerop (from))
 	  {
--- gcc/testsuite/g++.dg/cpp1z/pr83918.C.jj	2018-01-18 10:46:04.405439296 +0100
+++ gcc/testsuite/g++.dg/cpp1z/pr83918.C	2018-01-18 10:45:42.934442290 +0100
@@ -0,0 +1,32 @@ 
+// PR c++/83918
+// { dg-do compile }
+// { dg-options "-std=c++17" }
+
+constexpr unsigned
+foo (unsigned x, unsigned y)
+{
+  return x > y ? x : y;
+}
+
+template <typename, typename> struct A;
+template <auto ...> struct B;
+template <auto S, auto ... T, auto U, auto ... V>
+struct A <B <S, T...>, B <U, V...>>
+{
+  enum : unsigned
+  {
+    u = foo (sizeof (S), sizeof (U)),
+    v = A <B <T...>, B <V...>>::w,
+    w = foo (u, v)
+  };
+};
+
+template <>
+struct A <B <>, B <>>
+{
+  enum : unsigned { w = 0 };
+};
+
+constexpr static const auto v { A <B <1,2,3,4,5,6,7,8,9>,
+				   B <9,8,7,6,5,4,3,2,1>>::w };
+static_assert (v == sizeof (int));