> On Jul 15, 2025, at 02:32, Richard Biener <richard.guent...@gmail.com> wrote:
> 
> On Mon, Jul 14, 2025 at 10:58 PM Qing Zhao <qing.z...@oracle.com> wrote:
>> 
>> 
>>> On Jul 7, 2025, at 13:07, Qing Zhao <qing.z...@oracle.com> wrote:
>>> 
>>> As I mentioned in the latest email I replied to the thread, the original 
>>> implementation of the counted_by for pointer was implemented without the 
>>> additional indirection.
>>> But that implementation has a fundamental bug during testing.  then I 
>>> changed the implementation like the current.
>>> 
>>> I need spending a little more time to find the details of that fundamental 
>>> bug with the original implementation.
>>> 
>>> If the current bug is urgent to be fixed. and you are not comfortable with 
>>> the simple Patch Sid provided, then I am okay to back it out now and then 
>>> push it back with the fix to this current bug at a later time after 
>>> everyone is comfortable with the current implementation.
>>> 
>>> Thanks a lot!
>>> 
>>> Qing
>> 
>> 
>> Hi,  this is an update on the above fundamental issue I mentioned 
>> previously. (I finally located this issue and recorded it here)
>> 
>> 1. Based on the previous discussion on how to resolve PR120929, we agreed 
>> the following solution:
>> 
>> struct S {
>>  int n;
>>  int *p __attribute__((counted_by(n)));
>> } *f;
>> 
>> when generating a call to .ACCESS_WITH_SIZE for f->p, instead of generating
>> *.ACCESS_WITH_SIZE (&f->p, &f->n,...)
>> 
>> We should generate
>> .ACCESS_WITH_SIZE (f->p, &f->n,...)
>> 
>> i.e.,
>> the return type and the type of the first argument of the call is the
>>   original pointer type in this version,
>>   instead of the pointer to the original pointer type in the 7th version;
>> 
>> 2. I implemented this new .ACCESS_WITH_SIZE generation for pointers in my 
>> local workspace. It looked fine in the beginning,
>> However, during testing, I finally located the _fundamental issue_ with this 
>> design.
>> 
>> This issue can be shown clearly with the following simple testing case:
>> (Note, the numbers on the left in the following testing case is the line #)
>> 
>> $ cat t1.c
>>  1 struct annotated {
>>  2   int b;
>>  3   int *c __attribute__ ((counted_by (b)));
>>  4 } *p_array_annotated;
>>  5
>>  6 void __attribute__((__noinline__)) setup (int annotated_count)
>>  7 {
>>  8   p_array_annotated
>>  9     = (struct annotated *) __builtin_malloc (sizeof (struct annotated));
>> 10   p_array_annotated->c = (int *) __builtin_malloc (annotated_count * 
>> sizeof (int));
>> 11   p_array_annotated->c[2] = 10;
>> 12   p_array_annotated->b = annotated_count;
> 
> But isn't this bogus since you access c[2] while it's counted_by value
> is still uninitialized?

Sorry for my mistake in the testing case.

If we switch the line 11 and 12 in the above testing case to make the 
counted_by value is initialized
before p_array_annotated->c is used, the same issue.

  1 struct annotated {
  2   int b;
  3   int *c __attribute__ ((counted_by (b)));
  4 } *p_array_annotated;
  5    
  6 void __attribute__((__noinline__)) setup (int annotated_count)
  7 {
  8   p_array_annotated
  9     = (struct annotated *) __builtin_malloc (sizeof (struct annotated));
 10   p_array_annotated->c = (int *) __builtin_malloc (annotated_count * sizeof 
(int));
 11   p_array_annotated->b = annotated_count;
 12   p_array_annotated->c[2] = 10;
 13   return;
 14 }
 15   
 16 int main(int argc, char *argv[])
 17 {
 18   setup (10);
 19   return 0;
 20 }


> I'd say by using counted_by you now invoke UB here.
The UB is because the way we generate the call to .ACCESS_WITH_SIZE: (for the 
above updated testing case, the gimple dump is):

  1 __attribute__((noinline))
  2 void setup (int annotated_count)
  3 {
  4   int * D.2969;
  5   
  6   _1 = __builtin_malloc (16);
  7   p_array_annotated = _1;
  8   _2 = (long unsigned int) annotated_count;
  9   _3 = _2 * 4;
 10   p_array_annotated.0_4 = p_array_annotated;
 11   _5 = p_array_annotated.0_4->c;
 12   p_array_annotated.1_6 = p_array_annotated;
 13   _7 = &p_array_annotated.1_6->b;
 14   D.2969 = .ACCESS_WITH_SIZE (_5, _7, 0B, 4);
 15   _8 = __builtin_malloc (_3);
 16   D.2969 = _8;
 17   p_array_annotated.2_9 = p_array_annotated;
 18   p_array_annotated.2_9->b = annotated_count;
 19   p_array_annotated.3_11 = p_array_annotated;
 20   _12 = p_array_annotated.3_11->c;
 21   p_array_annotated.4_13 = p_array_annotated;
 22   _14 = &p_array_annotated.4_13->b;
 23   _10 = .ACCESS_WITH_SIZE (_12, _14, 0B, 4);
 24   _15 = _10 + 8;
 25   *_15 = 10;
 26   return;
 27 }
 28 

In the above,  for the source code at line 10: 
10   p_array_annotated->c = (int *) __builtin_malloc (annotated_count * sizeof 
(int));

The ILs are:

 11   _5 = p_array_annotated.0_4->c;
 13   _7 = &p_array_annotated.1_6->b;
 14   D.2969 = .ACCESS_WITH_SIZE (_5, _7, 0B, 4);
 15   _8 = __builtin_malloc (_3);
 16   D.2969 = _8;

Note, at line 11, the value of the pointer “p_array_annotated->c” is assigned 
to _5, and this _5
is passed as the first argument to the call to .ACCESS_WITH_SIZE.

Later when .ACCESS_WITH_SIZE is expand to its first argument, the IL is:

11       _5 = p_array_annotated.0_4->c;
14      D.2969 = _5;
15      _8 = __builtin_malloc (_3);
16      D.2969 = _8:

Since the value of p_array_annotated->c at line 11 is uninitialized, yes, you 
are right, such implementation
bring UB into the program. 

So,  for 

struct S {
 int n;
 int *p __attribute__((counted_by(n)));
} *f;

When we generate  a call to .ACCESS_WITH_SIZE for the above  f->p as following 
(the agreed solution to PR120929)

.ACCESS_WITH_SIZE (f->p, &f->n,…)

Will bring UB into the program, therefore is not correct. 

On the other hand, generating the call to .ACCESS_WITH_SIZE as:

*.ACCESS_WITH_SIZE (&f->p, &f->n,…)

to pass the address of the original pointer should be the correct design as 
following (As I did in the previous implementation)

  9   _6 = &p_array_annotated.0_5->c;
 11   _8 = &p_array_annotated.1_7->b;
 12   _4 = .ACCESS_WITH_SIZE (_6, _8, 1, 0, -1, 0B);
 13   _9 = __builtin_malloc (_3);
 14   *_4 = _9;


Do I missing anything here?

Thanks. 

Qing

> 
> 13   return;
> 14 }
> 15
> 16 int main(int argc, char *argv[])
> 17 {
> 18   setup (10);
> 19   return 0;
> 20 }
> 
> $my-gcc t1.c    -O0 -g  -o ./t1.exe -fdump-tree-gimple
> $ ./t1.exe
> Segmentation fault (core dumped)
> 
> 3. As I debugged, the segmentation fault happened at line 11: 
> p_array_annotated->c[2] = 10;
>    Since the value of the pointer "p_array_annotated->c” is  0x0.
> 
> 4. Study the gimple dump t1.c.007t.gimple as following:
> 
>  1 __attribute__((noinline))
>  2 void setup (int annotated_count)
>  3 {
>  4   int * D.2969;
>  5    6   _1 = __builtin_malloc (16);
>  7   p_array_annotated = _1;
>  8   _2 = (long unsigned int) annotated_count;
>  9   _3 = _2 * 4;
> 10   p_array_annotated.0_4 = p_array_annotated;
> 11   _5 = p_array_annotated.0_4->c;
> 12   p_array_annotated.1_6 = p_array_annotated;
> 13   _7 = &p_array_annotated.1_6->b;
> 14   D.2969 = .ACCESS_WITH_SIZE (_5, _7, 0B, 4);
> 15   _8 = __builtin_malloc (_3);
> 16   D.2969 = _8;
> 17   p_array_annotated.2_10 = p_array_annotated;
> 18   _11 = p_array_annotated.2_10->c;
> 19   p_array_annotated.3_12 = p_array_annotated;
> 20   _13 = &p_array_annotated.3_12->b;
> 21   _9 = .ACCESS_WITH_SIZE (_11, _13, 0B, 4);
> 22   _14 = _9 + 8;
> 23   *_14 = 10;
> 24   p_array_annotated.4_15 = p_array_annotated;
> 25   p_array_annotated.4_15->b = annotated_count;
> 26   return;
> 27 }
> 
> We can see the root cause of this problem is because we passed the _value_ of 
>  “p_array_annotated->c”
> instead of the _address_ of “p_array_annotated->c” to .ACCESS_WITH_SIZE:
> 
> At line 11, the value of “p_array_annotated.0_4->c” is 0x0 when it was 
> assigned to “_5”;
> At line 14, the value of “_5” is passed to the call to .ACCESS_WITH_SIZE, 
> which also is 0x0;
> 
> And later when we expand .ACCESS_WITH_SIZE (_5, _7, 0B, 4), we replace it 
> with its first argument “_5”,
> As a result, the IL after the expand will look like the following:
> 
> 11       _5 = p_array_annotated.0_4->c;
> 14      D.2969 = _5;
> 15      _8 = __builtin_malloc (_3);
> 16      D.2969 = _8:
> 
> We can clearly see that the above IL is wrong: p_array_annotated->c is 
> initialized as 0x0, and this value is passed
> to the pointer D.2969, which is 0x0 too.
> 
> 5.  This is exactly the fundamental issue I met in the very beginning during 
> my implementation of the counted_by for
> pointers. I should record this issue at that time and sent email to the alias 
> at that time.
> 
> 6.  Based on this issue, I changed the .ACCESS_WITH_SIZE for pointers to
> 
> *.ACCESS_WITH_SIZE (&f->p, &f->n,…)
> 
> at that time.
> 
> And this resolved this fundamental issue.
> 
> In a summary:
> 
> 1. We still need to keep the design of  .ACCESS_WITH_SIZE for pointers  as in 
> patch version #7, i.e:
> 
> *.ACCESS_WITH_SIZE (&f->p, &f->n,…)
> 
> 2. As a side effect of the above IL , the change in gcc/tree-object-size.cc 
> <http://tree-object-size.cc/> in patch version #7 is still needed:
> 
> @@ -1854,6 +1856,17 @@ 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);
> +           /* Handle the following stmt #2 to propagate the size from the
> +              stmt #1 to #3:
> +               1  _1 = .ACCESS_WITH_SIZE (_3, _4, 1, 0, -1, 0B);
> +               2  _5 = *_1;
> +               3  _6 = __builtin_dynamic_object_size (_5, 1);
> +            */
> +           else if (TREE_CODE (rhs) == MEM_REF
> +                    && POINTER_TYPE_P (TREE_TYPE (rhs))
> +                    && TREE_CODE (TREE_OPERAND (rhs, 0)) == SSA_NAME
> +                    && integer_zerop (TREE_OPERAND (rhs, 1)))
> +             reexamine = merge_object_sizes (osi, var, TREE_OPERAND (rhs, 
> 0));
> 
> But, we should add one more condition to the above in order to limit such 
> propagation ONLY to
> the pointee is .ACCESS_WITH_SIZE, i.e (as Sid proposed)
> 
> +/* Return true if PTR is defined with .ACCESS_WITH_SIZE.  */
> +
> +static bool
> +is_access_with_size (tree ptr)
> +{
> +  gcc_assert (TREE_CODE (ptr) == SSA_NAME);
> +
> +  gimple *stmt = SSA_NAME_DEF_STMT (ptr);
> +  if (gimple_code (stmt) != GIMPLE_CALL)
> +    return false;
> +
> +  return gimple_call_internal_p (as_a <gcall *> (stmt), 
> IFN_ACCESS_WITH_SIZE);
> +}
> +
> 
> 
> +           /* Handle the following stmt #2 to propagate the size from the
> +              stmt #1 to #3:
> +               1  _1 = .ACCESS_WITH_SIZE (_3, _4, 1, 0, -1, 0B);
> +               2  _5 = *_1;
> +               3  _6 = __builtin_dynamic_object_size (_5, 1);
> +            */
> +           else if (TREE_CODE (rhs) == MEM_REF
> +                    && POINTER_TYPE_P (TREE_TYPE (rhs))
> +                    && TREE_CODE (TREE_OPERAND (rhs, 0)) == SSA_NAME
> +                    && integer_zerop (TREE_OPERAND (rhs, 1))
> +                     && is_access_with_size (TREE_OPERAND (rhs, 0)))
> +             reexamine = merge_object_sizes (osi, var, TREE_OPERAND (rhs, 
> 0));
> 
> Let me know if you have comments or suggestions to this fundamental issue and 
> whether my
> solution to it was reasonable.
> 
> Thanks a lot.
> 
> Qing
> 
> 
> 
> 

Reply via email to