Hi, Sid,
Thanks a lot for the review.
I will update the testing cases per your suggestions.
> On Jun 19, 2025, at 12:07, Siddhesh Poyarekar <siddh...@gotplt.org> wrote:
>
> On 2025-06-16 18:08, Qing Zhao wrote:
>> gcc/ChangeLog:
>> * tree-object-size.cc (access_with_size_object_size): Handle pointers
>> with counted_by.
>
> This should probably just say "Update comment for .ACCESS_WITH_SIZE.".
Yes, you are right.
>
>> (collect_object_sizes_for): Likewise.
>> gcc/testsuite/ChangeLog:
>> * gcc.dg/pointer-counted-by-4.c: New test.
>> * gcc.dg/pointer-counted-by-5.c: New test.
>> * gcc.dg/pointer-counted-by-6.c: New test.
>> * gcc.dg/pointer-counted-by-7.c: New test.
>> ---
>
> I can't approve, but here's a review. In summary, I've suggested some
> modifications to the tests, the tree-object-size change itself looks fine to
> me.
>
>> gcc/testsuite/gcc.dg/pointer-counted-by-4.c | 63 +++++++++++++++++++++
>> gcc/testsuite/gcc.dg/pointer-counted-by-5.c | 48 ++++++++++++++++
>> gcc/testsuite/gcc.dg/pointer-counted-by-6.c | 47 +++++++++++++++
>> gcc/testsuite/gcc.dg/pointer-counted-by-7.c | 30 ++++++++++
>> gcc/tree-object-size.cc | 12 +++-
>> 5 files changed, 197 insertions(+), 3 deletions(-)
>> create mode 100644 gcc/testsuite/gcc.dg/pointer-counted-by-4.c
>> create mode 100644 gcc/testsuite/gcc.dg/pointer-counted-by-5.c
>> create mode 100644 gcc/testsuite/gcc.dg/pointer-counted-by-6.c
>> create mode 100644 gcc/testsuite/gcc.dg/pointer-counted-by-7.c
>> diff --git a/gcc/testsuite/gcc.dg/pointer-counted-by-4.c
>> b/gcc/testsuite/gcc.dg/pointer-counted-by-4.c
>> new file mode 100644
>> index 00000000000..11e9421c819
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/pointer-counted-by-4.c
>> @@ -0,0 +1,63 @@
>> +/* Test the attribute counted_by for pointer field and its usage in
>> + * __builtin_dynamic_object_size. */
>> +/* { dg-do run } */
>> +/* { dg-options "-O2" } */
>> +
>> +#include "builtin-object-size-common.h"
>> +
>> +struct pointer_array {
>> + int b;
>> + int *c;
>> +} *p_array;
>> +
>> +struct annotated {
>> + int *c __attribute__ ((counted_by (b)));
>> + int b;
>> +} *p_array_annotated;
>> +
>> +struct nested_annotated {
>> + int *c __attribute__ ((counted_by (b)));
>> + struct {
>> + union {
>> + int b;
>> + float f;
>> + };
>> + int n;
>> + };
>> +} *p_array_nested_annotated;
>> +
>> +void __attribute__((__noinline__)) setup (int normal_count, int attr_count)
>> +{
>> + p_array
>> + = (struct pointer_array *) malloc (sizeof (struct pointer_array));
>> + p_array->c = (int *) malloc (sizeof (int) * normal_count);
>> + p_array->b = normal_count;
>> +
>> + p_array_annotated
>> + = (struct annotated *) malloc (sizeof (struct annotated));
>> + p_array_annotated->c = (int *) malloc (sizeof (int) * attr_count);
>> + p_array_annotated->b = attr_count;
>> +
>> + p_array_nested_annotated
>> + = (struct nested_annotated *) malloc (sizeof (struct nested_annotated));
>> + p_array_nested_annotated->c = (int *) malloc (sizeof (int) * attr_count);
>> + p_array_nested_annotated->b = attr_count;
>> +
>> + return;
>> +}
>> +
>> +void __attribute__((__noinline__)) test ()
>> +{
>> + EXPECT(__builtin_dynamic_object_size(p_array->c, 1), -1);
>> + EXPECT(__builtin_dynamic_object_size(p_array_annotated->c, 1),
>> + p_array_annotated->b * sizeof (int));
>> + EXPECT(__builtin_dynamic_object_size(p_array_nested_annotated->c, 1),
>> + p_array_nested_annotated->b * sizeof (int));
>> +}
>
> Suggest extending this test for at least char so that you verify scaling.
> e.g. you could have this in pointer-counted-by-4.c:
>
> ```
> #ifndef PTR_TYPE
> #define PTR_TYPE int
> #endif
>
> struct pointer_array {
> int b;
> PTR_TYPE *c;
> } *p_array;
>
> struct annotated {
> PTR_TYPE *c __attribute__ ((counted_by (b)));
> int b;
> } *p_array_annotated;
>
> struct nested_annotated {
> PTR_TYPE *c __attribute__ ((counted_by (b)));
> struct {
> union {
> int b;
> float f;
> };
> int n;
> };
> } *p_array_nested_annotated;
> ```
>
> and then have another test pointer-counted-by-4-char.c that simply includes
> this test:
>
> ```
> #define PTR_TYPE char
> #include "pointer-counted-by-4.c"
Okay. Thanks for the suggestions. I will do that.
> ```
>
>> +
>> +int main(int argc, char *argv[])
>> +{
>> + setup (10,10);
>> + test ();
>> + DONE ();
>> +}
>> diff --git a/gcc/testsuite/gcc.dg/pointer-counted-by-5.c
>> b/gcc/testsuite/gcc.dg/pointer-counted-by-5.c
>> new file mode 100644
>> index 00000000000..45918c3e654
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/pointer-counted-by-5.c
>> @@ -0,0 +1,48 @@
>> +/* Test the attribute counted_by for pointer fields and its usage in
>> + * __builtin_dynamic_object_size: when the counted_by field is negative. */
>> +/* { dg-do run } */
>> +/* { dg-options "-O2" } */
>> +
>> +#include "builtin-object-size-common.h"
>> +
>> +struct annotated {
>> + int b;
>> + int *c __attribute__ ((counted_by (b)));
>> +} *array_annotated;
>> +
>> +struct nested_annotated {
>> + int *c __attribute__ ((counted_by (b)));
>> + struct {
>> + union {
>> + int b;
>> + float f;
>> + };
>> + int n;
>> + };
>> +} *array_nested_annotated;
>> +
>> +void __attribute__((__noinline__)) setup (int attr_count)
>> +{
>> + array_annotated
>> + = (struct annotated *)malloc (sizeof (struct annotated));
>> + array_annotated->b = attr_count;
>> +
>> + array_nested_annotated
>> + = (struct nested_annotated *)malloc (sizeof (struct nested_annotated));
>> + array_nested_annotated->b = attr_count - 1;
>> +
>> + return;
>> +}
>> +
>> +void __attribute__((__noinline__)) test ()
>> +{
>> + EXPECT(__builtin_dynamic_object_size(array_annotated->c, 1), 0);
>> + EXPECT(__builtin_dynamic_object_size(array_nested_annotated->c, 1), 0);
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> + setup (-10);
>> + test ();
>> + DONE ();
>> +}
>
> OK.
>
>> diff --git a/gcc/testsuite/gcc.dg/pointer-counted-by-6.c
>> b/gcc/testsuite/gcc.dg/pointer-counted-by-6.c
>> new file mode 100644
>> index 00000000000..4a1baa41d9a
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/pointer-counted-by-6.c
>> @@ -0,0 +1,47 @@
>> +/* Test the attribute counted_by for pointer fields and its usage in
>> + * __builtin_dynamic_object_size: when the type of the pointer
>> + * is casting to another type. */
>> +/* { dg-do run } */
>> +/* { dg-options "-O2" } */
>> +
>> +#include "builtin-object-size-common.h"
>> +
>> +typedef unsigned short u16;
>> +
>> +struct info {
>> + u16 data_len;
>> + char *data __attribute__((counted_by(data_len)));
>> +};
>> +
>> +struct foo {
>> + int a;
>> + int b;
>> +};
>> +
>> +static __attribute__((__noinline__))
>> +struct info *setup ()
>> +{
>> + struct info *p;
>> + size_t bytes = 3 * sizeof(struct foo);
>> +
>> + p = (struct info *) malloc (sizeof (struct info));
>> + p->data = (char *) malloc (bytes);
>> + p->data_len = bytes;
>> +
>> + return p;
>> +}
>> +
>> +static void
>> +__attribute__((__noinline__)) report (struct info *p)
>> +{
>> + struct foo *bar = (struct foo *)p->data;
>> + EXPECT(__builtin_dynamic_object_size((char *)(bar + 1), 1), 16);
>> + EXPECT(__builtin_dynamic_object_size((char *)(bar + 2), 1), 8);
>
> How about this instead; it eliminates the magic constants and also preserves
> the intent of the test in a more general (i.e. integer size agnostic) sense:
>
> EXPECT(__builtin_dynamic_object_size((char *)(bar + 1), 1),
> sizeof (struct foo) * 2);
> EXPECT(__builtin_dynamic_object_size((char *)(bar + 2), 1),
> sizeof (struct foo));
>
Yeah, this is better, will update per your suggestion.
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> + struct info *p = setup();
>> + report(p);
>> + return 0;
>> +}
>> diff --git a/gcc/testsuite/gcc.dg/pointer-counted-by-7.c
>> b/gcc/testsuite/gcc.dg/pointer-counted-by-7.c
>> new file mode 100644
>> index 00000000000..01addbb857d
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/pointer-counted-by-7.c
>> @@ -0,0 +1,30 @@
>> +/* Additional test of the attribute counted_by for pointer field and its
>> usage
>> + in __builtin_dynamic_object_size. */
>> +/* { dg-do run } */
>> +/* { dg-options "-O2" } */
>> +
>> +#include "builtin-object-size-common.h"
>> +
>> +struct annotated {
>> + int b;
>> + int *c __attribute__ ((counted_by (b)));
>> +};
>> +
>> +struct annotated __attribute__((__noinline__)) setup (int attr_count)
>> +{
>> + struct annotated p_array_annotated;
>> + p_array_annotated.c = (int *) malloc (sizeof (int) * attr_count);
>> + p_array_annotated.b = attr_count;
>> +
>> + return p_array_annotated;
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> + struct annotated x = setup (10);
>> + int *p = x.c;
>> + x = setup (20);
>> + EXPECT(__builtin_dynamic_object_size (p, 1), 10 * sizeof (int));
>> + EXPECT(__builtin_dynamic_object_size (x.c, 1), 20 * sizeof (int));
>> + DONE ();
>> +}
>
> OK. One note: all of the tests leak memory, but it's not an unbounded leak,
> so it's probably OK to ignore.
I could add call to free to free the allocated memory.
>
>> diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
>> index f348673ae75..170f1197c8d 100644
>> --- a/gcc/tree-object-size.cc
>> +++ b/gcc/tree-object-size.cc
>> @@ -773,10 +773,12 @@ addr_object_size (struct object_size_info *osi,
>> const_tree ptr,
>> 4th argument TYPE_OF_SIZE: A constant 0 with its TYPE being the same as
>> the TYPE
>> of the object referenced by REF_TO_SIZE
>> 6th argument: A constant 0 with the pointer TYPE to the original flexible
>> - array type.
>> + array type or pointer field type.
>> The size of the element can be retrived from the TYPE of the 6th
>> argument
>> - of the call, which is the pointer to the array type. */
>> + of the call, which is the pointer to the original flexible array type or
>> + the type of the original pointer field. */
>> +
>> static tree
>> access_with_size_object_size (const gcall *call, int object_size_type)
>> {
>> @@ -786,7 +788,7 @@ access_with_size_object_size (const gcall *call, int
>> object_size_type)
>> gcc_assert (gimple_call_internal_p (call, IFN_ACCESS_WITH_SIZE));
>> /* The type of the 6th argument type is the pointer TYPE to the original
>> - flexible array type. */
>> + flexible array type or to the original pointer type. */
>> tree pointer_to_array_type = TREE_TYPE (gimple_call_arg (call, 5));
>> gcc_assert (POINTER_TYPE_P (pointer_to_array_type));
>> tree element_type = TREE_TYPE (TREE_TYPE (pointer_to_array_type));
>
> Only a comment update to indicate the scope change for .ACCESS_WITH_SIZE. OK.
>
>> @@ -1854,6 +1856,10 @@ collect_object_sizes_for (struct object_size_info
>> *osi, tree var)
>> if (TREE_CODE (rhs) == SSA_NAME
>> && POINTER_TYPE_P (TREE_TYPE (rhs)))
>> reexamine = merge_object_sizes (osi, var, rhs);
>> + else if (TREE_CODE (rhs) == MEM_REF
>> + && POINTER_TYPE_P (TREE_TYPE (rhs))
>> + && TREE_CODE (TREE_OPERAND (rhs, 0)) == SSA_NAME)
>> + reexamine = merge_object_sizes (osi, var, TREE_OPERAND (rhs, 0));
>> else
>> expr_object_size (osi, var, rhs);
>> }
>
> Interesting, looks like this would improve coverage for more than just this
> specific case of pointers within structs. Nice! Is this what
> pointer-counted-by-7.c covers?
Not only for pointer-counted-by-7.c, all the testing cases
pointer-counted-by-4.c, pointer-counted-by-5.c, and pointer-counted-by-7.c need
this change.
I will add comments to these change in the next version.
Qing
>
> Thanks,
> Sid
