On Fri, Sep 6, 2024 at 5:12 PM Qing Zhao <qing.z...@oracle.com> wrote: > > 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? > We might have to finagle things a bit better, as the compiler will probably be annoyed at whatever gets generated by the code when the 'counted_by' attribute isn't there. For example, do we implicitly change it into '*(void *)0 = COUNT;' and expect that the '__builtin_has_attribute' check was already done? Do we remove the assignment expression entirely? If this is all happening inside '__builtin_choose_expr' (or '_Generic'?) then it might not be too much of a concern. But if it's outside of that builtin, trouble? I don't rightly know the answers to those questions. I'm not a big fan of turning an assignment expression into a no-op outside of '__builtin_choose_expr'...
Let me think about it some more. If all else fails and we need that warning, we could always add a `-Wno-<flag>` to Linux. They have a long history of hating compiler warnings, even in cases where they explicitly *ask* for the warnings, so it's not likely to run into a NACK or anything... > 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 > >
