> On Dec 6, 2024, at 10:56, Martin Uecker <uec...@tugraz.at> wrote:
>
> Am Freitag, dem 06.12.2024 um 14:16 +0000 schrieb Qing Zhao:
>>
>>> 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.
>
> Ok, thanks! But I am a bit confused, because it seems it behaves
> this way also for FAMs
>
> https://godbolt.org/z/64a6z4cna
The behavior of the above testing case is exactly the additional feature we
provided for counted_by attribute for FAM:
=====
One important feature of the attribute is, a reference to the
flexible array member field uses the latest value assigned to the
field that represents the number of the elements before that
reference. For example,
p->count = val1;
p->array[20] = 0; // ref1 to p->array
p->count = val2;
p->array[30] = 0; // ref2 to p->array
in the above, 'ref1' uses 'val1' as the number of the elements in
'p->array', and 'ref2' uses 'val2' as the number of elements in
'p->array’.
=====
So, it’s the correct behavior for the counted_by attribute for FAM based on our
previous discussion and agreement.
However, as you pointed out, when the “counted_by” attribute is extended to
the pointer field, this feature will be problematic.
And we need to add the following additional new requirement for the
“counted_by” attribute of pointer field:
p->count and p->array can only be changed by changing the whole structure at
the same time.
So for a structure that includes a pointer field with a counted_by attribute,
the p->count and p->array must be updated at the same time by changing the
whole structure, therefore the testing case will not be valid for pointer
field, we MUST change it as the following to make it valid for pointer field
with counted_by attribute:
int main(int argc, char *argv[])
{
struct annotated *x = setup(10);
int *p = x->c;
x = setup(5);
printf("%d\n", (int)__builtin_dynamic_object_size (p, 1));
printf("%d\n", (int)__builtin_dynamic_object_size (x->c, 1));
exit(0);
}
Hope this is clear this time -:).
Qing
>
> and I thought this was changed, i.e. so that the bounds
> track the current value of b for this case. But apparently
> I was mistaken. I am fine with the behavior as shown here.
>
> Martin
>
>>
>> Qing
>>> Martin
>>>
>>>
>>>
>>
>
> --
> Univ.-Prof. Dr. rer. nat. Martin Uecker
> Graz University of Technology
> Institute of Biomedical Imaging
