Re: [PATCH v6 1/5] Provide counted_by attribute to flexible array member field (PR108896)

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 2024-02-16 14:47, Qing Zhao wrote:
> 'counted_by (COUNT)'
>       The '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 "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 '__builtin_dynamic_object_size'.
> 
>       For instance, the following code:
> 
>            struct P {
>              size_t count;
>              char other;
>              char array[] __attribute__ ((counted_by (count)));
>            } *p;
> 
>       specifies that the 'array' is a flexible array member whose number
>       of elements is given by the field '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 'counted_by' annotation defines a relationship between
>       two objects, 'p->array' and 'p->count', and there are the following
>       requirementthat on the relationship between this pair:
> 
>          * 'p->count' should be initialized before the first reference to
>            'p->array';
> 
>          * 'p->array' has _at least_ '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.
> 
>       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 '__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,
> 
>              p->count = val1;
>              p->array[20] = 0;  // ref1 to p->array
>              p->count = val2;
>              p->array[30] = 0;  // ref2 to p->array
> 
>       in the above, 'ref1' will use 'val1' as the number of the elements
>       in 'p->array', and 'ref2' will use 'val2' as the number of elements
>       in 'p->array'.

I can't approve of course, but here's a review of the code that should 
hopefully make it easier for the C frontend maintainers.

> 
> gcc/c-family/ChangeLog:
> 
> 	PR C/108896
> 	* c-attribs.cc (handle_counted_by_attribute): New function.
> 	(attribute_takes_identifier_p): Add counted_by attribute to the list.
> 	* c-common.cc (c_flexible_array_member_type_p): ...To this.
> 	* c-common.h (c_flexible_array_member_type_p): New prototype.
> 
> gcc/c/ChangeLog:
> 
> 	PR C/108896
> 	* c-decl.cc (flexible_array_member_type_p): Renamed and moved to...
> 	(add_flexible_array_elts_to_size): Use renamed function.
> 	(is_flexible_array_member_p): Use renamed function.
> 	(verify_counted_by_attribute): New function.
> 	(finish_struct): Use renamed function and verify counted_by
> 	attribute.
> 	* c-tree.h (lookup_field): New prototype.
> 	* c-typeck.cc (lookup_field): Expose as extern function.
> 
> gcc/ChangeLog:
> 
> 	PR C/108896
> 	* doc/extend.texi: Document attribute counted_by.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR C/108896
> 	* gcc.dg/flex-array-counted-by.c: New test.
> ---
>   gcc/c-family/c-attribs.cc                    | 54 ++++++++++++-
>   gcc/c-family/c-common.cc                     | 13 +++
>   gcc/c-family/c-common.h                      |  1 +
>   gcc/c/c-decl.cc                              | 85 ++++++++++++++++----
>   gcc/c/c-tree.h                               |  1 +
>   gcc/c/c-typeck.cc                            |  3 +-
>   gcc/doc/extend.texi                          | 64 +++++++++++++++
>   gcc/testsuite/gcc.dg/flex-array-counted-by.c | 40 +++++++++
>   8 files changed, 241 insertions(+), 20 deletions(-)
>   create mode 100644 gcc/testsuite/gcc.dg/flex-array-counted-by.c
> 
> diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
> index 40a0cf90295d..4395c0656b14 100644
> --- a/gcc/c-family/c-attribs.cc
> +++ b/gcc/c-family/c-attribs.cc
> @@ -105,6 +105,8 @@ static tree handle_warn_if_not_aligned_attribute (tree *, tree, tree,
>   						  int, bool *);
>   static tree handle_strict_flex_array_attribute (tree *, tree, tree,
>   						 int, bool *);
> +static tree handle_counted_by_attribute (tree *, tree, tree,
> +					   int, bool *);
>   static tree handle_weak_attribute (tree *, tree, tree, int, bool *) ;
>   static tree handle_noplt_attribute (tree *, tree, tree, int, bool *) ;
>   static tree handle_alias_ifunc_attribute (bool, tree *, tree, tree, bool *);
> @@ -412,6 +414,8 @@ const struct attribute_spec c_common_gnu_attributes[] =
>   			      handle_warn_if_not_aligned_attribute, NULL },
>     { "strict_flex_array",      1, 1, true, false, false, false,
>   			      handle_strict_flex_array_attribute, NULL },
> +  { "counted_by",	      1, 1, true, false, false, false,
> +			      handle_counted_by_attribute, NULL },

New attribute.  OK.

>     { "weak",                   0, 0, true,  false, false, false,
>   			      handle_weak_attribute, NULL },
>     { "noplt",                   0, 0, true,  false, false, false,
> @@ -659,7 +663,8 @@ attribute_takes_identifier_p (const_tree attr_id)
>     else if (!strcmp ("mode", spec->name)
>   	   || !strcmp ("format", spec->name)
>   	   || !strcmp ("cleanup", spec->name)
> -	   || !strcmp ("access", spec->name))
> +	   || !strcmp ("access", spec->name)
> +	   || !strcmp ("counted_by", spec->name))

counted_by takes an identifier as an argument.  OK.

>       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"?

> +  /* The argument should be an identifier.  */
> +  else if (TREE_CODE (argval) != IDENTIFIER_NODE)
> +    {
> +      error_at (DECL_SOURCE_LOCATION (decl),
> +		"%<counted_by%> 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.

> +
> +  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/

> +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 <argument name> 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 <argument_name> to counted_by not in the same structure as <fam 
name>.

> +
> +      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 <argument name> is not an 
integer.

and same question about errors and ignoring as above.

> +
> +	  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<location_t>, tree,
>   			      vec<tree, va_gc> *, vec<tree, va_gc> *, 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}.

> +
> +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/ ?

> +@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 <wchar.h>
> +
> +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" } */
> +};