c/101512 - fix missing address-taking in c_common_mark_addressable_vec

Message ID 54n4qs95-p776-4qop-rs65-po51r45848p@fhfr.qr
State New
Headers show
Series
  • c/101512 - fix missing address-taking in c_common_mark_addressable_vec
Related show

Commit Message

Richard Biener July 21, 2021, 8:06 a.m.
c_common_mark_addressable_vec fails to look through C_MAYBE_CONST_EXPR
in the case it isn't at the toplevel.

Bootstrapped and tested on x86_64-unknown-linux-gnu, OK?

Thanks,
Richard.

2021-07-21  Richard Biener  <rguenther@suse.de>

	PR c/101512
gcc/c-family/
	* c-common.c (c_common_mark_addressable_vec): Look through
	C_MAYBE_CONST_EXPR even if not at the toplevel.

	* gcc.dg/torture/pr101512.c: New testcase.
---
 gcc/c-family/c-common.c                 | 11 +++++++----
 gcc/testsuite/gcc.dg/torture/pr101512.c | 11 +++++++++++
 2 files changed, 18 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/torture/pr101512.c

-- 
2.26.2

Comments

Richard Biener via Gcc-patches July 21, 2021, 8:19 a.m. | #1
On Wed, Jul 21, 2021 at 10:06:51AM +0200, Richard Biener wrote:
> c_common_mark_addressable_vec fails to look through C_MAYBE_CONST_EXPR

> in the case it isn't at the toplevel.

> 

> Bootstrapped and tested on x86_64-unknown-linux-gnu, OK?

> 

> Thanks,

> Richard.

> 

> 2021-07-21  Richard Biener  <rguenther@suse.de>

> 

> 	PR c/101512

> gcc/c-family/

> 	* c-common.c (c_common_mark_addressable_vec): Look through

> 	C_MAYBE_CONST_EXPR even if not at the toplevel.

> 

> 	* gcc.dg/torture/pr101512.c: New testcase.


I wonder if instead when trying to wrap
C_MAYBE_CONST_EXPR into a VIEW_CONVERT_EXPR we shouldn't be
removing that C_MAYBE_CONST_EXPR and perhaps adding it around the
VIEW_CONVERT_EXPR.  E.g. various routines in c/c-typeck.c like
build_unary_op remember int_operands, remove_c_maybe_const_expr
and at the end note_integer_operands.

If Joseph thinks it is ok to have C_MAYBE_CONST_EXPR inside of
VCE, then the patch looks good to me.

	Jakub
Richard Biener July 28, 2021, 10:14 a.m. | #2
On Wed, 21 Jul 2021, Jakub Jelinek wrote:

> On Wed, Jul 21, 2021 at 10:06:51AM +0200, Richard Biener wrote:

> > c_common_mark_addressable_vec fails to look through C_MAYBE_CONST_EXPR

> > in the case it isn't at the toplevel.

> > 

> > Bootstrapped and tested on x86_64-unknown-linux-gnu, OK?

> > 

> > Thanks,

> > Richard.

> > 

> > 2021-07-21  Richard Biener  <rguenther@suse.de>

> > 

> > 	PR c/101512

> > gcc/c-family/

> > 	* c-common.c (c_common_mark_addressable_vec): Look through

> > 	C_MAYBE_CONST_EXPR even if not at the toplevel.

> > 

> > 	* gcc.dg/torture/pr101512.c: New testcase.

> 

> I wonder if instead when trying to wrap

> C_MAYBE_CONST_EXPR into a VIEW_CONVERT_EXPR we shouldn't be

> removing that C_MAYBE_CONST_EXPR and perhaps adding it around the

> VIEW_CONVERT_EXPR.  E.g. various routines in c/c-typeck.c like

> build_unary_op remember int_operands, remove_c_maybe_const_expr

> and at the end note_integer_operands.

> 

> If Joseph thinks it is ok to have C_MAYBE_CONST_EXPR inside of

> VCE, then the patch looks good to me.


Joseph - any comments?  The above mentioned issue is pre-existent,
EXPR_INT_CONST_OPERANDS should be false on vector types though,
so eventually the C_MAYBE_CONST_EXPR is entirely pointless,
but it's unconditionally built in build_compound_literal but
C_MAYBE_CONST_EXPR_NON_CONST is set there.  The VIEW_CONVERT_EXPR
is built via convert(), the types have the same main variant,
just one is not named.  But nothing in convert() seems to care
about C_MAYBE_CONST_EXPR.

Thanks,
Richard.
Joseph Myers July 28, 2021, 10:02 p.m. | #3
On Wed, 21 Jul 2021, Jakub Jelinek via Gcc-patches wrote:

> I wonder if instead when trying to wrap

> C_MAYBE_CONST_EXPR into a VIEW_CONVERT_EXPR we shouldn't be

