Re: [PING] [C PATCH] Fix ICEs related to VM types in C [PR106465, PR107557, PR108423, PR109450]

[Joseph Myers]

On Thu, 18 May 2023, Martin Uecker via Gcc-patches wrote:

> +  /* we still have to evaluate size expressions */

Comments should start with a capital letter and end with ".  ".

> diff --git a/gcc/testsuite/gcc.dg/nested-vla-1.c 
> b/gcc/testsuite/gcc.dg/nested-vla-1.c
> new file mode 100644
> index 000..408a68524d8
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/nested-vla-1.c
> @@ -0,0 +1,37 @@
> +/* { dg-do run } */
> +/* { dg-options "-std=gnu99" } */

I'm concerned with various undefined behavior in this and other tests; 
they look very fragile, relying on some optimizations and not others 
taking place.  I think they should be adjusted to avoid undefined behavior 
if all the evaluations from the abstract machine (in particular, of sizeof 
operands with variable size) take place, and other undefined behavior from 
calling functions through function pointers with incompatible type.

> + struct bar { char x[++n]; } (*bar2)(void) = bar;/* { dg-warning 
> "incompatible pointer type" } */
> +
> + if (2 != n)
> + __builtin_abort();
> +
> + if (2 != sizeof((*bar2)()))
> + __builtin_abort();

You're relying on the compiler not noticing that a function is being 
called through an incompatible type and thus not turning the call (which 
should be evaluated, because the operand of sizeof has a type with 
variable size) into a call to abort.

> diff --git a/gcc/testsuite/gcc.dg/nested-vla-2.c 
> b/gcc/testsuite/gcc.dg/nested-vla-2.c
> new file mode 100644
> index 000..504eec48c80
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/nested-vla-2.c
> @@ -0,0 +1,33 @@
> +/* { dg-do run } */
> +/* { dg-options "-std=gnu99" } */
> +
> +
> +int main()
> +{
> + int n = 1;
> +
> + typeof(char (*)[++n]) bar(void) { }
> +
> + if (2 != n)
> + __builtin_abort();
> +
> + if (2 != sizeof(*bar()))
> + __builtin_abort();

In this test, *bar() is evaluated, i.e. an undefined pointer is 
dereferenced; it would be better to return a valid pointer to a 
sufficiently large array to avoid that undefined behavior.

> diff --git a/gcc/testsuite/gcc.dg/pr106465.c b/gcc/testsuite/gcc.dg/pr106465.c
> new file mode 100644
> index 000..b03e2442f12
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr106465.c
> @@ -0,0 +1,86 @@
> +/* PR c/106465
> + * { dg-do run }
> + * { dg-options "-std=gnu99" }
> + * */
> +
> +int main()
> +{
> + int n = 3;
> + 
> + void g1(int m, struct { char p[++m]; }* b)  /* { dg-warning 
> "anonymous struct" } */
> + {
> + if (3 != m)
> + __builtin_abort();
> +
> + if (3 != sizeof(b->p))
> + __builtin_abort();
> + }

> + g1(2, (void*)0);

Similarly, this is dereferencing a null pointer in the evaluated operand 
of sizeof.

-- 
Joseph S. Myers
jos...@codesourcery.com

[PING] [C PATCH] Fix ICEs related to VM types in C [PR106465, PR107557, PR108423, PR109450]

[Martin Uecker via Gcc-patches]

Ping. Ok, for trunk?

Bootstrapped and tested on x86_64-linux-gnu with no regressions.



Fix ICEs related to VM types in C [PR106465, PR107557, PR108423, PR109450]

Size expressions were sometimes lost and not gimplified correctly, leading 
to
ICEs and incorrect evaluation order.  Fix this by 1) not recursing into
pointers when gimplifying parameters in the middle-end (the code is merged 
with
gimplify_type_sizes), which is incorrect because it might access variables
declared later for incomplete structs, and 2) tracking size expressions for
struct/union members correctly, 3) emitting code to evaluate size 
expressions
for missing cases (nested functions, empty declarations, and 
structs/unions).

PR c/106465
PR c/107557
PR c/108423
PR c/109450

