> On Dec 5, 2024, at 17:31, Martin Uecker <uec...@tugraz.at> wrote:
>
> Am Donnerstag, dem 05.12.2024 um 21:09 +0000 schrieb Qing Zhao:
>>
>>> On Dec 3, 2024, at 10:29, Qing Zhao <qing.z...@oracle.com> wrote:
>
> ....
>
>>>>
>>>>>>
>>>>>>>>
>>>>>>>> It would be clearer if you the syntax ".n" which resembles
>>>>>>>> the syntax for designated initializers that is already used
>>>>>>>> in initializers to refer to struct members.
>>>>>>>>
>>>>>>>> constexpr int n;
>>>>>>>> struct foo {
>>>>>>>> {
>>>>>>>> char (*p)[n] __attribute__ ((counted_by (.n))
>>>>>>>> int n;
>>>>>>>> }
>>>>>>>>
>>>>>>> Yes, I agree.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> There is one important additional requirement:
>>>>>>>>>>>
>>>>>>>>>>> x->n, x->p can ONLY be changed by changing the whole structure at
>>>>>>>>>>> the same time.
>>>>>>>>>>> Otherwise, x->n might not be consistent with x->p.
>>>>>>>>>>
>>>>>>>>>> By itself, this would still not fix the issue I pointed out.
>>>>>>>>>>
>>>>>>>>>> struct foo x;
>>>>>>>>>> x = .. ; // set the whole structure
>>>>>>>>>> char *p = x->p;
>>>>>>>>>> x = ... ; // set the whole structure
>>>>>>>>>>
>>>>>>>>>> What is the bound for 'p' ?
>>>>>>>>>
>>>>>>>>> Since p was set to the pointer field of the old structure, then the
>>>>>>>>> bound of it should be the old bound.
>>>>>>>>>> With current rules it would be the old bound.
>>>>>>>>>
>>>>>>>>> I thought that this should be the correct behavior, isn’t it?
>>>>>>>>
>>>>>>>> Yes, sorry, what I meant was "with the current rules it would be
>>>>>>>> the *new* bound”.
>>>>>>>
>>>>>>> struct foo x;
>>>>>>> x=… ; // set the whole structure 1
>>>>>>> char *p = x->p;
>>>>>>> x=… ; // set the whole structure 2
>>>>>>>
>>>>>>> In the above, when “set the whole structure 1”, x1, x1->n and x1->p are
>>>>>>> set at the same time;
>>>>>>> After *p = x->p; the pointer “p” is pointing to “x1->p”, it’s bound
>>>>>>> is “x1->n”;
>>>>>>
>>>>>> I agree.
>>>>>>>
>>>>>>> Then when “set the whole structure 2”, x2 is different than x1, x2->n
>>>>>>> and x2->p are set at the same time, the pointer
>>>>>>> ‘p’ still points to “x1->p”, therefore it’s bound should be “x1->n”.
>>>>>>>
>>>>>>> So, as long as the whole structure is set at the same time, should be
>>>>>>> fine.
>>>>>>>
>>>>>>> Do I miss anything here?
>>>>>>
>>>>>> I was talking aout the pointer "p" which was obtained before setting the
>>>>>> struct the second time in
>>>>>>
>>>>>> char *p = x->p;
>>>>>>
>>>>>> This pointer is still set to x1->p but the bound refers to x.n which is
>>>>>> now set to x2->n.
>>>>>
>>>>> You mean:
>>>>>
>>>>> struct foo x;
>>>>> x=… ; // set the whole structure 1
>>>>> char *p = x->p;
>>>>> x=… ; // set the whole structure 2
>>>>> p[index] = 10; // at this point, p’s bound is x2->n, not x1->n?
>>>>>
>>>>> Yes, you are right here.
>>>>>
>>>>> So, is there similar problem with the corresponding language extension?
>>>>>
>>>>
>>>> The language extension does not exist yet, so there is no problem.
>>> Yeah, I should mention this as “corresponding future language extension” -:)
>>>>
>>>> But I hope we will get it and then specify it so that this works
>>>> correctly without this footgun.
>>>>
>>>> Of course, if GCC gets the "counted_by" attribute wrong, there will
>>>> be arguments later in WG14 why the feature is then different to it.
>>>
>>> I think that we need to resolve this issue first in the design of
>>> “counted_by” for pointer fields.
>>> I guess that we might need to come up with some additional limitations for
>>> using the “counted_by”
>>> attribute for pointer fields at the source code level in order to avoid
>>> such potential error. But not
>>> sure what exactly the additional limitation should be at this moment.
>>>
>>> Need some study here.
>>
>> Actually, I found out that this is really not a problem with the current
>> design, for the following new testing case I added for my current
>> implementation of the counted_by for pointer field:
>>
>> [ gcc.dg]$ cat pointer-counted-by-7.c
>> /* 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 annotated {
>> int b;
>> int *c __attribute__ ((counted_by (b)));
>> };
>>
>> struct annotated *__attribute__((__noinline__)) setup (int attr_count)
>> {
>> struct annotated *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;
>>
>> 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 ();
>> }
>>
>> This test case passed without any issue.
>>
>> Our previous introduction of the new internal function .ACCESS_WITH_SIZE
>> already resolved this issue.
>>
>> So, I think that as long as the whole structure is set at the same time,
>> should be fine.
>>
>> Let me know if you have more comment here.
>
> I think the relevant scenario is where you assign the struct and
> not a pointer to the struct, i.e. something like the following:
>
> #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 ();
> }
>
With the above testing case, my current implementation based on
.ACCESS_WITH_SIZE succeed without any issue. -:)
The design of .ACCESS_WITH_SIZE already resolved this issue.
Qing
> Martin
>
>
>