> removing that C_MAYBE_CONST_EXPR and perhaps adding it around the

> VIEW_CONVERT_EXPR.  E.g. various routines in c/c-typeck.c like

> build_unary_op remember int_operands, remove_c_maybe_const_expr

> and at the end note_integer_operands.

> 

> If Joseph thinks it is ok to have C_MAYBE_CONST_EXPR inside of

> VCE, then the patch looks good to me.


There are specific cases when a C_MAYBE_CONST_EXPR mustn't appear inside 
another expression: any case where the inner expression is required to be 
fully folded (this implies nested C_MAYBE_CONST_EXPR aren't allowed) and 
any case where the expression might appear (possibly unevaluated) in an 
integer constant expression (any C_MAYBE_CONST_EXPR noting that needs to 
be at top level).

If the expressions involved here can never appear in an integer constant 
expression and do not need to be fully folded, I think it's OK to have 
C_MAYBE_CONST_EXPR inside VIEW_CONVERT_EXPR.

-- 
Joseph S. Myers
joseph@codesourcery.com
Richard Biener July 29, 2021, 6:12 a.m. | #4
On Wed, 28 Jul 2021, Joseph Myers wrote:

> On Wed, 21 Jul 2021, Jakub Jelinek via Gcc-patches wrote:

> 

> > I wonder if instead when trying to wrap

> > C_MAYBE_CONST_EXPR into a VIEW_CONVERT_EXPR we shouldn't be

> > removing that C_MAYBE_CONST_EXPR and perhaps adding it around the

> > VIEW_CONVERT_EXPR.  E.g. various routines in c/c-typeck.c like

> > build_unary_op remember int_operands, remove_c_maybe_const_expr

> > and at the end note_integer_operands.

> > 

> > If Joseph thinks it is ok to have C_MAYBE_CONST_EXPR inside of

> > VCE, then the patch looks good to me.

> 

> There are specific cases when a C_MAYBE_CONST_EXPR mustn't appear inside 

> another expression: any case where the inner expression is required to be 

> fully folded (this implies nested C_MAYBE_CONST_EXPR aren't allowed) and 

> any case where the expression might appear (possibly unevaluated) in an 

> integer constant expression (any C_MAYBE_CONST_EXPR noting that needs to 

> be at top level).

> 

> If the expressions involved here can never appear in an integer constant 

> expression and do not need to be fully folded, I think it's OK to have 

> C_MAYBE_CONST_EXPR inside VIEW_CONVERT_EXPR.


Since the expression in question involves GCC extensions to the C language
(vector types), it's not clear if they can be part of an integer constant
expression.  The case is sth like

typedef long V __attribute__((vector_size(16)));

const long x = ((V)(V){1+1, 2+2})[2-1];

which we reject.  In particular c_fully_fold_internal is fed

VIEW_CONVERT_EXPR<long int[2]>(VIEW_CONVERT_EXPR<vector(2) long int>(<<< 
Unknown tree: compound_literal_expr
    static V __compound_literal.0 = { 2, 4 }; >>>))[3]

in this case (no C_MAYBE_CONST_EXPR here), but we are not even turning
the COMPOUND_LITERAL_EXPR into a VECTOR_CST.

So I conclude that indeed the expressions involved here can never
appear in an integer constant expression.

Pushed to trunk.

Richard.

Patch

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index aacdfb46a02..21da679cd3c 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -6894,10 +6894,13 @@  complete_flexible_array_elts (tree init)
 void 
 c_common_mark_addressable_vec (tree t)
 {   
-  if (TREE_CODE (t) == C_MAYBE_CONST_EXPR)
-    t = C_MAYBE_CONST_EXPR_EXPR (t);
-  while (handled_component_p (t))
-    t = TREE_OPERAND (t, 0);
+  while (handled_component_p (t) || TREE_CODE (t) == C_MAYBE_CONST_EXPR)
+    {
+      if (TREE_CODE (t) == C_MAYBE_CONST_EXPR)
+	t = C_MAYBE_CONST_EXPR_EXPR (t);
+      else
+	t = TREE_OPERAND (t, 0);
+    }
   if (!VAR_P (t)
       && TREE_CODE (t) != PARM_DECL
       && TREE_CODE (t) != COMPOUND_LITERAL_EXPR)
diff --git a/gcc/testsuite/gcc.dg/torture/pr101512.c b/gcc/testsuite/gcc.dg/torture/pr101512.c
new file mode 100644
index 00000000000..a25da2aa0b5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr101512.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-w -Wno-psabi" } */
+
+int n();
+typedef unsigned long V __attribute__ ((vector_size (64)));
+V
+foo (int i, V v)
+{
+  i = ((V)(V){n()})[n()];
+  return v + i;
+}