> On Sep 7, 2024, at 02:16, Martin Uecker <[email protected]> wrote:
>
> Am Samstag, dem 07.09.2024 um 00:12 +0000 schrieb Qing Zhao:
>> Now, if
>>
>> 1. __builtin_get_counted_by should return a LVALUE instead of a pointer
>> (required by CLANG’s design)
>> And
>> 2. It’s better not to change the behavior of __builtin_choose_expr.
>>
>> Then the solution left is:
>>
>> __builtin_get_counted_by (p->FAM) returns a LVALUE as p->COUNT if p->FAM has
>> a counted_by attribute, if p->FAM does not have a counted_by attribute,
>> silently do nothing. (Or just issue warning if Linux is OKEY with such
>> waning).
>
> What does silently do nothing mean?
>
> /* do nothing */ = counter;
>
> will still fail to compile. So I guess you have something
> else in mind?
Ah, you are right here -:)
In my current implementation, I am doing the following:
If (p->FAM does not have counted_by attribute)
{
error_at (loc, "the argument must have %<counted_by%> "
"attribute %<__builtin_counted_by_ref%>”);
expr.set_error ();
goto error_exit;
}
i.e, when there is no counted_by, compilation time error.
So If I eliminate the compilation time error, this approach will still fail
during compilation time…
>
> The new _Generic selection also works if you return a
> lvalue of type void:
>
> struct foo x;
> _Generic(typeof(__counted_by(&x)), void: (int){ 0 },
> default: __counted_by(&x)) = 10;
Thanks a lot for the suggestion!
Yeah, looks like for the lvalue approach, the following will work:
1. When there is no counted_by attribute, return a LVALUE with type void;
2. Using the above _Generic expression to choose the expression based on the
type
>
> https://godbolt.org/z/41E3oj84o
>
> So why not do this then?
Thanks for the suggestion.
Qing
>
> Martin
>
>>
>> Is this acceptable?
>>
>> thanks.
>>
>> Qing
>>> On Sep 6, 2024, at 16:59, Bill Wendling <[email protected]> wrote:
>>>
>>> On Fri, Sep 6, 2024 at 12:32 PM Martin Uecker <[email protected]> wrote:
>>>>
>>>> Am Freitag, dem 06.09.2024 um 13:59 +0000 schrieb Qing Zhao:
>>>>>
>>>>>> On Sep 5, 2024, at 18:22, Bill Wendling <[email protected]> wrote:
>>>>>>
>>>>>> Hi Qing,
>>>>>>
>>>>>> Sorry for my late reply.
>>>>>>
>>>>>> On Thu, Aug 29, 2024 at 7:22 AM Qing Zhao <[email protected]> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Thanks for the information.
>>>>>>>
>>>>>>> Yes, providing a unary operator similar as __counted_by(PTR) as
>>>>>>> suggested by multiple people previously is a cleaner approach.
>>>>>>>
>>>>>>> Then the programmer will use the following:
>>>>>>>
>>>>>>> __builtin_choose_expr(
>>>>>>> __builtin_has_attribute (__p->FAM, "counted_by”)
>>>>>>> __builtin_get_counted_by(__p->FAM) = COUNT, 0);
>>>>>>>
>>>>>>> From the programmer’s point of view, it’s cleaner too.
>>>>>>>
>>>>>>> However, there is one issue with “__builtin_choose_expr” currently in
>>>>>>> GCC, its documentation explicitly mentions this limitation:
>>>>>>> (https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005fchoose_005fexpr)
>>>>>>>
>>>>>>> "Note: This construct is only available for C. Furthermore, the unused
>>>>>>> expression (exp1 or exp2 depending on the value of const_exp) may still
>>>>>>> generate syntax errors. This may change in future revisions.”
>>>>>>>
>>>>>>> So, due to this limitation, when there is no counted_by attribute, the
>>>>>>> __builtin_get_counted_by() still is evaluated by the compiler and
>>>>>>> errors is issued and the compilation stops, this can be show from the
>>>>>>> small testing case:
>>>>>>>
>>>>>>> [opc@qinzhao-ol8u3-x86 gcc]$ cat ttt.c
>>>>>>>
>>>>>>> struct flex {
>>>>>>> unsigned int b;
>>>>>>> int c[];
>>>>>>> } *array_flex;
>>>>>>>
>>>>>>> #define MY_ALLOC(P, FAM, COUNT) ({ \
>>>>>>> typeof(P) __p; \
>>>>>>> unsigned int __size = sizeof(*P) + sizeof(*P->FAM) * COUNT; \
>>>>>>> __p = (typeof(P)) __builtin_malloc(__size); \
>>>>>>> __builtin_choose_expr( \
>>>>>>> __builtin_has_attribute (__p->FAM, counted_by), \
>>>>>>> __builtin_counted_by_ref(__p->FAM) = COUNT, 0); \
>>>>>>> P = __p; \
>>>>>>> })
>>>>>>>
>>>>>>> int main(int argc, char *argv[])
>>>>>>> {
>>>>>>> MY_ALLOC(array_flex, c, 20);
>>>>>>> return 0;
>>>>>>> }
>>>>>>> [opc@qinzhao-ol8u3-x86 gcc]$ sh t
>>>>>>> ttt.c: In function ‘main’:
>>>>>>> ttt.c:13:5: error: the argument must have ‘counted_by’ attribute
>>>>>>> ‘__builtin_counted_by_ref’
>>>>>>> ttt.c:19:3: note: in expansion of macro ‘MY_ALLOC’
>>>>>>>
>>>>>>> I checked the FE code on handling “__buiiltin_choose_expr”, Yes, it
>>>>>>> does parse the __builtin_counted_by_ref(__p->FAM) even when
>>>>>>> __builtin_has_attribute(__p->FAM, counted_by) is FALSE, and issued the
>>>>>>> error when parsing __builtin_counted_by_ref and stopped the compilation.
>>>>>>>
>>>>>>> So, in order to support this approach, we first must fix the issue in
>>>>>>> the current __builtin_choose_expr in GCC. Otherwise, it’s impossible
>>>>>>> for the user to use this new builtin.
>>>>>>>
>>>>>>> Let me know your comments and suggestions.
>>>>>>>
>>>>>> Do you need to emit a diagnostic if the FAM doesn't have the
>>>>>> counted_by attribute? It was originally supposed to "silently fail" if
>>>>>> it didn't. We may need to do the same for Clang if so.
>>>>>
>>>>> Yes, “silently fail” should workaround this problem if fixing the issue
>>>>> in the current __builtin_choose_expr is too complicate.
>>>>>
>>>>> I will study a little bit on how to fix the issue in
>>>>> __builtin_choose_expr first.
>>>>>
>>>>> Martin and Joseph, any comment or suggestion from you?
>>>>
>>>> My recommendation would be not to change __builtin_choose_expr.
>>>>
>>>> The design where __builtin_get_counted_by returns a null
>>>> pointer constant (void*)0 seems good. Most users will
>>>> get an error which I think is what we want and for those
>>>> that want it to work even if the attribute is not there, the
>>>> following code seems perfectly acceptable to me:
>>>>
>>>> auto p = __builtin_get_counted_by(__p->FAM)
>>>> *_Generic(p, void*: &(int){}, default: p) = 1;
>>>>
>>>>
>>>> Kees also seemed happy with it. And if I understood it correctly,
>>>> also Clang's bounds checking people can work with this.
>>>>
>>> The problem with this is explained in the Clang RFC [1]. Apple's team
>>> rejects taking the address of the 'counter' field when using
>>> -fbounds-safety. They suggested this as an alternative:
>>>
>>> __builtin_bounds_attr_arg(ptr->FAM) = COUNT;
>>>
>>> The __builtin_bounds_attr_arg(ptr->FAM) is replaced by an L-value to
>>> the 'ptr->count' field during SEMA, and life goes on as normal. There
>>> are a few reasons for this:
>>>
>>> 1. They want to track the relationship between the FAM and the
>>> counter so that one isn't modified without the other being modified.
>>> Allowing the address to be taken makes that check vastly harder.
>>>
>>> 2. Apple's implementation supports expressions in the '__counted_by'
>>> attribute, thus the 'count' may be an R-value that can't have its
>>> address taken.
>>>
>>> [1]
>>> https://discourse.llvm.org/t/rfc-introducing-new-clang-builtin-builtin-get-counted-by/80836/
>>>
>>> -bw
>>
>>
>
