> On Sep 7, 2024, at 02:16, Martin Uecker <ma.uec...@gmail.com> 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 <isanb...@gmail.com> wrote:
>>> 
>>> On Fri, Sep 6, 2024 at 12:32 PM Martin Uecker <ma.uec...@gmail.com> wrote:
>>>> 
>>>> Am Freitag, dem 06.09.2024 um 13:59 +0000 schrieb Qing Zhao:
>>>>> 
>>>>>> On Sep 5, 2024, at 18:22, Bill Wendling <isanb...@gmail.com> wrote:
>>>>>> 
>>>>>> Hi Qing,
>>>>>> 
>>>>>> Sorry for my late reply.
>>>>>> 
>>>>>> On Thu, Aug 29, 2024 at 7:22 AM Qing Zhao <qing.z...@oracle.com> 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
>> 
>> 
> 

Reply via email to