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

[Qing Zhao <qing.zhao@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 16682 bytes --]

Sid,

Thanks a lot for your time to review the code.
See my reply below:

On Mar 11, 2024, at 10:57, Siddhesh Poyarekar <siddhesh@gotplt.org> 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),
+ "%<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.
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<http://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>.

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 <argument name> 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<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}.

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 <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" } */
+};