Re: [V1][PATCH 2/3] Use the element_count atribute info in builtin object size [PR108896].
Thank you for working on this!
Here are a couple of comments:
How is the size for an object with FAM defined?
There are at least three possible choices:
offset(..) + N * sizeof
sizeof(..) + N * sizeof
or the size of a struct with the replacement array.
Or is this not relevant here?
I would personally prefer an attribute which does
not use a string, but uses C expressions, so that
one could write something like this (although I would
limit it initially to the most simple case)
struct {
struct bar { int n; }* ptr;
int buf[] [[element_count(.ptr->n + 3)]];
};
Of course, we could still support this later even
if we use a string now.
Martin
Am Donnerstag, dem 25.05.2023 um 16:14 + schrieb Qing Zhao:
> 2023-05-17 Qing Zhao
>
> gcc/ChangeLog:
>
> PR C/108896
> * tree-object-size.cc (addr_object_size): Use the element_count
> attribute info.
> * tree.cc (component_ref_has_element_count_p): New function.
> (component_ref_get_element_count): New function.
> * tree.h (component_ref_has_element_count_p): New prototype.
> (component_ref_get_element_count): New prototype.
>
> gcc/testsuite/ChangeLog:
>
> PR C/108896
> * gcc.dg/flex-array-element-count-2.c: New test.
> ---
> .../gcc.dg/flex-array-element-count-2.c | 56 +++
> gcc/tree-object-size.cc | 37 ++--
> gcc/tree.cc | 93 +++
> gcc/tree.h| 10 ++
> 4 files changed, 189 insertions(+), 7 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/flex-array-element-count-2.c
>
> diff --git a/gcc/testsuite/gcc.dg/flex-array-element-count-2.c
> b/gcc/testsuite/gcc.dg/flex-array-element-count-2.c
> new file mode 100644
> index 000..5a280e8c731
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/flex-array-element-count-2.c
> @@ -0,0 +1,56 @@
> +/* test the attribute element_count and its usage in
> + * __builtin_dynamic_object_size. */
> +/* { dg-do run } */
> +/* { dg-options "-O2" } */
> +
> +#include "builtin-object-size-common.h"
> +
> +#define expect(p, _v) do { \
> +size_t v = _v; \
> +if (p == v) \
> + __builtin_printf ("ok: %s == %zd\n", #p, p); \
> +else \
> + { \
> + __builtin_printf ("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
> + FAIL (); \
> + } \
> +} while (0);
> +
> +struct flex {
> + int b;
> + int c[];
> +} *array_flex;
> +
> +struct annotated {
> + int b;
> + int c[] __attribute__ ((element_count ("b")));
> +} *array_annotated;
> +
> +void __attribute__((__noinline__)) setup (int normal_count, int attr_count)
> +{
> + array_flex
> += (struct flex *)malloc (sizeof (struct flex)
> ++ normal_count * sizeof (int));
> + array_flex->b = normal_count;
> +
> + array_annotated
> += (struct annotated *)malloc (sizeof (struct annotated)
> + + attr_count * sizeof (int));
> + array_annotated->b = attr_count;
> +
> + return;
> +}
> +
> +void __attribute__((__noinline__)) test ()
> +{
> +expect(__builtin_dynamic_object_size(array_flex->c, 1), -1);
> +expect(__builtin_dynamic_object_size(array_annotated->c, 1),
> +array_annotated->b * sizeof (int));
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + setup (10,10);
> + test ();
> + DONE ();
> +}
> diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
> index 9a936a91983..f9aadd59054 100644
> --- a/gcc/tree-object-size.cc
> +++ b/gcc/tree-object-size.cc
> @@ -585,6 +585,7 @@ addr_object_size (struct object_size_info *osi,
> const_tree ptr,
> if (pt_var != TREE_OPERAND (ptr, 0))
> {
> tree var;
> + tree element_count_ref = NULL_TREE;
>
>
> if (object_size_type & OST_SUBOBJECT)
> {
> @@ -600,11 +601,12 @@ addr_object_size (struct object_size_info *osi,
> const_tree ptr,
> var = TREE_OPERAND (var, 0);
> if (var != pt_var && TREE_CODE (var) == ARRAY_REF)
> var = TREE_OPERAND (var, 0);
> - if (! TYPE_SIZE_UNIT (TREE_TYPE (var))
> + if (! component_ref_has_element_count_p (var)
> + && ((! TYPE_SIZE_UNIT (TREE_TYPE (var))
> || ! tree_fits_uhwi_p (TYPE_SIZE_UNIT (TREE_TYPE (var)))
> || (pt_var_size && TREE_CODE (pt_var_size) == INTEGER_CST
> && tree_int_cst_lt (pt_var_size,
> - TYPE_SIZE_UNIT (TREE_TYPE (var)
> + TYPE_SIZE_UNIT (TREE_TYPE (var)))
> var = pt_var;
> else if (var != pt_var && TREE_CODE (pt_var) == MEM_REF)
> {
> @@ -612,6 +614,7 @@ addr_object_size (struct object_size_info *osi,
> const_tree ptr,
> /* For >fld, compute object size if fld isn't a flexible array
> member. */
> bool is_flexible_array_mem_ref = false;
> +
> while (v && v != pt_var)
>
[C PATCH] -Wstringop-overflow for parameters with forward-declared sizes
This is a minor change so that parameter that have
forward declarations for with -Wstringop-overflow.
Bootstrapped and regression tested on x86_64.
c: -Wstringop-overflow for parameters with forward-declared sizes
Warnings from -Wstringop-overflow do not appear for parameters declared
as VLAs when the bound refers to a parameter forward declaration. This
is fixed by splitting the loop that passes through parameters into two,
first only recording the positions of all possible size expressions
and then processing the parameters.
PR c/109970
gcc/c-family:
* c-attribs.cc (build_attr_access_from_parms): Split loop to first
record all parameters.
gcc/testsuite:
* gcc.dg/pr109970.c: New test.
diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
index 072cfb69147..e2792ca6898 100644
--- a/gcc/c-family/c-attribs.cc
+++ b/gcc/c-family/c-attribs.cc
@@ -5278,6 +5278,15 @@ build_attr_access_from_parms (tree parms, bool
skip_voidptr)
tree argtype = TREE_TYPE (arg);
if (DECL_NAME (arg) && INTEGRAL_TYPE_P (argtype))
arg2pos.put (arg, argpos);
+}
+
+ argpos = 0;
+ for (tree arg = parms; arg; arg = TREE_CHAIN (arg), ++argpos)
+{
+ if (!DECL_P (arg))
+ continue;
+
+ tree argtype = TREE_TYPE (arg);
tree argspec = DECL_ATTRIBUTES (arg);
if (!argspec)
diff --git a/gcc/testsuite/gcc.dg/pr109970.c b/gcc/testsuite/gcc.dg/pr109970.c
new file mode 100644
index 000..d234e10455f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr109970.c
@@ -0,0 +1,15 @@
+/* PR109970
+ * { dg-do compile }
+ * { dg-options "-Wstringop-overflow" }
+ * */
+
+void bar(int x, char buf[x]);
+void foo(int x; char buf[x], int x);
+
+int main()
+{
+ char buf[10];
+ bar(11, buf); /* { dg-warning "accessing 11 bytes in a region of size
10" } */
+ foo(buf, 11); /* { dg-warning "accessing 11 bytes in a region of size
10" } */
+}
+
Re: [committed] testsuite: Require trampolines for nestev-vla tests
Thanks! I will try to not forget this next time.
Am Donnerstag, dem 25.05.2023 um 21:20 +0300 schrieb Dimitar Dimitrov:
> Three recent test cases declare nested C functions, so they fail on
> targets lacking support for trampolines. Fix by adding the necessary
> filter.
>
> Committed as obvious.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/nested-vla-1.c: Require effective target trampolines.
> * gcc.dg/nested-vla-2.c: Ditto.
> * gcc.dg/nested-vla-3.c: Ditto.
>
> CC: Martin Uecker
> Signed-off-by: Dimitar Dimitrov
> ---
> gcc/testsuite/gcc.dg/nested-vla-1.c | 1 +
> gcc/testsuite/gcc.dg/nested-vla-2.c | 1 +
> gcc/testsuite/gcc.dg/nested-vla-3.c | 1 +
> 3 files changed, 3 insertions(+)
>
> diff --git a/gcc/testsuite/gcc.dg/nested-vla-1.c
> b/gcc/testsuite/gcc.dg/nested-vla-1.c
> index 5b62c2c213a..d1b3dc3c5f8 100644
> --- a/gcc/testsuite/gcc.dg/nested-vla-1.c
> +++ b/gcc/testsuite/gcc.dg/nested-vla-1.c
> @@ -1,5 +1,6 @@
> /* { dg-do run } */
> /* { dg-options "-std=gnu99" } */
> +/* { dg-require-effective-target trampolines } */
>
>
>
>
> int main()
> diff --git a/gcc/testsuite/gcc.dg/nested-vla-2.c
> b/gcc/testsuite/gcc.dg/nested-vla-2.c
> index d83c90a0b16..294b01d370e 100644
> --- a/gcc/testsuite/gcc.dg/nested-vla-2.c
> +++ b/gcc/testsuite/gcc.dg/nested-vla-2.c
> @@ -1,5 +1,6 @@
> /* { dg-do run } */
> /* { dg-options "-std=gnu99" } */
> +/* { dg-require-effective-target trampolines } */
>
>
>
>
> int main()
> diff --git a/gcc/testsuite/gcc.dg/nested-vla-3.c
> b/gcc/testsuite/gcc.dg/nested-vla-3.c
> index 1ffb482da3b..d2ba04adab8 100644
> --- a/gcc/testsuite/gcc.dg/nested-vla-3.c
> +++ b/gcc/testsuite/gcc.dg/nested-vla-3.c
> @@ -1,5 +1,6 @@
> /* { dg-do run } */
> /* { dg-options "-std=gnu99" } */
> +/* { dg-require-effective-target trampolines } */
>
>
>
>
> int main()
Re: [C PATCH v3] Fix ICEs related to VM types in C 2/2
Am Dienstag, dem 23.05.2023 um 10:18 +0200 schrieb Richard Biener: > On Tue, May 23, 2023 at 8:24 AM Martin Uecker > wrote: > > > > Am Dienstag, dem 23.05.2023 um 08:13 +0200 schrieb Richard Biener: > > > On Mon, May 22, 2023 at 7:24 PM Martin Uecker via Gcc-patches > > > wrote: > > > > > > > > > > > > > > > > This version contains the middle-end changes for PR109450 > > > > and test cases as before. The main middle-end change is that > > > > we use gimplify_type_sizes also for parameters and remove > > > > the special code that also walked into pointers (which is > > > > incorrect). > > > > > > > > In addition, in the C FE this patch now also adds DECL_EXPR > > > > for vm-types which are pointed-to by parameters declared > > > > as arrays. The new function created contains the exact > > > > code previously used only for regular pointers, and is > > > > now also called for parameters declared as arrays. > > > > > > > > > > > > Martin > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fix ICEs related to VM types in C 2/2 [PR109450] > > > > > > > > Size expressions were sometimes lost and not gimplified > > > > correctly, > > > > leading to ICEs and incorrect evaluation order. Fix this > > > > by 1) not > > > > recursing 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) adding a decl expr for variably-modified > > > > arrays > > > > that are pointed to by parameters declared as arrays. > > > > > > > > PR c/109450 > > > > > > > > gcc/ > > > > * c/c-decl.cc (add_decl_expr): New function. > > > > (grokdeclarator): Add decl expr for size expression > > > > in > > > > types pointed to by parameters declared as arrays. > > > > * 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/pr109450-1.c: New test. > > > > * gcc.dg/pr109450-2.c: New test. > > > > * gcc.dg/vla-26.c: New test. > > > > > > > > diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc > > > > index 494d3cf1747..c35347734b2 100644 > > > > --- a/gcc/c/c-decl.cc > > > > +++ b/gcc/c/c-decl.cc > > > > @@ -6490,6 +6490,55 @@ smallest_type_quals_location (const > > > > location_t *locations, > > > > return loc; > > > > } > > > > > > > > + > > > > +/* We attach an artificial TYPE_DECL to pointed-to type > > > > + and arrange for it to be included in a DECL_EXPR. This > > > > + forces the sizes evaluation at a safe point and ensures it > > > > + is not deferred until e.g. within a deeper conditional > > > > context. > > > > + > > > > + PARM contexts have no enclosing statement list that > > > > + can hold the DECL_EXPR, so we need to use a BIND_EXPR > > > > + instead, and add it to the list of expressions that > > > > + need to be evaluated. > > > > + > > > > + TYPENAME contexts do have an enclosing statement list, > > > > + but it would be incorrect to use it, as the size should > > > > + only be evaluated if the containing expression is > > > > + evaluated. We might also be in the middle of an > > > > + expression with side effects on the pointed-to type size > > > > + "arguments" prior to the pointer declaration point and > > > > + the fake TYPE_DECL in the enclosing context would force > > > > + the size evaluation prior to the side effects. We > > > > therefore > > &
Re: [C PATCH v3] Fix ICEs related to VM types in C 2/2
Am Dienstag, dem 23.05.2023 um 08:13 +0200 schrieb Richard Biener:
> On Mon, May 22, 2023 at 7:24 PM Martin Uecker via Gcc-patches
> wrote:
> >
> >
> >
> > This version contains the middle-end changes for PR109450
> > and test cases as before. The main middle-end change is that
> > we use gimplify_type_sizes also for parameters and remove
> > the special code that also walked into pointers (which is
> > incorrect).
> >
> > In addition, in the C FE this patch now also adds DECL_EXPR
> > for vm-types which are pointed-to by parameters declared
> > as arrays. The new function created contains the exact
> > code previously used only for regular pointers, and is
> > now also called for parameters declared as arrays.
> >
> >
> > Martin
> >
> >
> >
> >
> >
> >
> >
> > Fix ICEs related to VM types in C 2/2 [PR109450]
> >
> > Size expressions were sometimes lost and not gimplified correctly,
> > leading to ICEs and incorrect evaluation order. Fix this by 1) not
> > recursing 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) adding a decl expr for variably-modified arrays
> > that are pointed to by parameters declared as arrays.
> >
> > PR c/109450
> >
> > gcc/
> > * c/c-decl.cc (add_decl_expr): New function.
> > (grokdeclarator): Add decl expr for size expression in
> > types pointed to by parameters declared as arrays.
> > * 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/pr109450-1.c: New test.
> > * gcc.dg/pr109450-2.c: New test.
> > * gcc.dg/vla-26.c: New test.
> >
> > diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
> > index 494d3cf1747..c35347734b2 100644
> > --- a/gcc/c/c-decl.cc
> > +++ b/gcc/c/c-decl.cc
> > @@ -6490,6 +6490,55 @@ smallest_type_quals_location (const location_t
> > *locations,
> > return loc;
> > }
> >
> > +
> > +/* We attach an artificial TYPE_DECL to pointed-to type
> > + and arrange for it to be included in a DECL_EXPR. This
> > + forces the sizes evaluation at a safe point and ensures it
> > + is not deferred until e.g. within a deeper conditional context.
> > +
> > + PARM contexts have no enclosing statement list that
> > + can hold the DECL_EXPR, so we need to use a BIND_EXPR
> > + instead, and add it to the list of expressions that
> > + need to be evaluated.
> > +
> > + TYPENAME contexts do have an enclosing statement list,
> > + but it would be incorrect to use it, as the size should
> > + only be evaluated if the containing expression is
> > + evaluated. We might also be in the middle of an
> > + expression with side effects on the pointed-to type size
> > + "arguments" prior to the pointer declaration point and
> > + the fake TYPE_DECL in the enclosing context would force
> > + the size evaluation prior to the side effects. We therefore
> > + use BIND_EXPRs in TYPENAME contexts too. */
> > +static void
> > +add_decl_expr(location_t loc, enum decl_context decl_context, tree type,
> > tree *expr)
> > +{
> > + tree bind = NULL_TREE;
> > + if (decl_context == TYPENAME || decl_context == PARM || decl_context ==
> > FIELD)
> > +{
> > + bind = build3 (BIND_EXPR, void_type_node, NULL_TREE, NULL_TREE,
> > NULL_TREE);
> > + TREE_SIDE_EFFECTS (bind) = 1;
> > + BIND_EXPR_BODY (bind) = push_stmt_list ();
> > + push_scope ();
> > +}
> > +
> > + tree decl = build_decl (loc, TYPE_DECL, NULL_TREE, type);
> > + pushdecl (decl);
> > + DECL_ARTIFICIAL (decl) = 1;
> > + add_stmt (build_stmt (DECL_SOURCE_LOCATION (decl), DECL_EXPR, decl));
> > + TYPE_NAME (type) = decl;
> > +
> > + if (bind)
> > +{
> > + pop_scope ();
> > + BIND_EXPR_BODY (bind) = pop_stmt_list (BIND_EXPR_BODY (bind));
> > + if (*expr)
> > + *expr = build2 (COMPOUND_EXPR, void_type_node, *expr, bind);
> > + else
>
[C PATCH v3] Fix ICEs related to VM types in C 2/2
This version contains the middle-end changes for PR109450
and test cases as before. The main middle-end change is that
we use gimplify_type_sizes also for parameters and remove
the special code that also walked into pointers (which is
incorrect).
In addition, in the C FE this patch now also adds DECL_EXPR
for vm-types which are pointed-to by parameters declared
as arrays. The new function created contains the exact
code previously used only for regular pointers, and is
now also called for parameters declared as arrays.
Martin
Fix ICEs related to VM types in C 2/2 [PR109450]
Size expressions were sometimes lost and not gimplified correctly,
leading to ICEs and incorrect evaluation order. Fix this by 1) not
recursing 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) adding a decl expr for variably-modified arrays
that are pointed to by parameters declared as arrays.
PR c/109450
gcc/
* c/c-decl.cc (add_decl_expr): New function.
(grokdeclarator): Add decl expr for size expression in
types pointed to by parameters declared as arrays.
* 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/pr109450-1.c: New test.
* gcc.dg/pr109450-2.c: New test.
* gcc.dg/vla-26.c: New test.
diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index 494d3cf1747..c35347734b2 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -6490,6 +6490,55 @@ smallest_type_quals_location (const location_t
*locations,
return loc;
}
+
+/* We attach an artificial TYPE_DECL to pointed-to type
+ and arrange for it to be included in a DECL_EXPR. This
+ forces the sizes evaluation at a safe point and ensures it
+ is not deferred until e.g. within a deeper conditional context.
+
+ PARM contexts have no enclosing statement list that
+ can hold the DECL_EXPR, so we need to use a BIND_EXPR
+ instead, and add it to the list of expressions that
+ need to be evaluated.
+
+ TYPENAME contexts do have an enclosing statement list,
+ but it would be incorrect to use it, as the size should
+ only be evaluated if the containing expression is
+ evaluated. We might also be in the middle of an
+ expression with side effects on the pointed-to type size
+ "arguments" prior to the pointer declaration point and
+ the fake TYPE_DECL in the enclosing context would force
+ the size evaluation prior to the side effects. We therefore
+ use BIND_EXPRs in TYPENAME contexts too. */
+static void
+add_decl_expr(location_t loc, enum decl_context decl_context, tree type, tree
*expr)
+{
+ tree bind = NULL_TREE;
+ if (decl_context == TYPENAME || decl_context == PARM || decl_context ==
FIELD)
+{
+ bind = build3 (BIND_EXPR, void_type_node, NULL_TREE, NULL_TREE,
NULL_TREE);
+ TREE_SIDE_EFFECTS (bind) = 1;
+ BIND_EXPR_BODY (bind) = push_stmt_list ();
+ push_scope ();
+}
+
+ tree decl = build_decl (loc, TYPE_DECL, NULL_TREE, type);
+ pushdecl (decl);
+ DECL_ARTIFICIAL (decl) = 1;
+ add_stmt (build_stmt (DECL_SOURCE_LOCATION (decl), DECL_EXPR, decl));
+ TYPE_NAME (type) = decl;
+
+ if (bind)
+{
+ pop_scope ();
+ BIND_EXPR_BODY (bind) = pop_stmt_list (BIND_EXPR_BODY (bind));
+ if (*expr)
+ *expr = build2 (COMPOUND_EXPR, void_type_node, *expr, bind);
+ else
+ *expr = bind;
+}
+}
+
/* Given declspecs and a declarator,
determine the name and type of the object declared
and construct a ..._DECL node for it.
@@ -7474,58 +7523,9 @@ grokdeclarator (const struct c_declarator *declarator,
This is expected to happen automatically when the pointed-to
type has a name/declaration of it's own, but special attention
- is required if the type is anonymous.
-
- We attach an artificial TYPE_DECL to such pointed-to type
- and arrange for it to be included in a DECL_EXPR. This
- forces the sizes evaluation at a safe point and ensures it
- is not deferred until e.g. within a deeper conditional context.
-
- PARM contexts have no enclosing statement list that
- can hold the DECL_EXPR, so we need to use a BIND_EXPR
- instead, and add it to the list of expressions that
- need to be evaluated.
-
- TYPENAME contexts do have an enclosing statement list,
- but it would be incorrect to use it, as the size should
- only be evaluated if the containing expression is
-
[C PATCH v3] Fix ICEs related to VM types in C 1/2
Hi Joseph,
I had to create another revision of the patch because of some
case I had overlooked (vm-types pointed-to by parameters
declared as I arrays). I splitted the patch up in two parts
for easier reviewing. The first part only has the FE changes
and most of the tests. The only minor change is that I replaced
a bug number because there was a duplicate and that I added
the specific example from this bug report (PR70418) as another
test. Since this half of the patch was already reviewed, I will
commit it tomorrow if there are no comments.
The second part contains the middle-end changes and the new
FE fix you haven't seen.
Both patches bootstrapped and regression tested on x86.
Martin
Fix ICEs related to VM types in C 1/2 [PR70418, PR107557, PR108423]
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/70418
PR c/106465
PR c/107557
PR c/108423
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.
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/pr70418.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/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 94ce760b55e..494d3cf1747 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -5364,7 +5364,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))
@@ -7498,7 +7499,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);
@@ -7507,10 +7509,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 ();
@@ -8709,7 +8712,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
[C PATCH] Remove dead code related to type compatibility across TUs.
Repost for stage 1.
C: Remove dead code related to type compatibility across TUs.
Code to detect struct/unions across the same TU is not needed
anymore. Code for determining compatibility of tagged types is
preserved as it will be used for C2X. Some errors in the unused
code are fixed.
Bootstrapped with no regressions for x86_64-pc-linux-gnu.
gcc/c/
* c-decl.cc (set_type_context): Remove.
(pop_scope, diagnose_mismatched_decls, pushdecl):
Remove dead code.
* c-typeck.cc (comptypes_internal): Remove dead code.
(same_translation_unit_p): Remove.
(tagged_types_tu_compatible_p): Some fixes.
diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index f63c1108ab5..70345b4b019 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -1155,16 +1155,6 @@ update_label_decls (struct c_scope *scope)
}
}
-/* Set the TYPE_CONTEXT of all of TYPE's variants to CONTEXT. */
-
-static void
-set_type_context (tree type, tree context)
-{
- for (type = TYPE_MAIN_VARIANT (type); type;
- type = TYPE_NEXT_VARIANT (type))
-TYPE_CONTEXT (type) = context;
-}
-
/* Exit a scope. Restore the state of the identifier-decl mappings
that were in effect when this scope was entered. Return a BLOCK
node containing all the DECLs in this scope that are of interest
@@ -1253,7 +1243,6 @@ pop_scope (void)
case ENUMERAL_TYPE:
case UNION_TYPE:
case RECORD_TYPE:
- set_type_context (p, context);
/* Types may not have tag-names, in which case the type
appears in the bindings list with b->id NULL. */
@@ -1364,12 +1353,7 @@ pop_scope (void)
the TRANSLATION_UNIT_DECL. This makes same_translation_unit_p
work. */
if (scope == file_scope)
- {
DECL_CONTEXT (p) = context;
- if (TREE_CODE (p) == TYPE_DECL
- && TREE_TYPE (p) != error_mark_node)
- set_type_context (TREE_TYPE (p), context);
- }
gcc_fallthrough ();
/* Parameters go in DECL_ARGUMENTS, not BLOCK_VARS, and have
@@ -2318,21 +2302,18 @@ diagnose_mismatched_decls (tree newdecl, tree olddecl,
{
if (DECL_INITIAL (olddecl))
{
- /* If both decls are in the same TU and the new declaration
-isn't overriding an extern inline reject the new decl.
-In c99, no overriding is allowed in the same translation
-unit. */
- if ((!DECL_EXTERN_INLINE (olddecl)
- || DECL_EXTERN_INLINE (newdecl)
- || (!flag_gnu89_inline
- && (!DECL_DECLARED_INLINE_P (olddecl)
- || !lookup_attribute ("gnu_inline",
-DECL_ATTRIBUTES (olddecl)))
- && (!DECL_DECLARED_INLINE_P (newdecl)
- || !lookup_attribute ("gnu_inline",
-DECL_ATTRIBUTES (newdecl
- )
- && same_translation_unit_p (newdecl, olddecl))
+ /* If the new declaration isn't overriding an extern inline
+reject the new decl. In c99, no overriding is allowed
+in the same translation unit. */
+ if (!DECL_EXTERN_INLINE (olddecl)
+ || DECL_EXTERN_INLINE (newdecl)
+ || (!flag_gnu89_inline
+ && (!DECL_DECLARED_INLINE_P (olddecl)
+ || !lookup_attribute ("gnu_inline",
+ DECL_ATTRIBUTES (olddecl)))
+ && (!DECL_DECLARED_INLINE_P (newdecl)
+ || !lookup_attribute ("gnu_inline",
+ DECL_ATTRIBUTES (newdecl)
{
auto_diagnostic_group d;
error ("redefinition of %q+D", newdecl);
@@ -3360,18 +3341,11 @@ pushdecl (tree x)
type to the composite of all the types of that declaration.
After the consistency checks, it will be reset to the
composite of the visible types only. */
- if (b && (TREE_PUBLIC (x) || same_translation_unit_p (x, b->decl))
- && b->u.type)
+ if (b && b->u.type)
TREE_TYPE (b->decl) = b->u.type;
- /* The point of the same_translation_unit_p check here is,
-we want to detect a duplicate decl for a construct like
-foo() { extern bar(); } ... static bar(); but not if
-they are in different translation units. In any case,
-the static does not go in the externals scope. */
- if (b
- && (TREE_PUBLIC (x) || same_translation_unit_p (x, b->decl))
- && duplicate_decls (x, b->decl))
+ /* the static does not go in the externals scope. */
+ if (b && duplicate_decls (x, b->decl))
[C PATCH v2] Fix ICEs related to VM types in C [PR106465, PR107557, PR108423, PR109450]
Thanks Joseph!
Revised version attached. Ok?
But I wonder whether we generally need to do something
about
sizeof *x
when x is NULL or not initialized. This is quite commonly
used in C code and if the type is not of variable size,
it is also unproblematic. So the UB for variable size is
unfortunate and certainly also affects existing code in
the wild. In practice it does not seem to cause
problems because there is no lvalue conversion and this
then seems to work. Maybe we document this as an
extension? (and make sure in the C FE that it
works) This would also make this idiom valid:
char (*buf)[n] = malloc(sizeof *buf);
Or if we do not want to do this, then I think we should
add some warnings (and UBSan check for null pointer)
which currently do not exist:
https://godbolt.org/z/fhWMKvYc8
Martin
Am Donnerstag, dem 18.05.2023 um 21:46 + schrieb 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.
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 && MA
[PING] [C PATCH] Fix ICEs related to VM types in C [PR106465, PR107557, PR108423, PR109450]
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 ?
[committed, gcc 12] Fix ICE related to implicit access attributes for VLA arguments [PR105660]
Bootstrapped and regression tested on x86-64.
Fix ICE related to implicit access attributes for VLA arguments [PR105660]
When constructing the specifier string when merging an access attribute
that encodes information about VLA arguments, the string was constructed
in random order by iterating through a hash table. Fix this by iterating
though the list of arguments.
PR c/105660
gcc/Changelog
* c-family/c-attribs.cc (append_access_attr): Use order
of arguments when construction string.
(append_access_attr_idxs): Rename and make static.
* c-familty/c-warn.cc (warn_parm_array_mismatch): Add assertion.
gcc/testsuite/ChangeLog:
* gcc.dg/pr105660-1.c: New test.
* gcc.dg/pr105660-2.c: New test.
diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
index 4667f6de311..072cfb69147 100644
--- a/gcc/c-family/c-attribs.cc
+++ b/gcc/c-family/c-attribs.cc
@@ -4728,22 +4728,27 @@ append_access_attr (tree node[3], tree attrs, const
char *attrstr,
rdwr_map cur_idxs;
init_attr_rdwr_indices (_idxs, attrs);
+ tree args = TYPE_ARG_TYPES (node[0]);
+ int argpos = 0;
std::string spec;
- for (auto it = new_idxs.begin (); it != new_idxs.end (); ++it)
+ for (tree arg = args; arg; arg = TREE_CHAIN (arg), argpos++)
{
- const auto = *it;
+ const attr_access* const newa = new_idxs.get (argpos);
+
+ if (!newa)
+ continue;
/* The map has two equal entries for each pointer argument that
has an associated size argument. Process just the entry for
the former. */
- if ((unsigned)newaxsref.first != newaxsref.second.ptrarg)
+ if ((unsigned)argpos != newa->ptrarg)
continue;
- const attr_access* const cura = cur_idxs.get (newaxsref.first);
+ const attr_access* const cura = cur_idxs.get (argpos);
if (!cura)
{
/* The new attribute needs to be added. */
- tree str = newaxsref.second.to_internal_string ();
+ tree str = newa->to_internal_string ();
spec += TREE_STRING_POINTER (str);
continue;
}
@@ -4751,7 +4756,6 @@ append_access_attr (tree node[3], tree attrs, const char
*attrstr,
/* The new access spec refers to an array/pointer argument for
which an access spec already exists. Check and diagnose any
conflicts. If no conflicts are found, merge the two. */
- const attr_access* const newa =
if (!attrstr)
{
@@ -4886,7 +4890,7 @@ append_access_attr (tree node[3], tree attrs, const char
*attrstr,
continue;
/* Merge the CURA and NEWA. */
- attr_access merged = newaxsref.second;
+ attr_access merged = *newa;
/* VLA seen in a declaration takes precedence. */
if (cura->minsize == HOST_WIDE_INT_M1U)
@@ -4912,9 +4916,9 @@ append_access_attr (tree node[3], tree attrs, const char
*attrstr,
/* Convenience wrapper for the above. */
-tree
-append_access_attr (tree node[3], tree attrs, const char *attrstr,
- char code, HOST_WIDE_INT idxs[2])
+static tree
+append_access_attr_idxs (tree node[3], tree attrs, const char *attrstr,
+char code, HOST_WIDE_INT idxs[2])
{
char attrspec[80];
int n = sprintf (attrspec, "%c%u", code, (unsigned) idxs[0] - 1);
@@ -5204,7 +5208,7 @@ handle_access_attribute (tree node[3], tree name, tree
args, int flags,
attributes specified on previous declarations of the same type
and if not, concatenate the two. */
const char code = attr_access::mode_chars[mode];
- tree new_attrs = append_access_attr (node, attrs, attrstr, code, idxs);
+ tree new_attrs = append_access_attr_idxs (node, attrs, attrstr, code, idxs);
if (!new_attrs)
return NULL_TREE;
@@ -5217,7 +5221,7 @@ handle_access_attribute (tree node[3], tree name, tree
args, int flags,
{
/* Repeat for the previously declared type. */
attrs = TYPE_ATTRIBUTES (TREE_TYPE (node[1]));
- new_attrs = append_access_attr (node, attrs, attrstr, code, idxs);
+ new_attrs = append_access_attr_idxs (node, attrs, attrstr, code, idxs);
if (!new_attrs)
return NULL_TREE;
diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc
index 29efce3f2c0..a6fb95b1e80 100644
--- a/gcc/c-family/c-warn.cc
+++ b/gcc/c-family/c-warn.cc
@@ -3617,6 +3617,8 @@ warn_parm_array_mismatch (location_t origloc, tree
fndecl, tree newparms)
for (tree newvbl = newa->size, curvbl = cura->size; newvbl;
newvbl = TREE_CHAIN (newvbl), curvbl = TREE_CHAIN (curvbl))
{
+ gcc_assert (curvbl);
+
tree newpos = TREE_PURPOSE (newvbl);
tree curpos = TREE_PURPOSE (curvbl);
diff --git a/gcc/testsuite/gcc.dg/pr105660-1.c
b/gcc/testsuite/gcc.dg/pr105660-1.c
new file mode 100644
index 000..d4454f04c43
--- /dev/null
+++
Re: Re: GCC 12.2.1 Status Report (2023-05-02), branch frozen for release
Am Donnerstag, dem 04.05.2023 um 09:53 + schrieb Richard Biener: > On Thu, 4 May 2023, Martin Uecker wrote: > > > > > Can I please get permission for fixing this ICE? > > > > https://gcc.gnu.org/pipermail/gcc-patches/2023-April/616221.html > > Please wait until after the branch is unfrozen. When patches were > approved for trunk and they are regressions there's no further > approval needed to backport them (but of course bootstrap & testing > is required on the branches backported to). Ok, thanks - good to know! Next time I just push important fixes for regressions approved for trunk. But does this mean we need to wait for about a year to get this fix into 12? This would be a bit unfortunate for this problem I think. Martin
Re: Re: GCC 12.2.1 Status Report (2023-05-02), branch frozen for release
Can I please get permission for fixing this ICE? https://gcc.gnu.org/pipermail/gcc-patches/2023-April/616221.html Martin
PING [C PATCH - backport 12] Fix ICE related to implicit access attributes for VLA arguments [PR105660]
Ok for 12.3 ?
Am Mittwoch, dem 19.04.2023 um 18:39 +0200 schrieb Martin Uecker:
> Ok to cherrypick for 12? It is in GCC 13 and fixes an
> annoying ICE.
>
> Martin
>
>
>
> Here is a fix for PR105660.
>
> Bootstrapped and regression tested on x86-64.
>
>
> Fix ICE related to implicit access attributes for VLA arguments [PR105660]
>
>
> When constructing the specifier string when merging an access attribute
> that encodes information about VLA arguments, the string was constructed
> in random order by iterating through a hash table. Fix this by iterating
> though the list of arguments.
>
>
> PR c/105660
>
>
> gcc/Changelog
> * c-family/c-attribs.cc (append_access_attr): Use order
> of arguments when construction string.
> (append_access_attr_idxs): Rename and make static.
> * c-familty/c-warn.cc (warn_parm_array_mismatch): Add assertion.
>
>
> gcc/testsuite/ChangeLog:
> * gcc.dg/pr105660-1.c: New test.
> * gcc.dg/pr105660-2.c: New test.
>
>
> diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
> index 4667f6de311..072cfb69147 100644
> --- a/gcc/c-family/c-attribs.cc
> +++ b/gcc/c-family/c-attribs.cc
> @@ -4728,22 +4728,27 @@ append_access_attr (tree node[3], tree attrs, const
> char *attrstr,
> rdwr_map cur_idxs;
> init_attr_rdwr_indices (_idxs, attrs);
>
>
> + tree args = TYPE_ARG_TYPES (node[0]);
> + int argpos = 0;
> std::string spec;
> - for (auto it = new_idxs.begin (); it != new_idxs.end (); ++it)
> + for (tree arg = args; arg; arg = TREE_CHAIN (arg), argpos++)
> {
> - const auto = *it;
> + const attr_access* const newa = new_idxs.get (argpos);
> +
> + if (!newa)
> + continue;
>
>
> /* The map has two equal entries for each pointer argument that
> has an associated size argument. Process just the entry for
> the former. */
> - if ((unsigned)newaxsref.first != newaxsref.second.ptrarg)
> + if ((unsigned)argpos != newa->ptrarg)
> continue;
>
>
> - const attr_access* const cura = cur_idxs.get (newaxsref.first);
> + const attr_access* const cura = cur_idxs.get (argpos);
> if (!cura)
> {
> /* The new attribute needs to be added. */
> - tree str = newaxsref.second.to_internal_string ();
> + tree str = newa->to_internal_string ();
> spec += TREE_STRING_POINTER (str);
> continue;
> }
> @@ -4751,7 +4756,6 @@ append_access_attr (tree node[3], tree attrs, const
> char *attrstr,
> /* The new access spec refers to an array/pointer argument for
> which an access spec already exists. Check and diagnose any
> conflicts. If no conflicts are found, merge the two. */
> - const attr_access* const newa =
>
>
> if (!attrstr)
> {
> @@ -4886,7 +4890,7 @@ append_access_attr (tree node[3], tree attrs, const
> char *attrstr,
> continue;
>
>
> /* Merge the CURA and NEWA. */
> - attr_access merged = newaxsref.second;
> + attr_access merged = *newa;
>
>
> /* VLA seen in a declaration takes precedence. */
> if (cura->minsize == HOST_WIDE_INT_M1U)
> @@ -4912,9 +4916,9 @@ append_access_attr (tree node[3], tree attrs, const
> char *attrstr,
>
>
> /* Convenience wrapper for the above. */
>
>
> -tree
> -append_access_attr (tree node[3], tree attrs, const char *attrstr,
> - char code, HOST_WIDE_INT idxs[2])
> +static tree
> +append_access_attr_idxs (tree node[3], tree attrs, const char *attrstr,
> + char code, HOST_WIDE_INT idxs[2])
> {
> char attrspec[80];
> int n = sprintf (attrspec, "%c%u", code, (unsigned) idxs[0] - 1);
> @@ -5204,7 +5208,7 @@ handle_access_attribute (tree node[3], tree name, tree
> args, int flags,
> attributes specified on previous declarations of the same type
> and if not, concatenate the two. */
> const char code = attr_access::mode_chars[mode];
> - tree new_attrs = append_access_attr (node, attrs, attrstr, code, idxs);
> + tree new_attrs = append_access_attr_idxs (node, attrs, attrstr, code,
> idxs);
> if (!new_attrs)
> return NULL_TREE;
>
>
> @@ -5217,7 +5221,7 @@ handle_access_attribute (tree node[3], tree name, tree
> args, int flags,
> {
> /* Repeat for the previously declared type. */
> attrs = TYPE_ATTRIBUTES (TREE_TYPE (node[1]));
> - new_attrs = append_access_attr (node, attrs, attrstr
Re: [PATCH] Less warnings for parameters declared as arrays [PR98541, PR98536]
Am Dienstag, dem 04.04.2023 um 19:31 -0600 schrieb Jeff Law: > > On 4/3/23 13:34, Martin Uecker via Gcc-patches wrote: > > > > > > With the relatively new warnings (11..) affecting VLA bounds, > > I now get a lot of false positives with -Wall. In general, I find > > the new warnings very useful, but they seem a bit too > > aggressive and some minor tweaks are needed, otherwise they are > > too noisy. This patch suggests two changes: > > > > 1. For VLA bounds non-null is implied only when 'static' is > > used (similar to clang) and not already when a bound > 0 is > > specified: > > > > int foo(int n, char buf[static n]); > > > > int foo(10, 0); // warning with 'static' but not without. > > > > > > (It also seems problematic to require a size of 0 to indicate > > that the pointer may be null, because 0 is not allowed in > > ISO C as a size. It is also inconsistent to how arrays with > > static bound behave.) > > > > There seems to be agreement about this change in PR98541. > > > > > > 2. GCC always warns when the number of unspecified > > bounds is different between two declarations: > > > > int foo(int n, char buf[*]); > > int foo(int n, char buf[n]); > > > > or > > > > int foo(int n, char buf[n]); > > int foo(int n, char buf[*]); > > > > But the first version is useful if the size expression > > can not be specified in a header (e.g. because it uses > > a macro or variable not available there) and there is > > currently no easy way to avoid this. The warning for > > both cases was by design, but I suggest to limit the > > warning to the second case. > > > > Note that the logic currently applied by GCC is too > > simplistic anyway, as GCC does not warn for > > > > int foo(int x, int y, double m[*][y]); > > int foo(int x, int y, double m[x][*]); > > > > because the number of specified / unspecified bounds > > is the same. So I suggest to go with the attached > > patch now and add more precise warnings later > > if there is more experience with these warning > > in gernal and if this then still seems desirable. > > > > > > Martin > > > > > > Less warnings for parameters declared as arrays [PR98541, PR98536] > > > > > > > > > > To avoid false positivies, tune the warnings for parameters declared > > as arrays with size expressions. Only warn about null arguments with > > 'static'. Also do not warn when more bounds are specified in the new > > declaration than before. > > > > > > > > > > PR c/98541 > > PR c/98536 > > > > > > > > > > c-family/ > > * c-warn.cc (warn_parm_array_mismatch): Do not warn if more > > bounds are specified. > > > > > > > > > > gcc/ > > * gimple-ssa-warn-access.cc > > (pass_waccess::maybe_check_access_sizes): For VLA bounds > > in parameters, only warn about null pointers with 'static'. > > > > > > > > > > gcc/testsuite: > > * gcc.dg/Wnonnull-4: Adapt test. > > * gcc.dg/Wstringop-overflow-40.c: Adapt test. > > * gcc.dg/Wvla-parameter-4.c: Adapt test. > > * gcc.dg/attr-access-2.c: Adapt test. > Neither appears to be a regression. Seems like it should defer to gcc-14. Then ok for trunk now? Martin
[C PATCH - backport 12] Fix ICE related to implicit access attributes for VLA arguments [PR105660]
Ok to cherrypick for 12? It is in GCC 13 and fixes an
annoying ICE.
Martin
Here is a fix for PR105660.
Bootstrapped and regression tested on x86-64.
Fix ICE related to implicit access attributes for VLA arguments [PR105660]
When constructing the specifier string when merging an access attribute
that encodes information about VLA arguments, the string was constructed
in random order by iterating through a hash table. Fix this by iterating
though the list of arguments.
PR c/105660
gcc/Changelog
* c-family/c-attribs.cc (append_access_attr): Use order
of arguments when construction string.
(append_access_attr_idxs): Rename and make static.
* c-familty/c-warn.cc (warn_parm_array_mismatch): Add assertion.
gcc/testsuite/ChangeLog:
* gcc.dg/pr105660-1.c: New test.
* gcc.dg/pr105660-2.c: New test.
diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
index 4667f6de311..072cfb69147 100644
--- a/gcc/c-family/c-attribs.cc
+++ b/gcc/c-family/c-attribs.cc
@@ -4728,22 +4728,27 @@ append_access_attr (tree node[3], tree attrs, const
char *attrstr,
rdwr_map cur_idxs;
init_attr_rdwr_indices (_idxs, attrs);
+ tree args = TYPE_ARG_TYPES (node[0]);
+ int argpos = 0;
std::string spec;
- for (auto it = new_idxs.begin (); it != new_idxs.end (); ++it)
+ for (tree arg = args; arg; arg = TREE_CHAIN (arg), argpos++)
{
- const auto = *it;
+ const attr_access* const newa = new_idxs.get (argpos);
+
+ if (!newa)
+ continue;
/* The map has two equal entries for each pointer argument that
has an associated size argument. Process just the entry for
the former. */
- if ((unsigned)newaxsref.first != newaxsref.second.ptrarg)
+ if ((unsigned)argpos != newa->ptrarg)
continue;
- const attr_access* const cura = cur_idxs.get (newaxsref.first);
+ const attr_access* const cura = cur_idxs.get (argpos);
if (!cura)
{
/* The new attribute needs to be added. */
- tree str = newaxsref.second.to_internal_string ();
+ tree str = newa->to_internal_string ();
spec += TREE_STRING_POINTER (str);
continue;
}
@@ -4751,7 +4756,6 @@ append_access_attr (tree node[3], tree attrs, const char
*attrstr,
/* The new access spec refers to an array/pointer argument for
which an access spec already exists. Check and diagnose any
conflicts. If no conflicts are found, merge the two. */
- const attr_access* const newa =
if (!attrstr)
{
@@ -4886,7 +4890,7 @@ append_access_attr (tree node[3], tree attrs, const char
*attrstr,
continue;
/* Merge the CURA and NEWA. */
- attr_access merged = newaxsref.second;
+ attr_access merged = *newa;
/* VLA seen in a declaration takes precedence. */
if (cura->minsize == HOST_WIDE_INT_M1U)
@@ -4912,9 +4916,9 @@ append_access_attr (tree node[3], tree attrs, const char
*attrstr,
/* Convenience wrapper for the above. */
-tree
-append_access_attr (tree node[3], tree attrs, const char *attrstr,
- char code, HOST_WIDE_INT idxs[2])
+static tree
+append_access_attr_idxs (tree node[3], tree attrs, const char *attrstr,
+char code, HOST_WIDE_INT idxs[2])
{
char attrspec[80];
int n = sprintf (attrspec, "%c%u", code, (unsigned) idxs[0] - 1);
@@ -5204,7 +5208,7 @@ handle_access_attribute (tree node[3], tree name, tree
args, int flags,
attributes specified on previous declarations of the same type
and if not, concatenate the two. */
const char code = attr_access::mode_chars[mode];
- tree new_attrs = append_access_attr (node, attrs, attrstr, code, idxs);
+ tree new_attrs = append_access_attr_idxs (node, attrs, attrstr, code, idxs);
if (!new_attrs)
return NULL_TREE;
@@ -5217,7 +5221,7 @@ handle_access_attribute (tree node[3], tree name, tree
args, int flags,
{
/* Repeat for the previously declared type. */
attrs = TYPE_ATTRIBUTES (TREE_TYPE (node[1]));
- new_attrs = append_access_attr (node, attrs, attrstr, code, idxs);
+ new_attrs = append_access_attr_idxs (node, attrs, attrstr, code, idxs);
if (!new_attrs)
return NULL_TREE;
diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc
index 29efce3f2c0..a6fb95b1e80 100644
--- a/gcc/c-family/c-warn.cc
+++ b/gcc/c-family/c-warn.cc
@@ -3617,6 +3617,8 @@ warn_parm_array_mismatch (location_t origloc, tree
fndecl, tree newparms)
for (tree newvbl = newa->size, curvbl = cura->size; newvbl;
newvbl = TREE_CHAIN (newvbl), curvbl = TREE_CHAIN (curvbl))
{
+ gcc_assert (curvbl);
+
tree newpos = TREE_PURPOSE (newvbl);
tree curpos = TREE_PURPOSE (curvbl);
diff --git a/gcc/testsuite/gcc.dg/pr105660-1.c
Re: Fix ICEs related to VM types in C [PR106465, PR107557, PR108424, PR109450]
Am Mittwoch, dem 12.04.2023 um 00:32 + schrieb Joseph Myers: > On Tue, 11 Apr 2023, Martin Uecker via Gcc-patches wrote: > > > Ok, here is another attempt on fixing issues with size expression. > > Not all are regressions, but it does not make sense to try to split > > it up. > > This wording implies this is version 2 or later of the patch, could you > please give a reference to whatever previous patch posting / discussion > there may have been? The previous one was the following for PR107557 and PR108423. https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611562.html While revising it I realized that recursing for parameter when gimplifying is incorrect for the same reason as recursing in other cases (see the comment in gimplify_type_sizes). This lead to PR109450. > > > Fix ICEs related to VM types in C [PR106465, PR107557, PR108424, > > PR109450] > > 108424 seems to be the wrong PR number. This should be: PR108423 Martin
Fix ICEs related to VM types in C [PR106465, PR107557, PR108424, PR109450]
Ok, here is another attempt on fixing issues with size expression.
Not all are regressions, but it does not make sense to try to split
it up.
Martin
Fix ICEs related to VM types in C [PR106465, PR107557, PR108424,
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 e537d33f398..c76cbb3115f 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -5371,7 +5371,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))
@@ -7500,7 +7501,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);
@@ -7509,10 +7511,9 @@ 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);
+ add_stmt (build_stmt (DECL_SOURCE_LOCATION (decl), DECL_EXPR,
decl));
TYPE_NAME (type) = decl;
+
if (bind)
{
pop_scope ();
@@ -8711,7 +8712,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;
@@ -8768,7 +8769,7 @@ grokfield (location_t loc,
}
value = grokdeclarator (declarator, declspecs, FIELD, false,
- width ? : NULL, decl_attrs, NULL, NULL,
+ width ? : NULL, decl_attrs, expr, NULL,
DEPRECATED_NORMAL);
finish_decl (value, loc, NULL_TREE,
Re: [PATCH] Less warnings for parameters declared as arrays [PR98541, PR98536]
Am Dienstag, dem 04.04.2023 um 19:31 -0600 schrieb Jeff Law: > > On 4/3/23 13:34, Martin Uecker via Gcc-patches wrote: > > > > > > With the relatively new warnings (11..) affecting VLA bounds, > > I now get a lot of false positives with -Wall. In general, I find > > the new warnings very useful, but they seem a bit too > > aggressive and some minor tweaks are needed, otherwise they are > > too noisy. This patch suggests two changes: > > > > 1. For VLA bounds non-null is implied only when 'static' is > > used (similar to clang) and not already when a bound > 0 is > > specified: > > > > int foo(int n, char buf[static n]); > > > > int foo(10, 0); // warning with 'static' but not without. > > > > > > (It also seems problematic to require a size of 0 to indicate > > that the pointer may be null, because 0 is not allowed in > > ISO C as a size. It is also inconsistent to how arrays with > > static bound behave.) > > > > There seems to be agreement about this change in PR98541. > > > > > > 2. GCC always warns when the number of unspecified > > bounds is different between two declarations: > > > > int foo(int n, char buf[*]); > > int foo(int n, char buf[n]); > > > > or > > > > int foo(int n, char buf[n]); > > int foo(int n, char buf[*]); > > > > But the first version is useful if the size expression > > can not be specified in a header (e.g. because it uses > > a macro or variable not available there) and there is > > currently no easy way to avoid this. The warning for > > both cases was by design, but I suggest to limit the > > warning to the second case. > > > > Note that the logic currently applied by GCC is too > > simplistic anyway, as GCC does not warn for > > > > int foo(int x, int y, double m[*][y]); > > int foo(int x, int y, double m[x][*]); > > > > because the number of specified / unspecified bounds > > is the same. So I suggest to go with the attached > > patch now and add more precise warnings later > > if there is more experience with these warning > > in gernal and if this then still seems desirable. > > > > > > Martin > > > > > > Less warnings for parameters declared as arrays [PR98541, PR98536] > > > > > > > > > > > > > > > > > > To avoid false positivies, tune the warnings for parameters declared > > as arrays with size expressions. Only warn about null arguments with > > 'static'. Also do not warn when more bounds are specified in the new > > declaration than before. > > > > > > > > > > > > > > > > > > PR c/98541 > > PR c/98536 > > > > > > > > > > > > > > > > > > c-family/ > > * c-warn.cc (warn_parm_array_mismatch): Do not warn if more > > bounds are specified. > > > > > > > > > > > > > > > > > > gcc/ > > * gimple-ssa-warn-access.cc > > (pass_waccess::maybe_check_access_sizes): For VLA bounds > > in parameters, only warn about null pointers with 'static'. > > > > > > > > > > > > > > > > > > gcc/testsuite: > > * gcc.dg/Wnonnull-4: Adapt test. > > * gcc.dg/Wstringop-overflow-40.c: Adapt test. > > * gcc.dg/Wvla-parameter-4.c: Adapt test. > > * gcc.dg/attr-access-2.c: Adapt test. > Neither appears to be a regression. Seems like it should defer to gcc-14. The false positives are a regression relative to earlier version of GCC because the warnings are completely new (since GCC 11). I could bring this back later, but this means there is another version of GCC where one might need to turn off these warnings. My concern is also that an otherwise very useful feature that brings some safety benefits gets underused. Martin
[PATCH] Less warnings for parameters declared as arrays [PR98541, PR98536]
With the relatively new warnings (11..) affecting VLA bounds,
I now get a lot of false positives with -Wall. In general, I find
the new warnings very useful, but they seem a bit too
aggressive and some minor tweaks are needed, otherwise they are
too noisy. This patch suggests two changes:
1. For VLA bounds non-null is implied only when 'static' is
used (similar to clang) and not already when a bound > 0 is
specified:
int foo(int n, char buf[static n]);
int foo(10, 0); // warning with 'static' but not without.
(It also seems problematic to require a size of 0 to indicate
that the pointer may be null, because 0 is not allowed in
ISO C as a size. It is also inconsistent to how arrays with
static bound behave.)
There seems to be agreement about this change in PR98541.
2. GCC always warns when the number of unspecified
bounds is different between two declarations:
int foo(int n, char buf[*]);
int foo(int n, char buf[n]);
or
int foo(int n, char buf[n]);
int foo(int n, char buf[*]);
But the first version is useful if the size expression
can not be specified in a header (e.g. because it uses
a macro or variable not available there) and there is
currently no easy way to avoid this. The warning for
both cases was by design, but I suggest to limit the
warning to the second case.
Note that the logic currently applied by GCC is too
simplistic anyway, as GCC does not warn for
int foo(int x, int y, double m[*][y]);
int foo(int x, int y, double m[x][*]);
because the number of specified / unspecified bounds
is the same. So I suggest to go with the attached
patch now and add more precise warnings later
if there is more experience with these warning
in gernal and if this then still seems desirable.
Martin
Less warnings for parameters declared as arrays [PR98541, PR98536]
To avoid false positivies, tune the warnings for parameters declared
as arrays with size expressions. Only warn about null arguments with
'static'. Also do not warn when more bounds are specified in the new
declaration than before.
PR c/98541
PR c/98536
c-family/
* c-warn.cc (warn_parm_array_mismatch): Do not warn if more
bounds are specified.
gcc/
* gimple-ssa-warn-access.cc
(pass_waccess::maybe_check_access_sizes): For VLA bounds
in parameters, only warn about null pointers with 'static'.
gcc/testsuite:
* gcc.dg/Wnonnull-4: Adapt test.
* gcc.dg/Wstringop-overflow-40.c: Adapt test.
* gcc.dg/Wvla-parameter-4.c: Adapt test.
* gcc.dg/attr-access-2.c: Adapt test.
diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc
index 9ac43a1af6e..f79fb876142 100644
--- a/gcc/c-family/c-warn.cc
+++ b/gcc/c-family/c-warn.cc
@@ -3599,23 +3599,13 @@ warn_parm_array_mismatch (location_t origloc, tree
fndecl, tree newparms)
continue;
}
- if (newunspec != curunspec)
+ if (newunspec > curunspec)
{
location_t warnloc = newloc, noteloc = origloc;
const char *warnparmstr = newparmstr.c_str ();
const char *noteparmstr = curparmstr.c_str ();
unsigned warnunspec = newunspec, noteunspec = curunspec;
- if (newunspec < curunspec)
- {
- /* If the new declaration has fewer unspecified bounds
-point the warning to the previous declaration to make
-it clear that that's the one to change. Otherwise,
-point it to the new decl. */
- std::swap (warnloc, noteloc);
- std::swap (warnparmstr, noteparmstr);
- std::swap (warnunspec, noteunspec);
- }
if (warning_n (warnloc, OPT_Wvla_parameter, warnunspec,
"argument %u of type %s declared with "
"%u unspecified variable bound",
@@ -3641,16 +3631,11 @@ warn_parm_array_mismatch (location_t origloc, tree
fndecl, tree newparms)
continue;
}
}
-
/* Iterate over the lists of VLA variable bounds, comparing each
-pair for equality, and diagnosing mismatches. The case of
-the lists having different lengths is handled above so at
-this point they do . */
- for (tree newvbl = newa->size, curvbl = cura->size; newvbl;
+pair for equality, and diagnosing mismatches. */
+ for (tree newvbl = newa->size, curvbl = cura->size; newvbl && curvbl;
newvbl = TREE_CHAIN (newvbl), curvbl = TREE_CHAIN (curvbl))
{
- gcc_assert (curvbl);
-
tree newpos = TREE_PURPOSE (newvbl);
tree curpos = TREE_PURPOSE (curvbl);
@@ -3663,7 +3648,6 @@ warn_parm_array_mismatch (location_t origloc, tree
fndecl, tree newparms)
and both are the same
Re: [PATCH] gimplify size expressions in parameters for all types [PR107557] [PR108423]
Am Dienstag, dem 21.02.2023 um 14:21 + schrieb Richard Biener:
> On Tue, 21 Feb 2023, Martin Uecker wrote:
>
> >
> >
> > Hi Richard,
> >
> > can you look at this middle-end patch? It fixes two regressions.
>
> But gimplify_type_sizes recurses itself, but in particular _not_
> for pointer types. Iff recursing in parameter context is
> necessary and safe I'd rather have gimplify_type_sizes know that
> (bool for_param_p?) and do the recursion?
>
I just want to point out that without the patch
gimplify_parm_sizes already does the recursion
into pointers types. But other types are also
not reached. But maybe there was never a good
reason for this split?
I will try merging this all into gimplify_type_sizes.
Martin
> Richard.
>
> > Martin
> >
> >
> > PS: I happy to do something about at variably_modified_type_p
> > in the middle-end, if you have a recommendation.
> >
> >
> >
> >
> > Am Mittwoch, dem 08.02.2023 um 13:02 +0100 schrieb Martin Uecker:
> > >
> > > Here is a fix for PR107557 and PR108423.
> > >
> > >
> > > Bootstrapped and regression tested on x86-64.
> > >
> > >
> > >
> > > Gimplify more size expression in parameters [PR107557] and [PR108234]
> > >
> > >
> > > gimplify_parm_type only recursives into pointer type and
> > > size expressions in other types (e.g. function types) were
> > > not reached.
> > >
> > >
> > > PR c/107557
> > > PR c/108234
> > >
> > >
> > > gcc/Changelog
> > > * gcc/function.cc (gimplify_parm_type): Also recursive into
> > > non-pointer types.
> > >
> > >
> > > gcc/testsuite/ChangeLog:
> > > * 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.
> > >
> > >
> > >
> > >
> > > diff --git a/gcc/function.cc b/gcc/function.cc
> > > index cfc4d2f74af..d777348aeb4 100644
> > > --- a/gcc/function.cc
> > > +++ b/gcc/function.cc
> > > @@ -3880,20 +3880,15 @@ static tree
> > > gimplify_parm_type (tree *tp, int *walk_subtrees, void *data)
> > > {
> > > tree t = *tp;
> > > -
> > > *walk_subtrees = 0;
> > > if (TYPE_P (t))
> > > {
> > > - if (POINTER_TYPE_P (t))
> > > - *walk_subtrees = 1;
> > > - else if (TYPE_SIZE (t) && !TREE_CONSTANT (TYPE_SIZE (t))
> > > + if (TYPE_SIZE (t) && !TREE_CONSTANT (TYPE_SIZE (t))
> > > && !TYPE_SIZES_GIMPLIFIED (t))
> > > - {
> > > - gimplify_type_sizes (t, (gimple_seq *) data);
> > > - *walk_subtrees = 1;
> > > - }
> > > -}
> > > + gimplify_type_sizes (t, (gimple_seq *) data);
> > >
> > >
> > > + *walk_subtrees = 1;
> > > +}
> > > return NULL;
> > > }
> > >
> > >
> > > diff --git a/gcc/testsuite/gcc.dg/pr107557-1.c
> > > b/gcc/testsuite/gcc.dg/pr107557-1.c
> > > new file mode 100644
> > > index 000..88c248b6564
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/pr107557-1.c
> > > @@ -0,0 +1,24 @@
> > > +/* PR107557
> > > + * { dg-do compile }
> > > + * { dg-options "-flto -fsanitize=undefined -fexceptions
> > > -Wno-incompatible-pointer-types" }
> > > + */
> > > +
> > > +
> > > +int c[1][3*2];
> > > +int f(int * const m, int (**v)[*m * 2])
> > > +{
> > > + return &(c[0][*m]) == &((*v)[0][*m]);
> > > +}
> > > +int test(int n, int (*(*fn)(void))[n])
> > > +{
> > > + return (*fn())[0];
> > > +}
> > > +int main()
> > > +{
> > > + int m = 3;
> > > + int (*d)[3*2] = c;
> > > + int (*fn[m])(void);
> > > + return f(, ) + test(m, );
> > > +}
> > > +
> > > +
> > > diff --git a/gcc/testsuite/gcc.dg/pr107557-2.c
> > > b/gcc/testsuite/gcc.dg/pr107557-2.c
> > > new file mode 100644
> > > index
Re: [PATCH] gimplify size expressions in parameters for all types [PR107557] [PR108423]
Hi Richard,
can you look at this middle-end patch? It fixes two regressions.
Martin
PS: I happy to do something about at variably_modified_type_p
in the middle-end, if you have a recommendation.
Am Mittwoch, dem 08.02.2023 um 13:02 +0100 schrieb Martin Uecker:
>
> Here is a fix for PR107557 and PR108423.
>
>
> Bootstrapped and regression tested on x86-64.
>
>
>
> Gimplify more size expression in parameters [PR107557] and [PR108234]
>
>
> gimplify_parm_type only recursives into pointer type and
> size expressions in other types (e.g. function types) were
> not reached.
>
>
> PR c/107557
> PR c/108234
>
>
> gcc/Changelog
> * gcc/function.cc (gimplify_parm_type): Also recursive into
> non-pointer types.
>
>
> gcc/testsuite/ChangeLog:
> * 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.
>
>
>
>
> diff --git a/gcc/function.cc b/gcc/function.cc
> index cfc4d2f74af..d777348aeb4 100644
> --- a/gcc/function.cc
> +++ b/gcc/function.cc
> @@ -3880,20 +3880,15 @@ static tree
> gimplify_parm_type (tree *tp, int *walk_subtrees, void *data)
> {
> tree t = *tp;
> -
> *walk_subtrees = 0;
> if (TYPE_P (t))
> {
> - if (POINTER_TYPE_P (t))
> - *walk_subtrees = 1;
> - else if (TYPE_SIZE (t) && !TREE_CONSTANT (TYPE_SIZE (t))
> + if (TYPE_SIZE (t) && !TREE_CONSTANT (TYPE_SIZE (t))
> && !TYPE_SIZES_GIMPLIFIED (t))
> - {
> - gimplify_type_sizes (t, (gimple_seq *) data);
> - *walk_subtrees = 1;
> - }
> -}
> + gimplify_type_sizes (t, (gimple_seq *) data);
>
>
> + *walk_subtrees = 1;
> +}
> return NULL;
> }
>
>
> diff --git a/gcc/testsuite/gcc.dg/pr107557-1.c
> b/gcc/testsuite/gcc.dg/pr107557-1.c
> new file mode 100644
> index 000..88c248b6564
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr107557-1.c
> @@ -0,0 +1,24 @@
> +/* PR107557
> + * { dg-do compile }
> + * { dg-options "-flto -fsanitize=undefined -fexceptions
> -Wno-incompatible-pointer-types" }
> + */
> +
> +
> +int c[1][3*2];
> +int f(int * const m, int (**v)[*m * 2])
> +{
> + return &(c[0][*m]) == &((*v)[0][*m]);
> +}
> +int test(int n, int (*(*fn)(void))[n])
> +{
> + return (*fn())[0];
> +}
> +int main()
> +{
> + int m = 3;
> + int (*d)[3*2] = c;
> + int (*fn[m])(void);
> + return f(, ) + test(m, );
> +}
> +
> +
> diff --git a/gcc/testsuite/gcc.dg/pr107557-2.c
> b/gcc/testsuite/gcc.dg/pr107557-2.c
> new file mode 100644
> index 000..2d26bb0b16a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr107557-2.c
> @@ -0,0 +1,23 @@
> +/* PR107557
> + * { dg-do compile }
> + * { dg-options "-flto -fsanitize=undefined -fexceptions
> -Wno-incompatible-pointer-types" }
> + */
> +
> +
> +int c[1][3*2];
> +int f(int * const m, int (**(*v))[*m * 2])
> +{
> + return &(c[0][*m]) == &((*v)[0][*m]); /* { dg-warning "lacks a cast"
> } */
> +}
> +int test(int n, int (*(*(*fn))(void))[n])
> +{
> + return (*(*fn)())[0];
> +}
> +int main()
> +{
> + int m = 3;
> + int (*d)[3*2] = c;
> + int (*fn[m])(void);
> + return f(, ) + test(m, );
> +}
> +
> diff --git a/gcc/testsuite/gcc.dg/pr108423-1.c
> b/gcc/testsuite/gcc.dg/pr108423-1.c
> new file mode 100644
> index 000..0c98d1d46b9
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr108423-1.c
> @@ -0,0 +1,16 @@
> +/* PR108423
> + * { dg-do compile }
> + * { dg-options "-O2 -Wno-int-conversion -Wno-incompatible-pointer-types" }
> + */
> +int f (int n, int (**(*a)(void))[n])
> +{
> + return (*a())[0];
> +}
> +int g ()
> +{
> + int m = 3;
> + int (*a[m])(void);
> + return f(m, );
> +}
> +
> +
> diff --git a/gcc/testsuite/gcc.dg/pr108423-2.c
> b/gcc/testsuite/gcc.dg/pr108423-2.c
> new file mode 100644
> index 000..006e45a9629
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr108423-2.c
> @@ -0,0 +1,16 @@
> +/* PR108423
> + * { dg-do compile }
> + * { dg-options "-O2" }
> + */
> +
> +void f(int n, int (*a(void))[n])
> +{
> + (a())[0];
> +}
> +
> +void g(void)
> +{
> + int (*a(void
[C PATCH] Detect all variably modified types [PR108375]
Here is a patch for PR108375.
Bootstrapped and regession tested on x86_64-linux-gnu
Also there is a middle-end patch for PR107557 and PR108423:
https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611562.html
and another C FE patch for PR105660:
https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611817.html
All regressions.
C: Detect all variably modified types [PR108375]
Some variably modified types were not detected correctly.
Define C_TYPE_VARIABLY_MODIFIED via TYPE_LANG_FLAG 6 in the CFE.
This flag records whether a type is variably modified and is
set for all such types including arrays with variably modified
element type or structures and unions with variably modified
members. This is then used to detect such types in the C FE
and middle-end (via the existing language hook).
PR 108375
gcc/c/
* c/c-decl.c (decl_jump_unsafe): Use c_type_variably_modified_p.
(diagnose_mismatched_decl): Dito.
(warn_about_goto): Dito:
(c_check_switch_jump_warnings): Dito.
(finish_decl): Dito.
(finish_struct): Dito.
(grokdeclarator): Set C_TYPE_VARIABLY_MODIFIED.
(finish_struct): Set C_TYPE_VARIABLY_MODIFIED.
* c/c-objc-common.cc (c_var_mod_p): New function.
(c_var_unspec_p): Remove.
* c/c-objc-common.h: Set lang hook.
* c/c-parser.cc (c_parser_declararion_or_fndef): Use
c_type_variably_modified_p.
(c_parser_typeof_specifier): Dito.
(c_parser_has_attribute_expression): Dito.
(c_parser_generic_selection): Dito.
* c/c-tree.h: Define C_TYPE_VARIABLY_MODIFIED and define
c_var_mode_p.
* c/c-typeck.h: Remove c_vla_mod_p and use C_TYPE_VARIABLY_MODIFIED.
gcc/testsuite/
* gcc.dg/pr108375-1.c: New test.
* gcc.dg/pr108375-2.c: New test.
diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index 20e7d1855bf..08078eadeb8 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -683,7 +683,7 @@ decl_jump_unsafe (tree decl)
/* Always warn about crossing variably modified types. */
if ((VAR_P (decl) || TREE_CODE (decl) == TYPE_DECL)
- && variably_modified_type_p (TREE_TYPE (decl), NULL_TREE))
+ && c_type_variably_modified_p (TREE_TYPE (decl)))
return true;
/* Otherwise, only warn if -Wgoto-misses-init and this is an
@@ -2247,7 +2247,7 @@ diagnose_mismatched_decls (tree newdecl, tree olddecl,
|| warning_suppressed_p (olddecl, OPT_Wpedantic))
return true; /* Allow OLDDECL to continue in use. */
- if (variably_modified_type_p (newtype, NULL))
+ if (c_type_variably_modified_p (newtype))
{
error ("redefinition of typedef %q+D with variably modified type",
newdecl);
@@ -3975,7 +3975,7 @@ static void
warn_about_goto (location_t goto_loc, tree label, tree decl)
{
auto_diagnostic_group d;
- if (variably_modified_type_p (TREE_TYPE (decl), NULL_TREE))
+ if (c_type_variably_modified_p (TREE_TYPE (decl)))
error_at (goto_loc,
"jump into scope of identifier with variably modified type");
else
@@ -4249,7 +4249,7 @@ c_check_switch_jump_warnings (struct c_spot_bindings
*switch_bindings,
{
auto_diagnostic_group d;
bool emitted;
- if (variably_modified_type_p (TREE_TYPE (b->decl), NULL_TREE))
+ if (c_type_variably_modified_p (TREE_TYPE (b->decl)))
{
saw_error = true;
error_at (case_loc,
@@ -5862,7 +5862,7 @@ finish_decl (tree decl, location_t init_loc, tree init,
if (TREE_CODE (decl) == TYPE_DECL)
{
if (!DECL_FILE_SCOPE_P (decl)
- && variably_modified_type_p (TREE_TYPE (decl), NULL_TREE))
+ && c_type_variably_modified_p (TREE_TYPE (decl)))
add_stmt (build_stmt (DECL_SOURCE_LOCATION (decl), DECL_EXPR, decl));
rest_of_decl_compilation (decl, DECL_FILE_SCOPE_P (decl), 0);
@@ -6682,7 +6682,7 @@ grokdeclarator (const struct c_declarator *declarator,
if ((decl_context == NORMAL || decl_context == FIELD)
&& current_scope == file_scope
- && variably_modified_type_p (type, NULL_TREE))
+ && c_type_variably_modified_p (type))
{
if (name)
error_at (loc, "variably modified %qE at file scope", name);
@@ -6928,6 +6928,8 @@ grokdeclarator (const struct c_declarator *declarator,
array_parm_static = false;
}
+ bool varmod = C_TYPE_VARIABLY_MODIFIED (type);
+
switch (declarator->kind)
{
case cdk_attrs:
@@ -7282,8 +7284,7 @@ grokdeclarator (const struct c_declarator *declarator,
variable size, so the enclosing shared array type
must too. */
if (size && TREE_CODE (size) == INTEGER_CST)
- type
-
[C PATCH] Fix ICE related to implicit access attributes for VLA arguments [PR105660]
Here is a fix for PR105660.
Bootstrapped and regression tested on x86-64.
Fix ICE related to implicit access attributes for VLA arguments [PR105660]
When constructing the specifier string when merging an access attribute
that encodes information about VLA arguments, the string was constructed
in random order by iterating through a hash table. Fix this by iterating
though the list of arguments.
PR c/105660
gcc/Changelog
* c-family/c-attribs.cc (append_access_attr): Use order
of arguments when construction string.
(append_access_attr_idxs): Rename and make static.
* c-familty/c-warn.cc (warn_parm_array_mismatch): Add assertion.
gcc/testsuite/ChangeLog:
* gcc.dg/pr105660-1.c: New test.
* gcc.dg/pr105660-2.c: New test.
diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
index 4667f6de311..072cfb69147 100644
--- a/gcc/c-family/c-attribs.cc
+++ b/gcc/c-family/c-attribs.cc
@@ -4728,22 +4728,27 @@ append_access_attr (tree node[3], tree attrs, const
char *attrstr,
rdwr_map cur_idxs;
init_attr_rdwr_indices (_idxs, attrs);
+ tree args = TYPE_ARG_TYPES (node[0]);
+ int argpos = 0;
std::string spec;
- for (auto it = new_idxs.begin (); it != new_idxs.end (); ++it)
+ for (tree arg = args; arg; arg = TREE_CHAIN (arg), argpos++)
{
- const auto = *it;
+ const attr_access* const newa = new_idxs.get (argpos);
+
+ if (!newa)
+ continue;
/* The map has two equal entries for each pointer argument that
has an associated size argument. Process just the entry for
the former. */
- if ((unsigned)newaxsref.first != newaxsref.second.ptrarg)
+ if ((unsigned)argpos != newa->ptrarg)
continue;
- const attr_access* const cura = cur_idxs.get (newaxsref.first);
+ const attr_access* const cura = cur_idxs.get (argpos);
if (!cura)
{
/* The new attribute needs to be added. */
- tree str = newaxsref.second.to_internal_string ();
+ tree str = newa->to_internal_string ();
spec += TREE_STRING_POINTER (str);
continue;
}
@@ -4751,7 +4756,6 @@ append_access_attr (tree node[3], tree attrs, const char
*attrstr,
/* The new access spec refers to an array/pointer argument for
which an access spec already exists. Check and diagnose any
conflicts. If no conflicts are found, merge the two. */
- const attr_access* const newa =
if (!attrstr)
{
@@ -4886,7 +4890,7 @@ append_access_attr (tree node[3], tree attrs, const char
*attrstr,
continue;
/* Merge the CURA and NEWA. */
- attr_access merged = newaxsref.second;
+ attr_access merged = *newa;
/* VLA seen in a declaration takes precedence. */
if (cura->minsize == HOST_WIDE_INT_M1U)
@@ -4912,9 +4916,9 @@ append_access_attr (tree node[3], tree attrs, const char
*attrstr,
/* Convenience wrapper for the above. */
-tree
-append_access_attr (tree node[3], tree attrs, const char *attrstr,
- char code, HOST_WIDE_INT idxs[2])
+static tree
+append_access_attr_idxs (tree node[3], tree attrs, const char *attrstr,
+char code, HOST_WIDE_INT idxs[2])
{
char attrspec[80];
int n = sprintf (attrspec, "%c%u", code, (unsigned) idxs[0] - 1);
@@ -5204,7 +5208,7 @@ handle_access_attribute (tree node[3], tree name, tree
args, int flags,
attributes specified on previous declarations of the same type
and if not, concatenate the two. */
const char code = attr_access::mode_chars[mode];
- tree new_attrs = append_access_attr (node, attrs, attrstr, code, idxs);
+ tree new_attrs = append_access_attr_idxs (node, attrs, attrstr, code, idxs);
if (!new_attrs)
return NULL_TREE;
@@ -5217,7 +5221,7 @@ handle_access_attribute (tree node[3], tree name, tree
args, int flags,
{
/* Repeat for the previously declared type. */
attrs = TYPE_ATTRIBUTES (TREE_TYPE (node[1]));
- new_attrs = append_access_attr (node, attrs, attrstr, code, idxs);
+ new_attrs = append_access_attr_idxs (node, attrs, attrstr, code, idxs);
if (!new_attrs)
return NULL_TREE;
diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc
index 29efce3f2c0..a6fb95b1e80 100644
--- a/gcc/c-family/c-warn.cc
+++ b/gcc/c-family/c-warn.cc
@@ -3617,6 +3617,8 @@ warn_parm_array_mismatch (location_t origloc, tree
fndecl, tree newparms)
for (tree newvbl = newa->size, curvbl = cura->size; newvbl;
newvbl = TREE_CHAIN (newvbl), curvbl = TREE_CHAIN (curvbl))
{
+ gcc_assert (curvbl);
+
tree newpos = TREE_PURPOSE (newvbl);
tree curpos = TREE_PURPOSE (curvbl);
diff --git a/gcc/testsuite/gcc.dg/pr105660-1.c
b/gcc/testsuite/gcc.dg/pr105660-1.c
new file mode 100644
index 000..d4454f04c43
--- /dev/null
+++
[PATCH] gimplify size expressions in parameters for all types [PR107557] [PR108423]
Here is a fix for PR107557 and PR108423.
Bootstrapped and regression tested on x86-64.
Gimplify more size expression in parameters [PR107557] and [PR108234]
gimplify_parm_type only recursives into pointer type and
size expressions in other types (e.g. function types) were
not reached.
PR c/107557
PR c/108234
gcc/Changelog
* gcc/function.cc (gimplify_parm_type): Also recursive into
non-pointer types.
gcc/testsuite/ChangeLog:
* 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.
diff --git a/gcc/function.cc b/gcc/function.cc
index cfc4d2f74af..d777348aeb4 100644
--- a/gcc/function.cc
+++ b/gcc/function.cc
@@ -3880,20 +3880,15 @@ static tree
gimplify_parm_type (tree *tp, int *walk_subtrees, void *data)
{
tree t = *tp;
-
*walk_subtrees = 0;
if (TYPE_P (t))
{
- if (POINTER_TYPE_P (t))
- *walk_subtrees = 1;
- else if (TYPE_SIZE (t) && !TREE_CONSTANT (TYPE_SIZE (t))
+ if (TYPE_SIZE (t) && !TREE_CONSTANT (TYPE_SIZE (t))
&& !TYPE_SIZES_GIMPLIFIED (t))
- {
- gimplify_type_sizes (t, (gimple_seq *) data);
- *walk_subtrees = 1;
- }
-}
+ gimplify_type_sizes (t, (gimple_seq *) data);
+ *walk_subtrees = 1;
+}
return NULL;
}
diff --git a/gcc/testsuite/gcc.dg/pr107557-1.c
b/gcc/testsuite/gcc.dg/pr107557-1.c
new file mode 100644
index 000..88c248b6564
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr107557-1.c
@@ -0,0 +1,24 @@
+/* PR107557
+ * { dg-do compile }
+ * { dg-options "-flto -fsanitize=undefined -fexceptions
-Wno-incompatible-pointer-types" }
+ */
+
+
+int c[1][3*2];
+int f(int * const m, int (**v)[*m * 2])
+{
+ return &(c[0][*m]) == &((*v)[0][*m]);
+}
+int test(int n, int (*(*fn)(void))[n])
+{
+ return (*fn())[0];
+}
+int main()
+{
+ int m = 3;
+ int (*d)[3*2] = c;
+ int (*fn[m])(void);
+ return f(, ) + test(m, );
+}
+
+
diff --git a/gcc/testsuite/gcc.dg/pr107557-2.c
b/gcc/testsuite/gcc.dg/pr107557-2.c
new file mode 100644
index 000..2d26bb0b16a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr107557-2.c
@@ -0,0 +1,23 @@
+/* PR107557
+ * { dg-do compile }
+ * { dg-options "-flto -fsanitize=undefined -fexceptions
-Wno-incompatible-pointer-types" }
+ */
+
+
+int c[1][3*2];
+int f(int * const m, int (**(*v))[*m * 2])
+{
+ return &(c[0][*m]) == &((*v)[0][*m]); /* { dg-warning "lacks a cast"
} */
+}
+int test(int n, int (*(*(*fn))(void))[n])
+{
+ return (*(*fn)())[0];
+}
+int main()
+{
+ int m = 3;
+ int (*d)[3*2] = c;
+ int (*fn[m])(void);
+ return f(, ) + test(m, );
+}
+
diff --git a/gcc/testsuite/gcc.dg/pr108423-1.c
b/gcc/testsuite/gcc.dg/pr108423-1.c
new file mode 100644
index 000..0c98d1d46b9
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr108423-1.c
@@ -0,0 +1,16 @@
+/* PR108423
+ * { dg-do compile }
+ * { dg-options "-O2 -Wno-int-conversion -Wno-incompatible-pointer-types" }
+ */
+int f (int n, int (**(*a)(void))[n])
+{
+ return (*a())[0];
+}
+int g ()
+{
+ int m = 3;
+ int (*a[m])(void);
+ return f(m, );
+}
+
+
diff --git a/gcc/testsuite/gcc.dg/pr108423-2.c
b/gcc/testsuite/gcc.dg/pr108423-2.c
new file mode 100644
index 000..006e45a9629
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr108423-2.c
@@ -0,0 +1,16 @@
+/* PR108423
+ * { dg-do compile }
+ * { dg-options "-O2" }
+ */
+
+void f(int n, int (*a(void))[n])
+{
+ (a())[0];
+}
+
+void g(void)
+{
+ int (*a(void))[1];
+ f(1, a);
+}
+
diff --git a/gcc/testsuite/gcc.dg/pr108423-3.c
b/gcc/testsuite/gcc.dg/pr108423-3.c
new file mode 100644
index 000..c1987c42b40
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr108423-3.c
@@ -0,0 +1,17 @@
+/* PR108423
+ * { dg-do compile }
+ * { dg-options "-O2" }
+ */
+
+void f(int n, int (*(*b)(void))[n])
+{
+sizeof (*(*b)());
+}
+
+int (*a(void))[1];
+
+void g(void)
+{
+ f(1, );
+}
+
diff --git a/gcc/testsuite/gcc.dg/pr108423-4.c
b/gcc/testsuite/gcc.dg/pr108423-4.c
new file mode 100644
index 000..91336f3f283
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr108423-4.c
@@ -0,0 +1,17 @@
+/* PR108423
+ * { dg-do compile }
+ * { dg-options "-O2" }
+ */
+
+void f(int n, int (*a(void))[n])
+{
+sizeof (*a());
+}
+
+int (*a(void))[1];
+
+void g(void)
+{
+ f(1, a);
+}
+
diff --git a/gcc/testsuite/gcc.dg/pr108423-5.c
b/gcc/testsuite/gcc.dg/pr108423-5.c
new file mode 100644
index 000..7e4fffb2870
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr108423-5.c
@@ -0,0 +1,17 @@
+/* PR108423
+ * { dg-do compile }
+ * { dg-options "-O2" }
+ */
+
+void f(int n, int (*(*a)(void))[n])
+{
+sizeof ((*a)());
+}
+
+int (*a(void))[1];
+
[PATCH] regression tests for 103770 fixed on trunk
This adds regression tests for an ICE on valid code that
seems gone on trunk, but the cause is still unclear.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103770
regressions tests for PR103770
This adds tests from bugzilla for PR103770 and duplicates.
testsuite/gcc.dg/
* pr103770.c: New test.
* pr103859.c: New test.
* pr105065.c: New test.
diff --git a/gcc/testsuite/gcc.dg/pr103770.c b/gcc/testsuite/gcc.dg/pr103770.c
new file mode 100644
index 000..f7867d1284c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr103770.c
@@ -0,0 +1,27 @@
+/* PR middle-end/103770 */
+/* { dg-do compile } */
+/* { dg-options "" } */
+
+struct struct_s {
+ void* ptr;
+ void* ptr2;
+ void* ptr3;
+};
+
+struct struct_s struct_create(int N, const long vla[N]);
+
+void fun(int N)
+{
+ long vla[N];
+ struct struct_s st = struct_create(N, vla);
+}
+
+
+extern _Complex float g(int N, int dims[N]);
+
+void f(void)
+{
+ int dims[1];
+ _Complex float val = g(1, dims);
+}
+
diff --git a/gcc/testsuite/gcc.dg/pr103859.c b/gcc/testsuite/gcc.dg/pr103859.c
new file mode 100644
index 000..c58be5c15af
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr103859.c
@@ -0,0 +1,23 @@
+/* PR middle-end/103859 */
+/* { dg-do compile } */
+/* { dg-options "" } */
+
+typedef struct dcmplx dcmplx;
+
+struct dcmplx {
+ double re;
+ double im;
+};
+
+dcmplx horner(int n, dcmplx p[n], dcmplx x);
+
+int main(void)
+{
+ int i, n;
+ dcmplx x[n + 1], f[n + 1];
+
+ horner(n + 1, f, x[i]);
+
+ return 0;
+}
+
diff --git a/gcc/testsuite/gcc.dg/pr105065.c b/gcc/testsuite/gcc.dg/pr105065.c
new file mode 100644
index 000..da46d2bb4d8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr105065.c
@@ -0,0 +1,16 @@
+/* PR middle-end/105065 */
+/* { dg-do compile } */
+/* { dg-options "" } */
+
+typedef struct
+{
+ char filler[17];
+} big_struct;
+
+big_struct dummy(int size, char array[size]);
+
+int main()
+{
+ dummy(0, 0);
+}
+
[C PATCH] (for STAGE 1) UBSan instrumentation for assignment of VM types
Here is a first patch to add UBSan instrumentation to
assignment, return, initialization of pointers
to variably modified types. This is based on the
other patch I just sent. Separating these should make
reviewing easier.
Here, I did not add tests for function arguments as
this is more complicated, but this will follow...
c: UBSan instrumentation for assignment of VM types
This adds instrumentation that checks that corresponding
size expression in variably modified types evaluate to
the same value in assignment, function return, and
initialization.
gcc/c-family/
* c-ubsan.cc (ubsan_instrument_vm_assign): New.
gcc/c/
* c-typeck.cc (comptypes_check_enum_int_instr,
comptypes_check_enum_int): New interface for
instrumentation.
(comptypes_internal,convert_for_assignment):
Add instrumentation.
(comp_target_types_instr,comp_target_types): Add
new interface for instrumentation.
gcc/testsuite/gcc.dg/ubsan/
* vm-bounds-1.c: New test.
* vm-bounds-2.c: New test.
diff --git a/gcc/c-family/c-ubsan.cc b/gcc/c-family/c-ubsan.cc
index 360ba82250c..7de4e6e7057 100644
--- a/gcc/c-family/c-ubsan.cc
+++ b/gcc/c-family/c-ubsan.cc
@@ -334,6 +334,48 @@ ubsan_instrument_vla (location_t loc, tree size)
return t;
}
+/* Instrument assignment of variably modified types. */
+
+tree
+ubsan_instrument_vm_assign (location_t loc, tree a, tree b)
+{
+ tree t, tt;
+
+ gcc_assert (TREE_CODE (a) == ARRAY_TYPE);
+ gcc_assert (TREE_CODE (b) == ARRAY_TYPE);
+
+ tree as = TYPE_MAX_VALUE (TYPE_DOMAIN (a));
+ tree bs = TYPE_MAX_VALUE (TYPE_DOMAIN (b));
+
+ as = fold_build2 (PLUS_EXPR, sizetype, as, size_one_node);
+ bs = fold_build2 (PLUS_EXPR, sizetype, bs, size_one_node);
+
+ t = build2 (NE_EXPR, boolean_type_node, as, bs);
+ if (flag_sanitize_trap & SANITIZE_VLA)
+tt = build_call_expr_loc (loc, builtin_decl_explicit (BUILT_IN_TRAP), 0);
+ else
+{
+ tree data = ubsan_create_data ("__ubsan_vm_data", 1, ,
+ubsan_type_descriptor (a,
UBSAN_PRINT_ARRAY),
+ubsan_type_descriptor (b,
UBSAN_PRINT_ARRAY),
+ubsan_type_descriptor (sizetype),
+NULL_TREE, NULL_TREE);
+ data = build_fold_addr_expr_loc (loc, data);
+ enum built_in_function bcode
+ = (flag_sanitize_recover & SANITIZE_VLA)
+ ? BUILT_IN_UBSAN_HANDLE_VM_BOUNDS_MISMATCH
+ : BUILT_IN_UBSAN_HANDLE_VM_BOUNDS_MISMATCH_ABORT;
+ tt = builtin_decl_explicit (bcode);
+ tt = build_call_expr_loc (loc, tt, 3, data,
+ ubsan_encode_value (as),
+ ubsan_encode_value (bs));
+}
+ t = build3 (COND_EXPR, void_type_node, t, tt, void_node);
+
+ return t;
+}
+
+
/* Instrument missing return in C++ functions returning non-void. */
tree
diff --git a/gcc/c-family/c-ubsan.h b/gcc/c-family/c-ubsan.h
index 2f31ba36df4..327313c6684 100644
--- a/gcc/c-family/c-ubsan.h
+++ b/gcc/c-family/c-ubsan.h
@@ -26,6 +26,7 @@ extern tree ubsan_instrument_shift (location_t, enum
tree_code, tree, tree);
extern tree ubsan_instrument_vla (location_t, tree);
extern tree ubsan_instrument_return (location_t);
extern tree ubsan_instrument_bounds (location_t, tree, tree *, bool);
+extern tree ubsan_instrument_vm_assign (location_t, tree, tree);
extern bool ubsan_array_ref_instrumented_p (const_tree);
extern void ubsan_maybe_instrument_array_ref (tree *, bool);
extern void ubsan_maybe_instrument_reference (tree *);
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index ebc9ba88afe..a58b96083e9 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -93,6 +93,7 @@ static tree qualify_type (tree, tree);
struct comptypes_data;
static int tagged_types_compatible_p (const_tree, const_tree, struct
comptypes_data *);
static int comp_target_types (location_t, tree, tree);
+static int comp_target_types_instr (location_t, tree, tree, tree *);
static int function_types_compatible_p (const_tree, const_tree, struct
comptypes_data *);
static int type_lists_compatible_p (const_tree, const_tree, struct
comptypes_data *);
static tree lookup_field (tree, tree);
@@ -1053,6 +1054,9 @@ struct comptypes_data {
bool enum_and_int_p;
bool different_types_p;
+
+ location_t loc;
+ tree instrument_expr;
};
/* Return 1 if TYPE1 and TYPE2 are compatible types for assignment
@@ -1075,20 +1079,35 @@ comptypes (tree type1, tree type2)
/* Like comptypes, but if it returns non-zero because enum and int are
compatible, it sets *ENUM_AND_INT_P to true. */
-int
-comptypes_check_enum_int (tree type1, tree type2, bool *enum_and_int_p)
+static int
+comptypes_check_enum_int_instr (tree type1, tree type2, bool *enum_and_int_p,
location_t loc, tree *instrument_expr)
{
int val;
struct comptypes_data data = { };
+
+ data.loc = loc;
+
+ if (NULL != instrument_expr)
+
[C PATCH] (for STAGE 1) Reorganize comptypes and related functions
Because I want to add another argument to comptypes
and co. for UBSan instrumentation and this then
starts to become a bit unwiedly, here is a patch to
reorganize and simplify this a bit. This can wait
until stage 1. (The cache can be simplified further
by allocating it on the stack, but this can be done
later).
c: Reorganize comptypes and related functions.
Move common arguments and a global variable (the
cache for type compatibility) used by comptypes
and childen into a single structure and pass a
pointer to it.
gcc/c/
* c/c-typeck.cc: (comptypes,comptypes_check_enum_int,
comptypes_check_different_types,comptypes_internal,
tagged_types_tu_compatible_p,function_types_compatible_p,
type_lists_compatible_p): Introduce a structure argument.
(alloc_tagged_tu_seen_cache,free_all_tagged_tu_seen_up_to):
Reorganize cache and remove tu from function names.
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index 47fa36f4ec8..ebc9ba88afe 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -90,12 +90,11 @@ static bool require_constexpr_value;
static bool null_pointer_constant_p (const_tree);
static tree qualify_type (tree, tree);
-static int tagged_types_tu_compatible_p (const_tree, const_tree, bool *,
-bool *);
+struct comptypes_data;
+static int tagged_types_compatible_p (const_tree, const_tree, struct
comptypes_data *);
static int comp_target_types (location_t, tree, tree);
-static int function_types_compatible_p (const_tree, const_tree, bool *,
- bool *);
-static int type_lists_compatible_p (const_tree, const_tree, bool *, bool *);
+static int function_types_compatible_p (const_tree, const_tree, struct
comptypes_data *);
+static int type_lists_compatible_p (const_tree, const_tree, struct
comptypes_data *);
static tree lookup_field (tree, tree);
static int convert_arguments (location_t, vec, tree,
vec *, vec *, tree,
@@ -125,7 +124,7 @@ static tree find_init_member (tree, struct obstack *);
static void readonly_warning (tree, enum lvalue_use);
static int lvalue_or_else (location_t, const_tree, enum lvalue_use);
static void record_maybe_used_decl (tree);
-static int comptypes_internal (const_tree, const_tree, bool *, bool *);
+static int comptypes_internal (const_tree, const_tree, struct comptypes_data
*data);
/* Return true if EXP is a null pointer constant, false otherwise. */
@@ -189,17 +188,16 @@ remove_c_maybe_const_expr (tree expr)
/* This is a cache to hold if two types are compatible or not. */
-struct tagged_tu_seen_cache {
- const struct tagged_tu_seen_cache * next;
+struct tagged_seen_cache {
+ const struct tagged_seen_cache * next;
const_tree t1;
const_tree t2;
- /* The return value of tagged_types_tu_compatible_p if we had seen
+ /* The return value of tagged_types_compatible_p if we had seen
these two types already. */
int val;
};
-static const struct tagged_tu_seen_cache * tagged_tu_seen_base;
-static void free_all_tagged_tu_seen_up_to (const struct tagged_tu_seen_cache
*);
+static void free_all_tagged_seen (const struct tagged_seen_cache *);
/* Do `exp = require_complete_type (loc, exp);' to make sure exp
does not have an incomplete type. (That includes void types.)
@@ -1049,6 +1047,14 @@ common_type (tree t1, tree t2)
return c_common_type (t1, t2);
}
+struct comptypes_data {
+
+ const struct tagged_seen_cache *seen_base;
+
+ bool enum_and_int_p;
+ bool different_types_p;
+};
+
/* Return 1 if TYPE1 and TYPE2 are compatible types for assignment
or various other operations. Return 2 if they are compatible
but a warning may be needed if you use them together. */
@@ -1056,11 +1062,12 @@ common_type (tree t1, tree t2)
int
comptypes (tree type1, tree type2)
{
- const struct tagged_tu_seen_cache * tagged_tu_seen_base1 =
tagged_tu_seen_base;
int val;
- val = comptypes_internal (type1, type2, NULL, NULL);
- free_all_tagged_tu_seen_up_to (tagged_tu_seen_base1);
+ struct comptypes_data data = { };
+ val = comptypes_internal (type1, type2, );
+
+ free_all_tagged_seen (data.seen_base);
return val;
}
@@ -1071,11 +1078,13 @@ comptypes (tree type1, tree type2)
int
comptypes_check_enum_int (tree type1, tree type2, bool *enum_and_int_p)
{
- const struct tagged_tu_seen_cache * tagged_tu_seen_base1 =
tagged_tu_seen_base;
int val;
- val = comptypes_internal (type1, type2, enum_and_int_p, NULL);
- free_all_tagged_tu_seen_up_to (tagged_tu_seen_base1);
+ struct comptypes_data data = { };
+ val = comptypes_internal (type1, type2, );
+ *enum_and_int_p = data.enum_and_int_p;
+
+ free_all_tagged_seen (data.seen_base);
return val;
}
@@ -1087,31 +1096,33 @@ int
comptypes_check_different_types (tree type1, tree type2,
bool *different_types_p)
{
- const struct tagged_tu_seen_cache * tagged_tu_seen_base1 =
tagged_tu_seen_base;
int val;
- val
Re: [C PATCH] remove same_translation_unit_p
Am Dienstag, dem 20.12.2022 um 20:04 + schrieb Joseph Myers: > On Tue, 20 Dec 2022, Martin Uecker via Gcc-patches wrote: > > > Here is a patch to remove the unused function > > same_translation_unit_p and related code. The > > code to check for structural equivalency of > > structs / unions is kept (with some fixes) > > because it will be needed for C2X. > > Could you repost this in development stage 1, since this is not a bug > fix? Sure, I will repost for stage 1. I plan to send some other similar patches with no functional change (with no expectation that they get reviewed now) because I like to get some early feedback rom Martin Liška on some UBsan changes which are layered on top of them. Martin
[C PATCH] remove same_translation_unit_p
Here is a patch to remove the unused function
same_translation_unit_p and related code. The
code to check for structural equivalency of
structs / unions is kept (with some fixes)
because it will be needed for C2X.
c: Remove dead code related to type compatibility across TUs.
Code to detect struct/unions across the same TU is not needed
anymore. Code for determining compatibility of tagged types is
preserved as it will be used for C2X. Some errors in the unused
code are fixed.
Bootstrapped with no regressions for x86_64-pc-linux-gnu.
gcc/c/
* c-decl.cc (set_type_context): Remove.
(pop_scope, diagnose_mismatched_decls, pushdecl):
Remove dead code.
* c-typeck.cc (comptypes_internal): Remove dead code.
(same_translation_unit_p): Remove.
(tagged_types_tu_compatible_p): Some fixes.
diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index e47ca6718b3..307f7a09f5a 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -1155,16 +1155,6 @@ update_label_decls (struct c_scope *scope)
}
}
-/* Set the TYPE_CONTEXT of all of TYPE's variants to CONTEXT. */
-
-static void
-set_type_context (tree type, tree context)
-{
- for (type = TYPE_MAIN_VARIANT (type); type;
- type = TYPE_NEXT_VARIANT (type))
-TYPE_CONTEXT (type) = context;
-}
-
/* Exit a scope. Restore the state of the identifier-decl mappings
that were in effect when this scope was entered. Return a BLOCK
node containing all the DECLs in this scope that are of interest
@@ -1253,7 +1243,6 @@ pop_scope (void)
case ENUMERAL_TYPE:
case UNION_TYPE:
case RECORD_TYPE:
- set_type_context (p, context);
/* Types may not have tag-names, in which case the type
appears in the bindings list with b->id NULL. */
@@ -1361,12 +1350,7 @@ pop_scope (void)
the TRANSLATION_UNIT_DECL. This makes
same_translation_unit_p
work. */
if (scope == file_scope)
- {
DECL_CONTEXT (p) = context;
- if (TREE_CODE (p) == TYPE_DECL
- && TREE_TYPE (p) != error_mark_node)
- set_type_context (TREE_TYPE (p), context);
- }
gcc_fallthrough ();
/* Parameters go in DECL_ARGUMENTS, not BLOCK_VARS, and have
@@ -2308,21 +2292,18 @@ diagnose_mismatched_decls (tree newdecl, tree
olddecl,
{
if (DECL_INITIAL (olddecl))
{
- /* If both decls are in the same TU and the new
declaration
-isn't overriding an extern inline reject the new
decl.
-In c99, no overriding is allowed in the same
translation
-unit. */
- if ((!DECL_EXTERN_INLINE (olddecl)
- || DECL_EXTERN_INLINE (newdecl)
- || (!flag_gnu89_inline
- && (!DECL_DECLARED_INLINE_P (olddecl)
- || !lookup_attribute ("gnu_inline",
-DECL_ATTRIBUTES
(olddecl)))
- && (!DECL_DECLARED_INLINE_P (newdecl)
- || !lookup_attribute ("gnu_inline",
-DECL_ATTRIBUTES
(newdecl
- )
- && same_translation_unit_p (newdecl, olddecl))
+ /* If the new declaration isn't overriding an extern
inline
+reject the new decl. In c99, no overriding is allowed
+in the same translation unit. */
+ if (!DECL_EXTERN_INLINE (olddecl)
+ || DECL_EXTERN_INLINE (newdecl)
+ || (!flag_gnu89_inline
+ && (!DECL_DECLARED_INLINE_P (olddecl)
+ || !lookup_attribute ("gnu_inline",
+ DECL_ATTRIBUTES
(olddecl)))
+ && (!DECL_DECLARED_INLINE_P (newdecl)
+ || !lookup_attribute ("gnu_inline",
+ DECL_ATTRIBUTES
(newdecl)
{
auto_diagnostic_group d;
error ("redefinition of %q+D", newdecl);
@@ -3350,18 +3331,11 @@ pushdecl (tree x)
type to the composite of all the types of that declaration.
After the consistency checks, it will be reset to the
composite of the visible types only. */
- if (b && (TREE_PUBLIC (x) || same_translation_unit_p (x, b-
>decl))
- && b->u.type)
+ if (b && b->u.type)
TREE_TYPE (b->decl) = b->u.type;
- /* The point of the same_translation_unit_p check here is,
-we want to detect a duplicate decl for a construct like
-foo() { extern bar(); } ... static bar(); but not if
-they are in different translation units. In any case,
-the static does not go in the externals scope. */
- if (b
- && (TREE_PUBLIC (x) ||
Re: [PATCH] middle-end IFN_ASSUME support [PR106654]
Am Samstag, den 15.10.2022, 10:53 +0200 schrieb Jakub Jelinek: > On Sat, Oct 15, 2022 at 10:07:46AM +0200, Martin Uecker wrote: > > But why? Do we really want to encourage people to > > write such code? > > Of course these ++ cases inside of expressions are just obfuscation. > But the point is to support using predicates that can be inlined and > simplified into something really simple the optimizers can understand. This makes sense,. > The paper shows as useful e.g. being able to assert something is finite: > [[assume (std::isfinite (x)]]; > and with the recent changes on the GCC side it is now or shortly will be > possible to take advantage of such predicates. > It is true that > [[assume (__builtin_isfinite (x)]]; > could work if we check TREE_SIDE_EFFECTS on the GCC side because > it is a const function, but that is a GNU extension, so the standard > can't count with that. std::isfinite isn't even marked const in libstdc++ > and one can figure that out during IPA propagation only. Hm, that already seems to work with if (!std::isfinite(x)) __builtin_unreachable(); https://godbolt.org/z/hj3WrEhjb > There are many similar predicates, or user could have some that are useful > to his program. And either in the end it wouldn't have side-effects > but the compiler doesn't know, or would but those side-effects would be > unimportant to the optimizations the compiler can derive from those. I still have the feeling that relying on something such as the pure and const attributes might then be a better approach for this. >From the standards point of view, this is OK as GCC can just set its own rules as long as it is a subset of what the standard allows. > As the spec defines it well what happens with the side-effects and it > is an attribute, not a function and the languages have non-evaluated > contexts in other places, I don't see where a user confusion could come. The user confusion might come when somebody writes something such as [[assume(1 == ++i)]] and I expect that people will start doing this once this works. But I am also a a bit worried about the slipperly slope of exploiting this more because what "would evaluate to true" implies in case of I/O, atomic accesses, volatile accesses etc. does not seem clear to me. But maybe I am worrying too much. > We don't warn for sizeof (i++) and similar either. Which is also confusing and clang does indeed warn about it outside of macros and I think GCC should too. > __builtin_assume (i++) is a bad choice because it looks like a function > call (after all, the compilers have many similar builtins) and its argument > looks like normal argument to the function, so it is certainly unexpected > that the side-effects aren't evaluated. I agree. Best Martin
Re: [PATCH] middle-end IFN_ASSUME support [PR106654]
Am Freitag, den 14.10.2022, 23:20 +0200 schrieb Jakub Jelinek:
> On Fri, Oct 14, 2022 at 10:43:16PM +0200, Martin Uecker wrote:
> > Am Montag, den 10.10.2022, 10:54 +0200 schrieb Jakub Jelinek:
> > > Hi!
> > >
> > > My earlier patches gimplify the simplest non-side-effects assumptions
> > > into if (cond) ; else __builtin_unreachable (); and throw the rest
> > > on the floor.
> > > The following patch attempts to do something with the rest too.
> >
> > My recommendation would be to only process side-effect-free
> > assumptions and warn about the rest (clang does this for
> > __builtin_assume). I do not think this is worth the
>
> I think that is bad choice and makes it useless.
I think
[[assume(10 == i + 1)]]
could be useful as it is nicer syntax than
if (10 != i + 1)
unreachable();
but
[[assume(10 == i++)]]
is confusing / untestable and therefor seems a bit
dangerous.
But we do not have to agree, I just stating my opinion
here. I would personally then suggest to have an
option for warning about side effects in assume.
> > complexity and I am not so sure the semantics of a
> > hypothetical evaluation are terribly well defined.
>
> I think the C++23 paper is quite clear. Yes, you can't verify it
> in debug mode, but there are many other UBs that are hard to verify
> through runtime instrumentation.
And none are good. Some are very useful though
(or even required in the context of C/C++). But I think
there should be a very good reason for adding more.
Personally, I do not see [[assume]] how side effects
is useful enough to justify adding another kind of
untestable UB.
Especially also because it has somewhat vaguely
defined semantics. I don't know any formalization the
proposed semantics and the normative wording
"the converted expression would evaluate to true"
is probably good starting point for a PhD thesis.
> And, OpenMP has a similar feature (though, in that case it is even
> a stronger guarantee where something is guaranteed to hold across
> a whole region rather than just on its entry.
>
> > That you can not verify this properly by turning it
> > into traps in debug mode (as this would execute the
> > side effects) also makes this a terrible feature IMHO.
> >
> > MSVC which this feature was based does not seem to make
> > much to sense to me: https://godbolt.org/z/4Ebar3G9b
>
> So maybe their feature is different from what is in C++23,
> or is badly implemented?
The paper says as the first sentence in the abstract:
"We propose a standard facility providing the semantics
of existing compiler built-ins such as
__builtin_assume (Clang) and __assume (MSVC, ICC)."
But Clang does not support side effects and MSVC
is broken. But yes, the paper then describes these
extended semantics for expression with side effects.
According to the author this was based on discussions
with MSVC developers, but - to me - the fact that MSVC
still implements something else which does not seem
to make sense speaks against this feature.
> I think with what we have in the works for GCC we'll be able to optimize
> in
> int f(int i)
> {
> [[assume(1 == i++)]];
> return (1 == i++);
> }
>
> int g(int i)
> {
> [[assume(1 == ++i)]];
> return (1 == ++i);
> }
>
> extern int i;
>
> int h(void)
> {
> [[assume(1 == ++i)]];
> return (1 == ++i);
> }
>
>
> int k(int i)
> {
> [[assume(42 == ++i)]];
> return i;
> }
> at least f/g to return 1; and k to return 41;
> The h case might take a while to take advantage of.
But why? Do we really want to encourage people to
write such code?
Martin
Re: [PATCH] middle-end IFN_ASSUME support [PR106654]
Am Montag, den 10.10.2022, 10:54 +0200 schrieb Jakub Jelinek:
> Hi!
>
> My earlier patches gimplify the simplest non-side-effects assumptions
> into if (cond) ; else __builtin_unreachable (); and throw the rest
> on the floor.
> The following patch attempts to do something with the rest too.
My recommendation would be to only process side-effect-free
assumptions and warn about the rest (clang does this for
__builtin_assume). I do not think this is worth the
complexity and I am not so sure the semantics of a
hypothetical evaluation are terribly well defined.
That you can not verify this properly by turning it
into traps in debug mode (as this would execute the
side effects) also makes this a terrible feature IMHO.
MSVC which this feature was based does not seem to make
much to sense to me: https://godbolt.org/z/4Ebar3G9b
Martin
> For -O0, it actually throws even the simplest assumptions on the floor,
> we don't expect optimizations and the assumptions are there to allow
> optimizations. Otherwise, it keeps the handling of the simplest
> assumptions as is, and otherwise arranges for the assumptions to be
> visible in the IL as
> .ASSUME (_Z2f4i._assume.0, i_1(D));
> call where there is an artificial function like:
> bool _Z2f4i._assume.0 (int i)
> {
> bool _2;
>
>[local count: 1073741824]:
> _2 = i_1(D) == 43;
> return _2;
>
> }
> with the semantics that there is UB unless the assumption function
> would return true.
>
> Aldy, could ranger handle this? If it sees .ASSUME call,
> walk the body of such function from the edge(s) to exit with the
> assumption that the function returns true, so above set _2 [true, true]
> and from there derive that i_1(D) [43, 43] and then map the argument
> in the assumption function to argument passed to IFN_ASSUME (note,
> args there are shifted by 1)?
>
> During gimplification it actually gimplifies it into
> D.2591 = .ASSUME ();
> if (D.2591 != 0) goto ; else goto ;
> :
> {
> i = i + 1;
> D.2591 = i == 44;
> }
> :
> .ASSUME (D.2591);
> with the condition wrapped into a GIMPLE_BIND (I admit the above isn't
> extra clean but it is just something to hold it from gimplifier until
> gimple low pass; it reassembles if (condition_never_true) { cond; };
> an alternative would be introduce GOMP_ASSUME statement that would have
> the guard var as operand and the GIMPLE_BIND as body, but for the
> few passes (tree-nested and omp lowering) in between that looked like
> an overkill to me) which is then pattern matched during gimple lowering
> and outlined into a separate function. Variables declared inside of the
> condition (both static and automatic) just change context, automatic
> variables from the caller are turned into parameters (note, as the code
> is never executed, I handle this way even non-POD types, we don't need to
> bother pretending there would be user copy constructors etc. involved).
>
> The assume_function artificial functions are then optimized until RTL
> expansion, which isn't done for them nor any later pass, on the other
> side bodies are not freed when done.
>
> Earlier version of the patch has been successfully bootstrapped/regtested
> on x86_64-linux and i686-linux and all assume tests have passed even with
> this version. Ok for trunk if it succeeds on another bootstrap/regtest?
>
> There are a few further changes I'd like to do, like ignoring the
> .ASSUME calls in inlining size estimations (but haven't figured out where
> it is done), or for LTO arrange for the assume functions to be emitted
> in all partitions that reference those (usually there will be just one,
> unless code with the assumption got inlined, versioned etc.).
>
> 2022-10-10 Jakub Jelinek
>
> PR c++/106654
> gcc/
> * function.h (struct function): Add assume_function bitfield.
> * gimplify.cc (gimplify_call_expr): For -O0, always throw
> away assumptions on the floor immediately. Otherwise if the
> assumption isn't simple enough, expand it into IFN_ASSUME
> guarded block.
> * gimple-low.cc (create_assumption_fn): New function.
> (struct lower_assumption_data): New type.
> (find_assumption_locals_r, assumption_copy_decl,
> adjust_assumption_stmt_r, adjust_assumption_stmt_op,
> lower_assumption): New functions.
> (lower_stmt): Handle IFN_ASSUME guarded block.
> * tree-ssa-ccp.cc (pass_fold_builtins::execute): Remove
> IFN_ASSUME calls.
> * lto-streamer-out.cc (output_struct_function_base): Pack
> assume_function bit.
> * lto-streamer-in.cc (input_struct_function_base): And unpack it.
> * cgraphunit.cc (cgraph_node::expand): Don't verify assume_function
> has TREE_ASM_WRITTEN set and don't release its body.
> * cfgexpand.cc (pass_expand::execute): Don't expand assume_function
> into RTL, just destroy loops and exit.
> * internal-fn.cc (expand_ASSUME): Remove gcc_unreachable.
> * passes.cc
Re: [PATCH, CFE] N2863: Improved Rules for Tag Compatibility
Am Dienstag, den 07.06.2022, 14:22 + schrieb Joseph Myers:
> On Tue, 7 Jun 2022, Martin Uecker wrote:
>
> > here is a preliminary patch the implements the proposed
> > tag compatibility rules for C23 in GCC (N2863). It works
>
> I don't see any response on the reflector to my comments on that proposal
> (message 21374, Fri, 14 Jan 2022 23:32:47 +). Nor do I see any tests
> in this patch dealing with the questions of exactly when struct and union
> types are complete or incomplete, as in my first comment there (if there
> are any tests concerning that, it's not apparent for lack of comments
> explaining what exactly the tests are trying to test). I think we'll need
> a version of the proposal without known issues before the patch is fully
> reviewable.
Thanks Joseph! I will revisit the wording next and then resend
the patch with the corresponing tests.
Martin
>
> > - the feature has a flag (-ftag-compat) which is now turned
> > on by default in all language modes to facilitate testing
> > and to identify backwards compatibility problems. Turned on,
> > it survives bootstrapping and regression testing with
> > only a few cases that test for diagnostics that go
> > away changed to turn it off.
>
> Turning on by default in past language modes seems questionable other than
> for this sort of preliminary testing (in any case, incompatible with
> previous standard requirements so can't be enabled for strict conformance
> modes).
> (And in general I'd discourage adding options for individual
> language feature like that, with the resulting proliferation of dialects
> with different combinations of features - again, it may be useful for
> testing purposes, especially before we know whether the feature gets into
> C23, and it may be useful within the compiler sources to distinguish in
> some way which places are checking for this feature rather than just
> testing flag_isoc2x, but actually releasing with a command-line option for
> it is more problematic.)
>
> > diff --git a/gcc/testsuite/gcc.dg/tag-compat2.c
> > b/gcc/testsuite/gcc.dg/tag-compat2.c
> > new file mode 100644
> > index 000..20dc1a9c894
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/tag-compat2.c
> > @@ -0,0 +1,47 @@
> > +/*
> > + * { dg-do compile }
> > + * { dg-options "-ftag-compat" }
> > + */
> > +
> > +typedef struct bar { int x; } X;
> > +typedef struct bar { float x; } Y; /* { dg-warning "redefinition of
> > struct or union" } */
>
> I'd expect conflicting definitions of a type in the same scope to remain
> errors, not warnings, regardless of this feature.
>
[PATCH, CFE] N2863: Improved Rules for Tag Compatibility
Hello Joseph and all,
here is a preliminary patch the implements the proposed
tag compatibility rules for C23 in GCC (N2863). It works
by tweaking the diagnostics in the FE and by recording
structs/union types to be able to set TYPE_CANONICAL to
a canonical definition (as per previous discussions).
Overall, this seems to work very well when testing
on my own projects. There are still some issues
left that I want to point out:
- at the moment, all struct/union types are collected
in a vector. This needs to be replaced by a hash table.
- the feature has a flag (-ftag-compat) which is now turned
on by default in all language modes to facilitate testing
and to identify backwards compatibility problems. Turned on,
it survives bootstrapping and regression testing with
only a few cases that test for diagnostics that go
away changed to turn it off.
- The new rules are not applied to structs with variable
sized members (which are a GNU extension).
- In contrast to the published proposal, structs without
tags are now treated as incompatible as requested by WG14.
- There is still one assertion in ipa-free-lang-data I had
to conditionally turn off and did not have time to
investigate. Otherwise, there are only C FE changes.
LTO may still need some more testing.
- It fixes some bugs in (formerly) unused FE code
and removes some other dead code. This could be
moved into its own patch.
- If adopted into C, I assume we need some
compatibility warnings. From testing, I could
not identify any backwards compatibility problems.
- There are certainly some issues I may have
overlooked.
Martin
gcc/
* c-family/c.opt: Add -ftag-compat flag.
* c/c-decl.cc (pop_scope): Remove dead code.
(diagnose_mismatched_decls): Support for
new tag compatibility rules.
(start_struct): Dito.
(finish_struct): Dito.
(start_enum): Dito.
(finish_enum): Dito.
(build_enumerator): Pass enumtype to build_decl.
(c_simulate_enum_decl): Pass enumtype to
build_enumerator.
* c/c-parser.cc (c_parser_enum_specifier): Dito.
* c/c-tree.h (build_enumerator): Add enumtype
argument.
* c/c-typeck.cc (comptypes_internal): Support
for new tag compatibility rules.
(same_translation_unit_p): Removed.
(tagged_types_tu_compatible_p): Bug fixes and
support for new tag compatibility rules.
(convert_for_assignment): Support for new tag
compatibility rules
(digest_init): Dito.
* ipa-free-lang-data.cc (fld_incomplete_type_of):
Conditionally turn of assertion related to
TYPE_CANONICAL if -ftag-compat is on.
doc/
* invoke.texi: Document -ftag-compat flag.
testsuite/
* gcc.dg/asan/pr81460.c: Add -fno-tag-compat.
* gcc.dg/c99-tag-1.c: Add -fno-tag-compat.
* gcc.dg/c99-tag-2.c: Add -fno-tag-compat.
* gcc.dg/decl-3.c: Add -fno-tag-compat.
* gcc.dg/enum-redef-1.c: Add -fno-tag-compat.
* gcc.dg/parm-incomplete-1.c: Add -fno-tag-compat.
* gcc.dg/pr17188-1.c: Add -fno-tag-compat.
* gcc.dg/pr18809-1.c: Add -pedantic-errors and
-fno-tag-compat.
* gcc.dg/pr27953.c: Add -fno-tag-compat.
* gcc.dg/pr39084.c: Add -fno-tag-compat.
* gcc.dg/pr68533.c: Add -fno-tag-compat.
* gcc.dg/pr79983.c: Add -fno-tag-compat.
* gcc.dg/pr89211.c: Add -fno-tag-compat.
* gcc.dg/tag-compat.c: New test.
* gcc.dg/tag-compat10.c: New test.
* gcc.dg/tag-compat11.c: New test.
* gcc.dg/tag-compat12.c: New test.
* gcc.dg/tag-compat2.c: New test.
* gcc.dg/tag-compat3.c: New test.
* gcc.dg/tag-compat4.c: New test.
* gcc.dg/tag-compat5.c: New test.
* gcc.dg/tag-compat6.c: New test.
* gcc.dg/tag-compat7.c: New test.
* gcc.dg/tag-compat8.c: New test.
* gcc.dg/tag-compat9.c: New test.
* gcc.dg/vla-11.c: Add -fno-tag-compat.
* gcc.dg/vla-stexp-2.c: Add -fno-tag-compat.
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 41a20bc625e..cd3164018f2 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -2108,6 +2108,9 @@ Enum(strong_eval_order) String(some) Value(1)
EnumValue
Enum(strong_eval_order) String(all) Value(2)
+ftag-compat
+C Var(flag_tag_compat) Init(1)
+
ftemplate-backtrace-limit=
C++ ObjC++ Joined RejectNegative UInteger
Var(template_backtrace_limit) Init(10)
Set the maximum number of template instantiation notes for a single
warning or error.
diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index 5266a61b859..df208a310f9 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -599,6 +599,10 @@ public:
auto_vec typedefs_seen;
};
+
+/* All tagged typed so that TYPE_CANONICAL can be set correctly. */
+static auto_vec all_structs;
+
/* Information for the struct or union currently being parsed, or
NULL if not parsing a struct or union. */
static class c_struct_parse_info *struct_parse_info;
@@ -1354,8 +1358,8 @@ pop_scope (void)
BLOCK_VARS (block) = extp;
}
/* If this is the file scope set DECL_CONTEXT of each decl
to
-the TRANSLATION_UNIT_DECL. This makes
same_translation_unit_p
-work. */
+the TRANSLATION_UNIT_DECL. */
+
if (scope == file_scope)
{
Re: [PATCH 0/2] tree-optimization/104530 - proposed re-evaluation.
> > On 2/22/2022 10:57 AM, Jakub Jelinek via Gcc-patches wrote: > > On Tue, Feb 22, 2022 at 12:39:28PM -0500, Andrew MacLeod wrote: > >>> That is EH, then there are calls that might not return because they leave > >>> in some other way (e.g. longjmp), or might loop forever, might exit, might > >>> abort, trap etc. > >> Generally speaking, calls which do not return should not now be a > >> problem... > >> as long as they do not transfer control to somewhere else in the current > >> function. > > I thought all of those cases are very relevant to PR104530. > > If we have: > >_1 = ptr_2(D) == 0; > >// unrelated code in the same bb > >_3 = *ptr_2(D); > > then in light of PR104288, we can optimize ptr_2(D) == 0 into true only if > > there are no calls inside of "// unrelated code in the same bb" > > or if all calls in "// unrelated code in the same bb" are guaranteed to > > return exactly once. Because, if there is a call in there which could > > exit (that is the PR104288 testcase), or abort, or trap, or loop forever, > > or throw externally, or longjmp or in any other non-UB way > > cause the _1 = ptr_2(D) == 0; stmt to be invoked at runtime but > > _3 = *ptr_2(D) not being invoked, then we can't optimize the earlier > > comparison because ptr_2(D) could be NULL in a valid program. > > While if there are no calls (and no problematic inline asms) and no trapping > > insns in between, we can and PR104530 is asking that we continue to optimize > > that. > Right. This is similar to some of the restrictions we deal with in the > path isolation pass. Essentially we have a path, when traversed, would > result in a *0. We would like to be able to find the edge upon-which > the *0 is control dependent and optimize the test so that it always went > to the valid path rather than the *0 path. > > The problem is there may be observable side effects on the *0 path > between the test and the actual *0 -- including calls to nonreturning > functions, setjmp/longjmp, things that could trap, etc. This case is > similar. We can't back-propagate the non-null status through any > statements with observable side effects. There are cases with volatile accesses where this is currently not the case. Also there is a proposal for C++ to require an explicit std::observable fence to make sure observable side effects are not undone by later UB. (see the discussion about "reordering of trapping operations and volatile" in Janunary on the gcc list) In my opinion it is much better if a compiler makes sure to preserve observable side effects even in code path with later UB (because the behavior may otherwise be surprising and existing code may be broken in a subtle way). If I understand you correctly, GCC intends to do this. Can we then also agree that the remaining volatile cases used be fixed? Martin
[C PATCH] Evaluate argument of sizeof that are structs of variable size.
Joseph,
here is the patch for c_expr_sizeof_type you had suggested.
Best,
Martin
Evaluate type arguments of sizeof that are structs of variable size [PR101838]
Evaluate type arguments of sizeof for all types of variable size
and not just for VLAs. This fixes PR101838 and some issues related
to PR29970 where statement expressions need to be evaluated so that
the size is well defined.
2021-08-12 Martin Uecker
gcc/c/
PR c/101838
PR c/29970
* c-typeck.c (c_expr_sizeof_type): Evaluate
size expressions for structs of variable size.
gcc/testsuite/
PR c/101838
* gcc.dg/vla-stexp-2.c: New test.
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index c5bf3372321..eb5c87dc57a 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -3022,8 +3022,14 @@ c_expr_sizeof_type (location_t loc, struct c_type_name
*t)
c_last_sizeof_loc = loc;
ret.original_code = SIZEOF_EXPR;
ret.original_type = NULL;
+ if (type == error_mark_node)
+{
+ ret.value = error_mark_node;
+ ret.original_code = ERROR_MARK;
+}
+ else
if ((type_expr || TREE_CODE (ret.value) == INTEGER_CST)
- && c_vla_type_p (type))
+ && C_TYPE_VARIABLE_SIZE (type))
{
/* If the type is a [*] array, it is a VLA but is represented as
having a size of zero. In such a case we must ensure that
diff --git a/gcc/testsuite/gcc.dg/vla-stexp-2.c
b/gcc/testsuite/gcc.dg/vla-stexp-2.c
new file mode 100644
index 000..4d4a15d34a6
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vla-stexp-2.c
@@ -0,0 +1,33 @@
+/* PR101838-*/
+/* { dg-do run } */
+/* { dg-options "-Wpedantic -O0" } */
+
+
+int bar0(
+ int (*a)[*],
+ int (*b)[sizeof(*a)]
+);
+
+
+int bar(
+ struct f { /* { dg-warning "will not be visible outside of
this definition" } */
+ int a[*]; } v, /* { dg-warning "variably modified type" } */
+ int (*b)[sizeof(struct f)] // should not warn about zero size
+);
+
+int foo(void)
+{
+ int n = 0;
+ return sizeof(typeof(*({ n = 10; struct foo { /* { dg-warning
"braced-groups" } */
+ int x[n]; /* { dg-warning
"variably modified type" } */
+ } x; })));
+}
+
+
+int main()
+{
+ if (sizeof(struct foo { int x[10]; }) != foo())
+ __builtin_abort();
+
+ return 0;
+}
Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]
Am Montag, den 09.08.2021, 15:52 +0200 schrieb Eric Botcazou:
> > I think the patch makes sense but the comment says
> >
> > Java requires that we elaborated nodes in source order. That
> > means we must gimplify the inner expression followed by each
> > of
> > the indices, in order. But we can't gimplify the inner
> > expression until we deal with any variable bounds, sizes, or
> > positions in order to deal with PLACEHOLDER_EXPRs.
> >
> > and I don't really understand that (not to say Java is no longer
> > supported
> > by GCC, but Ada does use PLACEHOLDER_EXPRs). In fact the comment
> > suggests that we _should_ gimplify the inner expression first ...
> > at least
> > it seems to after your explanations ;)
>
> We already gimplify the inner expression first, i.e. before indices
> and operands, but we gimplify the "annotations" (special operands)
> before.
>
> > Eric - do you know anything about this?
>
> If the comment is right, then Martin's patch should break Ada indeed.
>
Yes, it breaks Ada. I already found this out
in the meanwhile.
But I do not understand how this works with the
PLACEHOLDER_EXPR.
My understanding of this is that this is for referring
to some field of an outer struct which is then used in the
size expression, e.g. something like this (using
C syntax):
struct foo {
int len;
float (*x)[3][len];
};
And then for
struct foo* p;
p->x[1][1];
we use the field 'len' of the struct pointed to by 'p'
the size expression.
But then why would you gimplify the size expression before
the base expression? I would assume that you also want
to process the base expression first, because it is the
source of the struct which we access in the size expression.
Best,
Martin
Re: Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]
Am Dienstag, den 03.08.2021, 11:26 +0200 schrieb Richard Biener:
> On Tue, Aug 3, 2021 at 10:28 AM Martin Uecker wrote:
>
> Does the same issue arise with writing the testcases as
>
> ({ ... }) + i;
>
> ? How can we fix it then if you also need to support
>
> i + ({ ...});
>
> ?
Here, the FE always moves the pointer to the right and
then produces something like:
*((int *) TARGET_EXPR + ((sizetype) SAVE_EXPR + 1) * 20);
So these are the cases which already work without
the path, although maybe it is wrong to have
the n in the SAVE_EXPR?
It gets gimplified to something like this,
which works:
int[0:D.1959][0:D.1955] * D.1952;
int n.0;
sizetype D.1955;
sizetype D.1959;
{
int n;
int[0:D.1959][0:D.1955] * x;
n = 10;
n.0 = n;
...
_32 = (sizetype) n.0;
_33 = (sizetype) n.1;
_34 = _32 * _33;
_35 = _34 * 4;
x = __builtin_malloc (_35);
D.1952 = x;
}
_36 = (sizetype) n.0;
_37 = _36 + 1;
_38 = _37 * 20;
_39 = D.1952 + _38;
For the array ref, the FE produces:
(*TARGET_EXPR )[5][5];
With the patch, we get something like
the following in GIMPLE, which seems correct:
int[0:D.1958][0:D.1954] * D.1951;
int n.0;
sizetype D.1954;
{
int n;
int[0:D.1958][0:D.1954] * x;
n = 10;
n.0 = n;
_7 = (sizetype) n.0;
_8 = _7 * 4;
D.1956 = _8;
n.1 = n
_22 = (sizetype) n.0;
_23 = (sizetype) n.1;
_24 = _22 * _23;
_25 = _24 * 4;
x = __builtin_malloc (_25);
D.1951 = x;
}
_26 = D.1956 /[ex] 4;
c = (*D.1951)[5]{lb: 0 sz: _26 * 4}[5];
MArtin
Re: Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]
Am Dienstag, den 03.08.2021, 11:26 +0200 schrieb Richard Biener:
> On Tue, Aug 3, 2021 at 10:28 AM Martin Uecker
> wrote:
> >
> > Hi
> > Am Dienstag, den 03.08.2021, 10:10 +0200 schrieb Richard Biener:
> > > On Tue, Aug 3, 2021 at 7:32 AM Martin Uecker
> > > wrote:
> > > >
> > > > (resending from a different account, as emails seem to do not
> > > > go out from my other account at this time)
> > > >
> > > > Am Montag, den 02.08.2021, 16:05 +0200 schrieb Martin Uecker:
> > > > > > On Sun, Aug 1, 2021 at 7:37 PM Uecker, Martin
> > > > > > wrote:
> > > > > > > Here is an attempt to fix some old and annoying bugs
> > > > > > > related
> > > > > > > to VLAs and statement expressions. In particulary, this
> > > > > > > seems
> > > > > > > to fix the issues with variably-modified types which are
> > > > > > > returned from statement expressions (which works on
> > > > > > > clang),
> > > > > > > but there are still bugs remaining related to structs
> > > > > > > with VLA members (which seems to be a FE bug).
> > > > > > >
> > > > > > > Of course, I might be doing something stupid...
> > > > > >
> > > > > > How's evaluation order of (f())[g()] defined (with f
> > > > > > returning
> > > > > > a
> > > > > > pointer)?
> > > > > > Isn't that just f() + g()*sizeof(int) and thus undefined?
> > > > >
> > > > > Yes, in C it is
> > > > >
> > > > > f() + g()
> > > > >
> > > > > and it is unsequenced. But the order of 'f' and 'g'
> > > > > is not relevant here and also the patch does not change
> > > > > it (the base expression is gimplified before the index).
> > > > >
> > > > > Essentially, we have
> > > > >
> > > > > ({ ... }) + g() * sizeof(X)
> > > > >
> > > > > where X refers to a declaration in the statement expression.
> > > > > Without the patch the size expressions are gimplified before
> > > > > the base expression and also before the index expression.
> > > > > With the patch the ({ ... }) is gimplified also before the
> > > > > size expression.
> > > > >
> > > > > > If it's undefined then I think the incoming GENERIC is ill-
> > > > > > defined.
> > > > >
> > > > > I think it is OK because the arguments are evaluated
> > > > > before the operation. Without the patch, parts of the
> > > > > operation (the size expressions) are gimplified before
> > > > > the arguments and this seems wrong to me.
> > >
> > > But you said the evaluation order is undefined.
> >
> > The evaluation order of the two arguments (base
> > and index) is undefined. But the operation itself has
> > to happen after the arguments are evaluated like
> > the call to a is sequenced before f and g:
> >
> > a(f(), g())
> >
> >
> > Computing the correct step size in the pointer
> > arithmetic is part of the operation itself and not
> > part of the evaluation of the arguments.
> >
> > The problem here is that this part of the operation
> > is done before the arguments are evaluated, which
> > is a compiler bug.
>
> Yes, but the bug is IMHO in the C frontend which inserts the
> DECL_EXPR at a wrong spot, not making sure it is evaluated before it
> is used. Working around this deficiency in the gimplifier sounds
> incorrect.
The size if part of the type of the value which is
returned from the compound expression.
So that there is a declaration involved was maybe
misleading in my example and explanation.
So let me try again:
The size *expression* needs be be computed inside the
statement expression (also because of side effects)
and then the result of this computation needs to
returned together (as part of) the value returned
by the statement expression.
But the compilers tries to gimplify the size expression
that comes with the value already before the statement
expression is evaluated. This can not work, no matter
what the front end does.
>
> Does the same issue arise with writing the testcases as
>
> ({ ... }) + i;
>
> ? How can we fix it then if you also need to support
>
> i + ({ ...});
>
>
> ?
This already works correctly. I assume that here
the gimplifcation is already done in the right
order.
But ARRAY_REF is handled as a separate case and
there it is wrong.
Martin
>
> > > So IMHO the GENERIC is undefined in evaluating the size of sth
> > > that's not live?
> > >
> > > That said, given the statement expression
> > > result undergoes array to pointer decay doesn't this pointer
> > > refer to an object that ended its lifetime?
> > > "In a statement expression, any temporaries created within a
> > > statement are destroyed at that statement's end."
> > > That is, don't the testcases all invoke undefined behavior at
> > > runtime?
> >
> > This is true for one of the test cases (where not having
> > an ICE is then
> > a QoI issue), but not for the others
> > where the object is allocated by
> > malloc and a pointer
> > to the object is returned from the statement
> > expression.
> > This is supposed to work.
> >
> >
> > Martin
> >
> >
> >
Re: Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]
Hi
Am Dienstag, den 03.08.2021, 10:10 +0200 schrieb Richard Biener:
> On Tue, Aug 3, 2021 at 7:32 AM Martin Uecker wrote:
> >
> >
> > (resending from a different account, as emails seem to do not
> > go out from my other account at this time)
> >
> > Am Montag, den 02.08.2021, 16:05 +0200 schrieb Martin Uecker:
> > > > On Sun, Aug 1, 2021 at 7:37 PM Uecker, Martin
> > > > wrote:
> > > > >
> > > > > Here is an attempt to fix some old and annoying bugs related
> > > > > to VLAs and statement expressions. In particulary, this seems
> > > > > to fix the issues with variably-modified types which are
> > > > > returned from statement expressions (which works on clang),
> > > > > but there are still bugs remaining related to structs
> > > > > with VLA members (which seems to be a FE bug).
> > > > >
> > > > > Of course, I might be doing something stupid...
> > > >
> > > > How's evaluation order of (f())[g()] defined (with f returning
> > > > a
> > > > pointer)?
> > > > Isn't that just f() + g()*sizeof(int) and thus undefined?
> > >
> > > Yes, in C it is
> > >
> > > f() + g()
> > >
> > > and it is unsequenced. But the order of 'f' and 'g'
> > > is not relevant here and also the patch does not change
> > > it (the base expression is gimplified before the index).
> > >
> > > Essentially, we have
> > >
> > > ({ ... }) + g() * sizeof(X)
> > >
> > > where X refers to a declaration in the statement expression.
> > > Without the patch the size expressions are gimplified before
> > > the base expression and also before the index expression.
> > > With the patch the ({ ... }) is gimplified also before the
> > > size expression.
> > >
> > > > If it's undefined then I think the incoming GENERIC is ill-
> > > > defined.
> > >
> > > I think it is OK because the arguments are evaluated
> > > before the operation. Without the patch, parts of the
> > > operation (the size expressions) are gimplified before
> > > the arguments and this seems wrong to me.
>
> But you said the evaluation order is undefined.
The evaluation order of the two arguments (base
and index) is undefined. But the operation itself has
to happen after the arguments are evaluated like
the call to a is sequenced before f and g:
a(f(), g())
Computing the correct step size in the pointer
arithmetic is part of the operation itself and not
part of the evaluation of the arguments.
The problem here is that this part of the operation
is done before the arguments are evaluated, which
is a compiler bug.
> So IMHO the GENERIC is undefined in evaluating the size of sth
> that's not live?
>
> That said, given the statement expression
> result undergoes array to pointer decay doesn't this pointer
> refer to an object that ended its lifetime?
> "In a statement expression, any temporaries created within a
> statement are destroyed at that statement's end."
> That is, don't the testcases all invoke undefined behavior at
> runtime?
This is true for one of the test cases (where not having
an ICE is then
a QoI issue), but not for the others
where the object is allocated by
malloc and a pointer
to the object is returned from the statement
expression.
This is supposed to work.
Martin
Re: Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]
(resending from a different account, as emails seem to do not
go out from my other account at this time)
Am Montag, den 02.08.2021, 16:05 +0200 schrieb Martin Uecker:
> > On Sun, Aug 1, 2021 at 7:37 PM Uecker, Martin
> > wrote:
> > >
> > >
> > > Here is an attempt to fix some old and annoying bugs related
> > > to VLAs and statement expressions. In particulary, this seems
> > > to fix the issues with variably-modified types which are
> > > returned from statement expressions (which works on clang),
> > > but there are still bugs remaining related to structs
> > > with VLA members (which seems to be a FE bug).
> > >
> > > Of course, I might be doing something stupid...
> >
> > How's evaluation order of (f())[g()] defined (with f returning a
> > pointer)?
> > Isn't that just f() + g()*sizeof(int) and thus undefined?
>
> Yes, in C it is
>
> f() + g()
>
> and it is unsequenced. But the order of 'f' and 'g'
> is not relevant here and also the patch does not change
> it (the base expression is gimplified before the index).
>
> Essentially, we have
>
> ({ ... }) + g() * sizeof(X)
>
> where X refers to a declaration in the statement expression.
> Without the patch the size expressions are gimplified before
> the base expression and also before the index expression.
> With the patch the ({ ... }) is gimplified also before the
> size expression.
>
> > If it's undefined then I think the incoming GENERIC is ill-defined.
>
> I think it is OK because the arguments are evaluated
> before the operation. Without the patch, parts of the
> operation (the size expressions) are gimplified before
> the arguments and this seems wrong to me.
>
>
> Martin
>
>
>
>
Re: [PATCH] Document that -fno-trampolines is for Ada only [PR100735]
> > Jeff et. al. > > > On 9 Jun 2021, at 17:23, Jeff Law via Gcc-patches > > wrote: > > On 5/25/2021 2:23 PM, Paul Eggert wrote: > >> The GCC manual's documentation of -fno-trampolines was apparently > >> written from an Ada point of view. However, when I read it I > >> understandably mistook it to say that -fno-trampolines also works for > >> C, C++, etc. It doesn't: it is silently ignored for these languages, > >> and I assume for any language other than Ada. > >> > >> This confusion caused me to go in the wrong direction in a Gnulib > >> dicussion, as I mistakenly thought that entire C apps with nested > >> functions could be compiled with -fno-trampolines and then use nested > >> C functions in stack overflow handlers where the alternate stack > >> is allocated via malloc. I was wrong, as this won't work on common > >> platforms like x86-64 where malloc yields non-executable storage. > >> > >> gcc/ > >> * doc/invoke.texi (Code Gen Options): > >> * doc/tm.texi.in (Trampolines): > >> Document that -fno-trampolines and -ftrampolines work > >> only with Ada. > > So Martin Uecker probably has the most state on this. IIRC when we last > > discussed -fno- > trampolines the belief was that it could be easily made to work independent > of the language, but > that it was ultimately an ABI change. That ultimately derailed plans to use > -fno-trampolines for > other languages in the immediate term. > > This is correct, it’s not technically too hard to make it work for another > language (I have a hack > in my arm64-darwin branch that does this for gfortran). As noted for most > ports it is an ABI > break and thus not usable outside a such a work-around. > > For the record (for the arm64-darwin port in the first instance), together > with some of my friends > at embecosm we plan to implement a solution to the trampoline that does not > require executable > stack and does not require an ABI break. Perhaps such a solution will be of > interest to other > ports that do not want executable stack. > > We’re not quite ready to post the design yet - but will do so in the next few > weeks (all being > well). > For reference the discussion and PATCH for C can be found here: https://gcc.gnu.org/legacy-ml/gcc-patches/2018-12/msg01532.html FWIW: There is a proposal discussed in WG14 to add lambdas. In this context a more general and portable solution to this problem would also be useful. One related idea is to add a wider function pointer type to C that includes a data pointer. A regular function pointer could be converted to the wider pointer but back conversion only works if it originally was a regular function pointer. This pointer type could then also be used for interoperablity with other languages that have callable objects or closures. If GCC could add something like this and there is then implementation experience, we could later try to standardize it. (there was also positive feedback to this idea from some C++ people I spoke to) Martin
Re: [RFC, PATCH] nonzero attribute, static array parameter
Hi Marek,
sorry for taking so long to respond.
Fri, 15 May 2015 15:38:43 +0200
Marek Polacek pola...@redhat.com:
On Sat, May 09, 2015 at 09:42:23AM -0700, Martin Uecker wrote:
here is a tentative patch to implement a new attribute nonzero,
which is similar to nonnull, but is not a function attribute
but a type attribute.
One reason is that nonnull is awkward to use. For this reason,
clang allows the use of nonnull in function parameters, but this
is incompatible with old and current use of this attribute in gcc
(though in a rather obscure use case).
See: https://gcc.gnu.org/ml/gcc/2015-05/msg00077.html
Sorry, I quite fail to see how such an attribute would be useful.
First of all, it makes it much simpler to implement the desired warnings
and ubsan checks when passing NULL as an array parameters with 'static'.
It avoids implementing all this argument walking code which is needed
for nonnull. If we do not want a nonzero attribute, could we then
consider an internal attribute (e.g. non zero)?
It seems that the nonzero warning can only ever trigger when used
on function parameters or on a return value, much as the nonnull /
returns_nonnull attributes.
So far. But in the future this could be extended to other
cases where it makes sense.
The difference is that you can use
the nonzero attribute on a particular function parameter, but the
nonnull attribute has this ability as well:
__attribute__ ((nonnull (1))) void foo (int *, int *);
void
bar (void)
{
foo (0, 0);
}
I know. But this is ugly, cumbersome, and fragile, and some uses
are incompatible with clang. That nonnull is mis-designed
has been pointed out before:
https://gcc.gnu.org/ml/gcc/2006-04/msg00551.html
Unlike nonnull, nonzero attribute can be attached to a typedef, but
it doesn't seem to buy you anything you couldn't do with the nonnull /
returns_nonnull attributes.
It is much more expressive. A nonnull pointer _type_ seems really
useful to me. Many programming languages have something like this.
The nonzero attribute can appertain even to integer types, not only
pointer types. Can you give an example where this would come in handy?
It doesn't seem too useful to me.
One example:
void assert(__attribute__((nonzero)) int i);
would give a warning if it is already known at compile time
that the expression is zero.
+void foo1(int x[__attribute__((nonzero))]);
This looks weird,
It does look weird. But this is already documented in this way:
https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html#Attribute-Syntax
The use of 'const' and 'static' in an array declarator as
defined by ISO C99 is already weird.
the placement of the nonzero attribute here suggests
that the array should have at least zero elements (the same that
int x[static 0] does), but in fact it checks that the pointer passed
to foo1 is non-NULL, i.e. something that could be easily achieved
with the nonnull attribute.
In many case it would be much nicer to be able to declare a
nonzero type and use it in interfaces instead of having to
add __attribute__((nonnull(1,2,5,8))) with exactly the right
numbers to a lot of prototypes.
The other reason is that a nonzero type attribute is conceptually
much simpler and at the same time more general than the existing
nonnull and nonnull_return function attributes (and could replace
both), e.g. we can define non-zero types and diagnose all stores
of known constant 0 to variables of this type, use this for
computing better value ranges, etc.
Why would that be useful? What makes integer 0 special?
0 is special in many ways. In particular it represents false
in an if clause. Also, why artificially restrict it to pointers
when it is trivial to make it work for integers too?
Martin
[RFC, PATCH] nonzero attribute, static array parameter
Hi,
here is a tentative patch to implement a new attribute nonzero,
which is similar to nonnull, but is not a function attribute
but a type attribute.
One reason is that nonnull is awkward to use. For this reason,
clang allows the use of nonnull in function parameters, but this
is incompatible with old and current use of this attribute in gcc
(though in a rather obscure use case).
See: https://gcc.gnu.org/ml/gcc/2015-05/msg00077.html
The other reason is that a nonzero type attribute is conceptually
much simpler and at the same time more general than the existing
nonnull and nonnull_return function attributes (and could replace
both), e.g. we can define non-zero types and diagnose all stores
of known constant 0 to variables of this type, use this for
computing better value ranges, etc.
This patch implements the attribute and adds a warning if a
zero constant is passed as argument with nonzero type attribute.
Also infer_nonnull_range in gcc/gimple.c is extended to make use
of the attribute (which in turn is used by ubsan).
In addition, in C the attribute is automatically added to
pointers declared as array parameters with static, e.g:
void foo(int a[static 5]);
Martin
(tested on x86_64-unknown-linux-gnu)
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 77d9352..2f79344 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2015-05-09 Martin Uecker uec...@eecs.berkeley.edu
+
+ * gcc/gimple.c (infer_nonnull_range): Process nonzero attribute.
+ * doc/invoki.texi
+
2015-05-08 Jim Wilson jim.wil...@linaro.org
* doc/install.texi (--enable-languages): Add missing jit and lto info.
diff --git a/gcc/c-family/ChangeLog b/gcc/c-family/ChangeLog
index 57f83c9..809ecf1 100644
--- a/gcc/c-family/ChangeLog
+++ b/gcc/c-family/ChangeLog
@@ -1,3 +1,9 @@
+2015-05-09 Martin Uecker uec...@eecs.berkeley.edu
+
+ * c-common.c (handle_nonzero_attribute): New.
+ (check_function_nonzero): New.
+ (check_nonzero_arg): New.
+
2015-05-08 Marek Polacek pola...@redhat.com
PR c/64918
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 378f237..46fe749 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -380,6 +380,7 @@ static tree handle_deprecated_attribute (tree *, tree, tree, int,
static tree handle_vector_size_attribute (tree *, tree, tree, int,
bool *);
static tree handle_nonnull_attribute (tree *, tree, tree, int, bool *);
+static tree handle_nonzero_attribute (tree *, tree, tree, int, bool *);
static tree handle_nothrow_attribute (tree *, tree, tree, int, bool *);
static tree handle_cleanup_attribute (tree *, tree, tree, int, bool *);
static tree handle_warn_unused_result_attribute (tree *, tree, tree, int,
@@ -407,6 +408,7 @@ static tree handle_bnd_instrument (tree *, tree, tree, int, bool *);
static void check_function_nonnull (tree, int, tree *);
static void check_nonnull_arg (void *, tree, unsigned HOST_WIDE_INT);
+static void check_nonzero_arg (void *, tree, unsigned HOST_WIDE_INT);
static bool nonnull_check_p (tree, unsigned HOST_WIDE_INT);
static bool get_nonnull_operand (tree, unsigned HOST_WIDE_INT *);
static int resort_field_decl_cmp (const void *, const void *);
@@ -751,6 +753,8 @@ const struct attribute_spec c_common_attribute_table[] =
handle_tls_model_attribute, false },
{ nonnull,0, -1, false, true, true,
handle_nonnull_attribute, false },
+ { nonzero,0, -1, false, true, false,
+ handle_nonzero_attribute, false },
{ nothrow,0, 0, true, false, false,
handle_nothrow_attribute, false },
{ may_alias, 0, 0, false, true, false, NULL, false },
@@ -8940,6 +8944,26 @@ handle_vector_size_attribute (tree *node, tree name, tree args,
return NULL_TREE;
}
+
+/* Handle the nonzero attribute. */
+static tree
+handle_nonzero_attribute (tree *node, tree ARG_UNUSED (name),
+ tree ARG_UNUSED (args), int ARG_UNUSED (flags),
+ bool *no_add_attrs)
+{
+ tree n = *node;
+
+ if ((TREE_CODE (n) != POINTER_TYPE)
+ (TREE_CODE (n) != INTEGER_TYPE))
+{
+ warning (OPT_Wattributes, %qE attribute ignored, name);
+ *no_add_attrs = true;
+}
+
+ return NULL_TREE;
+}
+
+
/* Handle the nonnull attribute. */
static tree
handle_nonnull_attribute (tree *node, tree ARG_UNUSED (name),
@@ -9016,6 +9040,34 @@ handle_nonnull_attribute (tree *node, tree ARG_UNUSED (name),
return NULL_TREE;
}
+
+static void
+check_function_nonzero (const_tree fntype, int nargs, tree *argarray)
+{
+ if (TREE_CODE (fntype) != FUNCTION_TYPE) // C++ lambdas
+return;
+
+ function_args_iterator iter;
+ tree type;
+ int i = 0;
+
+ FOREACH_FUNCTION_ARGS (fntype, type, iter)
+{
+ if (i == nargs)
+ break;
+
+ tree a = lookup_attribute (nonzero, TYPE_ATTRIBUTES (type));
+
+ if (a != NULL_TREE)
+check_function_arguments_recurse (check_nonzero_arg, NULL,
+ argarray[i], i + 1
Re: [PATCH] ubsan: improve bounds checking, add -fsanitize=bounds-strict
Marek Polacek pola...@redhat.com:
On Fri, Feb 27, 2015 at 11:53:14AM -0800, Martin Uecker wrote:
I tested Marek's proposed change and it works correctly,
i.e. arrays which are not part of a struct are now
instrumented when accessed through a pointer. This also
means that the following case is diagnosed (correctly)
as undefined behaviour as pointed out by Richard:
int
main (void)
{
int *t = (int *) __builtin_malloc (sizeof (int) * 9);
int (*a)[3][3] = (int (*)[3][3])t;
(*a)[0][9] = 1;
}
I also wanted arrays which are the last elements of a
struct which are not flexible-array members instrumented
correctly. So I added -fsantitize=bounds-strict which does
this. It seems to do instrumentation similar to clang
with -fsanitize=bounds.
Comments?
Thanks for working on it. So I think we should split this patch in
two; one part is a bug fix (I've opened
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65280) that could go
into gcc 5 - that is, apply my fix along with test cases covering the
new cases, and the second part is an addition of a new option for
strict bounds checking - I'm afraid this part has to wait for gcc 6.
I can take care of the first part and let you do the second part, which I
could review. Does that sound ok to you?
Thank you Marek! Sounds good to me.
Martin
Re: [PATCH] ubsan: improve bounds checking, add -fsanitize=bounds-strict
Bootstrap and regression tested on x86_64.
A ubsan-bootstrap for C and C++ works too (although there
are other unrelated runtime errors).
I tried a bootstrap with -fsanitize=bounds-strict and
there are indeed many additional runtime errors due the
struct hack with last array element of size 1 (or even 2).
So Jakub and Richard were right and putting this into a
separate option was the right thing to do.
Code-wise it seems to come just from a couple of places
such as (rtl.h, tree.h, vec.h, sbitmap.h, ...).
Just to see how hard it would be to make this work with
strict bounds checking, I started doing this in my tree
using a macro:
#define CAST_TO_POINTER(x) ((__typeof(x[0])*)(x))
For example,
#define RTL_CHECK1(RTX, N, C1) ((RTX)-u.fld[N])
then becomes
#define RTL_CHECK1(RTX, N, C1) \
(CAST_TO_POINTER((RTX)-u.fld)[N])
Doing this in a few places gets rid of most of the
errors rather quickly (getting rid of all errors would
be a bit more work).
If one allows VLAs one could also cast to an array of
the correct length:
#defined CAST_TO_VLA(x, len) (*(__typeof(x[0])(*)[len])x)
For example:
#define RTL_CHECK1(RTX, N, C1) \
(CAST_TO_VLA((RTX)-u.fld, GET_RTX_LENGTH( ... ))[N])
and have ubsan auto-generate the bound checking which
are now often manually added.
Martin
Martin Uecker uec...@eecs.berkeley.edu:
I tested Marek's proposed change and it works correctly,
i.e. arrays which are not part of a struct are now
instrumented when accessed through a pointer. This also
means that the following case is diagnosed (correctly)
as undefined behaviour as pointed out by Richard:
int
main (void)
{
int *t = (int *) __builtin_malloc (sizeof (int) * 9);
int (*a)[3][3] = (int (*)[3][3])t;
(*a)[0][9] = 1;
}
I also wanted arrays which are the last elements of a
struct which are not flexible-array members instrumented
correctly. So I added -fsantitize=bounds-strict which does
this. It seems to do instrumentation similar to clang
with -fsanitize=bounds.
Comments?
(regression testing in progress, but ubsan-related
tests all pass)
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index ec2cb69..cb6df20 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,11 @@
+2015-02-27 Martin Uecker uec...@eecs.berkeley.edu
+
+ * opts.c(common_handle_option): Add option for
+ -fsanitize=bounds-strict
+ * flag-types.h: Add SANITIZE_BOUNDS_STRICT
+ * doc/invoke.texi: Improve description for
+ -fsanitize=bounds and document -fsanitize=bounds-strict
+
diff --git a/gcc/c-family/ChangeLog b/gcc/c-family/ChangeLog
index ffa01c6..44a1761 100644
--- a/gcc/c-family/ChangeLog
+++ b/gcc/c-family/ChangeLog
@@ -1,3 +1,9 @@
+2015-02-27 Martin Uecker uec...@eecs.berkeley.edu
+
+ * c-ubsan.c (ubsan_instrument_bounds): Instrument
+ arrays which are accessed directly through a pointer.
+ For strict checking, instrument last elements of a struct.
+
diff --git a/gcc/c-family/c-ubsan.c b/gcc/c-family/c-ubsan.c
index 90d59c0..1a0e2da 100644
--- a/gcc/c-family/c-ubsan.c
+++ b/gcc/c-family/c-ubsan.c
@@ -293,6 +293,7 @@ ubsan_instrument_bounds (location_t loc, tree array, tree
*index,
tree type = TREE_TYPE (array);
tree domain = TYPE_DOMAIN (type);
+ /* This also takes care of flexible array members. */
if (domain == NULL_TREE || TYPE_MAX_VALUE (domain) == NULL_TREE)
return NULL_TREE;
@@ -301,10 +302,13 @@ ubsan_instrument_bounds (location_t loc, tree array,
tree *index,
bound = fold_build2 (PLUS_EXPR, TREE_TYPE (bound), bound,
build_int_cst (TREE_TYPE (bound), 1));
- /* Detect flexible array members and suchlike. */
+ /* Don't instrument arrays which are the last element of
+ a struct. */
tree base = get_base_address (array);
- if (base (TREE_CODE (base) == INDIRECT_REF
-|| TREE_CODE (base) == MEM_REF))
+ if (!(flag_sanitize SANITIZE_BOUNDS_STRICT)
+ (TREE_CODE (array) == COMPONENT_REF)
+ base (TREE_CODE (base) == INDIRECT_REF
+ || TREE_CODE (base) == MEM_REF))
{
tree next = NULL_TREE;
tree cref = array;
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index ef4cc75..5a93757 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -5704,8 +5704,15 @@ a++;
@item -fsanitize=bounds
@opindex fsanitize=bounds
This option enables instrumentation of array bounds. Various out of bounds
-accesses are detected. Flexible array members and initializers of variables
-with static storage are not instrumented.
+accesses are detected. Accesses to arrays which are the last member of a
+struct and initializers of variables with static storage are not
instrumented.
+
+@item -fsanitize=bounds-strict
+@opindex fsanitize=bounds-strict
+This option enables strict instrumentation of array bounds. Most out of
bounds
+accesses are detected including accesses to arrays which
[PATCH] ubsan: improve bounds checking, add -fsanitize=bounds-strict
I tested Marek's proposed change and it works correctly,
i.e. arrays which are not part of a struct are now
instrumented when accessed through a pointer. This also
means that the following case is diagnosed (correctly)
as undefined behaviour as pointed out by Richard:
int
main (void)
{
int *t = (int *) __builtin_malloc (sizeof (int) * 9);
int (*a)[3][3] = (int (*)[3][3])t;
(*a)[0][9] = 1;
}
I also wanted arrays which are the last elements of a
struct which are not flexible-array members instrumented
correctly. So I added -fsantitize=bounds-strict which does
this. It seems to do instrumentation similar to clang
with -fsanitize=bounds.
Comments?
(regression testing in progress, but ubsan-related
tests all pass)
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index ec2cb69..cb6df20 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,11 @@
+2015-02-27 Martin Uecker uec...@eecs.berkeley.edu
+
+ * opts.c(common_handle_option): Add option for
+ -fsanitize=bounds-strict
+ * flag-types.h: Add SANITIZE_BOUNDS_STRICT
+ * doc/invoke.texi: Improve description for
+ -fsanitize=bounds and document -fsanitize=bounds-strict
+
diff --git a/gcc/c-family/ChangeLog b/gcc/c-family/ChangeLog
index ffa01c6..44a1761 100644
--- a/gcc/c-family/ChangeLog
+++ b/gcc/c-family/ChangeLog
@@ -1,3 +1,9 @@
+2015-02-27 Martin Uecker uec...@eecs.berkeley.edu
+
+ * c-ubsan.c (ubsan_instrument_bounds): Instrument
+ arrays which are accessed directly through a pointer.
+ For strict checking, instrument last elements of a struct.
+
diff --git a/gcc/c-family/c-ubsan.c b/gcc/c-family/c-ubsan.c
index 90d59c0..1a0e2da 100644
--- a/gcc/c-family/c-ubsan.c
+++ b/gcc/c-family/c-ubsan.c
@@ -293,6 +293,7 @@ ubsan_instrument_bounds (location_t loc, tree array, tree
*index,
tree type = TREE_TYPE (array);
tree domain = TYPE_DOMAIN (type);
+ /* This also takes care of flexible array members. */
if (domain == NULL_TREE || TYPE_MAX_VALUE (domain) == NULL_TREE)
return NULL_TREE;
@@ -301,10 +302,13 @@ ubsan_instrument_bounds (location_t loc, tree array, tree
*index,
bound = fold_build2 (PLUS_EXPR, TREE_TYPE (bound), bound,
build_int_cst (TREE_TYPE (bound), 1));
- /* Detect flexible array members and suchlike. */
+ /* Don't instrument arrays which are the last element of
+ a struct. */
tree base = get_base_address (array);
- if (base (TREE_CODE (base) == INDIRECT_REF
- || TREE_CODE (base) == MEM_REF))
+ if (!(flag_sanitize SANITIZE_BOUNDS_STRICT)
+ (TREE_CODE (array) == COMPONENT_REF)
+ base (TREE_CODE (base) == INDIRECT_REF
+ || TREE_CODE (base) == MEM_REF))
{
tree next = NULL_TREE;
tree cref = array;
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index ef4cc75..5a93757 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -5704,8 +5704,15 @@ a++;
@item -fsanitize=bounds
@opindex fsanitize=bounds
This option enables instrumentation of array bounds. Various out of bounds
-accesses are detected. Flexible array members and initializers of variables
-with static storage are not instrumented.
+accesses are detected. Accesses to arrays which are the last member of a
+struct and initializers of variables with static storage are not instrumented.
+
+@item -fsanitize=bounds-strict
+@opindex fsanitize=bounds-strict
+This option enables strict instrumentation of array bounds. Most out of bounds
+accesses are detected including accesses to arrays which are the last member
of a
+struct. Initializers of variables with static storage are not instrumented.
+
@item -fsanitize=alignment
@opindex fsanitize=alignment
diff --git a/gcc/flag-types.h b/gcc/flag-types.h
index bfdce44..c9ad4df 100644
--- a/gcc/flag-types.h
+++ b/gcc/flag-types.h
@@ -238,6 +238,7 @@ enum sanitize_code {
SANITIZE_RETURNS_NONNULL_ATTRIBUTE = 1UL 19,
SANITIZE_OBJECT_SIZE = 1UL 20,
SANITIZE_VPTR = 1UL 21,
+ SANITIZE_BOUNDS_STRICT = 1UL 22,
SANITIZE_UNDEFINED = SANITIZE_SHIFT | SANITIZE_DIVIDE | SANITIZE_UNREACHABLE
| SANITIZE_VLA | SANITIZE_NULL | SANITIZE_RETURN
| SANITIZE_SI_OVERFLOW | SANITIZE_BOOL | SANITIZE_ENUM
@@ -245,7 +246,7 @@ enum sanitize_code {
| SANITIZE_NONNULL_ATTRIBUTE
| SANITIZE_RETURNS_NONNULL_ATTRIBUTE
| SANITIZE_OBJECT_SIZE | SANITIZE_VPTR,
- SANITIZE_NONDEFAULT = SANITIZE_FLOAT_DIVIDE | SANITIZE_FLOAT_CAST
+ SANITIZE_NONDEFAULT = SANITIZE_FLOAT_DIVIDE | SANITIZE_FLOAT_CAST |
SANITIZE_BOUNDS_STRICT
};
/* flag_vtable_verify initialization levels. */
diff --git a/gcc/opts.c b/gcc/opts.c
index 39c190d..7fe77fa 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -1584,6 +1584,7 @@ common_handle_option (struct gcc_options *opts,
{ float-cast-overflow, SANITIZE_FLOAT_CAST,
sizeof float-cast-overflow - 1
Re: [PATCH] ubsan: remove bogus check for flexible array members
Am Thu, 26 Feb 2015 10:05:14 +0100
Jakub Jelinek ja...@redhat.com:
On Thu, Feb 26, 2015 at 12:59:08AM -0800, Martin Uecker wrote:
No, it is not bogus nor unnecessary.
This isn't about just real flexible arrays, but similar constructs,
C++ doesn't have flexible array members, nor C89, so people use the
GNU extension of struct S { ... ; char a[0]; } instead, or
The GNU extension is still allowed, i.e. not instrumented with
the patch.
use char a[1]; as the last member and still expect to be able to access
s-a[i] for i 0 say on heap allocations etc.
And this is broken code. I would argue that a user who uses the
ubsan *expects* this to be diagnosed. Atleast I was surprised
that it didn't catch more out-of-bounds accesses.
So can you explain what a C++ programmer can do portably?
Using broken code is not really portable either, because other
compilers diagnose this. Also, we are not talking about breaking
that code - they can simply continue to use that code. They could
just not expect ubsan to not diagnose it. This would make
them aware of the fact that their code is problematic - which is
exactly what I would expect from ubsan.
It has neither
flexible array members, nor without GNU extensions zero sized arrays.
If the array size is constant, perhaps turn the struct into a template,
but if it is variable? Ditto for C89 code.
The amount of code that uses this idiom in the wild is huge.
Ok, I will add an option then. Or should this be language dependent?
Maritn
Re: [PATCH] ubsan: remove bogus check for flexible array members
Marek Polacek pola...@redhat.com: (Hi Marek and Jakub, I really appreciate your work on GCC and ubsan. I just want to add the minor enhancements necessary to make it really useful for me). And this is broken code. I would argue that a user who uses the ubsan *expects* this to be diagnosed. Atleast I was surprised that it didn't catch more out-of-bounds accesses. I wouldn't say broken, it's being used pretty often. E.g. https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html In ISO C90, you would have to give contents a length of 1 Ok, let's add an option then. But that it is common doesn't mean it is not broken (especially since there is a legal way to that with GCC and C99) and I think there should be a way to diagnose it with ubsan. If this is indeed intentional, we should: - document these exceptions See the link above + docs say for -fsanitize=bounds that Flexible array members are not instrumented. Flexible array members should not be intrumented. But what is suprising to the user and should be documented is that other arrays (e.g. which hjust happen to be the last in a struct) are not instrumented. - remove the misleading comment Which one? The /* Detect flexible array members and suchlike. */ IMHO says exactly what's meant to be done. The reason it confused me was because flexible array members never see this code because the are catched by the 'if' condition directly before. - have test cases also for sizes 0 Yes, we should have that. - fix it not to prevent instrumentation of all array accesses through pointers (i.e. when the array is not a member of a struct) Yes, see the patch in my previous mail. Thank you! I will test this and report back - have a configuration option (e.g. -fsanitize=strict-bounds) Yeah, something like that would be useful to have. As Jakub mentioned, we discussed this in the past. Probably GCC 6 stuff though. I will look into this. Martin
Re: [PATCH] ubsan: remove bogus check for flexible array members
Thu, 26 Feb 2015 07:36:54 +0100
Jakub Jelinek ja...@redhat.com:
On Wed, Feb 25, 2015 at 10:01:07PM -0800, Martin Uecker wrote:
this patch removes a bogus check for flexible array members
which prevents array references to be instrumented in some
interesting cases. Arrays accessed through pointers are now
instrumented correctly.
The check was unnecessary because flexible arrays are not
instrumented anyway because of
TYPE_MAX_VALUE (domain) == NULL_TREE.
No, it is not bogus nor unnecessary.
This isn't about just real flexible arrays, but similar constructs,
C++ doesn't have flexible array members, nor C89, so people use the
GNU extension of struct S { ... ; char a[0]; } instead, or
The GNU extension is still allowed, i.e. not instrumented with
the patch.
use char a[1]; as the last member and still expect to be able to access
s-a[i] for i 0 say on heap allocations etc.
And this is broken code. I would argue that a user who uses the
ubsan *expects* this to be diagnosed. Atleast I was surprised
that it didn't catch more out-of-bounds accesses.
If this is indeed intentional, we should:
- document these exceptions
- remove the misleading comment
- have test cases also for sizes 0
- fix it not to prevent instrumentation of all array accesses
through pointers (i.e. when the array is not a member of a struct)
- have a configuration option (e.g. -fsanitize=strict-bounds)
I think we were talking at some point about having a strict-bounds or
something similar that would accept only real flexible array members and
nothing else and be more pedantic, at the disadvantage of diagnosing tons
of real-world code that is supported by GCC.
Perhaps if the reference is just an array, not a struct containing
flexible-array-member-like array, we could consider it strict always,
but that is certainly not what your patch does.
Indeed, currently the patch tries to make -fsanitize=bounds detect what
the standard considers undefined behaviour. At the moment, I do not
quite see why ubsan should be so permissive as you say.
But we can also add a new option -fsanitize=strict-bounds. Then I could
use this for my (real-world) projects.
* c-family/c-ubsan.c (ubsan_instrument_bounds): Remove
c-family/ shouldn't be in the ChangeLog entry, instead that part should go
into c-family/ChangeLog.
--- a/gcc/c-family/c-ubsan.c
+++ b/gcc/c-family/c-ubsan.c
@@ -293,6 +293,7 @@ ubsan_instrument_bounds (location_t loc, tree array,
tree *index,
tree type = TREE_TYPE (array);
tree domain = TYPE_DOMAIN (type);
+ /* This also takes care of flexilbe array members */
Typo.
+#ifdef __cplusplus
+extern C void* malloc(unsigned long x);
+#else
+extern void* malloc(unsigned long x);
+#endif
Why are you declaring malloc (wrongly), when it isn't used?
Malloc argument is size_t aka __SIZE_TYPE__, not unsigned long.
Left over from removing tests already done elsewhere.
Thank you for your comments,
Martin
[PATCH] ubsan: remove bogus check for flexible array members
Hi,
this patch removes a bogus check for flexible array members
which prevents array references to be instrumented in some
interesting cases. Arrays accessed through pointers are now
instrumented correctly.
The check was unnecessary because flexible arrays are not
instrumented anyway because of
TYPE_MAX_VALUE (domain) == NULL_TREE.
Regression tested.
2015-02-25 Martin Uecker uec...@eecs.berkeley.edu
gcc/
* c-family/c-ubsan.c (ubsan_instrument_bounds): Remove
bogus check for flexible array members.
gcc/testsuite/
* gcc.dg/ubsan/bounds-2.c: New
* c-c++-common/ubsan/bounds-8.c: New
diff --git a/gcc/c-family/c-ubsan.c b/gcc/c-family/c-ubsan.c
index 90d59c0..dba3bbc 100644
--- a/gcc/c-family/c-ubsan.c
+++ b/gcc/c-family/c-ubsan.c
@@ -293,6 +293,7 @@ ubsan_instrument_bounds (location_t loc, tree array, tree
*index,
tree type = TREE_TYPE (array);
tree domain = TYPE_DOMAIN (type);
+ /* This also takes care of flexilbe array members */
if (domain == NULL_TREE || TYPE_MAX_VALUE (domain) == NULL_TREE)
return NULL_TREE;
@@ -301,36 +302,6 @@ ubsan_instrument_bounds (location_t loc, tree array, tree
*index,
bound = fold_build2 (PLUS_EXPR, TREE_TYPE (bound), bound,
build_int_cst (TREE_TYPE (bound), 1));
- /* Detect flexible array members and suchlike. */
- tree base = get_base_address (array);
- if (base (TREE_CODE (base) == INDIRECT_REF
- || TREE_CODE (base) == MEM_REF))
-{
- tree next = NULL_TREE;
- tree cref = array;
-
- /* Walk all structs/unions. */
- while (TREE_CODE (cref) == COMPONENT_REF)
- {
- if (TREE_CODE (TREE_TYPE (TREE_OPERAND (cref, 0))) == RECORD_TYPE)
- for (next = DECL_CHAIN (TREE_OPERAND (cref, 1));
-next TREE_CODE (next) != FIELD_DECL;
-next = DECL_CHAIN (next))
- ;
- if (next)
- /* Not a last element. Instrument it. */
- break;
- /* Ok, this is the last field of the structure/union. But the
-aggregate containing the field must be the last field too,
-recursively. */
- cref = TREE_OPERAND (cref, 0);
- }
- if (!next)
- /* Don't instrument this flexible array member-like array in non-strict
- -fsanitize=bounds mode. */
-return NULL_TREE;
-}
-
/* Don't emit instrumentation in the most common cases. */
tree idx = NULL_TREE;
if (TREE_CODE (*index) == INTEGER_CST)
diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-8.c
b/gcc/testsuite/c-c++-common/ubsan/bounds-8.c
new file mode 100644
index 000..554693f
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ubsan/bounds-8.c
@@ -0,0 +1,26 @@
+/* { dg-do run } */
+/* { dg-options -O3 -fsanitize=bounds } */
+/* Origin: Martin Uecker uec...@eecs.berkeley.edu */
+
+#ifdef __cplusplus
+extern C void* malloc(unsigned long x);
+#else
+extern void* malloc(unsigned long x);
+#endif
+
+void foo(int (*a)[3])
+{
+ (*a)[3] = 1;// error
+ a[0][0] = 1;// ok
+ a[1][0] = 1;// ok
+ a[1][4] = 1;// error
+}
+
+int main()
+{
+ int a[20];
+ foo((int (*)[3])a);
+}
+
+/* { dg-output index 3 out of bounds for type 'int
\\\[3\\\]'\[^\n\r]*(\n|\r\n|\r) } */
+/* { dg-output \[^\n\r]*index 4 out of bounds for type 'int \\\[3\\\]' } */
diff --git a/gcc/testsuite/gcc.dg/ubsan/bounds-2.c
b/gcc/testsuite/gcc.dg/ubsan/bounds-2.c
new file mode 100644
index 000..722c54e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ubsan/bounds-2.c
@@ -0,0 +1,18 @@
+/* { dg-do run } */
+/* { dg-options -O3 -fsanitize=bounds } */
+/* Origin: Martin Uecker uec...@eecs.berkeley.edu */
+
+extern void* malloc(unsigned long x);
+
+void foo(int n, int (*b)[n])
+{
+ (*b)[n] = 1;// error
+}
+
+int main()
+{
+ int a[20];
+ foo(3, (int (*)[3])a);
+}
+
+/* { dg-output index 3 out of bounds for type 'int \\\[\\\*\\\]' } */
Re: [PATCH][2/2] Improve array-bound warnings and VRP
Richard Biener wrote: On Mon, 26 Jan 2015, Jakub Jelinek wrote: Then it probably should be ok. I'm really afraid of emitting more warnings with such high false positive rate now. As the patch also mitigates some of the code bloat we get with the complete peeling (regression against 4.7) I have installed it. It's also the easiest vehicle to verify range-info is not broken by passes between vrp1 and vrp2. You could make warnings appear only for warn_array_bounds 1 if there are concerns about false positives. For what it's worth, I tested the old version of both patches on one of my projects (mostly numerical algorithms) and it did not produce additional warnings. I really appreciate all improvements in this area. Martin
[committed] MAINTAINERS: add myself to write-after-approval list
2015-01-15 Martin Uecker uec...@eecs.berkeley.edu * MAINTAINERS: (Write After Approval): Add myself. Index: MAINTAINERS === --- MAINTAINERS (Revision 219713) +++ MAINTAINERS (Revision 219714) @@ -576,6 +576,7 @@ Philipp Tomsich philipp.toms...@theobroma-systems.com Konrad Trifunovic konrad.trifuno...@inria.fr Markus Trippelsdorfmar...@trippelsdorf.de +Martin Uecker uec...@eecs.berkeley.edu David Ung dav...@mips.com Neil Vachharajani nvach...@gmail.com Kris Van Hees kris.van.h...@oracle.com
Re: [PATCH] add option to emit more array bounds warnigs
Mon, 12 Jan 2015 11:00:44 -0700 Jeff Law l...@redhat.com: On 11/11/14 23:13, Martin Uecker wrote: ... * gcc/tree-vrp.c (check_array_ref): Emit more warnings for warn_array_bounds = 2. * gcc/testsuite/gcc.dg/Warray-bounds-11.c: New test-case. * gcc/c-family/c.opt: New option -Warray-bounds=. * gcc/common.opt: New option -Warray-bounds=. * gcc/doc/invoke.texi: Document new option. Has this patch been bootstrapped and regression tested, if so on what platform. x86_64-unknown-linux-gnu Given the new warnings (as implemented by the patch) are not enabled by default, I'm inclined to approve once Martin verifies things via bootstrap and regression test. Thank you, Martin
Re: [PATCH] add option to emit more array bounds warnigs
Jeff Law l...@redhat.com: On 01/13/15 17:40, Martin Uecker wrote: Jeff Law l...@redhat.com: On 01/13/15 10:34, Martin Uecker wrote: Mon, 12 Jan 2015 11:00:44 -0700 Jeff Law l...@redhat.com: On 11/11/14 23:13, Martin Uecker wrote: ... Has this patch been bootstrapped and regression tested, if so on what platform. x86_64-unknown-linux-gnu Approved. Please install on the trunk. Sorry about the delays. I don't have write access ;-( I fixed up the ChangeLog entries and installed the patch for you. Thank you, Jeff! If you plan to contribute regularly, you should go ahead and apply for write access to the repository so that you'll be able to commit your own patches once they're approved. I put a request in with you as sponsor (hope this is ok). You'll also need to make sure you have an assignment on file with the FSF.That patch was pretty small (the testcase was larger than the patch itself, which I always like :-) so I didn't request an assignment. Further submissions likely will require an assignment. I already have an assignment on file. Martin
Re: [PATCH] add option to emit more array bounds warnigs
Jeff Law l...@redhat.com: On 01/13/15 10:34, Martin Uecker wrote: Mon, 12 Jan 2015 11:00:44 -0700 Jeff Law l...@redhat.com: On 11/11/14 23:13, Martin Uecker wrote: ... Has this patch been bootstrapped and regression tested, if so on what platform. x86_64-unknown-linux-gnu Approved. Please install on the trunk. Sorry about the delays. I don't have write access ;-( Martin
[PING] Re: [PATCH] add option to emit more array bounds warnigs
Please consider this patch. The additional warnings would be useful
IMHO, are also emitted by clang, and the change seems trivial.
Previous discussion about potential false positives:
https://gcc.gnu.org/ml/gcc/2014-11/msg00114.html
Tue, 11 Nov 2014 22:13:20 -0800
Martin Uecker uec...@eecs.berkeley.edu:
Hi,
this proposed patch adds an option -Warray-bounds= in addition to
-Warray-bound. -Warray-bounds=1 corresponds to -Warray-bound.
For higher warning levels more warnings about optional accesses
outside of arrays are emitted. For example, warnings for
arrays accessed through pointers are now emitted:
void foo(int (*a)[3])
{
(*a)[4] = 1;
}
Also warnings for arrays which are the last element of a struct
are emitted, if it is not a flexible array member or does not use
the zero size extensions.
Because there is the risk of false positives, the higher warning
level is not used by default.
Martin
* gcc/tree-vrp.c (check_array_ref): Emit more warnings
for warn_array_bounds = 2.
* gcc/testsuite/gcc.dg/Warray-bounds-11.c: New test-case.
* gcc/c-family/c.opt: New option -Warray-bounds=.
* gcc/common.opt: New option -Warray-bounds=.
* gcc/doc/invoke.texi: Document new option.
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 064c69e..e61fc56 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -279,6 +279,10 @@ Warray-bounds
LangEnabledBy(C ObjC C++ ObjC++,Wall)
; in common.opt
+Warray-bounds=
+LangEnabledBy(C ObjC C++ ObjC++,Wall,1,0)
+; in common.opt
+
Wassign-intercept
ObjC ObjC++ Var(warn_assign_intercept) Warning
Warn whenever an Objective-C assignment is being intercepted by the garbage
collector
diff --git a/gcc/common.opt b/gcc/common.opt
index e104269..3d19875 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -529,6 +529,10 @@ Warray-bounds
Common Var(warn_array_bounds) Warning
Warn if an array is accessed out of bounds
+Warray-bounds=
+Common Joined RejectNegative UInteger Var(warn_array_bounds) Warning
+Warn if an array is accessed out of bounds
+
Wattributes
Common Var(warn_attributes) Init(1) Warning
Warn about inappropriate attribute usage
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index ed23f6f..f20cbf0 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -240,7 +240,7 @@ Objective-C and Objective-C++ Dialects}.
@gccoptlist{-fsyntax-only -fmax-errors=@var{n} -Wpedantic @gol
-pedantic-errors @gol
-w -Wextra -Wall -Waddress -Waggregate-return @gol
--Waggressive-loop-optimizations -Warray-bounds @gol
+-Waggressive-loop-optimizations -Warray-bounds -Warray-bounds=@var{n} @gol
-Wbool-compare @gol
-Wno-attributes -Wno-builtin-macro-redefined @gol
-Wc90-c99-compat -Wc99-c11-compat @gol
@@ -3382,7 +3382,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect
Options}.
@option{-Wall} turns on the following warning flags:
@gccoptlist{-Waddress @gol
--Warray-bounds @r{(only with} @option{-O2}@r{)} @gol
+-Warray-bounds=1 @r{(only with} @option{-O2}@r{)} @gol
-Wc++11-compat -Wc++14-compat@gol
-Wchar-subscripts @gol
-Wenum-compare @r{(in C/ObjC; this is on by default in C++)} @gol
@@ -4296,12 +4296,26 @@ Warn about overriding virtual functions that are not
marked with the override
keyword.
@item -Warray-bounds
+@itemx -Warray-bounds=@var{n}
@opindex Wno-array-bounds
@opindex Warray-bounds
This option is only active when @option{-ftree-vrp} is active
(default for @option{-O2} and above). It warns about subscripts to arrays
that are always out of bounds. This warning is enabled by @option{-Wall}.
+@table @gcctabopt
+@item -Warray-bounds=1
+This is the warning level of @option{-Warray-bounds} and is enabled
+by @option{-Wall}; higher levels are not, and must be explicitly requested.
+
+@item -Warray-bounds=2
+This warning level also warns about out of bounds access for
+arrays at the end of a struct and for arrays accessed through
+pointers. This warning level may give a larger number of
+false positives and is deactivated by default.
+@end table
+
+
@item -Wbool-compare
@opindex Wno-bool-compare
@opindex Wbool-compare
diff --git a/gcc/testsuite/gcc.dg/Warray-bounds-11.c
b/gcc/testsuite/gcc.dg/Warray-bounds-11.c
new file mode 100644
index 000..2e68498
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Warray-bounds-11.c
@@ -0,0 +1,96 @@
+/* { dg-do compile } */
+/* { dg-options -O3 -Warray-bounds=2 } */
+
+extern void* malloc(unsigned long x);
+
+int e[3];
+
+struct f { int f[3]; };
+
+extern void bar(int v[]);
+
+struct h {
+
+ int i;
+ int j[];
+};
+
+struct h0 {
+
+ int i;
+ int j[0];
+};
+
+struct h0b {
+
+ int i;
+ int j[0];
+ int k;
+};
+
+struct h1 {
+
+ int i;
+ int j[1];
+};
+
+struct h1b {
+
+ int i;
+ int j[1];
+ int k;
+};
+
+struct h3 {
+
+ int i;
+ int j[3];
+};
+
+struct h3b {
+
+ int i;
+ int j[3];
+ int k;
+};
+
+void foo(int (*a)[3
[PATCH v5] warning about const multidimensional array as function parameter
Another version of this patch. I fixed the formatting problems and
the spurios use of OPT_Wdiscarded_array_qualifiers. I also added
'-pedantic -Wdiscarded-array-qualifiers' to the dg-options in
'testsuite/gcc.dg/qual-component-1.c' and changed the expected
warnings to the new ones. For some reason, the changes to this
file were missing in my previous versions. (scroll down to the
end for this file)
Thank you,
Martin
2014-12-08 Martin Uecker uec...@eecs.berkeley.edu
* doc/invoke.texi: Document -Wdiscarded-array-qualifiers
* doc/extend.texi: Document new behavior for pointers to arrays with
qualifies
c/
* c-typeck.c: New behavious for pointers to arrays with qualifiers
(common-pointer-type): For pointers to arrays take qualifiers from
element type.
(build_conditional_expr): Add warnings for lost qualifiers.
(comp-target-types): Allow pointers to arrays with different qualifiers.
(convert-for-assignment): Adapt warnings for discarded qualifiers. Add
WARNING_FOR_QUALIFIERS macro and rename WARN_FOR_QUALIFIERS
to PEDWARN_FOR_QUALIFIERS.
c-family/
* c.opt (Wdiscarded-array-qualifiers): New option
testsuite/
* gcc.dg/Wwrite-strings-1.c: Change dg-warning
* gcc.dg/array-quals-1.c: Use -Wno-discarded-array-qualifiers
* gcc.dg/array-quals-2.c: Change dg-options, dg-warning
* gcc.dg/pointer-array-atomic.c: New test
* gcc.dg/pointer-array-quals-1.c: New test
* gcc.dg/pointer-array-quals-2.c: New test (-pedantic-errors)
* gcc.dg/qual-component-1.c: Change dg-options, dg-warnings
Index: gcc/c/c-typeck.c
===
--- gcc/c/c-typeck.c(Revision 218476)
+++ gcc/c/c-typeck.c(Arbeitskopie)
@@ -673,12 +673,13 @@ common_pointer_type (tree t1, tree t2)
mv2 = TYPE_MAIN_VARIANT (pointed_to_2);
target = composite_type (mv1, mv2);
+ /* Strip array types to get correct qualifier for pointers to arrays */
+ quals1 = TYPE_QUALS_NO_ADDR_SPACE (strip_array_types (pointed_to_1));
+ quals2 = TYPE_QUALS_NO_ADDR_SPACE (strip_array_types (pointed_to_2));
+
/* For function types do not merge const qualifiers, but drop them
if used inconsistently. The middle-end uses these to mark const
and noreturn functions. */
- quals1 = TYPE_QUALS_NO_ADDR_SPACE (pointed_to_1);
- quals2 = TYPE_QUALS_NO_ADDR_SPACE (pointed_to_2);
-
if (TREE_CODE (pointed_to_1) == FUNCTION_TYPE)
target_quals = (quals1 quals2);
else
@@ -1224,6 +1225,7 @@ static int
comp_target_types (location_t location, tree ttl, tree ttr)
{
int val;
+ int val_ped;
tree mvl = TREE_TYPE (ttl);
tree mvr = TREE_TYPE (ttr);
addr_space_t asl = TYPE_ADDR_SPACE (mvl);
@@ -1235,19 +1237,32 @@ comp_target_types (location_t location, tree ttl,
if (!addr_space_superset (asl, asr, as_common))
return 0;
- /* Do not lose qualifiers on element types of array types that are
- pointer targets by taking their TYPE_MAIN_VARIANT. */
- if (TREE_CODE (mvl) != ARRAY_TYPE)
-mvl = (TYPE_ATOMIC (mvl)
- ? c_build_qualified_type (TYPE_MAIN_VARIANT (mvl), TYPE_QUAL_ATOMIC)
- : TYPE_MAIN_VARIANT (mvl));
- if (TREE_CODE (mvr) != ARRAY_TYPE)
-mvr = (TYPE_ATOMIC (mvr)
- ? c_build_qualified_type (TYPE_MAIN_VARIANT (mvr), TYPE_QUAL_ATOMIC)
- : TYPE_MAIN_VARIANT (mvr));
+ /* For pedantic record result of comptypes on arrays before losing
+ qualifiers on the element type below. */
+ val_ped = 1;
+
+ if (TREE_CODE (mvl) == ARRAY_TYPE
+ TREE_CODE (mvr) == ARRAY_TYPE)
+val_ped = comptypes (mvl, mvr);
+
+ /* Qualifiers on element types of array types that are
+ pointer targets are lost by taking their TYPE_MAIN_VARIANT. */
+
+ mvl = (TYPE_ATOMIC (strip_array_types (mvl))
+? c_build_qualified_type (TYPE_MAIN_VARIANT (mvl), TYPE_QUAL_ATOMIC)
+: TYPE_MAIN_VARIANT (mvl));
+
+ mvr = (TYPE_ATOMIC (strip_array_types (mvr))
+? c_build_qualified_type (TYPE_MAIN_VARIANT (mvr), TYPE_QUAL_ATOMIC)
+: TYPE_MAIN_VARIANT (mvr));
+
enum_and_int_p = false;
val = comptypes_check_enum_int (mvl, mvr, enum_and_int_p);
+ if (val == 1 val_ped != 1)
+pedwarn (location, OPT_Wpedantic, pointers to arrays with different
qualifiers
+ are incompatible in ISO C);
+
if (val == 2)
pedwarn (location, OPT_Wpedantic, types are not quite compatible);
@@ -4609,6 +4624,13 @@ build_conditional_expr (location_t colon_loc, tree
else if (VOID_TYPE_P (TREE_TYPE (type1))
!TYPE_ATOMIC (TREE_TYPE (type1)))
{
+ if ((TREE_CODE (TREE_TYPE (type2)) == ARRAY_TYPE)
+ (TYPE_QUALS (strip_array_types (TREE_TYPE (type2)))
+ ~TYPE_QUALS (TREE_TYPE (type1
+ warning_at (colon_loc, OPT_Wdiscarded_array_qualifiers,
+ pointer to array loses
[PATCH] add option to emit more array bounds warnigs
Hi,
this proposed patch adds an option -Warray-bounds= in addition to
-Warray-bound. -Warray-bounds=1 corresponds to -Warray-bound.
For higher warning levels more warnings about optional accesses
outside of arrays are emitted. For example, warnings for
arrays accessed through pointers are now emitted:
void foo(int (*a)[3])
{
(*a)[4] = 1;
}
Also warnings for arrays which are the last element of a struct
are emitted, if it is not a flexible array member or does not use
the zero size extensions.
Because there is the risk of false positives, the higher warning
level is not used by default.
Martin
* gcc/tree-vrp.c (check_array_ref): Emit more warnings
for warn_array_bounds = 2.
* gcc/testsuite/gcc.dg/Warray-bounds-11.c: New test-case.
* gcc/c-family/c.opt: New option -Warray-bounds=.
* gcc/common.opt: New option -Warray-bounds=.
* gcc/doc/invoke.texi: Document new option.
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 66c62fb..0d5ecfd 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -279,6 +279,10 @@ Warray-bounds
LangEnabledBy(C ObjC C++ ObjC++,Wall)
; in common.opt
+Warray-bounds=
+LangEnabledBy(C ObjC C++ ObjC++,Wall,1,0)
+; in common.opt
+
Wassign-intercept
ObjC ObjC++ Var(warn_assign_intercept) Warning
Warn whenever an Objective-C assignment is being intercepted by the garbage
collector
diff --git a/gcc/common.opt b/gcc/common.opt
index b400636..d65085a 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -525,6 +525,10 @@ Warray-bounds
Common Var(warn_array_bounds) Warning
Warn if an array is accessed out of bounds
+Warray-bounds=
+Common Joined RejectNegative UInteger Var(warn_array_bounds) Warning
+Warn if an array is accessed out of bounds
+
Wattributes
Common Var(warn_attributes) Init(1) Warning
Warn about inappropriate attribute usage
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 57666db..afc4be8 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -239,7 +239,7 @@ Objective-C and Objective-C++ Dialects}.
@gccoptlist{-fsyntax-only -fmax-errors=@var{n} -Wpedantic @gol
-pedantic-errors @gol
-w -Wextra -Wall -Waddress -Waggregate-return @gol
--Waggressive-loop-optimizations -Warray-bounds @gol
+-Waggressive-loop-optimizations -Warray-bounds -Warray-bounds=@var{n} @gol
-Wbool-compare @gol
-Wno-attributes -Wno-builtin-macro-redefined @gol
-Wc90-c99-compat -Wc99-c11-compat @gol
@@ -3344,7 +3344,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect
Options}.
@option{-Wall} turns on the following warning flags:
@gccoptlist{-Waddress @gol
--Warray-bounds @r{(only with} @option{-O2}@r{)} @gol
+-Warray-bounds=1 @r{(only with} @option{-O2}@r{)} @gol
-Wc++11-compat @gol
-Wchar-subscripts @gol
-Wenum-compare @r{(in C/ObjC; this is on by default in C++)} @gol
@@ -4239,12 +4239,26 @@ hiearchy graph is more complete. It is recommended to
first consider suggestins
of @option{-Wsuggest-final-types} and then rebuild with new annotations.
@item -Warray-bounds
+@itemx -Warray-bounds=@var{n}
@opindex Wno-array-bounds
@opindex Warray-bounds
This option is only active when @option{-ftree-vrp} is active
(default for @option{-O2} and above). It warns about subscripts to arrays
that are always out of bounds. This warning is enabled by @option{-Wall}.
+@table @gcctabopt
+@item -Warray-bounds=1
+This is the warning level of @option{-Warray-bounds} and is enabled
+by @option{-Wall}; higher levels are not, and must be explicitly requested.
+
+@item -Warray-bounds=2
+This warning level also warns about out of bounds access for
+arrays at the end of a struct and for arrays accessed through
+pointers. This warning level may give a larger number of
+false positives and is deactivated by default.
+@end table
+
+
@item -Wbool-compare
@opindex Wno-bool-compare
@opindex Wbool-compare
diff --git a/gcc/testsuite/gcc.dg/Warray-bounds-11.c
b/gcc/testsuite/gcc.dg/Warray-bounds-11.c
new file mode 100644
index 000..2e68498
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Warray-bounds-11.c
@@ -0,0 +1,96 @@
+/* { dg-do compile } */
+/* { dg-options -O3 -Warray-bounds=2 } */
+
+extern void* malloc(unsigned long x);
+
+int e[3];
+
+struct f { int f[3]; };
+
+extern void bar(int v[]);
+
+struct h {
+
+ int i;
+ int j[];
+};
+
+struct h0 {
+
+ int i;
+ int j[0];
+};
+
+struct h0b {
+
+ int i;
+ int j[0];
+ int k;
+};
+
+struct h1 {
+
+ int i;
+ int j[1];
+};
+
+struct h1b {
+
+ int i;
+ int j[1];
+ int k;
+};
+
+struct h3 {
+
+ int i;
+ int j[3];
+};
+
+struct h3b {
+
+ int i;
+ int j[3];
+ int k;
+};
+
+void foo(int (*a)[3])
+{
+ (*a)[4] = 1;/* { dg-warning subscript is above array bound } */
+ a[0][0] = 1;// ok
+ a[1][0] = 1;// ok
+ a[1][4] = 1;/* { dg-warning subscript is above array bound } */
+
+ int c[3] = { 0 };
+
+ c[4] = 1; /* { dg-warning subscript is
[PATCH v4] warning about const multidimensional array as function parameter
For completeness, this version adds the missing warning if the
'const' is lost in a conditional expression with a void* on the
other branch. The only code change relative to the previous
version is in c/c-typeck.c in build_conditional_expr (otherwise
I added the warnings to the testsuite and fixed some typos and
trailing whitespace errors).
I would rather have the 'const' propagate to the result
of the conditional expression as with regular pointers to
const, but I do not see an easy way to make the warnings
(or errors) which could result from this be dependent on
-Wdiscarded-array-qualifiers.
Joseph Myers jos...@codesourcery.com:
On Thu, 6 Nov 2014, Martin Uecker wrote:
This patch implements a new proposed behaviour for
pointers to arrays with qualifiers in C.
I found some time to work on this again, so here is another
revision. Main changes to the previous version of the patch:
- add more test cases for cast from/to (const) void*
- correctly uses pedwarn/warning_at
Thanks. I'll review this in more detail later, but I suspect it's quite
close to being ready to go in once the paperwork has been processed.
Thank you. I got the form and have sent it back, but did not get
a reply yet.
Martin
2014-11-10 Martin Uecker uec...@eecs.berkeley.edu
* doc/invoke.texi: Document -Wdiscarded-array-qualifiers
* doc/extend.texi: Document new behavior for pointers to arrays with
qualifies
c/
* c-typeck.c: New behavious for pointers to arrays with qualifiers
(common-pointer-type): For pointers to arrays take qualifiers from
element type.
(build_conditional_expr): Add warnings for lost qualifiers.
(comp-target-types): Allow pointers to arrays with different qualifiers.
(convert-for-assignment): Adapt warnings for discarded qualifiers. Add
WARNING_FOR_QUALIFIERS macro and rename WARN_FOR_QUALIFIERS
to PEDWARN_FOR_QUALIFIERS.
c-family/
* c.opt (Wdiscarded-array-qualifiers): New option
testsuite/
* gcc.dg/Wwrite-strings-1.c: Change dg-warning
* gcc.dg/array-quals-1.c: Use -Wno-discarded-array-qualifiers
* gcc.dg/array-quals-2.c: Change dg-options, dg-warning
* gcc.dg/pointer-array-atomic.c: New test
* gcc.dg/pointer-array-quals-1.c: New test
* gcc.dg/pointer-array-quals-2.c: New test (-pedantic-errors)
Index: gcc/c/c-typeck.c
===
--- gcc/c/c-typeck.c(Revision 217273)
+++ gcc/c/c-typeck.c(Arbeitskopie)
@@ -673,12 +673,13 @@ common_pointer_type (tree t1, tree t2)
mv2 = TYPE_MAIN_VARIANT (pointed_to_2);
target = composite_type (mv1, mv2);
+ /* Strip array types to get correct qualifier for pointers to arrays */
+ quals1 = TYPE_QUALS_NO_ADDR_SPACE (strip_array_types (pointed_to_1));
+ quals2 = TYPE_QUALS_NO_ADDR_SPACE (strip_array_types (pointed_to_2));
+
/* For function types do not merge const qualifiers, but drop them
if used inconsistently. The middle-end uses these to mark const
and noreturn functions. */
- quals1 = TYPE_QUALS_NO_ADDR_SPACE (pointed_to_1);
- quals2 = TYPE_QUALS_NO_ADDR_SPACE (pointed_to_2);
-
if (TREE_CODE (pointed_to_1) == FUNCTION_TYPE)
target_quals = (quals1 quals2);
else
@@ -1224,6 +1225,7 @@ static int
comp_target_types (location_t location, tree ttl, tree ttr)
{
int val;
+ int val_ped;
tree mvl = TREE_TYPE (ttl);
tree mvr = TREE_TYPE (ttr);
addr_space_t asl = TYPE_ADDR_SPACE (mvl);
@@ -1235,19 +1237,32 @@ comp_target_types (location_t location, tree ttl,
if (!addr_space_superset (asl, asr, as_common))
return 0;
- /* Do not lose qualifiers on element types of array types that are
- pointer targets by taking their TYPE_MAIN_VARIANT. */
- if (TREE_CODE (mvl) != ARRAY_TYPE)
-mvl = (TYPE_ATOMIC (mvl)
- ? c_build_qualified_type (TYPE_MAIN_VARIANT (mvl), TYPE_QUAL_ATOMIC)
- : TYPE_MAIN_VARIANT (mvl));
- if (TREE_CODE (mvr) != ARRAY_TYPE)
-mvr = (TYPE_ATOMIC (mvr)
- ? c_build_qualified_type (TYPE_MAIN_VARIANT (mvr), TYPE_QUAL_ATOMIC)
- : TYPE_MAIN_VARIANT (mvr));
+ /* For pedantic record result of comptypes on arrays before losing
+ qualifiers on the element type below. */
+ val_ped = 1;
+
+ if (TREE_CODE (mvl) == ARRAY_TYPE
+ TREE_CODE (mvr) == ARRAY_TYPE)
+val_ped = comptypes (mvl, mvr);
+
+ /* Qualifiers on element types of array types that are
+ pointer targets are lost by taking their TYPE_MAIN_VARIANT. */
+
+ mvl = (TYPE_ATOMIC (strip_array_types (mvl))
+? c_build_qualified_type (TYPE_MAIN_VARIANT (mvl), TYPE_QUAL_ATOMIC)
+: TYPE_MAIN_VARIANT (mvl));
+
+ mvr = (TYPE_ATOMIC (strip_array_types (mvr))
+? c_build_qualified_type (TYPE_MAIN_VARIANT (mvr), TYPE_QUAL_ATOMIC)
+: TYPE_MAIN_VARIANT (mvr));
+
enum_and_int_p = false;
val = comptypes_check_enum_int (mvl, mvr, enum_and_int_p
[PATCH v3] warning about const multidimensional array as function parameter
This patch implements a new proposed behaviour for
pointers to arrays with qualifiers in C.
I found some time to work on this again, so here is another
revision. Main changes to the previous version of the patch:
- add more test cases for cast from/to (const) void*
- correctly uses pedwarn/warning_at
previous versions (see v1 for a description of the main idea):
v2: https://gcc.gnu.org/ml/gcc/2014-10/msg00243.html
v1: https://gcc.gnu.org/ml/gcc/2014-10/msg00224.html
On Wed, 29 Oct 2014 16:43:16 +
Joseph S. Myers jos...@codesourcery.com:
On Tue, 28 Oct 2014, Martin Uecker wrote:
Note that there is now a semantic (and not only diagnostic) change.
Without this patch
const int a[1];
int b[1];
(x ? a : b)
would return a 'void*' and a warning about pointer type mismatch.
With this patch the conditional has type 'const int (*)[1]'.
I believe that is safe (in that that conditional expression isn't valid in
ISO C). What wouldn't be safe is making a conditional expression between
void * and const int (*)[] have type const void * instead of void *.
Yes, the cases which are changed in the patch should all
be illegal in ISO C.
On the other hand, not changing the case you mention means that
there is still inconsistent behaviour even with the patch:
void* v;
const int (*i)[1];
int (*ip)[1] = (1 ? v : i); // const is lost in conditional
const int (*i2);
int (*ip2) = (1 ? v : i2); // const is not lost - warning
Maybe I should at least add a warning for the first case where the
'const' is lost already in the conditional?
WARN_FOR_QUALIFIERS uses pedwarn. That means this is not safe, because
pedwarns become errors with -pedantic-errors, but this includes cases that
are valid in ISO C and so must not become errors with -pedantic-errors
(such as converting const int (*)[] to void *).
Ok. I added a new macro WARNING_FOR_QUALIFIERS and renamed the others
to PEDWARN_FOR_* . The full logic to make this work correctly turned
out to be quite simple.
The behaviour can be summarized like this:
p2ca = pointer to const array
p2cv = pointer to const void
...
lq = loss of qualifier
ip = incompatible pointer
'!' error with -pedantic-errors
'*' can be deactivated with a new option
conversionold new new+pedantic
p2ca - p2a : ip! | lq* | ip! + lq*
p2a - p2ca: ip! | | ip!
p2v - p2a : | |
p2v - p2ca : | |
p2cv - p2a : lq! | lq! | lq!
p2cv - p2ca: lq! | | lq!
p2a - p2v : | |
p2a - p2cv : | |
p2ca - p2v : | lq* | lq*
p2ca - p2cv: | |
2014-10-28 Martin Uecker uec...@eecs.berkeley.edu
* doc/invoke.texi: Document -Wdiscarded-array-qualifiers
* doc/extend.texi: Document new behavior for pointers to arrays with
qualifies
c/
* c-typeck.c: New behavious for pointers to arrays with qualifiers
(common-pointer-type): For pointers to arrays take qualifiers from
element type.
(comp-target-types): Allow pointers to arrays with different qualifiers.
(convert-for-assignment): Change warnings for discarded qualifiers. Add
WARNING_FOR_QUALIFIERS macro and rename WARN_FOR_QUALIFIERS
to PEDWARN_FOR_QUALIFIERS.
c-family/
* c.opt (Wdiscarded-array-qualifiers): New option
testsuite/
* gcc.dg/Wwrite-strings-1.c: Change dg-warning
* gcc.dg/array-quals-1.c: Use -Wno-discarded-array-qualifiers
* gcc.dg/array-quals-2.c: Change dg-options, dg-warning
* gcc.dg/pointer-array-atomic.c: New test
* gcc.dg/pointer-array-quals-1.c: New test
* gcc.dg/pointer-array-quals-2.c: New test (-pedantic-errors)
Index: gcc/c/c-typeck.c
===
--- gcc/c/c-typeck.c(Revision 217175)
+++ gcc/c/c-typeck.c(Arbeitskopie)
@@ -673,12 +673,13 @@ common_pointer_type (tree t1, tree t2)
mv2 = TYPE_MAIN_VARIANT (pointed_to_2);
target = composite_type (mv1, mv2);
+ /* Strip array types to get correct qualifier for pointers to arrays */
+ quals1 = TYPE_QUALS_NO_ADDR_SPACE (strip_array_types (pointed_to_1));
+ quals2 = TYPE_QUALS_NO_ADDR_SPACE (strip_array_types (pointed_to_2));
+
/* For function types do not merge const qualifiers, but drop them
if used inconsistently. The middle-end uses these to mark const
and noreturn functions. */
- quals1 = TYPE_QUALS_NO_ADDR_SPACE (pointed_to_1);
- quals2 = TYPE_QUALS_NO_ADDR_SPACE (pointed_to_2);
-
if (TREE_CODE (pointed_to_1) == FUNCTION_TYPE)
target_quals = (quals1 quals2);
else
@@ -1224,6 +1225,7 @@ static int
comp_target_types (location_t location, tree ttl, tree ttr)
{
int val;
+ int val_ped;
tree mvl = TREE_TYPE (ttl);
tree mvr = TREE_TYPE (ttr);
addr_space_t asl = TYPE_ADDR_SPACE (mvl);
@@ -1235,19 +1237,32 @@ comp_target_types (location_t location, tree ttl,
if (!addr_space_superset (asl, asr, as_common))
return 0;
- /* Do not lose qualifiers on element
