> On Aug 21, 2024, at 10:34, Martin Uecker <[email protected]> wrote:
>
> Am Mittwoch, dem 21.08.2024 um 14:12 +0000 schrieb Qing Zhao:
>
> ...
>>
>>>
>>>> + 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.
>
> Does it matter?
>
>>> 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.
>>
>> Yes, you are right. When I wrote the testing case, I felt wield too. (:-
>>
>> And another issue with the returned value of __builtin_get_counted_by(PTR)
>> is, since it returns a pointer, the TYPE of the pointee matters, especially
>> when returns a NULL pointer, I used a pointer type pointing to the size_t in
>> case of a NULL pointer being returned to avoid some strict-aliasing issue.
>> please see PR116316, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116316)
>> for details.
>
> It needs to return a pointer to the actual type of the count field.
> For NULL, it probably needs to be size_t* for above assignment
> to work.
Yes, that’s what I did for the patch after understanding PR116316. -:)
The current doc of the returned type is:
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.
>
> But if we changed it to return a void pointer, we could make this
> a compile-time check:
>
> auto ret = __builtin_get_counted_by(__p->FAM);
>
> _Generic(ret, void*: (void)0, default: *ret = COUNT);
Is there any benefit to return a void pointer than a SIZE_T pointer for the
NULL pointer?
>
>
>>
>> Yes, I do feel that the approach __builtin_get_counted_by is not very good.
>> Maybe it’s better to provide
>> A. __builtin_set_counted_by
>> or
>> B. The unary operator __counted_by(PTR) to return a Lvalue, in this case,
>> we need a __builtin_has_attribute first to check whether PTR has the
>> counted_by attribute first.
>
> You could potentially do the same __counted_by and test for type void.
>
> _Generic(typeof(__counted_by(PTR)), void: (void)0, __counted_by(PTR) = COUNT);
Oh, so, is there any benefit for the unary operator __counted_by(PTR) than the
current __builtin_get_counted_by?
Thanks.
Qing
>
> Martin
>
>>
>> Any suggestion?
>>
>> thanks.
>>
>> Qing
>>
>>
>>>
>>> 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
>>
>>
>
> --
> Univ.-Prof. Dr. rer. nat. Martin Uecker
> Graz University of Technology
> Institute of Biomedical Imaging
