diff mbox series

[C++] Fix offsetof constexpr handling (PR c++/85662)

Message ID 20180506175652.GA8577@tucnak
State New
Headers show
Series [C++] Fix offsetof constexpr handling (PR c++/85662) | expand

Commit Message

Jakub Jelinek May 6, 2018, 5:56 p.m. UTC 
Hi!

fold_offsetof_1 builds the base as usually INTEGER_CST with pointer type and
calls fold_build_pointer_plus with the offset gathered from the
COMPONENT_REF field offset or ARRAY_REF index or combination of them.
But most of the fold_* routines aren't recursive, they fold just one level,
so if the expression is more complex and with delayed folding we actually
can keep around POINTER_PLUS_EXPR of pointer-typed INTEGER_CST (usually 0)
and some large expression computing the offset.  In that case we reject it
as constant expression though, because we don't allow such arithmetics on
(null) pointers.

The following patch fixes it by only doing the pointer arithmetics if the
user actually wrote it that way (offsetof-like expression like &((struct S
*)0)->foo.bar[idx * 3 + 2]), but if it is __builtin_offsetof, uses integral
arithmetics from the beginning.  I think we should e.g. reject offsetof
when the expression inside of array index has undefined behavior, so we
don't want to cp_fold the fold_offsetof_1 result.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
What about release branches?

2018-05-06  Jakub Jelinek  <jakub@redhat.com>

	PR c++/85662
	* c-common.h (fold_offsetof_1): Add bool argument with false default
	argument.
	* c-common.c (fold_offsetof_1): Add NONPTR argument, if true, convert
	the pointer constant to sizetype and use size_binop with PLUS_EXPR
	instead of fold_build_pointer_plus.  Adjust recursive calls.
	(fold_offsetof): Pass true as NONPTR to fold_offsetof_1.

	* g++.dg/ext/offsetof2.C: New test.


	Jakub

Comments

