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
