> On Dec 3, 2024, at 10:29, Qing Zhao <[email protected]> wrote:
>
>
>
>> On Dec 3, 2024, at 10:07, Martin Uecker <[email protected]> wrote:
>>
>> Am Dienstag, dem 03.12.2024 um 14:31 +0000 schrieb Qing Zhao:
>>>
>>>> On Dec 3, 2024, at 01:33, Martin Uecker <[email protected]> wrote:
>>>>
>>>> Am Montag, dem 02.12.2024 um 22:58 +0000 schrieb Qing Zhao:
>>>>>
>>>>>> On Dec 2, 2024, at 16:13, Martin Uecker <[email protected]> wrote:
>>>>>>
>>>>>> Am Montag, dem 02.12.2024 um 20:15 +0000 schrieb Qing Zhao:
>>>>>>>
>>>>>>>> On Nov 30, 2024, at 07:22, Martin Uecker <[email protected]> wrote:
>>>>>>>>
>>>>>>>> Am Dienstag, dem 26.11.2024 um 20:59 +0000 schrieb Qing Zhao:
>>>>>>>>> Think this over these days, I have another thought that need some
>>>>>>>>> feedback:
>>>>>>>>>
>>>>>>>>> The major issue right now is:
>>>>>>>>>
>>>>>>>>> 1. For the following structure in which the “counted_by” attributes
>>>>>>>>> is attached to the pointer field.
>>>>>>>>>
>>>>>>>>> struct foo {
>>>>>>>>> int n;
>>>>>>>>> char *p __attribute__ ((counted_by (n)));
>>>>>>>>> } *x;
>>>>>>>>
>>>>>>>> BTW: I also do not like the syntax 'n' for a lookup of a member
>>>>>>>> in the member namespace. I think it should be '.n'. For FAM this
>>>>>>>> is less problematic because it always at the end, but here it is
>>>>>>>> problematic.
>>>>>>> During my implementation of extending this attribute to pointer fields
>>>>>>> of structure.
>>>>>>> I didn’t notice issue with the current syntax ’n’ for the pointer
>>>>>>> fields so far,
>>>>>>> even though when the field “n” is declared after the corresponding
>>>>>>> pointer field, i.e:
>>>>>>>
>>>>>>> struct foo {
>>>>>>> {
>>>>>>> char *p __attribute__ ((counted_by (n)));
>>>>>>> int n;
>>>>>>> }
>>>>>>>
>>>>>>> So, could you please explain a little bit more on what’s the potential
>>>>>>> issue here?
>>>>>>
>>>>>> My issue with it is that it is not consistent to how C's scoping
>>>>>> of identifiers work. In
>>>>>>
>>>>>> constexpr int n = 3;
>>>>>> struct foo {
>>>>>> {
>>>>>> char (*p)[n] __attribute__ ((counted_by (n))
>>>>>> int n;
>>>>>> }
>>>>>>
>>>>>> the n in the type of p refers to the previous n in scope while
>>>>>> the n in your attribute would refer to the member.
>>>>>
>>>>> Okay, I see your point here -:).
>>>>>
>>>>> Yes, I agree that if the compiler can accept the syntax “.n” in general,
>>>>> then it will make the
>>>>> “counted_by” attribute to allow more complicated expressions in general.
>>>>>
>>>>> We had this similar discussion before the design and implementation for
>>>>> the “counted_by” attribute
>>>>> on FAMs, and we agreed to delay the approach of accepting the syntax “.n”
>>>>> in the future possible
>>>>> language standard at that time.
>>>>>
>>>>> So, for the “counted_by attribute on FAMs, the implementation is,
>>>>> searching the “n” in all the fields of the
>>>>> containing structure and locating that specific field.
>>>>>
>>>>> Now, when extending “counted_by” attribute to pointer fields of
>>>>> structures, the implementation is similar.
>>>>
>>>>>>
>>>>>> This is incoherent and confusing. It becomes worse should
>>>>>> you ever want to allow more complicated expressions.
>>>>>
>>>>> You are right, it’s hard to allow more complicated expressions for
>>>>> “counted_by” based on the current
>>>>> design.
>>>>>
>>>>> If we agree to accept the “.n” syntax in GCC in general, that’s of course
>>>>> better.
>>>>> Then how about the current “counted_by” for FAMs? Shall we also change it
>>>>> to accept “.n” syntax?
>>>>
>>>> My recommendation would be to change it. It is also not ideal for
>>>> this case - only less problematic.
>>>
>>> If I remembered correctly, the “counted_by” attribute for FAM has been
>>> added into Linux Kernel already.
>>> Changing its syntax now also involves changing Linux Kernel source code.
>>>
>>> Kees, what’s your comments and suggestions for this?
>>>
>>> Another issue is, we had the similar discussion before implementing the
>>> “counted_by” attribute for FAM, at that time,
>>> We decided to use the current syntax, and CLANG also used the same syntax.
>>> Changing the syntax for the “counted_by” attribute for FAM at this moment
>>> is not very simple.
>>
>> We could add the new syntax and still allow the old syntax only for the
>> FAM case (possibly deprecating it).
>
> This is a good approach!
> So, for “counted_by” of pointer field, we will go with the “.n” syntax.
>
> After that, add this syntax for “counted_by” of FAM.
>
> Is that reasonable steps?
>>
>>>
>>> Why it’s LESS problematic for FAM case? (From implementation point of view,
>>> they are the same for both FAM or pointer inside a structure).
>>
>> Because the FAM is always last, so there is less room for confusion.
>> It is still annoying, which is why I pointed this out before.
> Okay. I see.
>>
>>
>>
>>>>
>>>>>>
>>>>>> 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.
Qing
