Am Samstag, dem 07.09.2024 um 08:16 +0200 schrieb Martin Uecker:
> 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?
>
>
> 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;
>
> https://godbolt.org/z/41E3oj84o
>
> So why not do this then?
>
> Martin
Although the pointer design is cleaner. And if the __counted_by
is not a lvalue as allowed by clang, assigning to it will fail
anyway, so returning a null pointer also seems better in this
scenario.
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
> >
> >
>
