Re: [V3][PATCH 1/3] Provide counted_by attribute to flexible array member field (PR108896)
> On Oct 18, 2023, at 11:18 AM, Siddhesh Poyarekar wrote: > > On 2023-10-18 10:51, Qing Zhao wrote: > + member FIELD_DECL is a valid field of the containing structure's > fieldlist, > + FIELDLIST, Report error and remove this attribute when it's not. */ > +static void > +verify_counted_by_attribute (tree fieldlist, 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. */ > + > + const char *fieldname > += IDENTIFIER_POINTER (TREE_VALUE (TREE_VALUE (attr_counted_by))); > + > + /* Verify the argument of the attrbute is a valid field of the s/attrbute/attribute/ > + containing structure. */ > + > + tree counted_by_field = get_named_field (fieldlist, 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))); Probably someone with English as a first language would make a better suggestion, but how about: Argument specified in %qE attribute is not a field declaration in the same structure, ignoring it. > + > + DECL_ATTRIBUTES (field_decl) > += remove_attribute ("counted_by", DECL_ATTRIBUTES (field_decl)); > +} > + else > + /* Error when the field is not with an integer type. */ Suggest: Flag an error when the field is not of 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))); Suggest: Argument specified in %qE attribute is not of an integer type, ignoring it. > + > + DECL_ATTRIBUTES (field_decl) > += remove_attribute ("counted_by", DECL_ATTRIBUTES (field_decl)); > +} > +} > + > + return; >>> >>> I forgot to mention the redundant return here. >> Could you please clarify a little bit here, why the return here is redundant? > > It's the last line in the function, so even without that statement the > function will return. Oh, I see. -:) Actually,I always put an explicit return there even though it’s the last line and return implicitly. Qing > > Thanks, > Sid
Re: [V3][PATCH 1/3] Provide counted_by attribute to flexible array member field (PR108896)
On 2023-10-18 10:51, Qing Zhao wrote: + member FIELD_DECL is a valid field of the containing structure's fieldlist, + FIELDLIST, Report error and remove this attribute when it's not. */ +static void +verify_counted_by_attribute (tree fieldlist, 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. */ + + const char *fieldname += IDENTIFIER_POINTER (TREE_VALUE (TREE_VALUE (attr_counted_by))); + + /* Verify the argument of the attrbute is a valid field of the s/attrbute/attribute/ + containing structure. */ + + tree counted_by_field = get_named_field (fieldlist, 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))); Probably someone with English as a first language would make a better suggestion, but how about: Argument specified in %qE attribute is not a field declaration in the same structure, ignoring it. + + DECL_ATTRIBUTES (field_decl) += remove_attribute ("counted_by", DECL_ATTRIBUTES (field_decl)); +} + else + /* Error when the field is not with an integer type. */ Suggest: Flag an error when the field is not of 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))); Suggest: Argument specified in %qE attribute is not of an integer type, ignoring it. + + DECL_ATTRIBUTES (field_decl) += remove_attribute ("counted_by", DECL_ATTRIBUTES (field_decl)); +} +} + + return; I forgot to mention the redundant return here. Could you please clarify a little bit here, why the return here is redundant? It's the last line in the function, so even without that statement the function will return. Thanks, Sid
Re: [V3][PATCH 1/3] Provide counted_by attribute to flexible array member field (PR108896)
>>> + member FIELD_DECL is a valid field of the containing structure's >>> fieldlist, >>> + FIELDLIST, Report error and remove this attribute when it's not. */ >>> +static void >>> +verify_counted_by_attribute (tree fieldlist, 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. */ >>> + >>> + const char *fieldname >>> += IDENTIFIER_POINTER (TREE_VALUE (TREE_VALUE (attr_counted_by))); >>> + >>> + /* Verify the argument of the attrbute is a valid field of the >> s/attrbute/attribute/ >>> + containing structure. */ >>> + >>> + tree counted_by_field = get_named_field (fieldlist, 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))); >> Probably someone with English as a first language would make a better >> suggestion, but how about: >> Argument specified in %qE attribute is not a field declaration in the >> same structure, ignoring it. >>> + >>> + DECL_ATTRIBUTES (field_decl) >>> += remove_attribute ("counted_by", DECL_ATTRIBUTES (field_decl)); >>> +} >>> + else >>> + /* Error when the field is not with an integer type. */ >> Suggest: Flag an error when the field is not of 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))); >> Suggest: >> Argument specified in %qE attribute is not of an integer type, >> ignoring it. >>> + >>> + DECL_ATTRIBUTES (field_decl) >>> += remove_attribute ("counted_by", DECL_ATTRIBUTES (field_decl)); >>> +} >>> +} >>> + >>> + return; > > I forgot to mention the redundant return here. Could you please clarify a little bit here, why the return here is redundant? > >>> +} >>> /* Fill in the fields of a RECORD_TYPE or UNION_TYPE node, T.
Re: [V3][PATCH 1/3] Provide counted_by attribute to flexible array member field (PR108896)
Hi, Sid, Thanks a lot for your time and effort to review this patch set! And sorry for my late reply due to a long vacation immediately after Cauldron, just came back this Monday.. See my reply embedded below: > On Oct 5, 2023, at 2:51 PM, Siddhesh Poyarekar wrote: > > On 2023-08-25 11:24, Qing Zhao wrote: >> Provide a new counted_by attribute to flexible array member field. > > The obligatory "I can't ack the patch but here's a review" disclaimer :) > >> 'counted_by (COUNT)' >> The 'counted_by' attribute may be attached to the 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. An explicit 'counted_by' annotation defines a >> relationship between two objects, 'p->array' and 'p->count', that >> 'p->array' has _at least_ 'p->count' number of elements available. >> This relationship must hold even after any of these related objects >> are updated. It's the user's responsibility to make sure this >> relationship to be kept all the time. Otherwise the results of the >> array bound sanitizer and the '__builtin_dynamic_object_size' might >> be incorrect. >> For instance, in the following example, the allocated array has >> less elements than what's specified by the 'sbuf->count', this is >> an user error. As a result, out-of-bounds access to the array >> might not be detected. >> #define SIZE_BUMP 10 >> struct P *sbuf; >> void alloc_buf (size_t nelems) >> { >> sbuf = (struct P *) malloc (MAX (sizeof (struct P), >>(offsetof (struct P, array[0]) >> + nelems * sizeof (char; >> sbuf->count = nelems + SIZE_BUMP; >> /* This is invalid when the sbuf->array has less than sbuf->count >>elements. */ >> } >> In the following example, the 2nd update to the field 'sbuf->count' >> of the above structure will permit out-of-bounds access to the >> array 'sbuf>array' as well. >> #define SIZE_BUMP 10 >> struct P *sbuf; >> void alloc_buf (size_t nelems) >> { >> sbuf = (struct P *) malloc (MAX (sizeof (struct P), >>(offsetof (struct P, array[0]) >> + (nelems + SIZE_BUMP) * sizeof >> (char; >> sbuf->count = nelems; >> /* This is valid when the sbuf->array has at least sbuf->count >>elements. */ >> } >> void use_buf (int index) >> { >> sbuf->count = sbuf->count + SIZE_BUMP + 1; >> /* Now the value of sbuf->count is larger than the number >>of elements of sbuf->array. */ >> sbuf->array[index] = 0; >> /* then the out-of-bound access to this array >>might not be detected. */ >> } >> 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. >> gcc/ChangeLog: >> PR C/108896 >> * doc/extend.texi: Document attribute counted_by. >> * tree.cc (get_named_field): New function. >> * tree.h (get_named_field): New prototype. >> 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 | 79
Re: [V3][PATCH 1/3] Provide counted_by attribute to flexible array member field (PR108896)
On 2023-10-05 14:51, Siddhesh Poyarekar wrote: On 2023-08-25 11:24, Qing Zhao wrote: Provide a new counted_by attribute to flexible array member field. The obligatory "I can't ack the patch but here's a review" disclaimer :) 'counted_by (COUNT)' The 'counted_by' attribute may be attached to the 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. An explicit 'counted_by' annotation defines a relationship between two objects, 'p->array' and 'p->count', that 'p->array' has _at least_ 'p->count' number of elements available. This relationship must hold even after any of these related objects are updated. It's the user's responsibility to make sure this relationship to be kept all the time. Otherwise the results of the array bound sanitizer and the '__builtin_dynamic_object_size' might be incorrect. For instance, in the following example, the allocated array has less elements than what's specified by the 'sbuf->count', this is an user error. As a result, out-of-bounds access to the array might not be detected. #define SIZE_BUMP 10 struct P *sbuf; void alloc_buf (size_t nelems) { sbuf = (struct P *) malloc (MAX (sizeof (struct P), (offsetof (struct P, array[0]) + nelems * sizeof (char; sbuf->count = nelems + SIZE_BUMP; /* This is invalid when the sbuf->array has less than sbuf->count elements. */ } In the following example, the 2nd update to the field 'sbuf->count' of the above structure will permit out-of-bounds access to the array 'sbuf>array' as well. #define SIZE_BUMP 10 struct P *sbuf; void alloc_buf (size_t nelems) { sbuf = (struct P *) malloc (MAX (sizeof (struct P), (offsetof (struct P, array[0]) + (nelems + SIZE_BUMP) * sizeof (char; sbuf->count = nelems; /* This is valid when the sbuf->array has at least sbuf->count elements. */ } void use_buf (int index) { sbuf->count = sbuf->count + SIZE_BUMP + 1; /* Now the value of sbuf->count is larger than the number of elements of sbuf->array. */ sbuf->array[index] = 0; /* then the out-of-bound access to this array might not be detected. */ } 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. gcc/ChangeLog: PR C/108896 * doc/extend.texi: Document attribute counted_by. * tree.cc (get_named_field): New function. * tree.h (get_named_field): New prototype. 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 | 79 +++- gcc/doc/extend.texi | 77 +++ gcc/testsuite/gcc.dg/flex-array-counted-by.c | 40 ++ gcc/tree.cc | 40 ++ gcc/tree.h | 5 ++ 8 files changed, 291 insertions(+), 18 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/flex-array-counted-by.c diff --git a/gcc/c-family/c-attribs.cc
Re: [V3][PATCH 1/3] Provide counted_by attribute to flexible array member field (PR108896)
On 2023-08-25 11:24, Qing Zhao wrote: Provide a new counted_by attribute to flexible array member field. The obligatory "I can't ack the patch but here's a review" disclaimer :) 'counted_by (COUNT)' The 'counted_by' attribute may be attached to the 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. An explicit 'counted_by' annotation defines a relationship between two objects, 'p->array' and 'p->count', that 'p->array' has _at least_ 'p->count' number of elements available. This relationship must hold even after any of these related objects are updated. It's the user's responsibility to make sure this relationship to be kept all the time. Otherwise the results of the array bound sanitizer and the '__builtin_dynamic_object_size' might be incorrect. For instance, in the following example, the allocated array has less elements than what's specified by the 'sbuf->count', this is an user error. As a result, out-of-bounds access to the array might not be detected. #define SIZE_BUMP 10 struct P *sbuf; void alloc_buf (size_t nelems) { sbuf = (struct P *) malloc (MAX (sizeof (struct P), (offsetof (struct P, array[0]) + nelems * sizeof (char; sbuf->count = nelems + SIZE_BUMP; /* This is invalid when the sbuf->array has less than sbuf->count elements. */ } In the following example, the 2nd update to the field 'sbuf->count' of the above structure will permit out-of-bounds access to the array 'sbuf>array' as well. #define SIZE_BUMP 10 struct P *sbuf; void alloc_buf (size_t nelems) { sbuf = (struct P *) malloc (MAX (sizeof (struct P), (offsetof (struct P, array[0]) + (nelems + SIZE_BUMP) * sizeof (char; sbuf->count = nelems; /* This is valid when the sbuf->array has at least sbuf->count elements. */ } void use_buf (int index) { sbuf->count = sbuf->count + SIZE_BUMP + 1; /* Now the value of sbuf->count is larger than the number of elements of sbuf->array. */ sbuf->array[index] = 0; /* then the out-of-bound access to this array might not be detected. */ } 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. gcc/ChangeLog: PR C/108896 * doc/extend.texi: Document attribute counted_by. * tree.cc (get_named_field): New function. * tree.h (get_named_field): New prototype. 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 | 79 +++- gcc/doc/extend.texi | 77 +++ gcc/testsuite/gcc.dg/flex-array-counted-by.c | 40 ++ gcc/tree.cc | 40 ++ gcc/tree.h | 5 ++ 8 files changed, 291 insertions(+), 18 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/flex-array-counted-by.c diff --git
Re: [V3][PATCH 1/3] Provide counted_by attribute to flexible array member field (PR108896)
PIng. thanks. Qing > On Aug 25, 2023, at 11:24 AM, Qing Zhao wrote: > > Provide a new counted_by attribute to flexible array member field. > > 'counted_by (COUNT)' > The 'counted_by' attribute may be attached to the 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. An explicit 'counted_by' annotation defines a > relationship between two objects, 'p->array' and 'p->count', that > 'p->array' has _at least_ 'p->count' number of elements available. > This relationship must hold even after any of these related objects > are updated. It's the user's responsibility to make sure this > relationship to be kept all the time. Otherwise the results of the > array bound sanitizer and the '__builtin_dynamic_object_size' might > be incorrect. > > For instance, in the following example, the allocated array has > less elements than what's specified by the 'sbuf->count', this is > an user error. As a result, out-of-bounds access to the array > might not be detected. > > #define SIZE_BUMP 10 > struct P *sbuf; > void alloc_buf (size_t nelems) > { >sbuf = (struct P *) malloc (MAX (sizeof (struct P), > (offsetof (struct P, array[0]) >+ nelems * sizeof (char; >sbuf->count = nelems + SIZE_BUMP; >/* This is invalid when the sbuf->array has less than sbuf->count > elements. */ > } > > In the following example, the 2nd update to the field 'sbuf->count' > of the above structure will permit out-of-bounds access to the > array 'sbuf>array' as well. > > #define SIZE_BUMP 10 > struct P *sbuf; > void alloc_buf (size_t nelems) > { >sbuf = (struct P *) malloc (MAX (sizeof (struct P), > (offsetof (struct P, array[0]) >+ (nelems + SIZE_BUMP) * sizeof > (char; >sbuf->count = nelems; >/* This is valid when the sbuf->array has at least sbuf->count > elements. */ > } > void use_buf (int index) > { >sbuf->count = sbuf->count + SIZE_BUMP + 1; >/* Now the value of sbuf->count is larger than the number > of elements of sbuf->array. */ >sbuf->array[index] = 0; >/* then the out-of-bound access to this array > might not be detected. */ > } > > 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. > > gcc/ChangeLog: > > PR C/108896 > * doc/extend.texi: Document attribute counted_by. > * tree.cc (get_named_field): New function. > * tree.h (get_named_field): New prototype. > > 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 | 79 +++- > gcc/doc/extend.texi | 77 +++ > gcc/testsuite/gcc.dg/flex-array-counted-by.c | 40 ++ > gcc/tree.cc | 40 ++ > gcc/tree.h | 5 ++ > 8 files changed, 291 insertions(+), 18 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/flex-array-counted-by.c > > diff --git