[C PATCH, v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]
Compared to the previous version I changed the name of the
warning to "Walloc-size" which matches "Wanalyzer-allocation-size"
but is still in line with the other -Walloc-something warnings
we have. I also added it to Wextra.
I found PR71219 that requests the warning and points out that
it is recommended by the C secure coding guidelines and added
the PR to the commit log (although the version with cast is not
diagnosed so far.)
I did not have time to implement the extensions suggested
on the list, i.e. warn when the size is not a multiple
of the size of the type and warn for if the size is not
suitable for a flexible array member. (this is also a bit
more complicated than it seems)
Bootstrapped and regression tested on x86_64.
Martin
Add option Walloc-size that warns about allocations that have
insufficient storage for the target type of the pointer the
storage is assigned to.
PR c/71219
gcc:
* doc/invoke.texi: Document -Walloc-size option.
gcc/c-family:
* c.opt (Walloc-size): New option.
gcc/c:
* c-typeck.cc (convert_for_assignment): Add warning.
gcc/testsuite:
* gcc.dg/Walloc-size-1.c: New test.
---
gcc/c-family/c.opt | 4
gcc/c/c-typeck.cc| 27 +
gcc/doc/invoke.texi | 10
gcc/testsuite/gcc.dg/Walloc-size-1.c | 36
4 files changed, 77 insertions(+)
create mode 100644 gcc/testsuite/gcc.dg/Walloc-size-1.c
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 7348ad42ee0..9ba08a1fb6d 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -319,6 +319,10 @@ Walloca
C ObjC C++ ObjC++ Var(warn_alloca) Warning
Warn on any use of alloca.
+Walloc-size
+C ObjC Var(warn_alloc_size) Warning
+Warn when allocating insufficient storage for the target type of the assigned
pointer.
+
Walloc-size-larger-than=
C ObjC C++ LTO ObjC++ Var(warn_alloc_size_limit) Joined Host_Wide_Int ByteSize
Warning Init(HOST_WIDE_INT_MAX)
-Walloc-size-larger-than= Warn for calls to allocation functions
that
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index e2bfd2caf85..c759c6245ed 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -7384,6 +7384,33 @@ convert_for_assignment (location_t location, location_t
expr_loc, tree type,
"request for implicit conversion "
"from %qT to %qT not permitted in C++", rhstype, type);
+ /* Warn of new allocations that are not big enough for the target
+type. */
+ tree fndecl;
+ if (warn_alloc_size
+ && TREE_CODE (rhs) == CALL_EXPR
+ && (fndecl = get_callee_fndecl (rhs)) != NULL_TREE
+ && DECL_IS_MALLOC (fndecl))
+ {
+ tree fntype = TREE_TYPE (fndecl);
+ tree fntypeattrs = TYPE_ATTRIBUTES (fntype);
+ tree alloc_size = lookup_attribute ("alloc_size", fntypeattrs);
+ if (alloc_size)
+ {
+ tree args = TREE_VALUE (alloc_size);
+ int idx = TREE_INT_CST_LOW (TREE_VALUE (args)) - 1;
+ /* For calloc only use the second argument. */
+ if (TREE_CHAIN (args))
+ idx = TREE_INT_CST_LOW (TREE_VALUE (TREE_CHAIN (args))) - 1;
+ tree arg = CALL_EXPR_ARG (rhs, idx);
+ if (TREE_CODE (arg) == INTEGER_CST
+ && tree_int_cst_lt (arg, TYPE_SIZE_UNIT (ttl)))
+warning_at (location, OPT_Walloc_size, "allocation of "
+"insufficient size %qE for type %qT with "
+"size %qE", arg, ttl, TYPE_SIZE_UNIT (ttl));
+ }
+ }
+
/* See if the pointers point to incompatible address spaces. */
asl = TYPE_ADDR_SPACE (ttl);
asr = TYPE_ADDR_SPACE (ttr);
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 33befee7d6b..a4fbcf5e1b5 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -8086,6 +8086,16 @@ always leads to a call to another @code{cold} function
such as wrappers of
C++ @code{throw} or fatal error reporting functions leading to @code{abort}.
@end table
+@opindex Wno-alloc-size
+@opindex Walloc-size
+@item -Walloc-size
+Warn about calls to allocation functions decorated with attribute
+@code{alloc_size} that specify insufficient size for the target type of
+the pointer the result is assigned to, including those to the built-in
+forms of the functions @code{aligned_alloc}, @code{alloca},
+@code{calloc},
+@code{malloc}, and @code{realloc}.
+
@opindex Wno-alloc-zero
@opindex Walloc-zero
@item -Walloc-zero
diff --git a/gcc/testsuite/gcc.dg/Walloc-size-1.c
b/gcc/testsuite/gcc.dg/Walloc-size-1.c
new file mode 100644
index 000..61806f58192
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloc-size-1.c
@@ -0,0 +1,36 @@
+/* Tests the warnings for insufficient allocation size.
+ { dg-do compile }
+ { dg-options "-Walloc-size" }
+ * */
+#include
+#include
+
+struct
[C PATCH 1/6 v2] c: reorganize recursive type checking
Thanks Joseph, below is a a revised version of this patch with slight additional changes to the comment of tagged_types_tu_compatible_p. ok for trunk? Martin Am Mittwoch, dem 06.09.2023 um 20:59 + schrieb Joseph Myers: > On Sat, 26 Aug 2023, Martin Uecker via Gcc-patches wrote: > > > -static int > > +static bool > > comp_target_types (location_t location, tree ttl, tree ttr) > > The comment above this function should be updated to refer to returning > true, not to returning 1. And other comments on common_pointer_type and > inside that function should be updated to refer to comp_target_types > returning true, not nonzero. > > > @@ -1395,17 +1382,13 @@ free_all_tagged_tu_seen_up_to (const struct > > tagged_tu_seen_cache *tu_til) > > > > /* Return 1 if two 'struct', 'union', or 'enum' types T1 and T2 are > > compatible. If the two types are not the same (which has been > > - checked earlier), this can only happen when multiple translation > > - units are being compiled. See C99 6.2.7 paragraph 1 for the exact > > - rules. ENUM_AND_INT_P and DIFFERENT_TYPES_P are as in > > - comptypes_internal. */ > > + checked earlier). */ > > > > -static int > > +static bool > > tagged_types_tu_compatible_p (const_tree t1, const_tree t2, > > - bool *enum_and_int_p, bool *different_types_p) > > + struct comptypes_data* data) > > Similarly, this comment should be updated for the new return type. Also > the GNU style is "struct comptypes_data *data" with space before not after > '*'. > > > @@ -1631,9 +1603,9 @@ tagged_types_tu_compatible_p (const_tree t1, > > const_tree t2, > > Otherwise, the argument types must match. > > ENUM_AND_INT_P and DIFFERENT_TYPES_P are as in comptypes_internal. */ > > > > -static int > > +static bool > > function_types_compatible_p (const_tree f1, const_tree f2, > > -bool *enum_and_int_p, bool *different_types_p) > > +struct comptypes_data *data) > > Another comment to update for a changed return type. > > > /* Check two lists of types for compatibility, returning 0 for > > - incompatible, 1 for compatible, or 2 for compatible with > > - warning. ENUM_AND_INT_P and DIFFERENT_TYPES_P are as in > > - comptypes_internal. */ > > + incompatible, 1 for compatible. ENUM_AND_INT_P and > > + DIFFERENT_TYPES_P are as in comptypes_internal. */ > > > > -static int > > +static bool > > type_lists_compatible_p (const_tree args1, const_tree args2, > > -bool *enum_and_int_p, bool *different_types_p) > > +struct comptypes_data *data) > > This one also needs updating to remove references to parameters that no > longer exist. > c: reorganize recursive type checking Reorganize recursive type checking to use a structure to store information collected during the recursion and returned to the caller (warning_needed, enum_and_init_p, different_types_p). gcc/c: * c-typeck.cc (struct comptypes_data): Add structure. (tagged_types_tu_compatible_p, function_types_compatible_p, type_lists_compatible_p, comptypes_internal): Add structure to interface, change return type to bool, and adapt calls. (comptarget_types): Change return type too bool. (comptypes, comptypes_check_enum_int, comptypes_check_different_types): Adapt calls. --- gcc/c/c-typeck.cc | 282 -- 1 file changed, 121 insertions(+), 161 deletions(-) diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc index e2bfd2caf85..e55e887da14 100644 --- a/gcc/c/c-typeck.cc +++ b/gcc/c/c-typeck.cc @@ -90,12 +90,14 @@ static bool require_constant_elements; static bool require_constexpr_value; static tree qualify_type (tree, tree); -static int tagged_types_tu_compatible_p (const_tree, const_tree, bool *, -bool *); -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 *); +struct comptypes_data; +static bool tagged_types_tu_compatible_p (const_tree, const_tree, + struct comptypes_data *); +static bool comp_target_types (location_t, tree, tree); +static bool function_types_compatible_p (const_tree, const_tree, +struct comptypes_data *); +static bool type_lists_compatible_p (const_tree, cons
[C PATCH] c: flag for tag compatibility rules
Add a flag to turn tag compatibility rules on or off
independent from the language version.
gcc/c-family:
* c.opt (flag_tag_compat): New flag.
gcc/c:
* c-decl.cc (diagnose_mismatched_decls, start_struct,
finish_struct, start_enum, finish_enum): Support flag.
* c-typeck.cc (composite_type_internal): Support flag.
gcc/doc:
* invoke.texi: Document flag.
gcc/testsuite:
* gcc.dg/asan/pr81470.c: Turn off tag compatibility.
* gcc.dg/c99-tag-1.c: Turn off tag compatibility.
* gcc.dg/c99-tag-2.c: Turn off tag compatibility.
* gcc.dg/decl-3.c: Turn off tag compatibility.
* gcc.dg/enum-redef-1.c: Turn off tag compatibility.
* gcc.dg/pr17188-1.c: Turn off tag compatibility.
* gcc.dg/pr18809-1.c: Turn off tag compatibility.
* gcc.dg/pr39084.c: Turn off tag compatibility.
* gcc.dg/pr79983.c: Turn off tag compatibility.
---
gcc/c-family/c.opt | 3 +++
gcc/c/c-decl.cc | 12 ++--
gcc/c/c-typeck.cc | 2 +-
gcc/doc/invoke.texi | 5 +
gcc/testsuite/gcc.dg/asan/pr81460.c | 1 +
gcc/testsuite/gcc.dg/c99-tag-1.c| 2 +-
gcc/testsuite/gcc.dg/c99-tag-2.c| 2 +-
gcc/testsuite/gcc.dg/decl-3.c | 1 +
gcc/testsuite/gcc.dg/enum-redef-1.c | 2 ++
gcc/testsuite/gcc.dg/pr17188-1.c| 2 +-
gcc/testsuite/gcc.dg/pr18809-1.c| 1 +
gcc/testsuite/gcc.dg/pr39084.c | 2 +-
gcc/testsuite/gcc.dg/pr79983.c | 2 +-
13 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 2242524cd3e..f95f12ba249 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -2214,6 +2214,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 2137ba8b845..6d1e0d5c382 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -2094,7 +2094,7 @@ diagnose_mismatched_decls (tree newdecl, tree olddecl,
given scope. */
if (TREE_CODE (olddecl) == CONST_DECL)
{
- if (flag_isoc2x
+ if ((flag_isoc2x || flag_tag_compat)
&& TYPE_NAME (DECL_CONTEXT (newdecl))
&& DECL_CONTEXT (newdecl) != DECL_CONTEXT (olddecl)
&& TYPE_NAME (DECL_CONTEXT (newdecl)) == TYPE_NAME (DECL_CONTEXT
(olddecl)))
@@ -8723,7 +8723,7 @@ start_struct (location_t loc, enum tree_code code, tree
name,
/* For C2X, even if we already have a completed definition,
we do not use it. We will check for consistency later. */
- if (flag_isoc2x && ref && TYPE_SIZE (ref))
+ if ((flag_isoc2x || flag_tag_compat) && ref && TYPE_SIZE (ref))
ref = NULL_TREE;
if (ref && TREE_CODE (ref) == code)
@@ -9515,7 +9515,7 @@ finish_struct (location_t loc, tree t, tree fieldlist,
tree attributes,
}
/* Check for consistency with previous definition */
- if (flag_isoc2x)
+ if (flag_isoc2x || flag_tag_compat)
{
tree vistype = previous_tag (t);
if (vistype
@@ -9534,7 +9534,7 @@ finish_struct (location_t loc, tree t, tree fieldlist,
tree attributes,
C_TYPE_BEING_DEFINED (t) = 0;
/* Set type canonical based on equivalence class. */
- if (flag_isoc2x)
+ if (flag_isoc2x || flag_tag_compat)
{
if (NULL == c_struct_htab)
c_struct_htab = hash_table::create_ggc (61);
@@ -9672,7 +9672,7 @@ start_enum (location_t loc, struct c_enum_contents
*the_enum, tree name,
if (name != NULL_TREE)
enumtype = lookup_tag (ENUMERAL_TYPE, name, true, &enumloc);
- if (flag_isoc2x && enumtype != NULL_TREE
+ if ((flag_isoc2x || flag_tag_compat) && enumtype != NULL_TREE
&& TREE_CODE (enumtype) == ENUMERAL_TYPE
&& TYPE_VALUES (enumtype) != NULL_TREE)
enumtype = NULL_TREE;
@@ -9941,7 +9941,7 @@ finish_enum (tree enumtype, tree values, tree attributes)
struct_parse_info->struct_types.safe_push (enumtype);
/* Check for consistency with previous definition */
- if (flag_isoc2x)
+ if (flag_isoc2x || flag_tag_compat)
{
tree vistype = previous_tag (enumtype);
if (vistype
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index 357367eab09..b99f0c3e2fd 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -512,7 +512,7 @@ composite_type_internal (tree t1, tree t2, struct
composite_cache* cache)
case RECORD_TYPE:
case UNION_TYPE:
- if (flag_isoc2x && !comptypes_same_p (t1, t2))
+ if ((flag_isoc2x || flag_tag_compat) && !comptypes_same_p (t1, t2))
{
gcc_checking_assert (COMPLETE_TYPE_P (t1) && COMPLETE_TYPE_P (t2));
gcc_checking_assert (comptypes (t1, t2));
diff --git a/gcc/doc/invoke.texi b/gcc/
[C PATCH 6/6] c23: construct composite type for tagged types
Support for constructing composite type for structs and unions
in C23.
gcc/c:
* c-typeck.cc (composite_type_internal): Adapted from
composite_type to support structs and unions.
(composite_type): New wrapper function.
(build_conditional_operator): Return composite type.
gcc/testsuite:
* gcc.dg/c2x-tag-composite-1.c: New test.
* gcc.dg/c2x-tag-composite-2.c: New test.
* gcc.dg/c2x-tag-composite-3.c: New test.
* gcc.dg/c2x-tag-composite-4.c: New test.
---
gcc/c/c-typeck.cc | 114 +
gcc/testsuite/gcc.dg/c2x-tag-composite-1.c | 26 +
gcc/testsuite/gcc.dg/c2x-tag-composite-2.c | 16 +++
gcc/testsuite/gcc.dg/c2x-tag-composite-3.c | 17 +++
gcc/testsuite/gcc.dg/c2x-tag-composite-4.c | 21
5 files changed, 176 insertions(+), 18 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-composite-1.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-composite-2.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-composite-3.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-composite-4.c
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index 2489fa1e3d1..357367eab09 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -381,8 +381,15 @@ build_functype_attribute_variant (tree ntype, tree otype,
tree attrs)
nonzero; if that isn't so, this may crash. In particular, we
assume that qualifiers match. */
+struct composite_cache {
+ tree t1;
+ tree t2;
+ tree composite;
+ struct composite_cache* next;
+};
+
tree
-composite_type (tree t1, tree t2)
+composite_type_internal (tree t1, tree t2, struct composite_cache* cache)
{
enum tree_code code1;
enum tree_code code2;
@@ -425,7 +432,8 @@ composite_type (tree t1, tree t2)
{
tree pointed_to_1 = TREE_TYPE (t1);
tree pointed_to_2 = TREE_TYPE (t2);
- tree target = composite_type (pointed_to_1, pointed_to_2);
+ tree target = composite_type_internal (pointed_to_1,
+ pointed_to_2, cache);
t1 = build_pointer_type_for_mode (target, TYPE_MODE (t1), false);
t1 = build_type_attribute_variant (t1, attributes);
return qualify_type (t1, t2);
@@ -433,7 +441,8 @@ composite_type (tree t1, tree t2)
case ARRAY_TYPE:
{
- tree elt = composite_type (TREE_TYPE (t1), TREE_TYPE (t2));
+ tree elt = composite_type_internal (TREE_TYPE (t1), TREE_TYPE (t2),
+ cache);
int quals;
tree unqual_elt;
tree d1 = TYPE_DOMAIN (t1);
@@ -501,9 +510,61 @@ composite_type (tree t1, tree t2)
return build_type_attribute_variant (t1, attributes);
}
-case ENUMERAL_TYPE:
case RECORD_TYPE:
case UNION_TYPE:
+ if (flag_isoc2x && !comptypes_same_p (t1, t2))
+ {
+ gcc_checking_assert (COMPLETE_TYPE_P (t1) && COMPLETE_TYPE_P (t2));
+ gcc_checking_assert (comptypes (t1, t2));
+
+ /* If a composite type for these two types is already under
+construction, return it. */
+
+ for (struct composite_cache *c = cache; c != NULL; c = c->next)
+ if (c->t1 == t1 && c->t2 == t2)
+ return c->composite;
+
+ /* Otherwise, create a new type node and link it into the cache. */
+
+ tree n = make_node (code1);
+ struct composite_cache cache2 = { t1, t2, n, cache };
+ cache = &cache2;
+
+ tree f1 = TYPE_FIELDS (t1);
+ tree f2 = TYPE_FIELDS (t2);
+ tree fields = NULL_TREE;
+
+ for (tree a = f1, b = f2; a && b;
+ a = DECL_CHAIN (a), b = DECL_CHAIN (b))
+ {
+ tree ta = TREE_TYPE (a);
+ tree tb = TREE_TYPE (b);
+
+ gcc_assert (DECL_NAME (a) == DECL_NAME (b));
+ gcc_assert (comptypes (ta, tb));
+
+ tree f = build_decl (input_location, FIELD_DECL, DECL_NAME (a),
+ composite_type_internal (ta, tb, cache));
+
+ DECL_FIELD_CONTEXT (f) = n;
+ DECL_CHAIN (f) = fields;
+ fields = f;
+ }
+
+ TYPE_NAME (n) = TYPE_NAME (t1);
+ TYPE_FIELDS (n) = nreverse (fields);
+ TYPE_ATTRIBUTES (n) = attributes;
+ layout_type (n);
+ n = build_type_attribute_variant (n, attributes);
+ n = qualify_type (n, t1);
+
+ gcc_checking_assert (comptypes (n, t1));
+ gcc_checking_assert (comptypes (n, t2));
+
+ return n;
+ }
+ /* FALLTHRU */
+case ENUMERAL_TYPE:
if (attributes != NULL)
{
/* Try harder not to create a new aggregate type. */
@@ -518,7 +579,8 @@ composite_type (tree t1, tree t2)
/* Function types: prefer the one that specified arg types.
If both do, merge the arg types. Also merge the return types. */
{
- tree valtype = composite_type (TREE_TYPE (t1), TREE
[C PATCH 5/6] c23: aliasing of compatible tagged types
Tell the backend which types are equivalent by setting
TYPE_CANONICAL to one struct in the set of equivalent
structs. Structs are considered equivalent by ignoring
all sizes of arrays nested in types below field level.
gcc/c:
* c-decl.cc (c_struct_hasher): Hash stable for struct
types.
(c_struct_hasher::hash, c_struct_hasher::equal): New functions.
(finish_struct): Set TYPE_CANONICAL to first struct in
equivalence class.
* c-objc-common.cc (c_get_alias_set): Let structs or
unions with variable size alias anything.
* c-tree.h (comptypes_equiv): New prototype.
* c-typeck.cc (comptypes_equiv): New function.
(comptypes_internal): Implement equivalence mode.
(tagged_types_tu_compatible): Implement equivalence mode.
gcc/testsuite:
* gcc.dg/c2x-tag-2.c: Remove xfail.
* gcc.dg/c2x-tag-6.c: Remove xfail.
* gcc.dg/c2x-tag-alias-1.c: New test.
* gcc.dg/c2x-tag-alias-2.c: New test.
* gcc.dg/c2x-tag-alias-3.c: New test.
* gcc.dg/c2x-tag-alias-4.c: New test.
* gcc.dg/c2x-tag-alias-5.c: New test.
* gcc.dg/c2x-tag-alias-6.c: New test.
* gcc.dg/c2x-tag-alias-7.c: New test.
* gcc.dg/c2x-tag-alias-8.c: New test.
---
gcc/c/c-decl.cc| 48 +
gcc/c/c-objc-common.cc | 5 ++
gcc/c/c-tree.h | 1 +
gcc/c/c-typeck.cc | 31
gcc/testsuite/gcc.dg/c2x-tag-2.c | 2 +-
gcc/testsuite/gcc.dg/c2x-tag-6.c | 2 +-
gcc/testsuite/gcc.dg/c2x-tag-alias-1.c | 48 +
gcc/testsuite/gcc.dg/c2x-tag-alias-2.c | 73 +++
gcc/testsuite/gcc.dg/c2x-tag-alias-3.c | 48 +
gcc/testsuite/gcc.dg/c2x-tag-alias-4.c | 73 +++
gcc/testsuite/gcc.dg/c2x-tag-alias-5.c | 30
gcc/testsuite/gcc.dg/c2x-tag-alias-6.c | 77
gcc/testsuite/gcc.dg/c2x-tag-alias-7.c | 98 ++
gcc/testsuite/gcc.dg/c2x-tag-alias-8.c | 90 +++
14 files changed, 624 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-alias-1.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-alias-2.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-alias-3.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-alias-4.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-alias-5.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-alias-6.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-alias-7.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-alias-8.c
diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index b514e8a35ee..2137ba8b845 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -603,6 +603,36 @@ public:
auto_vec typedefs_seen;
};
+
+/* Hash table for structs and unions. */
+struct c_struct_hasher : ggc_ptr_hash
+{
+ static hashval_t hash (tree t);
+ static bool equal (tree, tree);
+};
+
+/* Hash an RECORD OR UNION. */
+hashval_t
+c_struct_hasher::hash (tree type)
+{
+ inchash::hash hstate;
+
+ hstate.add_int (TREE_CODE (type));
+ hstate.add_object (TYPE_NAME (type));
+
+ return hstate.end ();
+}
+
+/* Compare two RECORD or UNION types. */
+bool
+c_struct_hasher::equal (tree t1, tree t2)
+{
+ return comptypes_equiv_p (t1, t2);
+}
+
+/* All tagged typed so that TYPE_CANONICAL can be set correctly. */
+static GTY (()) hash_table *c_struct_htab;
+
/* 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;
@@ -9503,6 +9533,24 @@ finish_struct (location_t loc, tree t, tree fieldlist,
tree attributes,
C_TYPE_BEING_DEFINED (t) = 0;
+ /* Set type canonical based on equivalence class. */
+ if (flag_isoc2x)
+{
+ if (NULL == c_struct_htab)
+ c_struct_htab = hash_table::create_ggc (61);
+
+ hashval_t hash = c_struct_hasher::hash (t);
+
+ tree *e = c_struct_htab->find_slot_with_hash (t, hash, INSERT);
+ if (*e)
+ TYPE_CANONICAL (t) = *e;
+ else
+ {
+ TYPE_CANONICAL (t) = t;
+ *e = t;
+ }
+}
+
tree incomplete_vars = C_TYPE_INCOMPLETE_VARS (TYPE_MAIN_VARIANT (t));
for (x = TYPE_MAIN_VARIANT (t); x; x = TYPE_NEXT_VARIANT (x))
{
diff --git a/gcc/c/c-objc-common.cc b/gcc/c/c-objc-common.cc
index e4aed61ed00..992225bbb29 100644
--- a/gcc/c/c-objc-common.cc
+++ b/gcc/c/c-objc-common.cc
@@ -389,6 +389,11 @@ c_get_alias_set (tree t)
if (TREE_CODE (t) == ENUMERAL_TYPE)
return get_alias_set (ENUM_UNDERLYING_TYPE (t));
+ /* Structs with variable size can alias different incompatible
+ structs. Let them alias anything. */
+ if (RECORD_OR_UNION_TYPE_P (t) && C_TYPE_VARIABLE_SIZE (t))
+return 0;
+
return c_common_get_alias_set (t);
}
diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h
index 511fd9ee0e5..1a8e8f072bd 100644
--- a/gcc/c/c-tree.h
+++ b/gcc/c/c-tree.h
[C PATCH 4/6] c23: tag compatibility rules for enums
Allow redefinition of enum types and enumerators.
gcc/c:
* c-decl.cc (start_num): Allow redefinition.
(finish_enum): Diagnose conflicts.
(build_enumerator): Set context.
(diagnose_mismatched_decls): Diagnose conflicting enumerators.
(push_decl): Preserve context for enumerators.
gcc/testsuide/:
* gcc.dg/c2x-tag-enum-1.c: New test.
* gcc.dg/c2x-tag-enum-2.c: New test.
* gcc.dg/c2x-tag-enum-3.c: New test.
* gcc.dg/c2x-tag-enum-4.c: New test.
---
gcc/c/c-decl.cc | 47 --
gcc/c/c-typeck.cc | 5 ++-
gcc/testsuite/gcc.dg/c2x-tag-enum-1.c | 56 +++
gcc/testsuite/gcc.dg/c2x-tag-enum-2.c | 23 +++
gcc/testsuite/gcc.dg/c2x-tag-enum-3.c | 7
gcc/testsuite/gcc.dg/c2x-tag-enum-4.c | 22 +++
6 files changed, 155 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-enum-1.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-enum-2.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-enum-3.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-enum-4.c
diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index c5c6a853fa9..b514e8a35ee 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -2064,9 +2064,24 @@ diagnose_mismatched_decls (tree newdecl, tree olddecl,
given scope. */
if (TREE_CODE (olddecl) == CONST_DECL)
{
- auto_diagnostic_group d;
- error ("redeclaration of enumerator %q+D", newdecl);
- locate_old_decl (olddecl);
+ if (flag_isoc2x
+ && TYPE_NAME (DECL_CONTEXT (newdecl))
+ && DECL_CONTEXT (newdecl) != DECL_CONTEXT (olddecl)
+ && TYPE_NAME (DECL_CONTEXT (newdecl)) == TYPE_NAME (DECL_CONTEXT
(olddecl)))
+ {
+ if (!simple_cst_equal (DECL_INITIAL (olddecl), DECL_INITIAL
(newdecl)))
+ {
+ auto_diagnostic_group d;
+ error ("conflicting redeclaration of enumerator %q+D", newdecl);
+ locate_old_decl (olddecl);
+ }
+ }
+ else
+ {
+ auto_diagnostic_group d;
+ error ("redeclaration of enumerator %q+D", newdecl);
+ locate_old_decl (olddecl);
+ }
return false;
}
@@ -3227,8 +3242,11 @@ pushdecl (tree x)
/* Must set DECL_CONTEXT for everything not at file scope or
DECL_FILE_SCOPE_P won't work. Local externs don't count
- unless they have initializers (which generate code). */
+ unless they have initializers (which generate code). We
+ also exclude CONST_DECLs because enumerators will get the
+ type of the enum as context. */
if (current_function_decl
+ && TREE_CODE (x) != CONST_DECL
&& (!VAR_OR_FUNCTION_DECL_P (x)
|| DECL_INITIAL (x) || !TREE_PUBLIC (x)))
DECL_CONTEXT (x) = current_function_decl;
@@ -9606,9 +9624,15 @@ start_enum (location_t loc, struct c_enum_contents
*the_enum, tree name,
if (name != NULL_TREE)
enumtype = lookup_tag (ENUMERAL_TYPE, name, true, &enumloc);
+ if (flag_isoc2x && enumtype != NULL_TREE
+ && TREE_CODE (enumtype) == ENUMERAL_TYPE
+ && TYPE_VALUES (enumtype) != NULL_TREE)
+enumtype = NULL_TREE;
+
if (enumtype == NULL_TREE || TREE_CODE (enumtype) != ENUMERAL_TYPE)
{
enumtype = make_node (ENUMERAL_TYPE);
+ TYPE_SIZE (enumtype) = NULL_TREE;
pushtag (loc, name, enumtype);
if (fixed_underlying_type != NULL_TREE)
{
@@ -9868,6 +9892,20 @@ finish_enum (tree enumtype, tree values, tree attributes)
&& !in_sizeof && !in_typeof && !in_alignof)
struct_parse_info->struct_types.safe_push (enumtype);
+ /* Check for consistency with previous definition */
+ if (flag_isoc2x)
+{
+ tree vistype = previous_tag (enumtype);
+ if (vistype
+ && TREE_CODE (vistype) == TREE_CODE (enumtype)
+ && !C_TYPE_BEING_DEFINED (vistype))
+ {
+ TYPE_STUB_DECL (vistype) = TYPE_STUB_DECL (enumtype);
+ if (!comptypes_same_p (enumtype, vistype))
+ error("conflicting redefinition of enum %qT", enumtype);
+ }
+}
+
C_TYPE_BEING_DEFINED (enumtype) = 0;
return enumtype;
@@ -10047,6 +10085,7 @@ build_enumerator (location_t decl_loc, location_t loc,
decl = build_decl (decl_loc, CONST_DECL, name, TREE_TYPE (value));
DECL_INITIAL (decl) = value;
+ DECL_CONTEXT (decl) = the_enum->enum_type;
pushdecl (decl);
return tree_cons (decl, value, NULL_TREE);
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index 802c727d9d3..2b79cbba950 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -1396,6 +1396,9 @@ tagged_types_tu_compatible_p (const_tree t1, const_tree
t2,
{
case ENUMERAL_TYPE:
{
+ if (!comptypes (ENUM_UNDERLYING_TYPE (t1), ENUM_UNDERLYING_TYPE (t2)))
+ return false;
+
/* Speed up the case where the type values are in the same order. */
tree tv1 = TYPE_VALUES (t1);
[C PATCH 3/6] c23: tag compatibility rules for struct and unions
Implement redeclaration and compatibility rules for
structures and unions in C23.
gcc/c/:
* c-decl.cc (previous_tag): New function.
(get_parm_info): Turn off warning for C2X.
(start_struct): Allow redefinitons.
(finish_struct): Diagnose conflicts.
* c-tree.h (comptypes_same_p): Add prototype.
* c-typeck.cc (comptypes_same_p): New function
(comptypes_internal): Activate comparison of tagged
types (convert_for_assignment): Ingore qualifiers.
(digest_init): Add error.
(initialized_elementwise_p): Allow compatible types.
gcc/testsuite/:
* gcc.dg/c2x-enum-7.c: Remove warning.
* gcc.dg/c2x-tag-1.c: New test.
* gcc.dg/c2x-tag-2.c: New test.
* gcc.dg/c2x-tag-3.c: New test.
* gcc.dg/c2x-tag-4.c: New test.
* gcc.dg/c2x-tag-5.c: New test.
* gcc.dg/c2x-tag-6.c: New test.
* gcc.dg/c2x-tag-7.c: New test.
* gcc.dg/c2x-tag-8.c: New test.
* gcc.dg/c2x-tag-9.c: New test.
* gcc.dg/c2x-tag-10.c: New test.
---
gcc/c/c-decl.cc | 56 ++---
gcc/c/c-tree.h| 1 +
gcc/c/c-typeck.cc | 38 +
gcc/testsuite/gcc.dg/c2x-enum-7.c | 6 +--
gcc/testsuite/gcc.dg/c2x-tag-1.c | 68 +++
gcc/testsuite/gcc.dg/c2x-tag-10.c | 31 ++
gcc/testsuite/gcc.dg/c2x-tag-2.c | 43 +++
gcc/testsuite/gcc.dg/c2x-tag-3.c | 16
gcc/testsuite/gcc.dg/c2x-tag-4.c | 19 +
gcc/testsuite/gcc.dg/c2x-tag-5.c | 26
gcc/testsuite/gcc.dg/c2x-tag-6.c | 34
gcc/testsuite/gcc.dg/c2x-tag-7.c | 28 +
gcc/testsuite/gcc.dg/c2x-tag-8.c | 25
gcc/testsuite/gcc.dg/c2x-tag-9.c | 12 ++
14 files changed, 387 insertions(+), 16 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-1.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-10.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-2.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-3.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-4.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-5.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-6.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-7.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-8.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-tag-9.c
diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index 1f9eb44dbaa..c5c6a853fa9 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -1993,6 +1993,24 @@ locate_old_decl (tree decl)
decl, TREE_TYPE (decl));
}
+static tree
+previous_tag (tree type)
+{
+ struct c_binding *b = NULL;
+ tree name = TYPE_NAME (type);
+
+ if (name)
+b = I_TAG_BINDING (name);
+
+ if (b)
+b = b->shadowed;
+
+ if (b && B_IN_CURRENT_SCOPE (b))
+return b->decl;
+
+ return NULL_TREE;
+}
+
/* Subroutine of duplicate_decls. Compare NEWDECL to OLDDECL.
Returns true if the caller should proceed to merge the two, false
if OLDDECL should simply be discarded. As a side effect, issues
@@ -8442,11 +8460,14 @@ get_parm_info (bool ellipsis, tree expr)
if (TREE_CODE (decl) != UNION_TYPE || b->id != NULL_TREE)
{
if (b->id)
- /* The %s will be one of 'struct', 'union', or 'enum'. */
- warning_at (b->locus, 0,
- "%<%s %E%> declared inside parameter list"
- " will not be visible outside of this definition or"
- " declaration", keyword, b->id);
+ {
+ /* The %s will be one of 'struct', 'union', or 'enum'. */
+ if (!flag_isoc2x)
+ warning_at (b->locus, 0,
+ "%<%s %E%> declared inside parameter list"
+ " will not be visible outside of this
definition or"
+ " declaration", keyword, b->id);
+ }
else
/* The %s will be one of 'struct', 'union', or 'enum'. */
warning_at (b->locus, 0,
@@ -8651,6 +8672,12 @@ start_struct (location_t loc, enum tree_code code, tree
name,
if (name != NULL_TREE)
ref = lookup_tag (code, name, true, &refloc);
+
+ /* For C2X, even if we already have a completed definition,
+ we do not use it. We will check for consistency later. */
+ if (flag_isoc2x && ref && TYPE_SIZE (ref))
+ref = NULL_TREE;
+
if (ref && TREE_CODE (ref) == code)
{
if (TYPE_STUB_DECL (ref))
@@ -9439,6 +9466,25 @@ finish_struct (location_t loc, tree t, tree fieldlist,
tree attributes,
warning_at (loc, 0, "union cannot be made transparent");
}
+ /* Check for consistency with previous definition */
+ if (flag_isoc2x)
+{
+ tree vistype = previous_tag (t);
+ if (vistype
+ && TREE_CODE (vistype) == TREE_CODE (t)
+ && !C_TY
[C PATCH 2/6] c23: recursive type checking of tagged type
Adapt the old and unused code for type checking for C23.
gcc/c/:
* c-typeck.c (struct comptypes_data): Add anon_field flag.
(comptypes, comptypes_check_unum_int,
comptypes_check_different_types): Remove old cache.
(tagged_tu_types_compatible_p): Rewrite.
---
gcc/c/c-typeck.cc | 261 +++---
1 file changed, 58 insertions(+), 203 deletions(-)
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index ed1520ed6ba..41ef05f005c 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -190,20 +190,14 @@ remove_c_maybe_const_expr (tree expr)
return expr;
}
- /* This is a cache to hold if two types are compatible or not. */
+ /* This is a cache to hold if two types are seen. */
struct tagged_tu_seen_cache {
const struct tagged_tu_seen_cache * next;
const_tree t1;
const_tree t2;
- /* The return value of tagged_types_tu_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
*);
-
/* Do `exp = require_complete_type (loc, exp);' to make sure exp
does not have an incomplete type. (That includes void types.)
LOC is the location of the use. */
@@ -1043,10 +1037,12 @@ common_type (tree t1, tree t2)
}
struct comptypes_data {
-
bool enum_and_int_p;
bool different_types_p;
bool warning_needed;
+ bool anon_field;
+
+ const struct tagged_tu_seen_cache* cache;
};
/* Return 1 if TYPE1 and TYPE2 are compatible types for assignment
@@ -1056,13 +1052,9 @@ struct comptypes_data {
int
comptypes (tree type1, tree type2)
{
- const struct tagged_tu_seen_cache * tagged_tu_seen_base1 =
tagged_tu_seen_base;
-
struct comptypes_data data = { };
bool ret = comptypes_internal (type1, type2, &data);
- free_all_tagged_tu_seen_up_to (tagged_tu_seen_base1);
-
return ret ? (data.warning_needed ? 2 : 1) : 0;
}
@@ -1072,14 +1064,10 @@ 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;
-
struct comptypes_data data = { };
bool ret = comptypes_internal (type1, type2, &data);
*enum_and_int_p = data.enum_and_int_p;
- free_all_tagged_tu_seen_up_to (tagged_tu_seen_base1);
-
return ret ? (data.warning_needed ? 2 : 1) : 0;
}
@@ -1090,14 +1078,10 @@ 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;
-
struct comptypes_data data = { };
bool ret = comptypes_internal (type1, type2, &data);
*different_types_p = data.different_types_p;
- free_all_tagged_tu_seen_up_to (tagged_tu_seen_base1);
-
return ret ? (data.warning_needed ? 2 : 1) : 0;
}
@@ -1334,53 +1318,7 @@ comp_target_types (location_t location, tree ttl, tree
ttr)
/* Subroutines of `comptypes'. */
-
-
-/* Allocate the seen two types, assuming that they are compatible. */
-
-static struct tagged_tu_seen_cache *
-alloc_tagged_tu_seen_cache (const_tree t1, const_tree t2)
-{
- struct tagged_tu_seen_cache *tu = XNEW (struct tagged_tu_seen_cache);
- tu->next = tagged_tu_seen_base;
- tu->t1 = t1;
- tu->t2 = t2;
-
- tagged_tu_seen_base = tu;
-
- /* The C standard says that two structures in different translation
- units are compatible with each other only if the types of their
- fields are compatible (among other things). We assume that they
- are compatible until proven otherwise when building the cache.
- An example where this can occur is:
- struct a
- {
- struct a *next;
- };
- If we are comparing this against a similar struct in another TU,
- and did not assume they were compatible, we end up with an infinite
- loop. */
- tu->val = 1;
- return tu;
-}
-
-/* Free the seen types until we get to TU_TIL. */
-
-static void
-free_all_tagged_tu_seen_up_to (const struct tagged_tu_seen_cache *tu_til)
-{
- const struct tagged_tu_seen_cache *tu = tagged_tu_seen_base;
- while (tu != tu_til)
-{
- const struct tagged_tu_seen_cache *const tu1
- = (const struct tagged_tu_seen_cache *) tu;
- tu = tu1->next;
- XDELETE (CONST_CAST (struct tagged_tu_seen_cache *, tu1));
-}
- tagged_tu_seen_base = tu_til;
-}
-
-/* Return 1 if two 'struct', 'union', or 'enum' types T1 and T2 are
+/* Return true if two 'struct', 'union', or 'enum' types T1 and T2 are
compatible. If the two types are not the same (which has been
checked earlier). */
@@ -1406,189 +1344,106 @@ tagged_types_tu_compatible_p (const_tree t1,
const_tree t2,
&& DECL_ORIGINAL_TYPE (TYPE_NAME (t2)))
t2 = DECL_ORIGINAL_TYPE (TYPE_NAME (t2));
- /* C90 didn't have the requirement that the two tags be the sa
[C PATCH 1/6] c: reorganize recursive type checking
Reorganize recursive type checking to use a structure to
store information collected during the recursion and
returned to the caller (warning_needed, enum_and_init_p,
different_types_p).
gcc/c:
* c-typeck.cc (struct comptypes_data): Add structure.
(tagged_types_tu_compatible_p,
function_types_compatible_p, type_lists_compatible_p,
comptypes_internal): Add structure to interface, change
return type to bool, and adapt calls.
(comptarget_types): Change return type too bool.
(comptypes, comptypes_check_enum_int,
comptypes_check_different_types): Adapt calls.
---
gcc/c/c-typeck.cc | 266 --
1 file changed, 114 insertions(+), 152 deletions(-)
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index e6ddf37d412..ed1520ed6ba 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -90,12 +90,14 @@ static bool require_constant_elements;
static bool require_constexpr_value;
static tree qualify_type (tree, tree);
-static int tagged_types_tu_compatible_p (const_tree, const_tree, bool *,
-bool *);
-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 *);
+struct comptypes_data;
+static bool tagged_types_tu_compatible_p (const_tree, const_tree,
+ struct comptypes_data *);
+static bool comp_target_types (location_t, tree, tree);
+static bool function_types_compatible_p (const_tree, const_tree,
+struct comptypes_data *);
+static bool 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 +127,8 @@ 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 bool comptypes_internal (const_tree, const_tree,
+ struct comptypes_data *data);
/* Return true if EXP is a null pointer constant, false otherwise. */
@@ -1039,6 +1042,13 @@ common_type (tree t1, tree t2)
return c_common_type (t1, t2);
}
+struct comptypes_data {
+
+ bool enum_and_int_p;
+ bool different_types_p;
+ bool warning_needed;
+};
+
/* 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. */
@@ -1047,12 +1057,13 @@ 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);
+ struct comptypes_data data = { };
+ bool ret = comptypes_internal (type1, type2, &data);
+
free_all_tagged_tu_seen_up_to (tagged_tu_seen_base1);
- return val;
+ return ret ? (data.warning_needed ? 2 : 1) : 0;
}
/* Like comptypes, but if it returns non-zero because enum and int are
@@ -1062,12 +1073,14 @@ 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);
+ struct comptypes_data data = { };
+ bool ret = comptypes_internal (type1, type2, &data);
+ *enum_and_int_p = data.enum_and_int_p;
+
free_all_tagged_tu_seen_up_to (tagged_tu_seen_base1);
- return val;
+ return ret ? (data.warning_needed ? 2 : 1) : 0;
}
/* Like comptypes, but if it returns nonzero for different types, it
@@ -1078,40 +1091,40 @@ 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 = comptypes_internal (type1, type2, NULL, different_types_p);
+ struct comptypes_data data = { };
+ bool ret = comptypes_internal (type1, type2, &data);
+ *different_types_p = data.different_types_p;
+
free_all_tagged_tu_seen_up_to (tagged_tu_seen_base1);
- return val;
+ return ret ? (data.warning_needed ? 2 : 1) : 0;
}
-/* 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. If
- ENUM_AND_INT_P is not NULL, and one type is an enum and the other a
- compatible integer type, then this sets *ENUM_AND_INT_P to true;
- *ENUM_AN
c23 type compatibility rules, v2
This is a revised series for the C23 rules for type compatibility. 1/6 c: reorganize recursive type checking 2/6 c23: recursive type checking of tagged type 3/6 c23: tag compatibility rules for struct and unions 4/6 c23: tag compatibility rules for enums 5/6 c23: aliasing of compatible tagged types 6/6 c23: construct composite type for tagged types x/x c: flag for tag compatibility rules 1. simplifies type checking without functionality changes as a preparation step. (This is based on a similar preparatory patch I posted before for checking size expressions). 2. implements the new rules in comptypes for tagged types but the code still remains unused. This removes a lot of old code because we now require union members to have the same order and merges the code for structs and unions. 3. implements the rules for structs and unions. 4. does the same for enum types and enumerators. 5. sets TYPE_CANONICAL based on a equivalence class of types which makes aliasing work correctly. For this there is a new comptypes_equiv_p that does relaxed checking (ignoring size expressions in nested types but not for fields). 6. adds support for the composite type. There is an extra patch that adds the a flag to activate the compatibility rules independently from language mode and activates it by default. 1-2 should cause no change in function. 3-6 implement the new semantics for C23. Bootstrapped and regression tested on x86_64 (also with the extra patch). Martin
[committed] fix misleading identation breaking bootstrap
Committed as obvious. fix misleading identation breaking bootstrap Fix identation issue introduced by 966f3c13 "Fix format attribute for printf". gcc/c-family/ChangeLog: * c-format.cc: Fix identation. diff --git a/gcc/c-family/c-format.cc b/gcc/c-family/c-format.cc index 122ff9bd1cd..b3ef2d44ce9 100644 --- a/gcc/c-family/c-format.cc +++ b/gcc/c-family/c-format.cc @@ -1214,8 +1214,8 @@ check_function_format (const_tree fn, tree attrs, int nargs, skipped_default_format = true; break; } - if (skipped_default_format) - continue; + if (skipped_default_format) +continue; } if (warn_format)
c: Support for -Wuseless-cast [RR84510]
This patch adds the missing support for -Wuseless-cast
to the C FE as requested by some users. It found about
50 useless casts in one of my projects without false
positives.
(I also implemented a detection for various
unneeded pointer casts in convert_for_assignment
such as unneeded casts from / to void or casts
followed by an implicit conversion to the original
type, but I did not figure out how to reliably
identify casts there... But this would be a potential
future enhancement.)
Regression tested on bootstrapped on x86_64-pc-linux-gnu.
c: Support for -Wuseless-cast [RR84510]
Add support for Wuseless-cast C (and ObjC).
PR c/84510
gcc/c/:
* c-typeck.cc (build_c_cast): Add warning.
gcc/doc/:
* invoke.texi: Update.
gcc/testsuite/:
* Wuseless-cast.c: New test.
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 0ed87fcc7be..c7b567ba7ab 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -1490,7 +1490,7 @@ C++ ObjC++ Var(warn_zero_as_null_pointer_constant) Warning
Warn when a literal '0' is used as null pointer.
Wuseless-cast
-C++ ObjC++ Var(warn_useless_cast) Warning
+C ObjC C++ ObjC++ Var(warn_useless_cast) Warning
Warn about useless casts.
Wsubobject-linkage
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index 7cf411155c6..6f2fff51683 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -6062,9 +6062,13 @@ build_c_cast (location_t loc, tree type, tree expr)
if (type == TYPE_MAIN_VARIANT (TREE_TYPE (value)))
{
- if (RECORD_OR_UNION_TYPE_P (type))
- pedwarn (loc, OPT_Wpedantic,
-"ISO C forbids casting nonscalar to the same type");
+ if (RECORD_OR_UNION_TYPE_P (type)
+ && pedwarn (loc, OPT_Wpedantic,
+ "ISO C forbids casting nonscalar to the same type"))
+ ;
+ else if (warn_useless_cast)
+ warning_at (loc, OPT_Wuseless_cast,
+ "useless cast to type %qT", type);
/* Convert to remove any qualifiers from VALUE's type. */
value = convert (type, value);
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 674f956f4b8..75ca72f3190 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -4772,7 +4772,7 @@ pointers after reallocation.
@opindex Wuseless-cast
@opindex Wno-useless-cast
-@item -Wuseless-cast @r{(C++ and Objective-C++ only)}
+@item -Wuseless-cast @r{(C, Objective-C, C++ and Objective-C++ only)}
Warn when an expression is cast to its own type. This warning does not
occur when a class object is converted to a non-reference type as that
is a way to create a temporary:
diff --git a/gcc/testsuite/gcc.dg/Wuseless-cast.c
b/gcc/testsuite/gcc.dg/Wuseless-cast.c
new file mode 100644
index 000..86e87584b87
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wuseless-cast.c
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-options "-Wuseless-cast" } */
+
+void foo(void)
+{
+ // casts to the same type
+ int i = 0;
+ const int ic = 0;
+ struct foo { int x; } x = { 0 };
+ int q[3];
+ (int)ic;/* { dg-warning "useless cast" } */
+ (int)i; /* { dg-warning "useless cast" } */
+ (const int)ic; /* { dg-warning "useless cast" } */
+ (const int)i; /* { dg-warning "useless cast" } */
+ (struct foo)x; /* { dg-warning "useless cast" } */
+ (int(*)[3])&q; /* { dg-warning "useless cast" } */
+ (_Atomic(int))i;/* { dg-warning "useless cast" } */
+
+ // not the same
+ int n = 3;
+ (int(*)[n])&q; // no warning
+ int j = (int)0UL;
+ enum X { A = 1 } xx = { A };
+ enum Y { B = 1 } yy = (enum Y)xx;
+}
+
Re: [V2][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
I am sure this has been discussed before, but seeing that you
test for a specific formula, let me point out the following:
There at least three different size expression which could
make sense. Consider
short foo { int a; short b; char t[]; };
Most people seem to use
sizeof(struct foo) + N * sizeof(foo->t);
which for N == 3 yields 11 bytes on x86-64 because the formula
adds the padding of the original struct. There is an example
in the C standard that uses this formula.
But he minimum size of an object which stores N elements is
max(sizeof (struct s), offsetof(struct s, t[n]))
which is 9 bytes.
This is what clang uses for statically allocated objects with
initialization, while GCC uses the rule above (11 bytes). But
bdos / bos then returns the smaller size of 9 which is a bit
confusing.
https://godbolt.org/z/K1hvaK1ns
https://github.com/llvm/llvm-project/issues/62929
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109956
Then there is also the size of a similar array where the FAM
is replaced with an array of static size:
struct foo { int a; short b; char t[3]; };
This would make the most sense to me, but it has 12 bytes
because the padding is according to the usual alignment
rules.
Martin
Am Montag, dem 07.08.2023 um 09:16 -0700 schrieb Kees Cook:
> On Fri, Aug 04, 2023 at 07:44:28PM +, Qing Zhao wrote:
> > This is the 2nd version of the patch, per our discussion based on the
> > review comments for the 1st version, the major changes in this version
> > are:
>
> Thanks for the update!
>
> >
> > 1. change the name "element_count" to "counted_by";
> > 2. change the parameter for the attribute from a STRING to an
> > Identifier;
> > 3. Add logic and testing cases to handle anonymous structure/unions;
> > 4. Clarify documentation to permit the situation when the allocation
> > size is larger than what's specified by "counted_by", at the same time,
> > it's user's error if allocation size is smaller than what's specified by
> > "counted_by";
> > 5. Add a complete testing case for using counted_by attribute in
> > __builtin_dynamic_object_size when there is mismatch between the
> > allocation size and the value of "counted_by", the expecting behavior
> > for each case and the explanation on why in the comments.
>
> All the "normal" test cases I have are passing; this is wonderful! :)
>
> I'm still seeing unexpected situations when I've intentionally set
> counted_by to be smaller than alloc_size, but I assume it's due to not
> yet having the patch you mention below.
>
> > As discussed, I plan to add two more separate patch sets after this initial
> > patch set is approved and committed.
> >
> > set 1. A new warning option and a new sanitizer option for the user error
> > when the allocation size is smaller than the value of "counted_by".
> > set 2. An improvement to __builtin_dynamic_object_size for the following
> > case:
> >
> > struct A
> > {
> > size_t foo;
> > int array[] __attribute__((counted_by (foo)));
> > };
> >
> > extern struct fix * alloc_buf ();
> >
> > int main ()
> > {
> > struct fix *p = alloc_buf ();
> > __builtin_object_size(p->array, 0) == sizeof(struct A) + p->foo *
> > sizeof(int);
> > /* with the current algorithm, it’s UNKNOWN */
> > __builtin_object_size(p->array, 2) == sizeof(struct A) + p->foo *
> > sizeof(int);
> > /* with the current algorithm, it’s UNKNOWN */
> > }
>
> Should the above be bdos instead of bos?
>
> > Bootstrapped and regression tested on both aarch64 and X86, no issue.
>
> I've updated the Linux kernel's macros for the name change and done
> build tests with my first pass at "easy" cases for adding counted_by:
> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=devel/counted_by&id=adc5b3cb48a049563dc673f348eab7b6beba8a9b
>
> Everything is working as expected. :)
>
> -Kees
>
--
Univ.-Prof. Dr. rer. nat. Martin Uecker
Graz University of Technology
Institute of Biomedical Imaging
[C PATCH] Support typename as selector in _Generic
Clang now has an extension which accepts a typename for
_Generic. This is simple to implement and is useful.
Do we want this?
Clang calls it a "Clang extension" in the pedantic
warning. I changed it to "an extension" I am not
sure what the policy is.
Do we need an extra warning option? Clang has one.
No documentation so far.
Bootstrapped and regression tested on x86_64-pc-linux-gnu.
Martin
c: Support typename as selector in _Generic
Support typenames as first argument to _Generic which is an
extension supported by Clang. It makes it easier to test
for types with qualifiers in combination with typeof.
gcc/c/:
* c-parser.cc (c_parser_generic_selection): Support typename
in _Generic selector.
gcc/testsuite/:
* gnu2x-generic.c: New test.
diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
index 57a01dc2fa3..9aea2425294 100644
--- a/gcc/c/c-parser.cc
+++ b/gcc/c/c-parser.cc
@@ -9312,30 +9312,51 @@ c_parser_generic_selection (c_parser *parser)
if (!parens.require_open (parser))
return error_expr;
- c_inhibit_evaluation_warnings++;
selector_loc = c_parser_peek_token (parser)->location;
- selector = c_parser_expr_no_commas (parser, NULL);
- selector = default_function_array_conversion (selector_loc, selector);
- c_inhibit_evaluation_warnings--;
- if (selector.value == error_mark_node)
+ if (c_token_starts_typename (c_parser_peek_token (parser)))
{
- c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, NULL);
- return selector;
-}
- mark_exp_read (selector.value);
- selector_type = TREE_TYPE (selector.value);
- /* In ISO C terms, rvalues (including the controlling expression of
- _Generic) do not have qualified types. */
- if (TREE_CODE (selector_type) != ARRAY_TYPE)
-selector_type = TYPE_MAIN_VARIANT (selector_type);
- /* In ISO C terms, _Noreturn is not part of the type of expressions
- such as &abort, but in GCC it is represented internally as a type
- qualifier. */
- if (FUNCTION_POINTER_TYPE_P (selector_type)
- && TYPE_QUALS (TREE_TYPE (selector_type)) != TYPE_UNQUALIFIED)
-selector_type
- = build_pointer_type (TYPE_MAIN_VARIANT (TREE_TYPE (selector_type)));
+ /* Language extension introduced by Clang. */
+ pedwarn (selector_loc, OPT_Wpedantic, "passing a type argument as "
+ "first argument to %<_Generic%> is an extension");
+ struct c_type_name *type_name;
+ c_inhibit_evaluation_warnings++;
+ type_name = c_parser_type_name (parser);
+ c_inhibit_evaluation_warnings--;
+ if (NULL == type_name)
+ {
+ c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, NULL);
+ return error_expr;
+ }
+ /* Qualifiers are preserved. */
+ selector_type = groktypename (type_name, NULL, NULL);
+}
+ else
+{
+ c_inhibit_evaluation_warnings++;
+ selector = c_parser_expr_no_commas (parser, NULL);
+ selector = default_function_array_conversion (selector_loc, selector);
+ c_inhibit_evaluation_warnings--;
+
+ if (selector.value == error_mark_node)
+ {
+ c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, NULL);
+ return selector;
+ }
+ mark_exp_read (selector.value);
+ selector_type = TREE_TYPE (selector.value);
+ /* In ISO C terms, rvalues (including the controlling expression of
+_Generic) do not have qualified types. */
+ if (TREE_CODE (selector_type) != ARRAY_TYPE)
+ selector_type = TYPE_MAIN_VARIANT (selector_type);
+ /* In ISO C terms, _Noreturn is not part of the type of expressions
+such as &abort, but in GCC it is represented internally as a type
+qualifier. */
+ if (FUNCTION_POINTER_TYPE_P (selector_type)
+ && TYPE_QUALS (TREE_TYPE (selector_type)) != TYPE_UNQUALIFIED)
+ selector_type
+ = build_pointer_type (TYPE_MAIN_VARIANT (TREE_TYPE (selector_type)));
+}
if (!c_parser_require (parser, CPP_COMMA, "expected %<,%>"))
{
@@ -9401,7 +9422,7 @@ c_parser_generic_selection (c_parser *parser)
assoc.expression = c_parser_expr_no_commas (parser, NULL);
if (!match)
- c_inhibit_evaluation_warnings--;
+ c_inhibit_evaluation_warnings--;
if (assoc.expression.value == error_mark_node)
{
diff --git a/gcc/testsuite/gcc.dg/gnu2x-generic.c
b/gcc/testsuite/gcc.dg/gnu2x-generic.c
new file mode 100644
index 000..82b09578072
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/gnu2x-generic.c
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+
+_Static_assert(_Generic(const int, const int: 1, int: 0), "");
+_Static_assert(_Generic( int, const int: 0, int: 1), "");
+_Static_assert(_Generic(int[4], int[4]: 1), "");
+_Static_assert(_Generic(typeof(int[4]), int[4]: 1), "");
+
+void foo(int n)
+{
+ _Static_assert(_Generic(int[n++], int[4]: 1), "");
+}
+
+#pragma GCC diagnostic warning "-Wpedantic"
+_Static_a
[committed] c: Less warnings for parameters declared as arrays [PR98536]
I splitted up the patch into two parts and committed only
the FE parts which were already approved and the tests.
This solves one of the two issues.
Bootstrapped and regression tested on x86_64-pc-linux-gnu.
Less warnings for parameters declared as arrays [PR98536]
To avoid false positivies, tune the warnings for parameters declared
as arrays with size expressions. Do not warn when more bounds are
specified in the declaration than before.
PR c/98536
c-family/
* c-warn.cc (warn_parm_array_mismatch): Do not warn if more
bounds are specified.
gcc/testsuite:
* 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 d4d62c48b20..b7c5d7c01a2 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",
@@ -3643,14 +3633,10 @@ warn_parm_array_mismatch (location_t origloc, tree
fndecl, tree newparms)
}
/* 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);
diff --git a/gcc/testsuite/gcc.dg/Wvla-parameter-4.c
b/gcc/testsuite/gcc.dg/Wvla-parameter-4.c
index 599ad19a3e4..f35faea361a 100644
--- a/gcc/testsuite/gcc.dg/Wvla-parameter-4.c
+++ b/gcc/testsuite/gcc.dg/Wvla-parameter-4.c
@@ -12,11 +12,6 @@ typedef int IA3[3];
/* Verify the warning points to the declaration with more unspecified
bounds, guiding the user to specify them rather than making them all
unspecified. */
-void* f_pIA3ax (IA3 *x[*]); // { dg-warning "argument 1 of type
'int \\\(\\\*\\\[\\\*]\\\)\\\[3]' .aka '\[^\n\r\}\]+'. declared with 1
unspecified variable bound" }
-void* f_pIA3ax (IA3 *x[*]);
-void* f_pIA3ax (IA3 *x[n]); // { dg-message "subsequently declared
as 'int \\\(\\\*\\\[n]\\\)\\\[3]' with 0 unspecified variable bounds" "note" }
-void* f_pIA3ax (IA3 *x[n]) { return x; }
-
void* f_pIA3an (IA3 *x[n]); // { dg-message "previously declared
as 'int \\\(\\\*\\\[n]\\\)\\\[3]' with 0 unspecified variable bounds" "note" }
void* f_pIA3an (IA3 *x[n]);
diff --git a/gcc/testsuite/gcc.dg/attr-access-2.c
b/gcc/testsuite/gcc.dg/attr-access-2.c
index 76baddffc9f..616b7a9527c 100644
--- a/gcc/testsuite/gcc.dg/attr-access-2.c
+++ b/gcc/testsuite/gcc.dg/attr-access-2.c
@@ -60,16 +60,6 @@ RW (2, 1) void f10 (int n, char a[n]) // { dg-warning
"attribute 'access *\\\(
// { dg-warning "argument 2 of type
'char\\\[n]' declared as a variable length array" "" { target *-*-* } .-1 }
{ (void)&n; (void)&a; }
-
-/* The following is diagnosed to point out declarations with the T[*]
- form in headers where specifying the bound is just as important as
- in the definition (to detect bugs). */
- void f11 (int, char[*]); // { dg-warning "argument 2 of type
'char\\\[\\\*\\\]' declared with 1 unspecified variable bound" }
- void f11 (int m, char a[m]); // { dg-message "subsequently declared
as 'char\\\[m]' with 0 unspecified variable bounds" "note" }
-RW (2, 1) void f11 (int n, char arr[n]) // { dg-message "subsequently declared
as 'char\\\[n]' with 0 unspecifi
[C PATCH] _Generic should not warn in non-active branches [PR68193,PR97100]
Here is a patch to reduce false positives in _Generic.
Bootstrapped and regression tested on x86_64-linux.
Martin
c: _Generic should not warn in non-active branches [PR68193,PR97100]
To avoid false diagnostics, use c_inhibit_evaluation_warnings when
a generic association is known to match during parsing. We may still
generate false positives if the default branch comes earler than
a specific association that matches.
PR c/68193
PR c/97100
gcc/c/:
* c-parser.cc (c_parser_generic_selection): Inhibit evaluation
warnings branches that are known not be taken during parsing.
gcc/testsuite/ChangeLog:
* gcc.dg/pr68193.c: New test.
diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
index 24a6eb6e459..d1863b301e0 100644
--- a/gcc/c/c-parser.cc
+++ b/gcc/c/c-parser.cc
@@ -9350,7 +9350,17 @@ c_parser_generic_selection (c_parser *parser)
return error_expr;
}
+ bool match = assoc.type == NULL_TREE
+ || comptypes (assoc.type, selector_type);
+
+ if (!match)
+ c_inhibit_evaluation_warnings++;
+
assoc.expression = c_parser_expr_no_commas (parser, NULL);
+
+ if (!match)
+ c_inhibit_evaluation_warnings--;
+
if (assoc.expression.value == error_mark_node)
{
c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, NULL);
@@ -9387,7 +9397,7 @@ c_parser_generic_selection (c_parser *parser)
match_found = associations.length ();
}
}
- else if (comptypes (assoc.type, selector_type))
+ else if (match)
{
if (match_found < 0 || matched_assoc.type == NULL_TREE)
{
diff --git a/gcc/testsuite/gcc.dg/pr68193.c b/gcc/testsuite/gcc.dg/pr68193.c
new file mode 100644
index 000..2267593e363
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr68193.c
@@ -0,0 +1,15 @@
+/* pr69193 */
+/* { dg-do compile } */
+/* { dg-options "-Wall" } */
+
+int
+main (void)
+{
+ int i = 0;
+ int j = _Generic (i,
+ int: 0,
+ long int: (i = (long int) 9223372036854775808UL));
+ return i + j;
+}
+
+
Re: [C PATCH]: Add Walloc-type to warn about insufficient size in allocations
Am Mittwoch, dem 02.08.2023 um 16:45 + schrieb Qing Zhao:
>
> > On Aug 1, 2023, at 10:31 AM, Martin Uecker wrote:
> >
> > Am Dienstag, dem 01.08.2023 um 13:27 + schrieb Qing Zhao:
> > >
> > > > On Aug 1, 2023, at 3:51 AM, Martin Uecker via Gcc-patches
> > > > wrote:
> > > >
> >
> >
> > > > > Hi Martin,
> > > > > Just wondering if it'd be a good idea perhaps to warn if alloc size is
> > > > > not a multiple of TYPE_SIZE_UNIT instead of just less-than ?
> > > > > So it can catch cases like:
> > > > > int *p = malloc (sizeof (int) + 2); // probably intended malloc
> > > > > (sizeof (int) * 2)
> > > > >
> > > > > FWIW, this is caught using -fanalyzer:
> > > > > f.c: In function 'f':
> > > > > f.c:3:12: warning: allocated buffer size is not a multiple of the
> > > > > pointee's size [CWE-131] [-Wanalyzer-allocation-size]
> > > > >3 | int *p = __builtin_malloc (sizeof(int) + 2);
> > > > > |^~
> > > > >
> > > > > Thanks,
> > > > > Prathamesh
> > > >
> > > > Yes, this is probably a good idea. It might need special
> > > > logic for flexible array members then...
> > >
> > > Why special logic for FAM on such warning? (Not a multiple of
> > > TYPE_SIZE_UNIT for the element).
> > >
> >
> > For
> >
> > struct { int n; char buf[]; } *p = malloc(sizeof *p + n);
> > p->n = n;
> >
> > the size would not be a multiple.
>
> But n is still a multiple of sizeof (char), right? Do I miss anything here?
Right, for a struct with FAM we could check that it is
sizeof () plus a multiple of the element size of the FAM.
Still special logic...
Martin
> Qing
> >
> > Martin
> >
> >
> >
> >
>
--
Univ.-Prof. Dr. rer. nat. Martin Uecker
Graz University of Technology
Institute of Biomedical Imaging
Re: [V1][PATCH 0/3] New attribute "element_count" to annotate bounds for C99 FAM(PR108896)
Am Dienstag, dem 01.08.2023 um 15:45 -0700 schrieb Kees Cook:
> On Mon, Jul 31, 2023 at 08:14:42PM +, Qing Zhao wrote:
> > /* In general, Due to type casting, the type for the pointee of a pointer
> >does not say anything about the object it points to,
> >So, __builtin_object_size can not directly use the type of the pointee
> >to decide the size of the object the pointer points to.
> >
> >there are only two reliable ways:
> >A. observed allocations (call to the allocation functions in the
> > routine)
> >B. observed accesses (read or write access to the location of the
> > pointer points to)
> >
> >that provide information about the type/existence of an object at
> >the corresponding address.
> >
> >for A, we use the "alloc_size" attribute for the corresponding allocation
> >functions to determine the object size;
> >
> >For B, we use the SIZE info of the TYPE attached to the corresponding
> > access.
> >(We treat counted_by attribute as a complement to the SIZE info of the
> > TYPE
> > for FMA)
> >
> >The only other way in C which ensures that a pointer actually points
> >to an object of the correct type is 'static':
> >
> >void foo(struct P *p[static 1]);
> >
> >See https://gcc.gnu.org/pipermail/gcc-patches/2023-July/624814.html
> >for more details. */
>
> This is a great explanation; thank you!
>
> In the future I might want to have a new builtin that will allow
> a program to query a pointer when neither A nor B have happened. But
> for the first version of the __counted_by infrastructure, the above
> limitations seen fine.
>
> For example, maybe __builtin_counted_size(p) (which returns sizeof(*p) +
> sizeof(*p->flex_array_member) * p->counted_by_member). Though since
> there might be multiple flex array members, maybe this can't work. :)
We had a _Lengthof proposal for arrays (instead of sizeof/sizeof)
and thought about how to extend this to structs with FAM. The
problem is that it can not rely on an attribute.
With GCC's VLA in structs you could do
struct foo { int n; char buf[n_init]; } *p = malloc(sizeof *p);
p->n_init = n;
and get sizeof and bounds checking with UBSan
https://godbolt.org/z/d4nneqs3P
(but also compiler bugs and other issues)
Also see my experimental container library, where you can do:
vec_decl(int);
vec(int)* v = vec_alloc(int);
vec_push(&v, 1);
vec_push(&v, 3);
auto p = &vec_array(v);
(*p)[1] = 1; // bounds check
Here, "vec_array()" would give you a regular C array view
of the vector contant and with correct dynamic size, so you
can apply "sizeof" and have bounds checking with UBSan and
it just works (with clang / GCC without changes).
https://github.com/uecker/noplate
Martin
Re: [C PATCH]: Add Walloc-type to warn about insufficient size in allocations
Am Dienstag, dem 01.08.2023 um 13:27 + schrieb Qing Zhao:
>
> > On Aug 1, 2023, at 3:51 AM, Martin Uecker via Gcc-patches
> > wrote:
> >
> > > Hi Martin,
> > > Just wondering if it'd be a good idea perhaps to warn if alloc size is
> > > not a multiple of TYPE_SIZE_UNIT instead of just less-than ?
> > > So it can catch cases like:
> > > int *p = malloc (sizeof (int) + 2); // probably intended malloc
> > > (sizeof (int) * 2)
> > >
> > > FWIW, this is caught using -fanalyzer:
> > > f.c: In function 'f':
> > > f.c:3:12: warning: allocated buffer size is not a multiple of the
> > > pointee's size [CWE-131] [-Wanalyzer-allocation-size]
> > > 3 | int *p = __builtin_malloc (sizeof(int) + 2);
> > > |^~
> > >
> > > Thanks,
> > > Prathamesh
> >
> > Yes, this is probably a good idea. It might need special
> > logic for flexible array members then...
>
> Why special logic for FAM on such warning? (Not a multiple of TYPE_SIZE_UNIT
> for the element).
>
For
struct { int n; char buf[]; } *p = malloc(sizeof *p + n);
p->n = n;
the size would not be a multiple.
Martin
Re: [C PATCH]: Add Walloc-type to warn about insufficient size in allocations
Am Dienstag, dem 01.08.2023 um 02:11 +0530 schrieb Prathamesh Kulkarni:
> On Fri, 21 Jul 2023 at 16:52, Martin Uecker via Gcc-patches
> wrote:
> >
> >
> >
> > This patch adds a warning for allocations with insufficient size
> > based on the "alloc_size" attribute and the type of the pointer
> > the result is assigned to. While it is theoretically legal to
> > assign to the wrong pointer type and cast it to the right type
> > later, this almost always indicates an error. Since this catches
> > common mistakes and is simple to diagnose, it is suggested to
> > add this warning.
> >
> >
> > Bootstrapped and regression tested on x86.
> >
> >
> > Martin
> >
> >
> >
> > Add option Walloc-type that warns about allocations that have
> > insufficient storage for the target type of the pointer the
> > storage is assigned to.
> >
> > gcc:
> > * doc/invoke.texi: Document -Wstrict-flex-arrays option.
> >
> > gcc/c-family:
> >
> > * c.opt (Walloc-type): New option.
> >
> > gcc/c:
> > * c-typeck.cc (convert_for_assignment): Add Walloc-type warning.
> >
> > gcc/testsuite:
> >
> > * gcc.dg/Walloc-type-1.c: New test.
> >
> >
> > diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
> > index 4abdc8d0e77..8b9d148582b 100644
> > --- a/gcc/c-family/c.opt
> > +++ b/gcc/c-family/c.opt
> > @@ -319,6 +319,10 @@ Walloca
> > C ObjC C++ ObjC++ Var(warn_alloca) Warning
> > Warn on any use of alloca.
> >
> > +Walloc-type
> > +C ObjC Var(warn_alloc_type) Warning
> > +Warn when allocating insufficient storage for the target type of the
> > assigned pointer.
> > +
> > Walloc-size-larger-than=
> > C ObjC C++ LTO ObjC++ Var(warn_alloc_size_limit) Joined Host_Wide_Int
> > ByteSize Warning Init(HOST_WIDE_INT_MAX)
> > -Walloc-size-larger-than= Warn for calls to allocation
> > functions that
> > diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
> > index 7cf411155c6..2e392f9c952 100644
> > --- a/gcc/c/c-typeck.cc
> > +++ b/gcc/c/c-typeck.cc
> > @@ -7343,6 +7343,32 @@ convert_for_assignment (location_t location,
> > location_t expr_loc, tree type,
> > "request for implicit conversion "
> > "from %qT to %qT not permitted in C++", rhstype,
> > type);
> >
> > + /* Warn of new allocations are not big enough for the target
> > type. */
> > + tree fndecl;
> > + if (warn_alloc_type
> > + && TREE_CODE (rhs) == CALL_EXPR
> > + && (fndecl = get_callee_fndecl (rhs)) != NULL_TREE
> > + && DECL_IS_MALLOC (fndecl))
> > + {
> > + tree fntype = TREE_TYPE (fndecl);
> > + tree fntypeattrs = TYPE_ATTRIBUTES (fntype);
> > + tree alloc_size = lookup_attribute ("alloc_size",
> > fntypeattrs);
> > + if (alloc_size)
> > + {
> > + tree args = TREE_VALUE (alloc_size);
> > + int idx = TREE_INT_CST_LOW (TREE_VALUE (args)) - 1;
> > + /* For calloc only use the second argument. */
> > + if (TREE_CHAIN (args))
> > + idx = TREE_INT_CST_LOW (TREE_VALUE (TREE_CHAIN
> > (args))) - 1;
> > + tree arg = CALL_EXPR_ARG (rhs, idx);
> > + if (TREE_CODE (arg) == INTEGER_CST
> > + && tree_int_cst_lt (arg, TYPE_SIZE_UNIT (ttl)))
> Hi Martin,
> Just wondering if it'd be a good idea perhaps to warn if alloc size is
> not a multiple of TYPE_SIZE_UNIT instead of just less-than ?
> So it can catch cases like:
> int *p = malloc (sizeof (int) + 2); // probably intended malloc
> (sizeof (int) * 2)
>
> FWIW, this is caught using -fanalyzer:
> f.c: In function 'f':
> f.c:3:12: warning: allocated buffer size is not a multiple of the
> pointee's size [CWE-131] [-Wanalyzer-allocation-size]
> 3 | int *p = __builtin_malloc (sizeof(int) + 2);
> |^~
>
> Thanks,
> Prathamesh
Yes, this is probably a good idea. It might need special
logic for flexible array members then...
Martin
> > +warning_at (location, OPT_Walloc_type, "allocation of
> > "
> > +"insufficient size %qE for type %qT with
> > "
> > +"size %qE", arg, ttl, TYPE_SIZE_UNIT
> > (ttl));
> &g
Re: [C PATCH]: Add Walloc-type to warn about insufficient size in allocations
Am Montag, dem 31.07.2023 um 15:39 -0400 schrieb Siddhesh Poyarekar: > On 2023-07-21 07:21, Martin Uecker via Gcc-patches wrote: > > > > > > This patch adds a warning for allocations with insufficient size > > based on the "alloc_size" attribute and the type of the pointer > > the result is assigned to. While it is theoretically legal to > > assign to the wrong pointer type and cast it to the right type > > later, this almost always indicates an error. Since this catches > > common mistakes and is simple to diagnose, it is suggested to > > add this warning. > > ... > > > > Wouldn't this be much more useful in later phases with ranger feedback > like with the warn_access warnings? That way the comparison won't be > limited to constant sizes. Possibly. Having it in the FE made it simple to implement and also reliable. One thing I considered is also looking deeper into the argument and detect obvious mistakes, e.g. if the type in a sizeof is the right one. Such extensions would be easier in the FE. But I wouldn't mind replacing or extending this with something smarter emitted from later phases. I probably do not have time to work on this is myself in the near future though. Martin
[PING 3] [PATCH] Less warnings for parameters declared as arrays [PR98541, PR98536]
Joseph, I would appreciate if you could take a look at this?
This fixes the remaining issues which requires me to turn the
warnings off with -Wno-vla-parameter and -Wno-nonnull in my
projects.
Am Montag, dem 03.04.2023 um 21:34 +0200 schrieb Martin Uecker:
>
> 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 dia
[C PATCH]: Add Walloc-type to warn about insufficient size in allocations
This patch adds a warning for allocations with insufficient size
based on the "alloc_size" attribute and the type of the pointer
the result is assigned to. While it is theoretically legal to
assign to the wrong pointer type and cast it to the right type
later, this almost always indicates an error. Since this catches
common mistakes and is simple to diagnose, it is suggested to
add this warning.
Bootstrapped and regression tested on x86.
Martin
Add option Walloc-type that warns about allocations that have
insufficient storage for the target type of the pointer the
storage is assigned to.
gcc:
* doc/invoke.texi: Document -Wstrict-flex-arrays option.
gcc/c-family:
* c.opt (Walloc-type): New option.
gcc/c:
* c-typeck.cc (convert_for_assignment): Add Walloc-type warning.
gcc/testsuite:
* gcc.dg/Walloc-type-1.c: New test.
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 4abdc8d0e77..8b9d148582b 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -319,6 +319,10 @@ Walloca
C ObjC C++ ObjC++ Var(warn_alloca) Warning
Warn on any use of alloca.
+Walloc-type
+C ObjC Var(warn_alloc_type) Warning
+Warn when allocating insufficient storage for the target type of the
assigned pointer.
+
Walloc-size-larger-than=
C ObjC C++ LTO ObjC++ Var(warn_alloc_size_limit) Joined Host_Wide_Int
ByteSize Warning Init(HOST_WIDE_INT_MAX)
-Walloc-size-larger-than= Warn for calls to allocation
functions that
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index 7cf411155c6..2e392f9c952 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -7343,6 +7343,32 @@ convert_for_assignment (location_t location,
location_t expr_loc, tree type,
"request for implicit conversion "
"from %qT to %qT not permitted in C++", rhstype,
type);
+ /* Warn of new allocations are not big enough for the target
type. */
+ tree fndecl;
+ if (warn_alloc_type
+ && TREE_CODE (rhs) == CALL_EXPR
+ && (fndecl = get_callee_fndecl (rhs)) != NULL_TREE
+ && DECL_IS_MALLOC (fndecl))
+ {
+ tree fntype = TREE_TYPE (fndecl);
+ tree fntypeattrs = TYPE_ATTRIBUTES (fntype);
+ tree alloc_size = lookup_attribute ("alloc_size",
fntypeattrs);
+ if (alloc_size)
+ {
+ tree args = TREE_VALUE (alloc_size);
+ int idx = TREE_INT_CST_LOW (TREE_VALUE (args)) - 1;
+ /* For calloc only use the second argument. */
+ if (TREE_CHAIN (args))
+ idx = TREE_INT_CST_LOW (TREE_VALUE (TREE_CHAIN
(args))) - 1;
+ tree arg = CALL_EXPR_ARG (rhs, idx);
+ if (TREE_CODE (arg) == INTEGER_CST
+ && tree_int_cst_lt (arg, TYPE_SIZE_UNIT (ttl)))
+warning_at (location, OPT_Walloc_type, "allocation of
"
+"insufficient size %qE for type %qT with
"
+"size %qE", arg, ttl, TYPE_SIZE_UNIT
(ttl));
+ }
+ }
+
/* See if the pointers point to incompatible address spaces. */
asl = TYPE_ADDR_SPACE (ttl);
asr = TYPE_ADDR_SPACE (ttr);
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 88e3c625030..6869bed64c3 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -8076,6 +8076,15 @@ always leads to a call to another @code{cold}
function such as wrappers of
C++ @code{throw} or fatal error reporting functions leading to
@code{abort}.
@end table
+@opindex Wno-alloc-type
+@opindex Walloc-type
+@item -Walloc-type
+Warn about calls to allocation functions decorated with attribute
+@code{alloc_size} that specify insufficient size for the target type
of
+the pointer the result is assigned to, including those to the built-in
+forms of the functions @code{aligned_alloc}, @code{alloca},
@code{calloc},
+@code{malloc}, and @code{realloc}.
+
@opindex Wno-alloc-zero
@opindex Walloc-zero
@item -Walloc-zero
diff --git a/gcc/testsuite/gcc.dg/Walloc-type-1.c
b/gcc/testsuite/gcc.dg/Walloc-type-1.c
new file mode 100644
index 000..bc62e5e9aa3
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloc-type-1.c
@@ -0,0 +1,37 @@
+/* Tests the warnings for insufficient allocation size.
+ { dg-do compile }
+ * { dg-options "-Walloc-type" }
+ * */
+#include
+#include
+
+struct b { int x[10]; };
+
+void fo0(void)
+{
+struct b *p = malloc(sizeof *p);
+}
+
+void fo1(void)
+{
+struct b *p = malloc(sizeof p);/* { dg-
warning "allocation of insufficient size" } */
+}
+
+void fo2(void)
+{
+struct b *p = alloca(sizeof p);/* { dg-
warning "allocation of insufficient size" } */
+}
+
+void fo3(void)
+{
+struct b *p = calloc(1, sizeof p); /* { dg-warning
"allocation of insufficient size" } */
+}
+
+void g(struct b* p);
+
+void fo4(void)
+{
+g(malloc(4)); /* { dg-warning "allocation of
insufficient size" } */
+}
+
+
[PING 2] [PATCH] Less warnings for parameters declared as arrays [PR98541, PR98536]
Ok for gcc-14 now? 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. > jeff
Re: [PATCH] core: Support heap-based trampolines
Am Mittwoch, dem 19.07.2023 um 15:23 +0100 schrieb Iain Sandoe: > Hi Martin, > > > On 19 Jul 2023, at 11:43, Martin Uecker via Gcc-patches > > wrote: > > > > Am Mittwoch, dem 19.07.2023 um 10:29 +0100 schrieb Iain Sandoe: > > > > > On 19 Jul 2023, at 10:04, Martin Uecker > > > > wrote: > > > > > > > > > On 17 Jul 2023, > > > > > > > > > > > > > > > > You mention setjmp/longjmp - on darwin and other platforms > > > > > requiring > > > > > > > non-stack based trampolines > > > > > > > does the system runtime provide means to deal with this issue > > > > > > > like > > > > > an > > > > > > > alternate allocation method > > > > > > > or a way to register cleanup? > > > > > > > > > > > > There is an alternate mechanism relying on system libraries > > > > > > that is > > > > > possible on darwin specifically (I don’t know for other targets) > > > > > but > > > > > it will only work for signed binaries, and would require us to > > > > > codesign everything produced by gcc. During development, it was > > > > > deemed too big an ask and the current strategy was chosen (Iain > > > > > can > > > > > surely add more background on that if needed). > > > > > > > > > > I do not think that this solves the setjump/longjump issue - > > > > > since > > > > > there’s still a notional allocation that takes place (it’s just > > > > > that > > > > > the mechanism for determining permissions is different). > > > > > > > > > > It is also a big barrier for the general user - and prevents > > > > > normal > > > > > folks from distributing GCC - since codesigning requires an > > > > > external > > > > > certificate (i.e. I would really rather avoid it). > > > > > > > > > > > > Was there ever an attempt to provide a "generic" trampoline > > > > > > > driven > > > > > by > > > > > > > a more complex descriptor? > > > > > > > My own opinion is that executable stack should go away on all > > > > > targets at some point, so a truly generic solution to the problem > > > > > would be great. > > > > > > > > > > indeed it would. > > > > > > > I think we need a solution rather sooner than later on all archs. > > > > > > AFAICS the heap-based trampolines can work for any arch**, this > > > issue is about > > > system security policy, rather than arch, specifically? > > > > > > It seems to me that for any system security policy that permits JIT, > > > (but not > > > executable stack) the heap-based trampolines are viable. > > > > I agree. > > > > BTW; One option we discussed before, was to map a page with > > pre-allocated trampolines, which look up the address of > > a callee and the static chain in a table based on its own > > address. Then no code generation is involved. > > That reads similar to the scheme Apple have implemented for libobjc and > libffi. > In order to be extensible (i.e to allow the table to grow at runtime), it > means > having some loadable executable object; if that is implemented in a way shared > between users (delivered as part of the implementation) then, for Darwin at > least, it must be codesigned - which is somewhere I really want to avoid going > with GCC. > > > The difficult part is avoiding leaks with longjmp / setjmp. > > One idea was to have a shadow stack consisting of the > > pre-allocated trampolines, but this probably causes other > > issues... > > With a per-thread table, I *think* for most targets, we discussed in the team > maintaining a ’tide mark’ of the stack as part of the saved data in the > trampoline (not used as part of the execution, but only as part of the > allocation > mangement)… but .. > > > I wonder how difficult it is to have longjmp / setjmp walk > > the stack in C? This would also be useful for C++ > > interoperability and to free heap-allocated VLAs. > > … this would be a better solution (as we can see trampolines are a small > leak c.f. the general uses)? > > > As a user of nested functions, from my side it would also > &g
Re: [PATCH] core: Support heap-based trampolines
Am Mittwoch, dem 19.07.2023 um 10:29 +0100 schrieb Iain Sandoe: > Hi Martin, > > > On 19 Jul 2023, at 10:04, Martin Uecker > > wrote: > > > > > On 17 Jul 2023, > > > > > > > > > > You mention setjmp/longjmp - on darwin and other platforms > > > requiring > > > > > non-stack based trampolines > > > > > does the system runtime provide means to deal with this issue > > > > > like > > > an > > > > > alternate allocation method > > > > > or a way to register cleanup? > > > > > > > > There is an alternate mechanism relying on system libraries > > > > that is > > > possible on darwin specifically (I don’t know for other targets) > > > but > > > it will only work for signed binaries, and would require us to > > > codesign everything produced by gcc. During development, it was > > > deemed too big an ask and the current strategy was chosen (Iain > > > can > > > surely add more background on that if needed). > > > > > > I do not think that this solves the setjump/longjump issue - > > > since > > > there’s still a notional allocation that takes place (it’s just > > > that > > > the mechanism for determining permissions is different). > > > > > > It is also a big barrier for the general user - and prevents > > > normal > > > folks from distributing GCC - since codesigning requires an > > > external > > > certificate (i.e. I would really rather avoid it). > > > > > > > > Was there ever an attempt to provide a "generic" trampoline > > > > > driven > > > by > > > > > a more complex descriptor? > > > > > > We did look at the “unused address bits” mechanism that Ada has > > > used > > > - but that is not really available to a non-private ABI (unless > > > the > > > system vendor agrees to change ABI to leave a bit spare) for the > > > base > > > arch either the bits are not there (e.g. X86) or reserved (e.g. > > > AArch64). > > > > > > Andrew Burgess did the original work he might have comments on > > > alternatives we tried > > > > > > > For reference, I proposed a patch for this in 2018. It was not > > accepted because minimum alignment for functions would increase > > for some archs: > > > > https://gcc.gnu.org/legacy-ml/gcc-patches/2018-12/msg01532.html > > Right - that was the one we originally looked at and has the issue > that it > breaks ABI - and thus would need vendor by-in to alter as you say. > > > > > > (well, it could be a bytecode interpreter and the trampoline > > > > > being > > > > > bytecode on the stack?!) > > > > > > > > My own opinion is that executable stack should go away on all > > > targets at some point, so a truly generic solution to the problem > > > would be great. > > > > > > indeed it would. > > > I think we need a solution rather sooner than later on all archs. > > AFAICS the heap-based trampolines can work for any arch**, this > issue is about > system security policy, rather than arch, specifically? > > It seems to me that for any system security policy that permits JIT, > (but not > executable stack) the heap-based trampolines are viable. I agree. BTW; One option we discussed before, was to map a page with pre-allocated trampolines, which look up the address of a callee and the static chain in a table based on its own address. Then no code generation is involved. The difficult part is avoiding leaks with longjmp / setjmp. One idea was to have a shadow stack consisting of the pre-allocated trampolines, but this probably causes other issues... I wonder how difficult it is to have longjmp / setjmp walk the stack in C? This would also be useful for C++ interoperability and to free heap-allocated VLAs. As a user of nested functions, from my side it would also ok to simply add a wide function pointer type that contains address + static chain. This would require changing code, but would also work with Clang's blocks and solve other language interoperability problems, while avoiding all existing ABI issues. > > This seems to be a useful step forward; and we can add some other > mechanism to the flag’s supported list if someone develops one? I think it is a useful step forward. Martin > > Iain > > ** modulo the target maintainers implementing the builtins. > > >
Re: [PATCH] core: Support heap-based trampolines
> > > On 17 Jul 2023, > > >> You mention setjmp/longjmp - on darwin and other platforms > requiring > >> non-stack based trampolines > >> does the system runtime provide means to deal with this issue like > an > >> alternate allocation method > >> or a way to register cleanup? > > > > There is an alternate mechanism relying on system libraries that is > possible on darwin specifically (I don’t know for other targets) but > it will only work for signed binaries, and would require us to > codesign everything produced by gcc. During development, it was > deemed too big an ask and the current strategy was chosen (Iain can > surely add more background on that if needed). > > I do not think that this solves the setjump/longjump issue - since > there’s still a notional allocation that takes place (it’s just that > the mechanism for determining permissions is different). > > It is also a big barrier for the general user - and prevents normal > folks from distributing GCC - since codesigning requires an external > certificate (i.e. I would really rather avoid it). > > >> Was there ever an attempt to provide a "generic" trampoline driven > by > >> a more complex descriptor? > > We did look at the “unused address bits” mechanism that Ada has used > - but that is not really available to a non-private ABI (unless the > system vendor agrees to change ABI to leave a bit spare) for the base > arch either the bits are not there (e.g. X86) or reserved (e.g. > AArch64). > > Andrew Burgess did the original work he might have comments on > alternatives we tried > For reference, I proposed a patch for this in 2018. It was not accepted because minimum alignment for functions would increase for some archs: https://gcc.gnu.org/legacy-ml/gcc-patches/2018-12/msg01532.html > >> (well, it could be a bytecode interpreter and the trampoline being > >> bytecode on the stack?!) > > > > My own opinion is that executable stack should go away on all > targets at some point, so a truly generic solution to the problem > would be great. > > indeed it would. > I think we need a solution rather sooner than later on all archs. Martin > > Having something that works reliably across all targets, like you > suggest, is a much bigger project that this patch, and I am not aware > of any previous attempt at it. > > The bytecode interpreter idea is neat; (a) I wonder about > performance and (b) it is, as FX says, a bigger project - certainly > bigger than the voluntary Darwin time available :( > > Iain > > > > > > >> Otherwise I suggest to split the patch into libgcc, generic and > target parts. > > > > >
Re: [V1][PATCH 0/3] New attribute "element_count" to annotate bounds for C99 FAM(PR108896)
Am Montag, dem 17.07.2023 um 16:40 -0700 schrieb Kees Cook: > On Mon, Jul 17, 2023 at 09:17:48PM +, Qing Zhao wrote: > > > > > On Jul 13, 2023, at 4:31 PM, Kees Cook > > > wrote: > > > > > > In the bug, the problem is that "p" isn't known to be allocated, > > > if I'm > > > reading that correctly? > > > > I think that the major point in PR109557 > > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109557): > > > > for the following pointer p.3_1, > > > > p.3_1 = p; > > _2 = __builtin_object_size (p.3_1, 0); > > > > Question: why the size of p.3_1 cannot use the TYPE_SIZE of the > > pointee of p when the TYPE_SIZE can be determined at compile time? > > > > Answer: From just knowing the type of the pointee of p, the > > compiler cannot determine the size of the object. > > Why is that? "p" points to "struct P", which has a fixed size. There > must be an assumption somewhere that a pointer is allocated, > otherwise > __bos would almost never work? It often does not work, because it relies on the optimizer propagating the information instead of the type system. This is why it would be better to have proper *bounds* checks, and not just object size checks. It is not quite clear to me how BOS and bounds checking is supposed to work together, but FAMs should be bounds checked. ... > > > > > > This may be > > > desirable in a few situations. One example would be a large > > > allocation > > > that is slowly filled up by the program. > > > > So, for such situation, whenever the allocation is filled up, the > > field that hold the “counted_by” attribute should be increased at > > the same time, > > Then, the “counted_by” value always sync with the real allocation. > > > I.e. the counted_by member is > > > slowly increased during runtime (but not beyond the true > > > allocation size). > > > > Then there should be source code to increase the “counted_by” field > > whenever the allocated space increased too. > > > > > > Of course allocation size is only available in limited > > > situations, so > > > the loss of that info is fine: we have counted_by for everything > > > else. > > > > The point is: allocation size should synced with the value of > > “counted_by”. LLVM’s RFC also have the similar requirement: > > https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854#maintaining-correctness-of-bounds-annotations-18 > > Right, I'm saying it would be nice if __alloc_size was checked as > well, > in the sense that if it is available, it knows without question what > the > size of the allocation is. If __alloc_size and __counted_by conflict, > the smaller of the two should be the truth. > > But, as I said, if there is some need to explicitly ignore > __alloc_size > when __counted_by is present, I can live with it; we just need to > document it. > > If the RFC and you agree that the __counted_by variable can only ever > be > (re)assigned after the flex array has been (re)allocated, then I > guess > we'll see how it goes. :) I think most places in the kernel using > __counted_by will be fine, but I suspect we may have cases where we > need > to update it like in the loop I described above. If that's true, we > can > revisit the requirement then. :) It should be the other way round: You should first set 'count' and then reassign the pointer, because you can then often check the pointer assignment (reading 'count'). The other way round this works only sometimes, i.e. if both assignments are close together and the optimizer can see this. Martin
Re: [V1][PATCH 0/3] New attribute "element_count" to annotate bounds for C99 FAM(PR108896)
Am Dienstag, dem 18.07.2023 um 16:25 + schrieb Qing Zhao:
>
>
> > On Jul 18, 2023, at 12:03 PM, Martin Uecker
> > wrote:
> >
> > Am Dienstag, dem 18.07.2023 um 15:37 + schrieb Qing Zhao:
> > >
> > >
> > > > On Jul 17, 2023, at 7:40 PM, Kees Cook
> > > > wrote:
> > > >
> > > > On Mon, Jul 17, 2023 at 09:17:48PM +, Qing Zhao wrote:
> > > > >
> > > > > > On Jul 13, 2023, at 4:31 PM, Kees Cook
> > > > > >
> > > > > > wrote:
> > > > > >
> > > > > > In the bug, the problem is that "p" isn't known to be
> > > > > > allocated, if I'm
> > > > > > reading that correctly?
> > > > >
> > > > > I think that the major point in PR109557
> > > > > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109557):
> > > > >
> > > > > for the following pointer p.3_1,
> > > > >
> > > > > p.3_1 = p;
> > > > > _2 = __builtin_object_size (p.3_1, 0);
> > > > >
> > > > > Question: why the size of p.3_1 cannot use the TYPE_SIZE of
> > > > > the
> > > > > pointee of p when the TYPE_SIZE can be determined at compile
> > > > > time?
> > > > >
> > > > > Answer: From just knowing the type of the pointee of p, the
> > > > > compiler cannot determine the size of the object.
> > > >
> > > > Why is that? "p" points to "struct P", which has a fixed size.
> > > > There
> > > > must be an assumption somewhere that a pointer is allocated,
> > > > otherwise
> > > > __bos would almost never work?
> > >
> > > My understanding from the comments in PR109557 was:
> > >
> > > In general the pointer could point to the first object of an
> > > array
> > > that has more elements, or to an object of a different type.
> > > Without seeing the real allocation to the pointer, the compiler
> > > cannot assume that the pointer point to an object that has
> > > the exact same type as its declaration.
> > >
> > > Sid and Martin, is the above understand correctly?
> >
> > Yes.
> >
> > In the example, it *could* work because the compiler
> > could inline 'store' or otherwise use its knowledge about
> > the function definition to conclude that 'p' points to
> > an object of size 'sizeof (struct P)'. But this is fragile
> > because it relies on optimization and will not work across
> > TUs.
> >
> > > Honestly, I am still not 100% clear on this yet.
> >
> > > Jakub, Sid and Martin, could you please explain a little bit
> > > more
> > > here, especially for the following small example?
> > >
> > > [opc@qinzhao-ol8u3-x86 109557]$ cat t.c
> > > #include
> > > #include
> > > struct P {
> > > int k;
> > > int x[10];
> > > } *p;
> > >
> > > void store(int a, int b)
> > > {
> > > p = (struct P *)malloc (sizeof (struct P));
> > > p->k = a;
> > > p->x[b] = 0;
> > > assert (__builtin_dynamic_object_size (p->x, 1) == sizeof
> > > (int[10]));
> > > assert (__builtin_dynamic_object_size (p, 1) == sizeof (struct
> > > P));
> > > return;
> > > }
> > >
> > > int main()
> > > {
> > > store(7, 7);
> > > assert (__builtin_dynamic_object_size (p->x, 1) == sizeof
> > > (int[10]));
> > > assert (__builtin_dynamic_object_size (p, 1) == sizeof (struct
> > > P));
> > > free (p);
> > > }
> > > [opc@qinzhao-ol8u3-x86 109557]$ sh t
> > > /home/opc/Install/latest/bin/gcc -O -fdump-tree-all t.c
> > > a.out: t.c:21: main: Assertion `__builtin_dynamic_object_size (p-
> > > >x,
> > > 1) == sizeof (int[10])' failed.
> > > t: line 19: 859944 Aborted (core dumped) ./a.out
> > >
> > >
> > > In the above, among the 4 assertions, only the last one failed.
> >
> > I don't know why this is the case.
> >
> > >
> > > Why GCC can use the TYPE_SIZE of the pointee of the pointer “p-
> > > >x” as
> > > the size of the object,
> >
> > I do not think it can do this in general. Is this how it
> > is implemented?
>
> No. -:)
>
> I guess that the implementation of this should base on your
> following case, “observed accesses”:
> Although I am not 100% sure on the definition of “observed accesses”.
>
> p->x is an access to the field of the object, so compiler can assume
> that the object exist and have
> the type associate with this access?
>
Only if the access happens at run-time, but the argument to BDOS is
not evaluated, so there is no access. At least, this is my guess based
on C's semantics.
> On the other hand, “p” is just a plain pointer, no observed access.
>
> > Thus would then seem incorrect to me.
> >
> > > but cannot use the TYPE_SIZE of the pointee of the pointer “p” as
> > > the
> > > size of the object?
> >
> > In general, the type of a pointer does not say anything about the
> > object it points to, because you could cast the pointer to a
> > different
> > type, pass it around, and then cast it back before use.
>
> Okay, I see.
> >
> > Only observed allocations or observed accesses provide information
> > about the type / existence of an object at the corresponding
> > address.
>
> What will be included in “observed accesses”?
Any read or write access can be used to determine that there must
be
Re: [V1][PATCH 0/3] New attribute "element_count" to annotate bounds for C99 FAM(PR108896)
Am Dienstag, dem 18.07.2023 um 15:37 + schrieb Qing Zhao:
>
>
> > On Jul 17, 2023, at 7:40 PM, Kees Cook
> > wrote:
> >
> > On Mon, Jul 17, 2023 at 09:17:48PM +, Qing Zhao wrote:
> > >
> > > > On Jul 13, 2023, at 4:31 PM, Kees Cook
> > > > wrote:
> > > >
> > > > In the bug, the problem is that "p" isn't known to be
> > > > allocated, if I'm
> > > > reading that correctly?
> > >
> > > I think that the major point in PR109557
> > > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109557):
> > >
> > > for the following pointer p.3_1,
> > >
> > > p.3_1 = p;
> > > _2 = __builtin_object_size (p.3_1, 0);
> > >
> > > Question: why the size of p.3_1 cannot use the TYPE_SIZE of the
> > > pointee of p when the TYPE_SIZE can be determined at compile
> > > time?
> > >
> > > Answer: From just knowing the type of the pointee of p, the
> > > compiler cannot determine the size of the object.
> >
> > Why is that? "p" points to "struct P", which has a fixed size.
> > There
> > must be an assumption somewhere that a pointer is allocated,
> > otherwise
> > __bos would almost never work?
>
> My understanding from the comments in PR109557 was:
>
> In general the pointer could point to the first object of an array
> that has more elements, or to an object of a different type.
> Without seeing the real allocation to the pointer, the compiler
> cannot assume that the pointer point to an object that has
> the exact same type as its declaration.
>
> Sid and Martin, is the above understand correctly?
Yes.
In the example, it *could* work because the compiler
could inline 'store' or otherwise use its knowledge about
the function definition to conclude that 'p' points to
an object of size 'sizeof (struct P)'. But this is fragile
because it relies on optimization and will not work across
TUs.
> Honestly, I am still not 100% clear on this yet.
> Jakub, Sid and Martin, could you please explain a little bit more
> here, especially for the following small example?
>
> [opc@qinzhao-ol8u3-x86 109557]$ cat t.c
> #include
> #include
> struct P {
> int k;
> int x[10];
> } *p;
>
> void store(int a, int b)
> {
> p = (struct P *)malloc (sizeof (struct P));
> p->k = a;
> p->x[b] = 0;
> assert (__builtin_dynamic_object_size (p->x, 1) == sizeof
> (int[10]));
> assert (__builtin_dynamic_object_size (p, 1) == sizeof (struct P));
> return;
> }
>
> int main()
> {
> store(7, 7);
> assert (__builtin_dynamic_object_size (p->x, 1) == sizeof
> (int[10]));
> assert (__builtin_dynamic_object_size (p, 1) == sizeof (struct P));
> free (p);
> }
> [opc@qinzhao-ol8u3-x86 109557]$ sh t
> /home/opc/Install/latest/bin/gcc -O -fdump-tree-all t.c
> a.out: t.c:21: main: Assertion `__builtin_dynamic_object_size (p->x,
> 1) == sizeof (int[10])' failed.
> t: line 19: 859944 Aborted (core dumped) ./a.out
>
>
> In the above, among the 4 assertions, only the last one failed.
I don't know why this is the case.
>
> Why GCC can use the TYPE_SIZE of the pointee of the pointer “p->x” as
> the size of the object,
I do not think it can do this in general. Is this how it
is implemented? Thus would then seem incorrect to me.
> but cannot use the TYPE_SIZE of the pointee of the pointer “p” as the
> size of the object?
In general, the type of a pointer does not say anything about the
object it points to, because you could cast the pointer to a different
type, pass it around, and then cast it back before use.
Only observed allocations or observed accesses provide information
about the type / existence of an object at the corresponding address.
The only other way in C which ensures that a pointer actually points
to an object of the correct type is 'static':
void foo(struct P *p[static 1]);
Martin
>
> >
> > > Therefore the bug has been closed.
> > >
> > > In your following testing 5:
> > >
> > > > I'm not sure this is a reasonable behavior, but
> > > > let me get into the specific test, which looks like this:
> > > >
> > > > TEST(counted_by_seen_by_bdos)
> > > > {
> > > > struct annotated *p;
> > > > int index = MAX_INDEX + unconst;
> > > >
> > > > p = alloc_annotated(index);
> > > >
> > > > REPORT_SIZE(p->array);
> > > > /* 1 */ EXPECT_EQ(sizeof(*p), offsetof(typeof(*p), array));
> > > > /* Check array size alone. */
> > > > /* 2 */ EXPECT_EQ(__builtin_object_size(p->array, 1),
> > > > SIZE_MAX);
> > > > /* 3 */ EXPECT_EQ(__builtin_dynamic_object_size(p->array, 1),
> > > > p->foo * sizeof(*p->array));
> > > > /* Check check entire object size. */
> > > > /* 4 */ EXPECT_EQ(__builtin_object_size(p, 1), SIZE_MAX);
> > > > /* 5 */ EXPECT_EQ(__builtin_dynamic_object_size(p, 1),
> > > > sizeof(*p) + p->foo * sizeof(*p->array));
> > > > }
> > > >
> > > > Test 5 should pass as well, since, again, p can be examined.
> > > > Passing p
> > > > to __bdos implies it is allocated and the __counted_by
> > > > annotation can be
> > > > examined.
> > >
> > > Sinc
Re: [V1][PATCH 0/3] New attribute "element_count" to annotate bounds for C99 FAM(PR108896)
Am Donnerstag, dem 06.07.2023 um 18:56 + schrieb Qing Zhao:
> Hi, Kees,
>
> I have updated my V1 patch with the following changes:
> A. changed the name to "counted_by"
> B. changed the argument from a string to an identifier
> C. updated the documentation and testing cases accordingly.
>
> And then used this new gcc to test
> https://github.com/kees/kernel-tools/blob/trunk/fortify/array-bounds.c (with
> the following change)
> [opc@qinzhao-ol8u3-x86 Kees]$ !1091
> diff array-bounds.c array-bounds.c.org
> 32c32
> < # define __counted_by(member) __attribute__((counted_by (member)))
> ---
> > # define __counted_by(member)
> > __attribute__((__element_count__(#member)))
> 34c34
> < # define __counted_by(member) __attribute__((counted_by (member)))
> ---
> > # define __counted_by(member) /*
> > __attribute__((__element_count__(#member))) */
>
> Then I got the following result:
> [opc@qinzhao-ol8u3-x86 Kees]$ ./array-bounds 2>&1 | grep -v ^'#'
> TAP version 13
> 1..12
> ok 1 global.fixed_size_seen_by_bdos
> ok 2 global.fixed_size_enforced_by_sanitizer
> not ok 3 global.unknown_size_unknown_to_bdos
> not ok 4 global.unknown_size_ignored_by_sanitizer
> ok 5 global.alloc_size_seen_by_bdos
> ok 6 global.alloc_size_enforced_by_sanitizer
> not ok 7 global.element_count_seen_by_bdos
> ok 8 global.element_count_enforced_by_sanitizer
> not ok 9 global.alloc_size_with_smaller_element_count_seen_by_bdos
> not ok 10 global.alloc_size_with_smaller_element_count_enforced_by_sanitizer
> ok 11 global.alloc_size_with_bigger_element_count_seen_by_bdos
> ok 12 global.alloc_size_with_bigger_element_count_enforced_by_sanitizer
>
> The same as your previous results. Then I took a look at all the failed
> testing: 3, 4, 7, 9, and 10. And studied the reasons for all of them.
>
> in a summary, there are two major issues:
> 1. The reason for the failed testing 7 is the same issue as I observed in
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109557
> Which is not a bug, it’s an expected behavior.
>
> 2. The common issue for the failed testing 3, 4, 9, 10 is:
>
> for the following annotated structure:
>
>
> struct annotated {
> unsigned long flags;
> size_t foo;
> int array[] __attribute__((counted_by (foo)));
> };
>
>
> struct annotated *p;
> int index = 16;
>
> p = malloc(sizeof(*p) + index * sizeof(*p->array)); // allocated real size
>
> p->foo = index + 2; // p->foo was set by a different value than the real
> size of p->array as in test 9 and 10
> or
> p->foo was not set to any value as in test 3 and 4
>
>
>
> i.e, the value of p->foo is NOT synced with the number of elements allocated
> for the array p->array.
>
> I think that this should be considered as an user error, and the
> documentation of the attribute should include
> this requirement. (In the LLVM’s RFC, such requirement was included in the
> programing model:
> https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854#maintaining-correctness-of-bounds-annotations-18)
>
> We can add a new warning option -Wcounted-by to report such user error if
> needed.
>
> What’s your opinion on this?
Additionally, we could also have a sanitizer that
checks this at run-time.
Personally, I am still not very happy that in the
following example the two 'n's refer to different
entities:
void f(int n)
{
struct foo {
int n;
int (*p[])[n] [[counted_by(n)]];
};
}
But I guess it will be difficult to convince everybody
that it would be wise to use a new syntax for
disambiguation:
void f(int n)
{
struct foo {
int n;
int (*p[])[n] [[counted_by(.n)]];
};
}
Martin
>
> thanks.
>
> Qing
>
>
> > On May 26, 2023, at 4:40 PM, Kees Cook wrote:
> >
> > On Thu, May 25, 2023 at 04:14:47PM +, Qing Zhao wrote:
> > > GCC will pass the number of elements info from the attached attribute to
> > > both
> > > __builtin_dynamic_object_size and bounds sanitizer to check the
> > > out-of-bounds
> > > or dynamic object size issues during runtime for flexible array members.
> > >
> > > This new feature will provide nice protection to flexible array members
> > > (which
> > > currently are completely ignored by both __builtin_dynamic_object_size and
> > > bounds sanitizers).
> >
> > Testing went pretty well, though I think I found some bdos issues:
> >
> > - some things that bdos can't know the size of, and correctly returned
> > SIZE_MAX in the past, now thinks are 0-sized.
> > - while bdos correctly knows the size of an element_count-annotated
> > flexible array, it doesn't know the size of the containing object
> > (i.e. it returns SIZE_MAX).
> >
> > Also, I think I found a precedence issue:
> >
> > - if both __alloc_size and 'element_count' are in use, the _smallest_
> > of the two is what I would expect to be enforced by the sanitizer
> > and reported by __bdos. As is, alloc_size appears to be used when
> >
Re: [V1][PATCH 1/3] Provide element_count attribute to flexible array member field (PR108896)
Am Freitag, dem 16.06.2023 um 16:21 + schrieb Joseph Myers:
> On Fri, 16 Jun 2023, Martin Uecker via Gcc-patches wrote:
>
> > > Note that no expressions can start with the '.' token at present. As
> > > soon
> > > as you invent a new kind of expression that can start with that token,
> > > you
> > > have syntactic ambiguity.
> > >
> > > struct s1 { int c; char a[(struct s2 { int c; char b[.c]; }) {.c=.c}.c];
> > > };
> > >
> > > Is ".c=.c" a use of the existing syntax for designated initializers, with
> > > the first ".c" being a designator and the second being a use of the new
> > > kind of expression, or is it an assignment expression, where both the LHS
> > > and the RHS of the assignment use the new kind of expression? And do
> > > those .c, when the use the new kind of expression, refer to the inner or
> > > outer struct definition?
> >
> > I would treat this is one integrated feature. Essentially .c is
> > somthing like this->c for the current struct for designated
> > initializer *and* size expressions because it is semantically
> > so close.In the initializer I would allow only
> > the current use for designated initialization for all names of
> > member of the currently initialized struct, so .c = .c would
> > be invalid. It should never refer to the outer struct if there
>
> I'm not clear on what the intended disambiguation rule here is, when "."
> is seen in initializer list context - does this rule depend on whether the
> following identifier is a member of the struct being initialized, so
> ".c=.c" would be OK above if the initialized struct didn't have a member
> called c but the outer struct definition did?
When initializers are parsed it is already clear what
the names of the members of the inner struct are, so
one can differentiate between designated initializers
and potential other uses in an expression.
So the main rule is: if you parse .something in a context
where a designator is allowed and "something" is a member
of the current struct, then it is a designator.
So for
struct foo { int c; int buf[(struct { int d; }){ .d = .c }]; };
one knows during parsing that the .d is a designator
and that .c is not. For
struct foo { int c; int buf[(struct { int d; }){ .c = .c }]; };
one knows that both uses of .c are not.
Whether these different use cases should be allowed or not
is a different question, but my point is that there does
not seem to be a problem directly identifying the uses
as a designator as usual. To me, this seems to imply that
it is safe to use the same syntax.
> That seems like a rather
> messy rule. And does "would allow only" apply other than in the ambiguous
> context? That seems to be implied by ".c=.c" being invalid above, because
> to make it invalid you need to disallow the new construct being used for
> the second .c, not just make the first .c interpreted as a designator.
Yes.
>
> Again, this sort of thing needs a detailed written specification, with
> multiple iterations discussed among different implementations.
Oh, I agree with this.
> The above
> paragraph doesn't make clear to me any of: the disambiguation rules; what
> is allowed in what context; how name lookup works (consider tricky cases
> such as a reference to an identifier declared *later* in the same struct,
> possibly in the context of C2x tag compatibility where a previous
> definition of the struct is visible); when these expressions get
> evaluated; what the underlying principles are behind those choices.
I also agree that all this needs careful consideration and written
rules. My point is mereley that there does not seem to be a
fundamental issue differentiating the new feature from
designators during parsing, so there may not be a risk using
the same syntax.
> Using a token (existing or new) other than '.' - one that doesn't
> introduce ambiguity in any context where expressions can be used - would
> help significantly, although some of the issues would still apply.
The cost of using a new symbol is that one has two different
syntax for something which is semantically equivalent, i.e.
a notion to refer to a member of the current struct.
Martin
>
Re: [V1][PATCH 1/3] Provide element_count attribute to flexible array member field (PR108896)
Am Donnerstag, dem 15.06.2023 um 16:55 + schrieb Joseph Myers:
> On Thu, 15 Jun 2023, Qing Zhao via Gcc-patches wrote:
>
...
> > 1. Update the routine “c_parser_postfix_expression” (is this the right
> > place? ) to accept the new designator syntax.
>
> Any design that might work with an expression is the sort of thing that
> would likely involve many iterations on the specification (i.e. proposed
> wording changes to the C standard) for the interpretation of the new kinds
> of expressions, including how to resolve syntactic ambiguities and how
> name lookup works, before it could be considered ready to implement, and
> then a lot more work on the specification based on implementation
> experience.
>
> Note that no expressions can start with the '.' token at present. As soon
> as you invent a new kind of expression that can start with that token, you
> have syntactic ambiguity.
>
> struct s1 { int c; char a[(struct s2 { int c; char b[.c]; }) {.c=.c}.c]; };
>
> Is ".c=.c" a use of the existing syntax for designated initializers, with
> the first ".c" being a designator and the second being a use of the new
> kind of expression, or is it an assignment expression, where both the LHS
> and the RHS of the assignment use the new kind of expression? And do
> those .c, when the use the new kind of expression, refer to the inner or
> outer struct definition?
I would treat this is one integrated feature. Essentially .c is
somthing like this->c for the current struct for designated
initializer *and* size expressions because it is semantically
so close.In the initializer I would allow only
the current use for designated initialization for all names of
member of the currently initialized struct, so .c = .c would
be invalid. It should never refer to the outer struct if there
is a member with the same name in the inner struct, i.e. the
outside member is then hidden.
So this would be ok:
struct s1 { int d; char a[(struct s2 { int c; char b[.c]; }) {.c=.d}.c]; };
Here the use of .d would be ok because it is not from the struct
currently initialized, but from an outside scope.
Martin
Re: [RFC] Add stdckdint.h header for C23
Hi Jakup,
two comments which may or may not be helpful:
Clang extended _Generic in a similar way:
https://github.com/llvm/llvm-project/commit/12728e144994efe84715f4e5dbb8c3104e9f0b5a
Although for _Generic you can achieve the same with checking
for compatiblilty of pointer to the type, and I do not think
this helps with the classification problem.
If I am not missing something, you should be able to check
for an enumerated type using _Generic by checking that the
type is not compatible to another enum type:
enum type_check { _X = 1 };
#define type_is_enum(x) \
_Generic(x, unsigned int: _Generic(x, enum type_check: 0, default: 1),
default: 0)
https://godbolt.org/z/j6z4a4Mdn
For C23 with fixed underlying type this may become more
complicated. Maybe this becomes to messy.
Martin
Re: [C PATCH 3/4] introduce ubsan checking for assigment of VM types 3/4
Am Dienstag, dem 30.05.2023 um 22:59 + schrieb Joseph Myers: > On Mon, 29 May 2023, Martin Uecker via Gcc-patches wrote: > > > Support instrumentation of function arguments for functions > > called via a declaration. We can support only simple size > > What do you mean by "via a declaration"? > > If the *definition* is visible (and known to be the definition used > at runtime rather than being interposed) then you can determine in > some cases that there is UB from bad bounds. If only some other > declaration is visible, or the definition might be interposed, VLA > sizes in the declaration are equivalent to [*]; it's suspicious if > they don't match, but it's not UB and so it would seem rather > questionable for UBSan to treat it as such (cf. the rejection in > GCC of sanitization for some questionable cases of unsigned integer > overflow that aren't UB either). You are right that it is UB only with the additional assumption that the bounds in the seen declaration are the same as the ones in the definition. But we now warn about any mismatch since GCC 11 with -Wall based on the understanding that any such mismatch should be considered a bug. There also does not seem to be any valid use case for having mismatching bounds and I think the intention of WG14 is clearly that they can be used for checking (cf. WG14 charter). So I think this is a different situation for unsigned integer overflow. Fom a practial point of view is is certainly very useful for users to be able to verify these bounds at run-time. But we could make it a separate UBSan option if it is really a concern. BTW: There was a similar discussion years ago about making certain bound checks for arrays part of UBSan because it is not clear that the bounds in the type of 'x' in x[n] are relevant rather than the ones of the underlying array (which may be different). In the end both GCC and clang have these UBSan checks now and I think everybody is happy about it. > > + /* Give up. If we do not understand a size expression, > > we can > > + also not instrument any of the others because it may > > have > > + side effects affecting them. (We could restart and > > instrument > > + the only the ones with integer constants.) */ > > + warning_at (location, 0, "Function call not > > instrumented."); > > + return void_node; > > This is not a properly formatted diagnostic message (should start > with a > lowercase letter and not end with '.'). Thanks. I would probably remove this warning and re-introduce it with another patch that also adds an option fir it. Martin
Re: [C PATCH 3/4] introduce ubsan checking for assigment of VM types 3/4
c: introduce ubsan checking for assigment of VM types 3/4
Support instrumentation of function arguments for functions
called via a declaration. We can support only simple size
expressions without side effects, because the UBSan
instrumentation is done before the call, but the expressions
are evaluated in the callee.
gcc/c-family:
* c-ubsan.cc (ubsan_instrument_vm_assign): Add arguments
for size expressions.
* c-ubsan.h (ubsan_instrument_vm_assign): Dito.
gcc/c:
* c-typeck.cc (process_vm_constraints): Add support
for instrumenting function arguments.
gcc/testsuide/gcc.dg:
* ubsan/vm-bounds-2.c: Update.
* ubsan/vm-bounds-3.c: New test.
* ubsan/vm-bounds-4.c: New test.
diff --git a/gcc/c-family/c-ubsan.cc b/gcc/c-family/c-ubsan.cc
index 59ef9708188..a8f95aa39e8 100644
--- a/gcc/c-family/c-ubsan.cc
+++ b/gcc/c-family/c-ubsan.cc
@@ -337,19 +337,13 @@ ubsan_instrument_vla (location_t loc, tree size)
/* Instrument assignment of variably modified types. */
tree
-ubsan_instrument_vm_assign (location_t loc, tree a, tree b)
+ubsan_instrument_vm_assign (location_t loc, tree a, tree as, tree b, tree bs)
{
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);
diff --git a/gcc/c-family/c-ubsan.h b/gcc/c-family/c-ubsan.h
index 1b07b0645f2..42be1d691a8 100644
--- a/gcc/c-family/c-ubsan.h
+++ b/gcc/c-family/c-ubsan.h
@@ -26,7 +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 tree ubsan_instrument_vm_assign (location_t, tree, tree, 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 a8fccc6f6ed..aeddac315fc 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -3408,7 +3408,8 @@ static tree
convert_argument (location_t ploc, tree function, tree fundecl,
tree type, tree origtype, tree val, tree valtype,
bool npc, tree rname, int parmnum, int argnum,
- bool excess_precision, int warnopt)
+ bool excess_precision, int warnopt,
+ vec *instr_vec)
{
/* Formal parm type is specified by a function prototype. */
@@ -3567,7 +3568,7 @@ convert_argument (location_t ploc, tree function, tree
fundecl,
val, origtype, ic_argpass,
npc, fundecl, function,
parmnum + 1, warnopt,
- NULL);
+ instr_vec);
if (targetm.calls.promote_prototypes (fundecl ? TREE_TYPE (fundecl) : 0)
&& INTEGRAL_TYPE_P (type)
@@ -3582,15 +3583,111 @@ convert_argument (location_t ploc, tree function, tree
fundecl,
static tree
process_vm_constraints (location_t location,
- vec *instr_vec)
+ vec *instr_vec,
+ tree function, tree fundecl, vec *values)
{
unsigned int i;
struct instrument_data* d;
tree instr_expr = void_node;
+ tree args = NULL;
+
+ /* Find the arguments for the function declaration / type. */
+ if (function)
+{
+ if (FUNCTION_DECL == TREE_CODE (function))
+ {
+ fundecl = function;
+ args = DECL_ARGUMENTS (fundecl);
+ }
+ else
+ {
+ /* Functions called via pointers are not yet supported. */
+ return void_node;
+ }
+}
FOR_EACH_VEC_SAFE_ELT (instr_vec, i, d)
{
- tree in = ubsan_instrument_vm_assign (location, d->t1, d->t2);
+ tree t1 = d->t1;
+ tree t2 = d->t2;
+
+ gcc_assert (ARRAY_TYPE == TREE_CODE (t1));
+ gcc_assert (ARRAY_TYPE == TREE_CODE (t2));
+
+ tree as = TYPE_MAX_VALUE (TYPE_DOMAIN (t1));
+ tree bs = TYPE_MAX_VALUE (TYPE_DOMAIN (t2));
+
+ gcc_assert (as);
+ gcc_assert (bs);
+
+ as = fold_build2 (PLUS_EXPR, sizetype, as, size_one_node);
+ bs = fold_build2 (PLUS_EXPR, sizetype, bs, size_one_node);
+
+
Re: [C PATCH 2/4] introduce ubsan checking for assigment of VM types 2/4
c: introduce ubsan checking for assigment of VM types 2/4
When checking compatibility of types during assignment, collect
all pairs of types where the outermost bound needs to match at
run-time. This list is then processed to add UBSan checks for
each bound.
gcc/c-family:
* c-ubsan.cc (ubsan_instrument_vm_assign): New function.
* c-ubsan.h (ubsan_instrument_vm_assign: New function.
gcc/c:
* c-typeck.cc (struct instrument_data). New structure.
(comp_target_types_instr convert_for_assignment_instrument): New
interfaces for existing functions.
(struct comptypes_data): Add instrumentation.
(comptypes_check_enum_int_intr): New interface.
(comptypes_check_enum_int): Old interface (calls new).
(comptypes_internal): Collect VLA types needed for UBSan.
(comp_target_types_instr): New interface.
(comp_target_types): Old interface (calls new).
(function_types_compatible_p): No instrumentation for function
arguments.
(process_vm_constraints): New function.
(convert_for_assignment_instrument): New interface.
(convert_for_assignment): Instrument assignments.
* sanitizer.def: Add sanitizer builtins.
gcc/testsuite:
* gcc.dg/ubsan/vm-bounds-1.c: New test.
* gcc.dg/ubsan/vm-bounds-1b.c: New test.
* gcc.dg/ubsan/vm-bounds-2.c: New test.
libsanitizer/ubsan:
* ubsan_checks.inc: Add UBSan check.
* ubsan_handlers.cpp (handleVMBoundsMismatch): New function.
* ubsan_handlers.h (struct VMBoundsMismatchData): New structure.
(vm_bounds_mismatch): New handler.
diff --git a/gcc/c-family/c-ubsan.cc b/gcc/c-family/c-ubsan.cc
index 51aa83a378d..59ef9708188 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, &loc,
+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 fef1033e1e4..1b07b0645f2 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 2a1b7321b45..a8fccc6f6ed 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -94,6 +94,9 @@ struct comptypes_data;
static int tagged_types_tu_compatible_p (const_tree, const_tree,
struct comptypes_data *);
static int comp_target_types (location_t, tree, tree);
+struct instrument_data;
+static int comp_target_types_instr (location_t, tree, tree,
+ vec *);
static int function_types_compatible_p (const_tree, const_tree,
struct comptypes_data *);
Re: [C PATCH 4/4] introduce ubsan checking for assigment of VM types 4/4
c: introduce ubsan checking for assigment of VM types 4/4
Support instrumentation of functions called via pointers. To do so,
record the declaration with the parameter types, so that it can be
retrieved later.
gcc/c:
c-decl.cc (get_parm_info): Record function declaration
for arguments.
c-type.cc (process_vm_constraints): Instrument functions
called via pointers.
gcc/testsuide/gcc.dg:
* ubsan/vm-bounds-2.c: Add warning.
* ubsan/vm-bounds-5.c: New test.
diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index 1af51c4acfc..c33adf7e5fe 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -8410,6 +8410,9 @@ get_parm_info (bool ellipsis, tree expr)
declared types. The back end may override this later. */
DECL_ARG_TYPE (decl) = type;
types = tree_cons (0, type, types);
+
+ /* Record the decl for use of UBSan bounds checking. */
+ TREE_PURPOSE (types) = decl;
}
break;
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index aeddac315fc..43e7b96a55f 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -3601,9 +3601,20 @@ process_vm_constraints (location_t location,
}
else
{
- /* Functions called via pointers are not yet supported. */
- return void_node;
+ while (FUNCTION_TYPE != TREE_CODE (function))
+ function = TREE_TYPE (function);
+
+ args = TREE_PURPOSE (TYPE_ARG_TYPES (function));
+
+ if (!args)
+ {
+ /* FIXME: this can happen when forming composite types for the
+conditional operator. */
+ warning_at (location, 0, "Function call not instrumented.");
+ return void_node;
+ }
}
+ gcc_assert (PARM_DECL == TREE_CODE (args));
}
FOR_EACH_VEC_SAFE_ELT (instr_vec, i, d)
diff --git a/gcc/testsuite/gcc.dg/ubsan/vm-bounds-2.c
b/gcc/testsuite/gcc.dg/ubsan/vm-bounds-2.c
index 22f06231eaa..093cbddd2ea 100644
--- a/gcc/testsuite/gcc.dg/ubsan/vm-bounds-2.c
+++ b/gcc/testsuite/gcc.dg/ubsan/vm-bounds-2.c
@@ -31,7 +31,7 @@ void f(void)
int u = 3; int v = 4;
char a[u][v];
- (1 ? f1 : f2)(u, v, a);
+ (1 ? f1 : f2)(u, v, a); /* { dg-warning "Function call not
instrumented." } */
}
/* size expression in parameter */
diff --git a/gcc/testsuite/gcc.dg/ubsan/vm-bounds-5.c
b/gcc/testsuite/gcc.dg/ubsan/vm-bounds-5.c
new file mode 100644
index 000..1a251e39deb
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ubsan/vm-bounds-5.c
@@ -0,0 +1,72 @@
+/* { dg-do run } */
+/* { dg-options "-fsanitize=vla-bound" } */
+
+
+void foo1(void (*p)(int n, char (*a)[n]))
+{
+ char A0[3];
+ (*p)(3, &A0);
+ (*p)(4, &A0); /* */
+ /* { dg-output "bound 4 of type 'char \\\[\\\*\\\]' does not match
bound 3 of type 'char \\\[3\\\]'\[^\n\r]*(\n|\r\n|\r)" } */
+}
+
+void b0(int n, char (*a)[n]) { }
+
+
+int n;
+
+void foo2(void (*p)(int n, char (*a)[n]))
+{
+ n = 4;
+ char A0[3];
+ (*p)(3, &A0);
+ (*p)(4, &A0);
+ /* { dg-output "\[^\n\r]*bound 4 of type 'char \\\[\\\*\\\]' does not
match bound 3 of type 'char \\\[3\\\]'\[^\n\r]*(\n|\r\n|\r)" } */
+}
+
+void foo3(void (*p)(int n0, char (*a)[n]))
+{
+ n = 4;
+ char A0[3];
+ (*p)(3, &A0); /* */
+ /* { dg-output "\[^\n\r]*bound 4 of type 'char \\\[\\\*\\\]' does not
match bound 3 of type 'char \\\[3\\\]'\[^\n\r]*(\n|\r\n|\r)" } */
+ (*p)(4, &A0); /* */
+ /* { dg-output "\[^\n\r]*bound 4 of type 'char \\\[\\\*\\\]' does not
match bound 3 of type 'char \\\[3\\\]'\[^\n\r]*(\n|\r\n|\r)" } */
+}
+
+void foo4(void (*p)(int n, char (*a)[n]))
+{
+ n = 3;
+ char A0[3];
+ (*p)(3, &A0);
+ (*p)(4, &A0); /* */
+ /* { dg-output "\[^\n\r]*bound 4 of type 'char \\\[\\\*\\\]' does not
match bound 3 of type 'char \\\[3\\\]'" } */
+}
+
+
+void foo5(void (*p)(int n0, char (*a)[n]))
+{
+ n = 3;
+ char A0[3];
+ (*p)(3, &A0);
+ (*p)(4, &A0);
+}
+
+
+void b1(int n0, char (*a)[n]) { }
+
+
+
+int main()
+{
+ foo1(&b0);
+
+ foo2(&b1);
+ foo3(&b1); // we should diagnose mismatch and run-time discrepancies
+
+ foo4(&b1);
+ foo5(&b1); // we should diagnose mismatch and run-time discrepancies
+}
+
+
+
[C PATCH 1/4] introduce ubsan checking for assigment of VM types 1/4
Hi Joseph and Martin,
this series adds UBSan checking for assignment of variably-modified
types, i.e. it checks that size expressions on both sides of the
assignment match.
1. no functional change, adds a structure argument to the
comptypes family functions in the C FE.
2. checking for all assignments except function arguments
including the libsanitizer changes (no upstream discussion so far)
3. checking for function arguments, but only when the function is
referenced using its declaration.
4. checking for functions called via a pointer
Q1: Should this be -fsanitize=vla-bound ? I used it because it is
related and does not have much functionality.
Q2: I now have warnings when a function can not be instrumented
because size expressions are too complicated or information was
lost before. Probably this needs to have a flag.
Martin
c: introduce ubsan checking for assigment of VM types 1/4
Reorganize recursive type checking to use a structure to
store information collected during the recursion and
returned to the caller (enum_and_init_p, different_types_p).
gcc/c:
* c-typeck.cc (struct comptypes_data): Add structure.
(tagged_types_tu_compatible_p,
function_types_compatible_p, type_lists_compatible_p,
comptyes_internal): Add structure to interface and
adapt calls.
(comptypes, comptypes_check_enum_int,
comptypes_check_different_types): Adapt calls.
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index 22e240a3c2a..2a1b7321b45 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -90,12 +90,14 @@ static bool require_constant_elements;
static bool require_constexpr_value;
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_tu_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 +127,8 @@ 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. */
@@ -1039,6 +1042,12 @@ common_type (tree t1, tree t2)
return c_common_type (t1, t2);
}
+struct comptypes_data {
+
+ 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. */
@@ -1049,7 +1058,9 @@ 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);
+ struct comptypes_data data = { };
+ val = comptypes_internal (type1, type2, &data);
+
free_all_tagged_tu_seen_up_to (tagged_tu_seen_base1);
return val;
@@ -1064,7 +1075,10 @@ 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);
+ struct comptypes_data data = { };
+ val = comptypes_internal (type1, type2, &data);
+ *enum_and_int_p = data.enum_and_int_p;
+
free_all_tagged_tu_seen_up_to (tagged_tu_seen_base1);
return val;
@@ -1080,7 +1094,10 @@ comptypes_check_different_types (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, different_types_p);
+ struct comptypes_data data = { };
+ val = comptypes_internal (type1, type2, &data);
+ *different_types_p = data.different_types_p;
+
free_all_tagged_tu_seen_up_to (tagged_tu_seen_base1);
return val;
@@ -1089,19 +1106,18 @@ comptypes_check_different_types (tree type1, tree type2,
/* Return 1 if TYPE1 and TYPE
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 &X->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 wid
[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 &am
[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 ? &width : NULL, decl_attrs, NULL, NULL,
+ wi
[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 (&cur_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 &newaxsref = *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 = &newaxsref.second;
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 (&cur_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 &newaxsref = *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 = &newaxsref.second;
>
>
> 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
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 (&cur_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 &newaxsref = *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 = &newaxsref.second;
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.
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 ? &width : NULL, decl_attrs, NULL, NULL,
+ width ? &width : NULL, decl_attrs, expr, NULL,
DEPRECATED_NORMAL);
finish_decl (value, loc, NU
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 expression
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(&m, &d) + test(m, &fn);
> > > +}
> > > +
> > > +
> > > 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(&m, &d) + test(m, &fn);
> > > +}
> > > +
> > > 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-type
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(&m, &d) + test(m, &fn);
> +}
> +
> +
> 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(&m, &d) + test(m, &fn);
> +}
> +
> 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, &a);
> +}
> +
> +
> 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, &a);
> +}
> +
> diff --git a/gcc/testsuite/gcc.dg/pr108423-4.c
> b/gcc/testsuite/gcc.dg/pr108423-4.c
> new file mode 100644
> index
[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 (&cur_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 &newaxsref = *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 = &newaxsref.second;
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..d4
[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(&m, &d) + test(m, &fn);
+}
+
+
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(&m, &d) + test(m, &fn);
+}
+
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, &a);
+}
+
+
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, &a);
+}
+
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
[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, &loc,
+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, &data);
+
+ 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, &data);
+ *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;
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) || sam
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 (pass_rest
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
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
