On Fri, Sep 6, 2024 at 10:50 PM Martin Uecker <ma.uec...@gmail.com> wrote: > > Am Freitag, dem 06.09.2024 um 13:59 -0700 schrieb Bill Wendling: > > 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. > > In the thread it is pointed out that returning a pointer works > too, and they would simply restrict passing the pointer elsewhere. > It's in rapidsna's initial reply titled "Taking the address of a count variable is forbidden":
https://discourse.llvm.org/t/rfc-introducing-new-clang-builtin-builtin-get-counted-by/80836/2 I didn't see her retreating from that idea. > I can't see "Apple's team rejects" and "vastly harder" at the > moment. > > > > > 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/ > > Yes, this would be a limitation. > And not one that I'm particularly excited about (allowing for (nearly) arbitrary expressions in the 'counted_by' attribute that is). -bw