gcc/
* c/c-decl.cc (start_decl): Make sure size expression are
evaluated only in correct context.
(grokdeclarator): Size expression in fields may need a bind
expression, make sure DECL_EXPR is always created.
(grokfield, declspecs_add_type): Pass along size expressions.
(finish_struct): Remove unneeded DECL_EXPR.
(start_function): Evaluate size expressions for nested functions.
* c/c-parser.cc (c_parser_struct_declarations,
c_parser_struct_or_union_specifier): Pass along size expressions.
(c_parser_declaration_or_fndef): Evaluate size expression.
(c_parser_objc_at_property_declaration,
c_parser_objc_class_instance_variables): Adapt.
* function.cc (gimplify_parm_type): Remove function.
(gimplify_parameters): Call gimplify_parm_sizes.
* gimplify.cc (gimplify_type_sizes): Make function static.
(gimplify_parm_sizes): New function.

gcc/testsuite/
* gcc.dg/nested-vla-1.c: New test.
* gcc.dg/nested-vla-2.c: New test.
* gcc.dg/nested-vla-3.c: New test.
* gcc.dg/pr106465.c: New test.
* gcc.dg/pr107557-1.c: New test.
* gcc.dg/pr107557-2.c: New test.
* gcc.dg/pr108423-1.c: New test.
* gcc.dg/pr108423-2.c: New test.
* gcc.dg/pr108423-3.c: New test.
* gcc.dg/pr108423-4.c: New test.
* gcc.dg/pr108423-5.c: New test.
* gcc.dg/pr108423-6.c: New test.
* gcc.dg/pr109450-1.c: New test.
* gcc.dg/pr109450-2.c: New test.
* gcc.dg/typename-vla-2.c: New test.
* gcc.dg/typename-vla-3.c: New test.
* gcc.dg/typename-vla-4.c: New test.
* gcc.misc-tests/gcov-pr85350.c: Adapt.

diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index 90d7cd27cd5..f63c1108ab5 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -5378,7 +5378,8 @@ start_decl (struct c_declarator *declarator, struct 
c_declspecs *declspecs,
 if (lastdecl != error_mark_node)
   *lastloc = DECL_SOURCE_LOCATION (lastdecl);
 
-  if (expr)
+  /* Make sure the size expression is evaluated at this point.  */
+  if (expr && !current_scope->parm_flag)
 add_stmt (fold_convert (void_type_node, expr));
 
   if (TREE_CODE (decl) != FUNCTION_DECL && MAIN_NAME_P (DECL_NAME (decl))
@@ -7510,7 +7511,8 @@ grokdeclarator (const struct c_declarator *declarator,
&& c_type_variably_modified_p (type))
  {
tree bind = NULL_TREE;
-   if (decl_context == TYPENAME || decl_context == PARM)
+   if (decl_context == TYPENAME || decl_context == PARM
+   || decl_context == FIELD)
  {
bind = build3 (BIND_EXPR, void_type_node, NULL_TREE,
   NULL_TREE, NULL_TREE);
@@ -7519,10 +7521,11 @@ grokdeclarator (const struct c_declarator *declarator,
push_scope ();
  }
tree decl = build_decl (loc, TYPE_DECL, NULL_TREE, type);
-   DECL_ARTIFICIAL (decl) = 1;
pushdecl (decl);
-   finish_decl (decl, loc, NULL_TREE, NULL_TREE, NULL_TREE);
+   DECL_ARTIFICIAL (decl) = 1;
+   add_stmt (build_stmt (DECL_SOURCE_LOCATION (decl), DECL_EXPR, 
decl));
TYPE_NAME (type) = decl;
+
if (bind)
  {
pop_scope ();
@@ -8721,7 +8724,7 @@ start_struct (location_t loc, enum tree_code code, tree 
name,
 tree
 grokfield (location_t loc,
   struct c_declarator *declarator, struct c_declspecs *declspecs,
-  tree width, tree *decl_attrs)
+  tree width, tree *decl_attrs, tree *expr)
 {
   tree value;
 
@@ -8778,7 +8781,7 @@ grokfield (location_t loc,
 }
 
   value = grokdeclarator (declarator, declspecs, FIELD, false,
- width ?  : NULL, decl_attrs, NULL, NULL,
+ width ?