Jason Merrill May 8, 2018, 5:03 p.m. UTC | #1 
On Sun, May 6, 2018 at 1:56 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> --- gcc/c-family/c-common.c.jj  2018-03-27 21:58:55.598502113 +0200
> +++ gcc/c-family/c-common.c     2018-05-05 10:55:47.951600802 +0200
> @@ -6171,7 +6171,7 @@ c_common_to_target_charset (HOST_WIDE_IN
>     traditional rendering of offsetof as a macro.  Return the folded result.  */
>
>  tree
> -fold_offsetof_1 (tree expr, enum tree_code ctx)
> +fold_offsetof_1 (tree expr, bool nonptr, enum tree_code ctx)

The comment needs to document the NONPTR parameter.

> @@ -6287,7 +6291,7 @@ fold_offsetof_1 (tree expr, enum tree_co
>  tree
>  fold_offsetof (tree expr)
>  {
> -  return convert (size_type_node, fold_offsetof_1 (expr));
> +  return convert (size_type_node, fold_offsetof_1 (expr, true));
>  }

Since all the uses of fold_offset_1 involve converting to a particular
type, I wonder about wrapping it so that the argument for nonptr is
determined from that type.

Jason
Jakub Jelinek May 8, 2018, 8:04 p.m. UTC | #2 
On Tue, May 08, 2018 at 01:03:00PM -0400, Jason Merrill wrote:
> On Sun, May 6, 2018 at 1:56 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > --- gcc/c-family/c-common.c.jj  2018-03-27 21:58:55.598502113 +0200
> > +++ gcc/c-family/c-common.c     2018-05-05 10:55:47.951600802 +0200
> > @@ -6171,7 +6171,7 @@ c_common_to_target_charset (HOST_WIDE_IN
> >     traditional rendering of offsetof as a macro.  Return the folded result.  */
> >
> >  tree
> > -fold_offsetof_1 (tree expr, enum tree_code ctx)
> > +fold_offsetof_1 (tree expr, bool nonptr, enum tree_code ctx)
> 
> The comment needs to document the NONPTR parameter.

Ok.

> > @@ -6287,7 +6291,7 @@ fold_offsetof_1 (tree expr, enum tree_co
> >  tree
> >  fold_offsetof (tree expr)
> >  {
> > -  return convert (size_type_node, fold_offsetof_1 (expr));
> > +  return convert (size_type_node, fold_offsetof_1 (expr, true));
> >  }
> 
> Since all the uses of fold_offset_1 involve converting to a particular
> type, I wonder about wrapping it so that the argument for nonptr is
> determined from that type.

So like this?

2018-05-08  Jakub Jelinek  <jakub@redhat.com>

	PR c++/85662
	* c-common.h (fold_offsetof_1): Add TYPE argument.
	* c-common.c (fold_offsetof_1): Add TYPE argument, if it is not a
	pointer type, convert the pointer constant to TYPE and use size_binop
	with PLUS_EXPR instead of fold_build_pointer_plus.  Adjust recursive
	calls.
	(fold_offsetof): Pass size_type_node as TYPE to fold_offsetof_1.

	* c-fold.c (c_fully_fold_internal): Pass TREE_TYPE (expr) as TYPE
	to fold_offsetof_1.
	* c-typeck.c (build_unary_op): Pass argtype as TYPE to fold_offsetof_1.

	* cp-gimplify.c (cp_fold): Pass TREE_TYPE (x) as TYPE to
	fold_offsetof_1.

	* g++.dg/ext/offsetof2.C: New test.

--- gcc/c-family/c-common.h.jj	2018-05-06 23:12:49.185619717 +0200
+++ gcc/c-family/c-common.h	2018-05-08 21:47:40.976737821 +0200
@@ -1033,7 +1033,7 @@ extern bool c_dump_tree (void *, tree);
 
 extern void verify_sequence_points (tree);
 
-extern tree fold_offsetof_1 (tree, tree_code ctx = ERROR_MARK);
+extern tree fold_offsetof_1 (tree, tree, tree_code ctx = ERROR_MARK);
 extern tree fold_offsetof (tree);
 
 extern int complete_array_type (tree *, tree, bool);
--- gcc/c-family/c-common.c.jj	2018-05-06 23:12:49.135619681 +0200
+++ gcc/c-family/c-common.c	2018-05-08 21:56:24.635088315 +0200
@@ -6168,10 +6168,12 @@ c_common_to_target_charset (HOST_WIDE_IN
 
 /* Fold an offsetof-like expression.  EXPR is a nested sequence of component
    references with an INDIRECT_REF of a constant at the bottom; much like the
-   traditional rendering of offsetof as a macro.  Return the folded result.  */
+   traditional rendering of offsetof as a macro.  TYPE is the desired type of
+   the whole expression to which it will be converted afterwards.
+   Return the folded result.  */
 
 tree
-fold_offsetof_1 (tree expr, enum tree_code ctx)
+fold_offsetof_1 (tree type, tree expr, enum tree_code ctx)
 {
   tree base, off, t;
   tree_code code = TREE_CODE (expr);
@@ -6196,10 +6198,12 @@ fold_offsetof_1 (tree expr, enum tree_co
 	  error ("cannot apply %<offsetof%> to a non constant address");
 	  return error_mark_node;
 	}
+      if (!POINTER_TYPE_P (type))
+	return convert (type, TREE_OPERAND (expr, 0));
       return TREE_OPERAND (expr, 0);
 
     case COMPONENT_REF:
-      base = fold_offsetof_1 (TREE_OPERAND (expr, 0), code);
+      base = fold_offsetof_1 (type, TREE_OPERAND (expr, 0), code);
       if (base == error_mark_node)
 	return base;
 
@@ -6216,7 +6220,7 @@ fold_offsetof_1 (tree expr, enum tree_co
       break;
 
     case ARRAY_REF:
-      base = fold_offsetof_1 (TREE_OPERAND (expr, 0), code);
+      base = fold_offsetof_1 (type, TREE_OPERAND (expr, 0), code);
       if (base == error_mark_node)
 	return base;
 
@@ -6273,12 +6277,14 @@ fold_offsetof_1 (tree expr, enum tree_co
       /* Handle static members of volatile structs.  */
       t = TREE_OPERAND (expr, 1);
       gcc_checking_assert (VAR_P (get_base_address (t)));
-      return fold_offsetof_1 (t);
+      return fold_offsetof_1 (type, t);
 
     default:
       gcc_unreachable ();
     }
 
+  if (!POINTER_TYPE_P (type))
+    return size_binop (PLUS_EXPR, base, convert (type, off));
   return fold_build_pointer_plus (base, off);
 }
 
@@ -6287,7 +6293,7 @@ fold_offsetof_1 (tree expr, enum tree_co
 tree
 fold_offsetof (tree expr)
 {
-  return convert (size_type_node, fold_offsetof_1 (expr));
+  return convert (size_type_node, fold_offsetof_1 (size_type_node, expr));
 }
 
 
--- gcc/c/c-fold.c.jj	2018-01-17 22:00:12.310228253 +0100
+++ gcc/c/c-fold.c	2018-05-08 21:52:43.303940175 +0200
@@ -473,7 +473,8 @@ c_fully_fold_internal (tree expr, bool i
 	  && (op1 = get_base_address (op0)) != NULL_TREE
 	  && INDIRECT_REF_P (op1)
 	  && TREE_CONSTANT (TREE_OPERAND (op1, 0)))
-	ret = fold_convert_loc (loc, TREE_TYPE (expr), fold_offsetof_1 (op0));
+	ret = fold_convert_loc (loc, TREE_TYPE (expr),
+				fold_offsetof_1 (TREE_TYPE (expr), op0));
       else if (op0 != orig_op0 || in_init)
 	ret = in_init
 	  ? fold_build1_initializer_loc (loc, code, TREE_TYPE (expr), op0)
--- gcc/c/c-typeck.c.jj	2018-04-25 11:35:11.974267304 +0200
+++ gcc/c/c-typeck.c	2018-05-08 21:53:04.582954414 +0200
@@ -4676,7 +4676,8 @@ build_unary_op (location_t location, enu
       if (val && INDIRECT_REF_P (val)
           && TREE_CONSTANT (TREE_OPERAND (val, 0)))
 	{
-	  ret = fold_convert_loc (location, argtype, fold_offsetof_1 (arg));
+	  ret = fold_convert_loc (location, argtype,
+				  fold_offsetof_1 (argtype, arg));
 	  goto return_build_unary_op;
 	}
 
--- gcc/cp/cp-gimplify.c.jj	2018-04-19 15:57:36.784482581 +0200
+++ gcc/cp/cp-gimplify.c	2018-05-08 21:53:45.439981764 +0200
@@ -2232,7 +2232,8 @@ cp_fold (tree x)
 	      val = TREE_OPERAND (val, 0);
 	      STRIP_NOPS (val);
 	      if (TREE_CODE (val) == INTEGER_CST)
-		return fold_convert (TREE_TYPE (x), fold_offsetof_1 (op0));
+		return fold_convert (TREE_TYPE (x),
+				     fold_offsetof_1 (TREE_TYPE (x), op0));
 	    }
 	}
       goto finish_unary;
--- gcc/testsuite/g++.dg/ext/offsetof2.C.jj	2018-05-08 21:47:20.406724048 +0200
+++ gcc/testsuite/g++.dg/ext/offsetof2.C	2018-05-08 21:47:20.406724048 +0200
@@ -0,0 +1,6 @@
+// PR c++/85662
+// { dg-do compile { target c++11 } }
+
+struct S { unsigned long x[31]; };
+struct T { bool b; S f; };
+static_assert (__builtin_offsetof (T, f.x[31 - 1]) == __builtin_offsetof (T, f.x[30]), "");


	Jakub
Jason Merrill May 9, 2018, 3:28 a.m. UTC | #3 
On Tue, May 8, 2018 at 4:04 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, May 08, 2018 at 01:03:00PM -0400, Jason Merrill wrote:
>> On Sun, May 6, 2018 at 1:56 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > --- gcc/c-family/c-common.c.jj  2018-03-27 21:58:55.598502113 +0200
>> > +++ gcc/c-family/c-common.c     2018-05-05 10:55:47.951600802 +0200
>> > @@ -6171,7 +6171,7 @@ c_common_to_target_charset (HOST_WIDE_IN
>> >     traditional rendering of offsetof as a macro.  Return the folded result.  */
>> >
>> >  tree
>> > -fold_offsetof_1 (tree expr, enum tree_code ctx)
>> > +fold_offsetof_1 (tree expr, bool nonptr, enum tree_code ctx)
>>
>> The comment needs to document the NONPTR parameter.
>
> Ok.
>
>> > @@ -6287,7 +6291,7 @@ fold_offsetof_1 (tree expr, enum tree_co
>> >  tree
>> >  fold_offsetof (tree expr)
>> >  {
>> > -  return convert (size_type_node, fold_offsetof_1 (expr));
>> > +  return convert (size_type_node, fold_offsetof_1 (expr, true));
>> >  }
>>
>> Since all the uses of fold_offset_1 involve converting to a particular
>> type, I wonder about wrapping it so that the argument for nonptr is
>> determined from that type.
>
> So like this?
>
> 2018-05-08  Jakub Jelinek  <jakub@redhat.com>
>
>         PR c++/85662
>         * c-common.h (fold_offsetof_1): Add TYPE argument.
>         * c-common.c (fold_offsetof_1): Add TYPE argument, if it is not a
>         pointer type, convert the pointer constant to TYPE and use size_binop
>         with PLUS_EXPR instead of fold_build_pointer_plus.  Adjust recursive
>         calls.
>         (fold_offsetof): Pass size_type_node as TYPE to fold_offsetof_1.
>
>         * c-fold.c (c_fully_fold_internal): Pass TREE_TYPE (expr) as TYPE
>         to fold_offsetof_1.
>         * c-typeck.c (build_unary_op): Pass argtype as TYPE to fold_offsetof_1.
>
>         * cp-gimplify.c (cp_fold): Pass TREE_TYPE (x) as TYPE to
>         fold_offsetof_1.
>
>         * g++.dg/ext/offsetof2.C: New test.
>
> --- gcc/c-family/c-common.h.jj  2018-05-06 23:12:49.185619717 +0200
> +++ gcc/c-family/c-common.h     2018-05-08 21:47:40.976737821 +0200
> @@ -1033,7 +1033,7 @@ extern bool c_dump_tree (void *, tree);
>
>  extern void verify_sequence_points (tree);
>
> -extern tree fold_offsetof_1 (tree, tree_code ctx = ERROR_MARK);
> +extern tree fold_offsetof_1 (tree, tree, tree_code ctx = ERROR_MARK);
>  extern tree fold_offsetof (tree);
>
>  extern int complete_array_type (tree *, tree, bool);
> --- gcc/c-family/c-common.c.jj  2018-05-06 23:12:49.135619681 +0200
> +++ gcc/c-family/c-common.c     2018-05-08 21:56:24.635088315 +0200
> @@ -6168,10 +6168,12 @@ c_common_to_target_charset (HOST_WIDE_IN
>
>  /* Fold an offsetof-like expression.  EXPR is a nested sequence of component
>     references with an INDIRECT_REF of a constant at the bottom; much like the
> -   traditional rendering of offsetof as a macro.  Return the folded result.  */
> +   traditional rendering of offsetof as a macro.  TYPE is the desired type of
> +   the whole expression to which it will be converted afterwards.
> +   Return the folded result.  */
>
>  tree
> -fold_offsetof_1 (tree expr, enum tree_code ctx)
> +fold_offsetof_1 (tree type, tree expr, enum tree_code ctx)
>  {
>    tree base, off, t;
>    tree_code code = TREE_CODE (expr);
> @@ -6196,10 +6198,12 @@ fold_offsetof_1 (tree expr, enum tree_co
>           error ("cannot apply %<offsetof%> to a non constant address");
>           return error_mark_node;
>         }
> +      if (!POINTER_TYPE_P (type))
> +       return convert (type, TREE_OPERAND (expr, 0));
>        return TREE_OPERAND (expr, 0);
>
>      case COMPONENT_REF:
> -      base = fold_offsetof_1 (TREE_OPERAND (expr, 0), code);
> +      base = fold_offsetof_1 (type, TREE_OPERAND (expr, 0), code);
>        if (base == error_mark_node)
>         return base;
>
> @@ -6216,7 +6220,7 @@ fold_offsetof_1 (tree expr, enum tree_co
>        break;
>
>      case ARRAY_REF:
> -      base = fold_offsetof_1 (TREE_OPERAND (expr, 0), code);
> +      base = fold_offsetof_1 (type, TREE_OPERAND (expr, 0), code);
>        if (base == error_mark_node)
>         return base;
>
> @@ -6273,12 +6277,14 @@ fold_offsetof_1 (tree expr, enum tree_co
>        /* Handle static members of volatile structs.  */
>        t = TREE_OPERAND (expr, 1);
>        gcc_checking_assert (VAR_P (get_base_address (t)));
> -      return fold_offsetof_1 (t);
> +      return fold_offsetof_1 (type, t);
>
>      default:
>        gcc_unreachable ();
>      }
>
> +  if (!POINTER_TYPE_P (type))
> +    return size_binop (PLUS_EXPR, base, convert (type, off));
>    return fold_build_pointer_plus (base, off);
>  }
>
> @@ -6287,7 +6293,7 @@ fold_offsetof_1 (tree expr, enum tree_co
>  tree
>  fold_offsetof (tree expr)
>  {
> -  return convert (size_type_node, fold_offsetof_1 (expr));
> +  return convert (size_type_node, fold_offsetof_1 (size_type_node, expr));
>  }

Maybe add a type parameter that defaults to size_type_node...

>
> --- gcc/c/c-fold.c.jj   2018-01-17 22:00:12.310228253 +0100
> +++ gcc/c/c-fold.c      2018-05-08 21:52:43.303940175 +0200
> @@ -473,7 +473,8 @@ c_fully_fold_internal (tree expr, bool i
>           && (op1 = get_base_address (op0)) != NULL_TREE
>           && INDIRECT_REF_P (op1)
>           && TREE_CONSTANT (TREE_OPERAND (op1, 0)))
> -       ret = fold_convert_loc (loc, TREE_TYPE (expr), fold_offsetof_1 (op0));
> +       ret = fold_convert_loc (loc, TREE_TYPE (expr),
> +                               fold_offsetof_1 (TREE_TYPE (expr), op0));

...and then this can be

  fold_offsetof (op0, TREE_TYPE (exp0))

?  Or a separate function called something like
fold_offsetof_with_type, if you'd prefer.

Jason
Jakub Jelinek May 9, 2018, 8:55 a.m. UTC | #4 
On Tue, May 08, 2018 at 11:28:18PM -0400, Jason Merrill wrote:
> Maybe add a type parameter that defaults to size_type_node...
> 
> >
> > --- gcc/c/c-fold.c.jj   2018-01-17 22:00:12.310228253 +0100
> > +++ gcc/c/c-fold.c      2018-05-08 21:52:43.303940175 +0200
> > @@ -473,7 +473,8 @@ c_fully_fold_internal (tree expr, bool i
> >           && (op1 = get_base_address (op0)) != NULL_TREE
> >           && INDIRECT_REF_P (op1)
> >           && TREE_CONSTANT (TREE_OPERAND (op1, 0)))
> > -       ret = fold_convert_loc (loc, TREE_TYPE (expr), fold_offsetof_1 (op0));
> > +       ret = fold_convert_loc (loc, TREE_TYPE (expr),
> > +                               fold_offsetof_1 (TREE_TYPE (expr), op0));
> 
> ...and then this can be
> 
>   fold_offsetof (op0, TREE_TYPE (exp0))

Like this then?

2018-05-09  Jakub Jelinek  <jakub@redhat.com>

	PR c++/85662
	* c-common.h (fold_offsetof_1): Removed.
	(fold_offsetof): Add TYPE argument defaulted to size_type_node and
	CTX argument defaulted to ERROR_MARK.
	* c-common.c (fold_offsetof_1): Renamed to ...
	(fold_offsetof): ... this.  Remove wrapper function.  Add TYPE
	argument, if it is not a pointer type, convert the pointer constant
	to TYPE and use size_binop with PLUS_EXPR instead of
	fold_build_pointer_plus.  Adjust recursive calls.

	* c-fold.c (c_fully_fold_internal): Use fold_offsetof rather than
	fold_offsetof_1, pass TREE_TYPE (expr) as TYPE to it.
	* c-typeck.c (build_unary_op): Use fold_offsetof rather than
	fold_offsetof_1, pass argtype as TYPE to it.

	* cp-gimplify.c (cp_fold): Use fold_offsetof rather than
	fold_offsetof_1, pass TREE_TYPE (x) as TYPE to it.

	* g++.dg/ext/offsetof2.C: New test.

--- gcc/c-family/c-common.h.jj	2018-05-06 23:12:49.185619717 +0200
+++ gcc/c-family/c-common.h	2018-05-09 10:28:20.149559265 +0200
@@ -1033,8 +1033,8 @@ extern bool c_dump_tree (void *, tree);
 
 extern void verify_sequence_points (tree);
 
-extern tree fold_offsetof_1 (tree, tree_code ctx = ERROR_MARK);
-extern tree fold_offsetof (tree);
+extern tree fold_offsetof (tree, tree = size_type_node,
+			   tree_code ctx = ERROR_MARK);
 
 extern int complete_array_type (tree *, tree, bool);
 
--- gcc/c-family/c-common.c.jj	2018-05-06 23:12:49.135619681 +0200
+++ gcc/c-family/c-common.c	2018-05-09 10:29:34.481650988 +0200
@@ -6168,10 +6168,12 @@ c_common_to_target_charset (HOST_WIDE_IN
 
 /* Fold an offsetof-like expression.  EXPR is a nested sequence of component
    references with an INDIRECT_REF of a constant at the bottom; much like the
-   traditional rendering of offsetof as a macro.  Return the folded result.  */
+   traditional rendering of offsetof as a macro.  TYPE is the desired type of
+   the whole expression to which it will be converted afterwards.
+   Return the folded result.  */
 
 tree
-fold_offsetof_1 (tree expr, enum tree_code ctx)
+fold_offsetof (tree expr, tree type, enum tree_code ctx)
 {
   tree base, off, t;
   tree_code code = TREE_CODE (expr);
@@ -6196,10 +6198,12 @@ fold_offsetof_1 (tree expr, enum tree_co
 	  error ("cannot apply %<offsetof%> to a non constant address");
 	  return error_mark_node;
 	}
+      if (!POINTER_TYPE_P (type))
+	return convert (type, TREE_OPERAND (expr, 0));
       return TREE_OPERAND (expr, 0);
 
     case COMPONENT_REF:
-      base = fold_offsetof_1 (TREE_OPERAND (expr, 0), code);
+      base = fold_offsetof (TREE_OPERAND (expr, 0), type, code);
       if (base == error_mark_node)
 	return base;
 
@@ -6216,7 +6220,7 @@ fold_offsetof_1 (tree expr, enum tree_co
       break;
 
     case ARRAY_REF:
-      base = fold_offsetof_1 (TREE_OPERAND (expr, 0), code);
+      base = fold_offsetof (TREE_OPERAND (expr, 0), type, code);
       if (base == error_mark_node)
 	return base;
 
@@ -6273,23 +6277,16 @@ fold_offsetof_1 (tree expr, enum tree_co
       /* Handle static members of volatile structs.  */
       t = TREE_OPERAND (expr, 1);
       gcc_checking_assert (VAR_P (get_base_address (t)));
-      return fold_offsetof_1 (t);
+      return fold_offsetof (t, type);
 
     default:
       gcc_unreachable ();
     }
 
+  if (!POINTER_TYPE_P (type))
+    return size_binop (PLUS_EXPR, base, convert (type, off));
   return fold_build_pointer_plus (base, off);
 }
-
-/* Likewise, but convert it to the return type of offsetof.  */
-
-tree
-fold_offsetof (tree expr)
-{
-  return convert (size_type_node, fold_offsetof_1 (expr));
-}
-
 
 /* *PTYPE is an incomplete array.  Complete it with a domain based on
    INITIAL_VALUE.  If INITIAL_VALUE is not present, use 1 if DO_DEFAULT
--- gcc/c/c-fold.c.jj	2018-01-17 22:00:12.310228253 +0100
+++ gcc/c/c-fold.c	2018-05-09 10:30:04.185687645 +0200
@@ -473,7 +473,8 @@ c_fully_fold_internal (tree expr, bool i
 	  && (op1 = get_base_address (op0)) != NULL_TREE
 	  && INDIRECT_REF_P (op1)
 	  && TREE_CONSTANT (TREE_OPERAND (op1, 0)))
-	ret = fold_convert_loc (loc, TREE_TYPE (expr), fold_offsetof_1 (op0));
+	ret = fold_convert_loc (loc, TREE_TYPE (expr),
+				fold_offsetof (op0, TREE_TYPE (expr)));
       else if (op0 != orig_op0 || in_init)
 	ret = in_init
 	  ? fold_build1_initializer_loc (loc, code, TREE_TYPE (expr), op0)
--- gcc/c/c-typeck.c.jj	2018-04-25 11:35:11.974267304 +0200
+++ gcc/c/c-typeck.c	2018-05-09 10:30:18.174704911 +0200
@@ -4676,7 +4676,8 @@ build_unary_op (location_t location, enu
       if (val && INDIRECT_REF_P (val)
           && TREE_CONSTANT (TREE_OPERAND (val, 0)))
 	{
-	  ret = fold_convert_loc (location, argtype, fold_offsetof_1 (arg));
+	  ret = fold_convert_loc (location, argtype,
+				  fold_offsetof (arg, argtype));
 	  goto return_build_unary_op;
 	}
 
--- gcc/cp/cp-gimplify.c.jj	2018-04-19 15:57:36.784482581 +0200
+++ gcc/cp/cp-gimplify.c	2018-05-09 10:30:48.287742071 +0200
@@ -2232,7 +2232,8 @@ cp_fold (tree x)
 	      val = TREE_OPERAND (val, 0);
 	      STRIP_NOPS (val);
 	      if (TREE_CODE (val) == INTEGER_CST)
-		return fold_convert (TREE_TYPE (x), fold_offsetof_1 (op0));
+		return fold_convert (TREE_TYPE (x),
+				     fold_offsetof (op0, TREE_TYPE (x)));
 	    }
 	}
       goto finish_unary;
--- gcc/testsuite/g++.dg/ext/offsetof2.C.jj	2018-05-08 21:47:20.406724048 +0200
+++ gcc/testsuite/g++.dg/ext/offsetof2.C	2018-05-08 21:47:20.406724048 +0200
@@ -0,0 +1,6 @@
+// PR c++/85662
+// { dg-do compile { target c++11 } }
+
+struct S { unsigned long x[31]; };
+struct T { bool b; S f; };
+static_assert (__builtin_offsetof (T, f.x[31 - 1]) == __builtin_offsetof (T, f.x[30]), "");


	Jakub
Jason Merrill May 9, 2018, 2:40 p.m. UTC | #5 
On Wed, May 9, 2018 at 4:55 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, May 08, 2018 at 11:28:18PM -0400, Jason Merrill wrote:
>> Maybe add a type parameter that defaults to size_type_node...
>>
>> > +       ret = fold_convert_loc (loc, TREE_TYPE (expr),
>> > +                               fold_offsetof_1 (TREE_TYPE (expr), op0));
>>
>> ...and then this can be
>>
>>   fold_offsetof (op0, TREE_TYPE (exp0))
>
> Like this then?
>
> +       ret = fold_convert_loc (loc, TREE_TYPE (expr),
> +                               fold_offsetof (op0, TREE_TYPE (expr)));

I was thinking that we then wouldn't need the fold_convert at the call
sites anymore, either.

Jason
Jakub Jelinek May 9, 2018, 2:47 p.m. UTC | #6 
On Wed, May 09, 2018 at 10:40:26AM -0400, Jason Merrill wrote:
> On Wed, May 9, 2018 at 4:55 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Tue, May 08, 2018 at 11:28:18PM -0400, Jason Merrill wrote:
> >> Maybe add a type parameter that defaults to size_type_node...
> >>
> >> > +       ret = fold_convert_loc (loc, TREE_TYPE (expr),
> >> > +                               fold_offsetof_1 (TREE_TYPE (expr), op0));
> >>
> >> ...and then this can be
> >>
> >>   fold_offsetof (op0, TREE_TYPE (exp0))
> >
> > Like this then?
> >
> > +       ret = fold_convert_loc (loc, TREE_TYPE (expr),
> > +                               fold_offsetof (op0, TREE_TYPE (expr)));
> 
> I was thinking that we then wouldn't need the fold_convert at the call
> sites anymore, either.

The patch only converts to non-pointer types, I'm not sure if it is
desirable to do the same with pointer types (and most of the other callers
don't use convert, but fold_convert which is significantly different, the
former is emitting diagnostics, the latter is just an conversion + optimization).
If it is ok to use the final pointer type rather than the initial one, but
convert is not ok, then it would be something like:
      if (!POINTER_TYPE_P (type))
	return convert (type, TREE_OPERAND (expr, 0));
      else
	return fold_convert (type, TREE_OPERAND (expr, 0));
on the innermost constant and then indeed no conversion would be needed.

	Jakub
Jason Merrill May 9, 2018, 3:01 p.m. UTC | #7 
On Wed, May 9, 2018 at 10:47 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, May 09, 2018 at 10:40:26AM -0400, Jason Merrill wrote:
>> On Wed, May 9, 2018 at 4:55 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > On Tue, May 08, 2018 at 11:28:18PM -0400, Jason Merrill wrote:
>> >> Maybe add a type parameter that defaults to size_type_node...
>> >>
>> >> > +       ret = fold_convert_loc (loc, TREE_TYPE (expr),
>> >> > +                               fold_offsetof_1 (TREE_TYPE (expr), op0));
>> >>
>> >> ...and then this can be
>> >>
>> >>   fold_offsetof (op0, TREE_TYPE (exp0))
>> >
>> > Like this then?
>> >
>> > +       ret = fold_convert_loc (loc, TREE_TYPE (expr),
>> > +                               fold_offsetof (op0, TREE_TYPE (expr)));
>>
>> I was thinking that we then wouldn't need the fold_convert at the call
>> sites anymore, either.
>
> The patch only converts to non-pointer types, I'm not sure if it is
> desirable to do the same with pointer types (and most of the other callers
> don't use convert, but fold_convert which is significantly different, the
> former is emitting diagnostics, the latter is just an conversion + optimization).

Is there a reason we can't use fold_convert for the non-pointer case,
too?  I don't think we're interested in diagnostics from this
particular call.

Jason
diff mbox series

Patch

--- gcc/c-family/c-common.h.jj	2018-03-13 00:38:23.846662269 +0100
+++ gcc/c-family/c-common.h	2018-05-05 10:43:53.069431079 +0200
@@ -1033,7 +1033,7 @@  extern bool c_dump_tree (void *, tree);
 
 extern void verify_sequence_points (tree);
 
-extern tree fold_offsetof_1 (tree, tree_code ctx = ERROR_MARK);
+extern tree fold_offsetof_1 (tree, bool = false, tree_code ctx = ERROR_MARK);
 extern tree fold_offsetof (tree);
 
 extern int complete_array_type (tree *, tree, bool);
--- gcc/c-family/c-common.c.jj	2018-03-27 21:58:55.598502113 +0200
+++ gcc/c-family/c-common.c	2018-05-05 10:55:47.951600802 +0200
@@ -6171,7 +6171,7 @@  c_common_to_target_charset (HOST_WIDE_IN
    traditional rendering of offsetof as a macro.  Return the folded result.  */
 
 tree
-fold_offsetof_1 (tree expr, enum tree_code ctx)
+fold_offsetof_1 (tree expr, bool nonptr, enum tree_code ctx)
 {
   tree base, off, t;
   tree_code code = TREE_CODE (expr);
@@ -6196,10 +6196,12 @@  fold_offsetof_1 (tree expr, enum tree_co
 	  error ("cannot apply %<offsetof%> to a non constant address");
 	  return error_mark_node;
 	}
+      if (nonptr)
+	return convert (sizetype, TREE_OPERAND (expr, 0));
       return TREE_OPERAND (expr, 0);
 
     case COMPONENT_REF:
-      base = fold_offsetof_1 (TREE_OPERAND (expr, 0), code);
+      base = fold_offsetof_1 (TREE_OPERAND (expr, 0), nonptr, code);
       if (base == error_mark_node)
 	return base;
 
@@ -6216,7 +6218,7 @@  fold_offsetof_1 (tree expr, enum tree_co
       break;
 
     case ARRAY_REF:
-      base = fold_offsetof_1 (TREE_OPERAND (expr, 0), code);
+      base = fold_offsetof_1 (TREE_OPERAND (expr, 0), nonptr, code);
       if (base == error_mark_node)
 	return base;
 
@@ -6273,12 +6275,14 @@  fold_offsetof_1 (tree expr, enum tree_co
       /* Handle static members of volatile structs.  */
       t = TREE_OPERAND (expr, 1);
       gcc_checking_assert (VAR_P (get_base_address (t)));
-      return fold_offsetof_1 (t);
+      return fold_offsetof_1 (t, nonptr);
 
     default:
       gcc_unreachable ();
     }
 
+  if (nonptr)
+    return size_binop (PLUS_EXPR, base, off);
   return fold_build_pointer_plus (base, off);
 }
 
@@ -6287,7 +6291,7 @@  fold_offsetof_1 (tree expr, enum tree_co
 tree
 fold_offsetof (tree expr)
 {
-  return convert (size_type_node, fold_offsetof_1 (expr));
+  return convert (size_type_node, fold_offsetof_1 (expr, true));
 }
 
 
--- gcc/testsuite/g++.dg/ext/offsetof2.C.jj	2018-05-05 10:58:22.796637643 +0200
+++ gcc/testsuite/g++.dg/ext/offsetof2.C	2018-05-05 10:57:24.281623720 +0200
@@ -0,0 +1,6 @@ 
+// PR c++/85662
+// { dg-do compile { target c++11 } }
+
+struct S { unsigned long x[31]; };
+struct T { bool b; S f; };
+static_assert (__builtin_offsetof (T, f.x[31 - 1]) == __builtin_offsetof (T, f.x[30]), "");