> 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 >> >> >