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