Sid, Thanks a lot for your time to review the code. See my reply below: On Mar 11, 2024, at 10:57, Siddhesh Poyarekar wrote: On 2024-02-16 14:47, Qing Zhao wrote: return true; else return targetm.attribute_takes_identifier_p (attr_id); @@ -2806,6 +2811,53 @@ handle_strict_flex_array_attribute (tree *node, tree name, return NULL_TREE; } +/* Handle a "counted_by" attribute; arguments as in + struct attribute_spec.handler. */ + +static tree +handle_counted_by_attribute (tree *node, tree name, + tree args, int ARG_UNUSED (flags), + bool *no_add_attrs) +{ + tree decl = *node; + tree argval = TREE_VALUE (args); + + /* This attribute only applies to field decls of a structure. */ + if (TREE_CODE (decl) != FIELD_DECL) + { + error_at (DECL_SOURCE_LOCATION (decl), + "%qE attribute may not be specified for non-field" + " declaration %q+D", name, decl); + *no_add_attrs = true; + } + /* This attribute only applies to field with array type. */ + else if (TREE_CODE (TREE_TYPE (decl)) != ARRAY_TYPE) + { + error_at (DECL_SOURCE_LOCATION (decl), + "%qE attribute may not be specified for a non-array field", + name); + *no_add_attrs = true; + } + /* This attribute only applies to a C99 flexible array member type. */ + else if (! c_flexible_array_member_type_p (TREE_TYPE (decl))) + { + error_at (DECL_SOURCE_LOCATION (decl), + "%qE attribute may not be specified for a non" + " flexible array member field", + name); + *no_add_attrs = true; + } How about "not allowed" instead of "may not be specified"? Okay, will update them. + /* The argument should be an identifier. */ + else if (TREE_CODE (argval) != IDENTIFIER_NODE) + { + error_at (DECL_SOURCE_LOCATION (decl), + "% argument not an identifier"); + *no_add_attrs = true; + } Validate that the attribute only applies to a C99 flexible array member of a structure and the argument should be an identifier node. OK. verify_counted_by_attribute does more extensive validation on argval. Yes. + + return NULL_TREE; +} + /* Handle a "weak" attribute; arguments as in struct attribute_spec.handler. */ diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc index e15eff698dfd..56d828e3dfaf 100644 --- a/gcc/c-family/c-common.cc +++ b/gcc/c-family/c-common.cc @@ -9909,6 +9909,19 @@ c_common_finalize_early_debug (void) (*debug_hooks->early_global_decl) (cnode->decl); } +/* Determine whether TYPE is a ISO C99 flexible array memeber type "[]". */ s/memeber/member/ Okay, will update it. +bool +c_flexible_array_member_type_p (const_tree type) +{ + if (TREE_CODE (type) == ARRAY_TYPE + && TYPE_SIZE (type) == NULL_TREE + && TYPE_DOMAIN (type) != NULL_TREE + && TYPE_MAX_VALUE (TYPE_DOMAIN (type)) == NULL_TREE) + return true; + + return false; +} + Moved from c/c-decl.cc. OK. /* Get the LEVEL of the strict_flex_array for the ARRAY_FIELD based on the values of attribute strict_flex_array and the flag_strict_flex_arrays. */ unsigned int diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index 2d5f53998855..3e0eed0548b0 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -904,6 +904,7 @@ extern tree fold_for_warn (tree); extern tree c_common_get_narrower (tree, int *); extern bool get_attribute_operand (tree, unsigned HOST_WIDE_INT *); extern void c_common_finalize_early_debug (void); +extern bool c_flexible_array_member_type_p (const_tree); extern unsigned int c_strict_flex_array_level_of (tree); extern bool c_option_is_from_cpp_diagnostics (int); extern tree c_hardbool_type_attr_1 (tree, tree *, tree *); diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc index fe20bc21c926..4348123502e4 100644 --- a/gcc/c/c-decl.cc +++ b/gcc/c/c-decl.cc @@ -5301,19 +5301,6 @@ set_array_declarator_inner (struct c_declarator *decl, return decl; } -/* Determine whether TYPE is a ISO C99 flexible array memeber type "[]". */ -static bool -flexible_array_member_type_p (const_tree type) -{ - if (TREE_CODE (type) == ARRAY_TYPE - && TYPE_SIZE (type) == NULL_TREE - && TYPE_DOMAIN (type) != NULL_TREE - && TYPE_MAX_VALUE (TYPE_DOMAIN (type)) == NULL_TREE) - return true; - - return false; -} - /* Determine whether TYPE is a one-element array type "[1]". */ static bool one_element_array_type_p (const_tree type) @@ -5350,7 +5337,7 @@ add_flexible_array_elts_to_size (tree decl, tree init) elt = CONSTRUCTOR_ELTS (init)->last ().value; type = TREE_TYPE (elt); - if (flexible_array_member_type_p (type)) + if (c_flexible_array_member_type_p (type)) { complete_array_type (&type, elt, false); DECL_SIZE (decl) @@ -9317,7 +9304,7 @@ is_flexible_array_member_p (bool is_last_field, bool is_zero_length_array = zero_length_array_type_p (TREE_TYPE (x)); bool is_one_element_array = one_element_array_type_p (TREE_TYPE (x)); - bool is_flexible_array = flexible_array_member_type_p (TREE_TYPE (x)); + bool is_flexible_array = c_flexible_array_member_type_p (TREE_TYPE (x)); unsigned int strict_flex_array_level = c_strict_flex_array_level_of (x); @@ -9347,6 +9334,60 @@ is_flexible_array_member_p (bool is_last_field, return false; } +/* Verify the argument of the counted_by attribute of the flexible array + member FIELD_DECL is a valid field of the containing structure, + STRUCT_TYPE, Report error and remove this attribute when it's not. */ +static void +verify_counted_by_attribute (tree struct_type, tree field_decl) +{ + tree attr_counted_by = lookup_attribute ("counted_by", + DECL_ATTRIBUTES (field_decl)); + + if (!attr_counted_by) + return; + + /* If there is an counted_by attribute attached to the field, + verify it. */ + + tree fieldname = TREE_VALUE (TREE_VALUE (attr_counted_by)); + + /* Verify the argument of the attrbute is a valid field of the + containing structure. */ + + tree counted_by_field = lookup_field (struct_type, fieldname); + + /* Error when the field is not found in the containing structure. */ + if (!counted_by_field) + { + error_at (DECL_SOURCE_LOCATION (field_decl), + "%qE attribute argument not a field declaration" + " in the same structure, ignore it", + (get_attribute_name (attr_counted_by))); How about: counted_by attribute ignored as its argument is not in the same structure. I have a question here though: if it's an error, is it necessary to mention that the attribute is ignored? Couldn't we just say here: Argument to counted_by not in the same structure as . Studied a little bit here, yes, I agree with you, since this is an error, the object file will not be generated, it’s not necessary to mention the attribute is ignored. I will update this as well. + + DECL_ATTRIBUTES (field_decl) + = remove_attribute ("counted_by", DECL_ATTRIBUTES (field_decl)); + } + else + /* Error when the field is not with an integer type. */ + { + while (TREE_CHAIN (counted_by_field)) + counted_by_field = TREE_CHAIN (counted_by_field); + tree real_field = TREE_VALUE (counted_by_field); + + if (TREE_CODE (TREE_TYPE (real_field)) != INTEGER_TYPE) + { + error_at (DECL_SOURCE_LOCATION (field_decl), + "%qE attribute argument not a field declaration" + " with integer type, ignore it", + (get_attribute_name (attr_counted_by))); Likewise: counted_by attibute ignored as its argument is not an integer. and same question about errors and ignoring as above. Agreed, will update. + + DECL_ATTRIBUTES (field_decl) + = remove_attribute ("counted_by", DECL_ATTRIBUTES (field_decl)); + } + } + + return; +} /* Fill in the fields of a RECORD_TYPE or UNION_TYPE node, T. LOC is the location of the RECORD_TYPE or UNION_TYPE's definition. @@ -9408,6 +9449,7 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes, until now.) */ bool saw_named_field = false; + tree counted_by_fam_field = NULL_TREE; for (x = fieldlist; x; x = DECL_CHAIN (x)) { /* Whether this field is the last field of the structure or union. @@ -9468,7 +9510,7 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes, DECL_PACKED (x) = 1; /* Detect flexible array member in an invalid context. */ - if (flexible_array_member_type_p (TREE_TYPE (x))) + if (c_flexible_array_member_type_p (TREE_TYPE (x))) { if (TREE_CODE (t) == UNION_TYPE) { @@ -9489,6 +9531,12 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes, "members"); TREE_TYPE (x) = error_mark_node; } + + /* If there is a counted_by attribute attached to this field, + record it here and do more verification later after the + whole structure is complete. */ + if (lookup_attribute ("counted_by", DECL_ATTRIBUTES (x))) + counted_by_fam_field = x; } if (pedantic && TREE_CODE (t) == RECORD_TYPE @@ -9503,7 +9551,7 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes, when x is an array and is the last field. */ if (TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE) TYPE_INCLUDES_FLEXARRAY (t) - = is_last_field && flexible_array_member_type_p (TREE_TYPE (x)); + = is_last_field && c_flexible_array_member_type_p (TREE_TYPE (x)); /* Recursively set TYPE_INCLUDES_FLEXARRAY for the context of x, t when x is an union or record and is the last field. */ else if (RECORD_OR_UNION_TYPE_P (TREE_TYPE (x))) @@ -9758,6 +9806,9 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes, struct_parse_info->struct_types.safe_push (t); } + if (counted_by_fam_field) + verify_counted_by_attribute (t, counted_by_fam_field); + return t; } diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h index 1fba9c8dae76..c7c23edc4840 100644 --- a/gcc/c/c-tree.h +++ b/gcc/c/c-tree.h @@ -776,6 +776,7 @@ extern struct c_expr convert_lvalue_to_rvalue (location_t, struct c_expr, extern tree decl_constant_value_1 (tree, bool); extern void mark_exp_read (tree); extern tree composite_type (tree, tree); +extern tree lookup_field (tree, tree); extern tree build_component_ref (location_t, tree, tree, location_t, location_t); extern tree build_array_ref (location_t, tree, tree); diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc index ddeab1e2a8a1..cead0a055068 100644 --- a/gcc/c/c-typeck.cc +++ b/gcc/c/c-typeck.cc @@ -101,7 +101,6 @@ 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, tree); @@ -2375,7 +2374,7 @@ default_conversion (tree exp) the component is embedded within (nested) anonymous structures or unions, the list steps down the chain to the component. */ -static tree +tree lookup_field (tree type, tree component) { tree field; diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index 2b8ba1949bf1..fc2fbc702b44 100644 --- a/gcc/doc/extend.texi +++ b/gcc/doc/extend.texi @@ -7753,6 +7753,70 @@ align them on any target. The @code{aligned} attribute can also be used for functions (@pxref{Common Function Attributes}.) +@cindex @code{counted_by} variable attribute +@item counted_by (@var{count}) +The @code{counted_by} attribute may be attached to the C99 flexible array +member of a structure. It indicates that the number of the elements of the +array is given by the field named "@var{count}" in the same structure as the +flexible array member. GCC uses this information to improve the results of +the array bound sanitizer and the @code{__builtin_dynamic_object_size}. Should this be more open-ended, to allow for the compiler to do more using this attribute in future? I can imagine the static analyzer or even the middle end diagnostics making use of this information. How about: GCC may this information to improve detection of object size information for such structures and provide better results in compile-time diagnostics and runtime features like the array bound sanitizer and @code{__builtin_dynamic_object_size}. Yes, this is reasonable, will update. + +For instance, the following code: + +@smallexample +struct P @{ + size_t count; + char other; + char array[] __attribute__ ((counted_by (count))); +@} *p; +@end smallexample + +@noindent +specifies that the @code{array} is a flexible array member whose number of +elements is given by the field @code{count} in the same structure. + +The field that represents the number of the elements should have an +integer type. Otherwise, the compiler will report a warning and ignore +the attribute. +When the field that represents the number of the elements is assigned a +negative integer value, the compiler will treat the value as zero. + +An explicit @code{counted_by} annotation defines a relationship between +two objects, @code{p->array} and @code{p->count}, and there are the +following requirementthat on the relationship between this pair: + +@itemize @bullet +@item +@code{p->count} should be initialized before the first reference to Maybe s/should/must/ ? Okay thanks. Qing +@code{p->array}; + +@item +@code{p->array} has @emph{at least} @code{p->count} number of elements +available all the time. This relationship must hold even after any of +these related objects are updated during the program. +@end itemize + +It's the user's responsibility to make sure the above requirements to +be kept all the time. Otherwise the compiler will report warnings, +at the same time, the results of the array bound sanitizer and the +@code{__builtin_dynamic_object_size} is undefined. + +One important feature of the attribute is, a reference to the flexible +array member field will use the latest value assigned to the field that +represents the number of the elements before that reference. For example, + +@smallexample + p->count = val1; + p->array[20] = 0; // ref1 to p->array + p->count = val2; + p->array[30] = 0; // ref2 to p->array +@end smallexample + +@noindent +in the above, @code{ref1} will use @code{val1} as the number of the elements in +@code{p->array}, and @code{ref2} will use @code{val2} as the number of elements +in @code{p->array}. + @cindex @code{alloc_size} variable attribute @item alloc_size (@var{position}) @itemx alloc_size (@var{position-1}, @var{position-2}) diff --git a/gcc/testsuite/gcc.dg/flex-array-counted-by.c b/gcc/testsuite/gcc.dg/flex-array-counted-by.c new file mode 100644 index 000000000000..f8ce9776bf86 --- /dev/null +++ b/gcc/testsuite/gcc.dg/flex-array-counted-by.c @@ -0,0 +1,40 @@ +/* testing the correct usage of attribute counted_by. */ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +#include + +int size; +int x __attribute ((counted_by (size))); /* { dg-error "attribute may not be specified for non-field declaration" } */ + +struct trailing { + int count; + int field __attribute ((counted_by (count))); /* { dg-error "attribute may not be specified for a non-array field" } */ +}; + +struct trailing_1 { + int count; + int array_1[0] __attribute ((counted_by (count))); /* { dg-error "attribute may not be specified for a non flexible array member field" } */ +}; + +int count; +struct trailing_array_2 { + int count; + int array_2[] __attribute ((counted_by ("count"))); /* { dg-error "argument not an identifier" } */ +}; + +struct trailing_array_3 { + int other; + int array_3[] __attribute ((counted_by (L"count"))); /* { dg-error "argument not an identifier" } */ +}; + +struct trailing_array_4 { + int other; + int array_4[] __attribute ((counted_by (count))); /* { dg-error "attribute argument not a field declaration in the same structure, ignore it" } */ +}; + +int count; +struct trailing_array_5 { + float count; + int array_5[] __attribute ((counted_by (count))); /* { dg-error "attribute argument not a field declaration with integer type, ignore it" } */ +};