On Tue, Aug 20, 2024 at 3:41 PM Qing Zhao <qing.z...@oracle.com> wrote:
>
>
>
> > On Aug 20, 2024, at 05:58, Richard Biener <richard.guent...@gmail.com>
> > wrote:
> >
> > On Tue, Aug 13, 2024 at 5:34 PM Qing Zhao <qing.z...@oracle.com> wrote:
> >>
> >> With the addition of the 'counted_by' attribute and its wide roll-out
> >> within the Linux kernel, a use case has been found that would be very
> >> nice to have for object allocators: being able to set the counted_by
> >> counter variable without knowing its name.
> >>
> >> For example, given:
> >>
> >> struct foo {
> >> ...
> >> int counter;
> >> ...
> >> struct bar array[] __attribute__((counted_by (counter)));
> >> } *p;
> >>
> >> The existing Linux object allocators are roughly:
> >>
> >> #define alloc(P, FAM, COUNT) ({ \
> >> size_t __size = sizeof(*P) + sizeof(*P->FAM) * COUNT; \
> >> kmalloc(__size, GFP); \
> >> })
> >>
> >> Right now, any addition of a counted_by annotation must also
> >> include an open-coded assignment of the counter variable after
> >> the allocation:
> >>
> >> p = alloc(p, array, how_many);
> >> p->counter = how_many;
> >>
> >> In order to avoid the tedious and error-prone work of manually adding
> >> the open-coded counted-by intializations everywhere in the Linux
> >> kernel, a new GCC builtin __builtin_get_counted_by will be very useful
> >> to be added to help the adoption of the counted-by attribute.
> >>
> >> -- Built-in Function: TYPE __builtin_get_counted_by (PTR)
> >> The built-in function '__builtin_get_counted_by' checks whether the
> >> array object pointed by the pointer PTR has another object
> >> associated with it that represents the number of elements in the
> >> array object through the 'counted_by' attribute (i.e. the
> >> counted-by object). If so, returns a pointer to the corresponding
> >> counted-by object. If such counted-by object does not exist,
> >> returns a NULL pointer.
> >>
> >> This built-in function is only available in C for now.
> >>
> >> The argument PTR must be a pointer to an array. The TYPE of the
> >> returned value must be a pointer type pointing to the corresponding
> >> type of the counted-by object or a pointer type pointing to the
> >> SIZE_T in case of a NULL pointer being returned.
> >>
> >> With this new builtin, the central allocator could be updated to:
> >>
> >> #define alloc(P, FAM, COUNT) ({ \
> >> typeof(P) __p; \
> >> size_t __size = sizeof(*P) + sizeof(*P->FAM) * COUNT; \
> >> __p = kmalloc(__size, GFP); \
> >> if (__builtin_get_counted_by (__p->FAM)) \
> >> *(__builtin_get_counted_by(__p->FAM)) = COUNT; \
> >> __p; \
> >> })
> >>
> >> And then structs can gain the counted_by attribute without needing
> >> additional open-coded counter assignments for each struct, and
> >> unannotated structs could still use the same allocator.
> >
> > Did you consider a __builtin_set_counted_by (PTR, VALUE)?
>
> Yes, that’s the initial request from Kees. -)
>
> The title of PR116016 is: add __builtin_set_counted_by(P->FAM, COUNT) or
> equivalent
>
> After extensive discussion (Martin Uecker raised the initial idea in
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116016#c24, more discussions
> followed, till comments #31). we decided to provide
> __builtin_get_counted_by(PTR) instead of __builtin_set_counted_by(PTR, VALUE)
> due to the following two reasons:
>
> 1. __builtin_get_counted_by should be enough to provide the functionality,
> and even simpler;
> 2. More flexible to be used by the programmer to be able to both WRITE and
> READ the counted-by field.
>
>
>
> >
> > Note that __builtin_get_counted_by to me suggests it returns the
> > value and not a pointer to the value.
>
> The syntax of __builtin_get_counted_by is:
>
> TYPE __builtin_get_counted_by (PTR)
>
> The returned value is:
>
> returns a pointer to the corresponding
> counted-by object. If such counted-by object does not exist,
> returns a NULL pointer.
>
> This built-in function is only available in C for now.
>
> The argument PTR must be a pointer to an array. The TYPE of the
> returned value must be a pointer type pointing to the corresponding
> type of the counted-by object or a pointer type pointing to the
> SIZE_T in case of a NULL pointer being returned.
>
>
> > A more proper language extension might involve a keyword
> > like __real, so __counted_by X would produce an lvalue, selecting
> > the counted-by member.
>
> Yes, if the returned value could be a LVALUE instead of a Pointer, that’s
> even simpler and cleaner.
> However, then as you mentioned below, another builtin
> “__builtin_has_attribute(PTR, counted_by)” need
> to be queried first to make sure the counted_by field exists.
>
> We have discussed this approach, and I preferred this approach too.
>
> However, the main reason we gave up on that direction is:
>
> There is NO __builtin_has_attribute (PTR, counted_by) been supported by
> CLANG, and not sure how difficult for CLANG to add this new builtin.
>
> In order to provide consistent interface to Linux kernel from both CLANG and
> GCC, I finally decided to provide the current interface in order to be
> consistent with CLANG.
>
> Bill, could you please provide a little bit more info on the possibility of a
> new builtin __builtin_has_attribute() in CLANG?
>
> > You'd need another way to query whether
> > 'X' has a counted-by member - the apparent runtime test in your
> > example is odd for a statically typed language, type selection
> > like with generics
>
> Could you please explain this a little bit more? (Not quite understood, a
> small example will be better, -:) thanks a lot.
> > or __builtin_classify_type instead of overloading
> > the value producing builtin might be more fit here?
>
> You mean the following lines from the testing case:
yes
> + if (__builtin_get_counted_by (__p->FAM)) \
> + *(__builtin_get_counted_by(__p->FAM)) = COUNT; \
>
> How to improve it? (Thanks a lot for your suggestion).
There's lack of syntactic guarantee that __builtin_get_counted_by (...) != 0 is
a constant expression. __builtin_set_counted_by (...) would avoid this
when it would be documented to expand to nothing for a type without a counted_by
member. Writing
size_t fake;
__builtin_choose_expr (__builtin_get_counted_by (__p-->FAM) != 0,
*(__builtin_get_counted_by(__p->FAM)), __fake) = COUNT;
would ensure this but of course requiring the __fake lvalue is ugly, too.
Richard.
>
> Qing
>
> >
> > No objection to the patch but I wanted to share my thoughts here.
> >
> > Richard.
> >
> >> Bootstrapped and regression tested on both X86 and aarch64, no issue.
> >>
> >> Okay for trunk?
> >>
> >> thanks.
> >>
> >> Qing.
> >>
> >>
> >> PR c/116016
> >>
> >> gcc/c-family/ChangeLog:
> >>
> >> * c-common.cc: Add new __builtin_get_counted_by.
> >> * c-common.h (enum rid): Add RID_BUILTIN_GET_COUNTED_BY.
> >>
> >> gcc/c/ChangeLog:
> >>
> >> * c-decl.cc (names_builtin_p): Add RID_BUILTIN_GET_COUNTED_BY.
> >> * c-parser.cc (has_counted_by_object): New routine.
> >> (get_counted_by_ref): New routine.
> >> (c_parser_postfix_expression): Handle New
> >> RID_BUILTIN_GET_COUNTED_BY.
> >>
> >> gcc/ChangeLog:
> >>
> >> * doc/extend.texi: Add documentation for __builtin_get_counted_by.
> >>
> >> gcc/testsuite/ChangeLog:
> >>
> >> * gcc.dg/builtin-get-counted-by-1.c: New test.
> >> * gcc.dg/builtin-get-counted-by.c: New test.
> >> ---
> >> gcc/c-family/c-common.cc | 1 +
> >> gcc/c-family/c-common.h | 1 +
> >> gcc/c/c-decl.cc | 1 +
> >> gcc/c/c-parser.cc | 72 +++++++++++++++
> >> gcc/doc/extend.texi | 55 +++++++++++
> >> .../gcc.dg/builtin-get-counted-by-1.c | 91 +++++++++++++++++++
> >> gcc/testsuite/gcc.dg/builtin-get-counted-by.c | 54 +++++++++++
> >> 7 files changed, 275 insertions(+)
> >> create mode 100644 gcc/testsuite/gcc.dg/builtin-get-counted-by-1.c
> >> create mode 100644 gcc/testsuite/gcc.dg/builtin-get-counted-by.c
> >>
> >> diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc
> >> index e7e371fd26f..4b27c6bfeeb 100644
> >> --- a/gcc/c-family/c-common.cc
> >> +++ b/gcc/c-family/c-common.cc
> >> @@ -430,6 +430,7 @@ const struct c_common_resword c_common_reswords[] =
> >> { "__builtin_choose_expr", RID_CHOOSE_EXPR, D_CONLY },
> >> { "__builtin_complex", RID_BUILTIN_COMPLEX, D_CONLY },
> >> { "__builtin_convertvector", RID_BUILTIN_CONVERTVECTOR, 0 },
> >> + { "__builtin_get_counted_by", RID_BUILTIN_GET_COUNTED_BY, D_CONLY },
> >> { "__builtin_has_attribute", RID_BUILTIN_HAS_ATTRIBUTE, 0 },
> >> { "__builtin_launder", RID_BUILTIN_LAUNDER, D_CXXONLY },
> >> { "__builtin_shuffle", RID_BUILTIN_SHUFFLE, 0 },
> >> diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
> >> index 2510ee4dbc9..5d5a297012f 100644
> >> --- a/gcc/c-family/c-common.h
> >> +++ b/gcc/c-family/c-common.h
> >> @@ -110,6 +110,7 @@ enum rid
> >> RID_TYPES_COMPATIBLE_P, RID_BUILTIN_COMPLEX,
> >> RID_BUILTIN_SHUFFLE,
> >> RID_BUILTIN_SHUFFLEVECTOR, RID_BUILTIN_CONVERTVECTOR,
> >> RID_BUILTIN_TGMATH,
> >> RID_BUILTIN_HAS_ATTRIBUTE, RID_BUILTIN_ASSOC_BARRIER,
> >> RID_BUILTIN_STDC,
> >> + RID_BUILTIN_GET_COUNTED_BY,
> >> RID_DFLOAT32, RID_DFLOAT64, RID_DFLOAT128,
> >>
> >> /* TS 18661-3 keywords, in the same sequence as the TI_* values. */
> >> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
> >> index 8cef8f2c289..40bc808878b 100644
> >> --- a/gcc/c/c-decl.cc
> >> +++ b/gcc/c/c-decl.cc
> >> @@ -11739,6 +11739,7 @@ names_builtin_p (const char *name)
> >> case RID_BUILTIN_SHUFFLE:
> >> case RID_BUILTIN_SHUFFLEVECTOR:
> >> case RID_BUILTIN_STDC:
> >> + case RID_BUILTIN_GET_COUNTED_BY:
> >> case RID_CHOOSE_EXPR:
> >> case RID_OFFSETOF:
> >> case RID_TYPES_COMPATIBLE_P:
> >> diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
> >> index 9b9284b1ba4..7b179449fb0 100644
> >> --- a/gcc/c/c-parser.cc
> >> +++ b/gcc/c/c-parser.cc
> >> @@ -10646,6 +10646,35 @@ c_parser_predefined_identifier (c_parser *parser)
> >> return expr;
> >> }
> >>
> >> +/* Check whether the ARRAY_REF has an counted-by object associated with it
> >> + through the "counted_by" attribute. */
> >> +static bool
> >> +has_counted_by_object (tree array_ref)
> >> +{
> >> + /* Currently, only when the array_ref is an indirect_ref to a call to
> >> the
> >> + .ACCESS_WITH_SIZE, return true.
> >> + More cases can be included later when the counted_by attribute is
> >> + extended to other situations. */
> >> + if ((TREE_CODE (array_ref) == INDIRECT_REF)
> >> + && is_access_with_size_p (TREE_OPERAND (array_ref, 0)))
> >> + return true;
> >> + return false;
> >> +}
> >> +
> >> +/* Get the reference to the counted-by object associated with the
> >> ARRAY_REF. */
> >> +static tree
> >> +get_counted_by_ref (tree array_ref)
> >> +{
> >> + /* Currently, only when the array_ref is an indirect_ref to a call to
> >> the
> >> + .ACCESS_WITH_SIZE, get the corresponding counted_by ref.
> >> + More cases can be included later when the counted_by attribute is
> >> + extended to other situations. */
> >> + if ((TREE_CODE (array_ref) == INDIRECT_REF)
> >> + && is_access_with_size_p (TREE_OPERAND (array_ref, 0)))
> >> + return CALL_EXPR_ARG (TREE_OPERAND (array_ref, 0), 1);
> >> + return NULL_TREE;
> >> +}
> >> +
> >> /* Parse a postfix expression (C90 6.3.1-6.3.2, C99 6.5.1-6.5.2,
> >> C11 6.5.1-6.5.2). Compound literals aren't handled here; callers have
> >> to
> >> call c_parser_postfix_expression_after_paren_type on encountering them.
> >> @@ -11740,6 +11769,49 @@ c_parser_postfix_expression (c_parser *parser)
> >> set_c_expr_source_range (&expr, loc, close_paren_loc);
> >> break;
> >> }
> >> + case RID_BUILTIN_GET_COUNTED_BY:
> >> + {
> >> + vec<c_expr_t, va_gc> *cexpr_list;
> >> + c_expr_t *e_p;
> >> + location_t close_paren_loc;
> >> +
> >> + c_parser_consume_token (parser);
> >> + if (!c_parser_get_builtin_args (parser,
> >> + "__builtin_get_counted_by",
> >> + &cexpr_list, false,
> >> + &close_paren_loc))
> >> + {
> >> + expr.set_error ();
> >> + break;
> >> + }
> >> + if (vec_safe_length (cexpr_list) != 1)
> >> + {
> >> + error_at (loc, "wrong number of arguments to "
> >> + "%<__builtin_get_counted_by%>");
> >> + expr.set_error ();
> >> + break;
> >> + }
> >> +
> >> + e_p = &(*cexpr_list)[0];
> >> +
> >> + if (TREE_CODE (TREE_TYPE (e_p->value)) != ARRAY_TYPE)
> >> + {
> >> + error_at (loc, "the argument must be an array"
> >> + "%<__builtin_get_counted_by%>");
> >> + expr.set_error ();
> >> + break;
> >> + }
> >> +
> >> + else if (has_counted_by_object ((*cexpr_list)[0].value))
> >> + expr.value
> >> + = get_counted_by_ref ((*cexpr_list)[0].value);
> >> + else
> >> + expr.value
> >> + = build_int_cst (build_pointer_type (size_type_node), 0);
> >> +
> >> + set_c_expr_source_range (&expr, loc, close_paren_loc);
> >> + break;
> >> + }
> >> case RID_BUILTIN_SHUFFLE:
> >> {
> >> vec<c_expr_t, va_gc> *cexpr_list;
> >> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> >> index 48b27ff9f39..485f6f8dd8d 100644
> >> --- a/gcc/doc/extend.texi
> >> +++ b/gcc/doc/extend.texi
> >> @@ -15198,6 +15198,61 @@ initializers of variables usable in constant
> >> expressions. For more details
> >> refer to the latest revision of the C++ standard.
> >> @enddefbuiltin
> >>
> >> +@defbuiltin{@var{type} __builtin_get_counted_by (@var{ptr})}
> >> +The built-in function @code{__builtin_get_counted_by} checks whether the
> >> array
> >> +object pointed by the pointer @var{ptr} has another object associated
> >> with it
> >> +that represents the number of elements in the array object through the
> >> +@code{counted_by} attribute (i.e. the counted-by object). If so, returns a
> >> +pointer to the corresponding counted-by object.
> >> +If such counted-by object does not exist, returns a NULL pointer.
> >> +
> >> +This built-in function is only available in C for now.
> >> +
> >> +The argument @var{ptr} must be a pointer to an array.
> >> +The @var{type} of the returned value must be a pointer type pointing to
> >> the
> >> +corresponding type of the counted-by object or a pointer type pointing to
> >> +the @var{size_t} in case of a NULL pointer being returned.
> >> +
> >> +For example:
> >> +
> >> +@smallexample
> >> +struct foo1 @{
> >> + int counter;
> >> + struct bar1 array[] __attribute__((counted_by (counter)));
> >> +@} *p;
> >> +
> >> +struct foo2 @{
> >> + int other;
> >> + struct bar2 array[];
> >> +@} *q;
> >> +@end smallexample
> >> +
> >> +@noindent
> >> +the following call to the built-in
> >> +
> >> +@smallexample
> >> +__builtin_get_counted_by (p->array)
> >> +@end smallexample
> >> +
> >> +@noindent
> >> +returns:
> >> +
> >> +@smallexample
> >> +&p->counter with type @code{int *}
> >> +@end smallexample
> >> +
> >> +@noindent
> >> +However, the following call to the built-in
> >> +
> >> +@smallexample
> >> +__builtin_get_counted_by (q->array)
> >> +@end smallexample
> >> +
> >> +@noindent
> >> +returns a NULL pointer with type @code{size_t *}.
> >> +
> >> +@enddefbuiltin
> >> +
> >> @defbuiltin{void __builtin_clear_padding (@var{ptr})}
> >> The built-in function @code{__builtin_clear_padding} function clears
> >> padding bits inside of the object representation of object pointed by
> >> diff --git a/gcc/testsuite/gcc.dg/builtin-get-counted-by-1.c
> >> b/gcc/testsuite/gcc.dg/builtin-get-counted-by-1.c
> >> new file mode 100644
> >> index 00000000000..fb195f03970
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.dg/builtin-get-counted-by-1.c
> >> @@ -0,0 +1,91 @@
> >> +/* Test the code generation for the new __builtin_get_counted_by. */
> >> +/* { dg-do run } */
> >> +/* { dg-options "-O2" } */
> >> +#include <stdio.h>
> >> +
> >> +struct annotated {
> >> + char b;
> >> + int c[] __attribute ((counted_by (b)));
> >> +} *array_annotated;
> >> +
> >> +struct flex {
> >> + short b;
> >> + int c[];
> >> +} *array_flex;
> >> +
> >> +struct nested_annotated {
> >> + struct {
> >> + union {
> >> + int b;
> >> + float f;
> >> + };
> >> + int n;
> >> + };
> >> + char c[] __attribute__ ((counted_by (b)));
> >> +} *array_nested_annotated;
> >> +
> >> +struct nested_flex {
> >> + struct {
> >> + union {
> >> + unsigned int b;
> >> + float f;
> >> + };
> >> + int n;
> >> + };
> >> + char c[];
> >> +} *array_nested_flex;
> >> +
> >> +#define MY_ALLOC(P, FAM, COUNT) ({ \
> >> + typeof(P) __p; \
> >> + size_t __size = sizeof(*P) + sizeof(*P->FAM) * COUNT; \
> >> + __p = (typeof(P)) __builtin_malloc(__size); \
> >> + __builtin_memset(__p, 0, __size); \
> >> + if (__builtin_get_counted_by (__p->FAM)) \
> >> + *(__builtin_get_counted_by(__p->FAM)) = COUNT; \
> >> + P = __p; \
> >> +})
> >> +
> >> +int count;
> >> +
> >> +int main(int argc, char *argv[])
> >> +{
> >> + MY_ALLOC(array_annotated, c, 10);
> >> + if (array_annotated->b != 10)
> >> + __builtin_abort ();
> >> +
> >> + MY_ALLOC(array_flex, c, 20);
> >> + if (array_flex->b == 20)
> >> + __builtin_abort ();
> >> +
> >> + MY_ALLOC(array_nested_annotated, c, 30);
> >> + if (array_nested_annotated->b != 30)
> >> + __builtin_abort ();
> >> +
> >> + MY_ALLOC(array_nested_flex, c, 40);
> >> + if (array_nested_flex->b == 40)
> >> + __builtin_abort ();
> >> +
> >> + count = array_annotated->b * 2 + array_nested_annotated->b * 3;
> >> + struct annotated * annotated_p;
> >> + struct flex * flex_p;
> >> + struct nested_annotated * nested_annotated_p;
> >> + struct nested_flex * nested_flex_p;
> >> +
> >> + MY_ALLOC(annotated_p, c, count);
> >> + if (annotated_p->b != count)
> >> + __builtin_abort ();
> >> +
> >> + MY_ALLOC(flex_p, c, count * 2);
> >> + if (flex_p->b == count * 2)
> >> + __builtin_abort ();
> >> +
> >> + MY_ALLOC(nested_annotated_p, c, count * 3);
> >> + if (nested_annotated_p->b != count * 3)
> >> + __builtin_abort ();
> >> +
> >> + MY_ALLOC(nested_flex_p, c, count * 4);
> >> + if (nested_flex_p->b == count * 4)
> >> + __builtin_abort ();
> >> +
> >> + return 0;
> >> +}
> >> diff --git a/gcc/testsuite/gcc.dg/builtin-get-counted-by.c
> >> b/gcc/testsuite/gcc.dg/builtin-get-counted-by.c
> >> new file mode 100644
> >> index 00000000000..5eca12bc992
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.dg/builtin-get-counted-by.c
> >> @@ -0,0 +1,54 @@
> >> +/* Testing the correct usage of the new __builtin_get_counted_by. */
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-O" } */
> >> +
> >> +#include <stdio.h>
> >> +
> >> +struct annotated {
> >> + size_t b;
> >> + int other;
> >> + int c[] __attribute ((counted_by (b)));
> >> +} *array_annotated;
> >> +
> >> +struct flex {
> >> + size_t b;
> >> + int other;
> >> + int c[];
> >> +} *array_flex;
> >> +
> >> +#define MY_ALLOC(P, FAM, COUNT) ({ \
> >> + typeof(P) __p; \
> >> + size_t __size = sizeof(*P) + sizeof(*P->FAM) * COUNT; \
> >> + __p = (typeof(P)) __builtin_malloc(__size); \
> >> + if (__builtin_get_counted_by (__p->FAM)) \
> >> + *(__builtin_get_counted_by(__p->FAM)) = COUNT; \
> >> + __p; \
> >> +})
> >> +
> >> +extern char c_count;
> >> +extern short s_count;
> >> +extern int i_count;
> >> +extern long l_count;
> >> +extern float f_count;
> >> +
> >> +extern int * foo ();
> >> +
> >> +int main(int argc, char *argv[])
> >> +{
> >> + /* The good usages. */
> >> + MY_ALLOC(array_annotated, c, 10);
> >> + MY_ALLOC(array_flex, c, 20);
> >> + MY_ALLOC(array_annotated, c, c_count);
> >> + MY_ALLOC(array_flex, c, i_count);
> >> + MY_ALLOC(array_annotated, c, l_count);
> >> + MY_ALLOC(array_flex, c, c_count * 3);
> >> + MY_ALLOC(array_annotated, c, l_count * i_count);
> >> +
> >> + /* The bad usages, issue errors. */
> >> + __builtin_get_counted_by (); /* { dg-error "wrong number of arguments
> >> to" } */
> >> + __builtin_get_counted_by (array_annotated->c, 10); /* { dg-error "wrong
> >> number of arguments to" } */
> >> + __builtin_get_counted_by (array_annotated->other); /* { dg-error "the
> >> argument must be an array" } */
> >> + __builtin_get_counted_by (foo()); /* { dg-error "the argument must be
> >> an array" } */
> >> +
> >> + return 0;
> >> +}
> >> --
> >> 2.31.1
>
>
