> On Apr 24, 2025, at 11:59, Martin Uecker <uec...@tugraz.at> wrote:
>
> Am Donnerstag, dem 24.04.2025 um 15:15 +0000 schrieb Qing Zhao:
>> Hi,
>>
>> Kees reported a segmentation failure when he used the patch to compiler
>> kernel,
>> and the reduced the testing case is something like the following:
>>
>> struct f {
>> void *g __attribute__((__counted_by__(h)));
>> long h;
>> };
>>
>> extern struct f *my_alloc (int);
>>
>> int i(void) {
>> struct f *iov = my_alloc (10);
>> int *j = (int *)iov->g;
>> return __builtin_dynamic_object_size(iov->g, 0);
>> }
>>
>> Basically, the problem is relating to the pointee type of the pointer array
>> being “void”,
>> As a result, the element size of the array is not available in the IR.
>> Therefore segmentation
>> fault when calculating the size of the whole object.
>>
>> Although it’s easy to fix this segmentation failure, I am not quite sure
>> what’s the best
>> solution to this issue:
>>
>> 1. Reject such usage of “counted_by” in the very beginning by reporting
>> warning to the
>> User, and delete the counted_by attribute from the field.
>>
>> Or:
>>
>> 2. Accept such usage, but issue warnings when calculating the object_size in
>> Middle-end.
>>
>> Personally, I prefer the above 1 since I think that when the pointee type is
>> void, we don’t know
>> The type of the element of the pointer array, there is no way to decide the
>> size of the pointer array.
>>
>> So, the counted_by information is not useful for the
>> __builtin_dynamic_object_size.
>>
>> But I am not sure whether the counted_by still can be used for bound
>> sanitizer?
>>
>> Thanks for suggestions and help.
>
> GNU C allows pointer arithmetic and sizeof on void pointers and
> that treats void as having size 1. So you could also allow counted_by
> and assume as size 1 for void.
>
> https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html
Okay, thanks for the info.
So,
1. should we issue warnings when doing this?
2. If the compilation option is explicitly asking for standard C,
shall we issue warning and delete the counted_by attribute from the field?
Qing
>
>
> Martin
>
>>
>> Qing
>>
>>
>>
>>
>>> On Apr 23, 2025, at 15:45, Qing Zhao <qing.z...@oracle.com> wrote:
>>>
>>> Hi,
>>>
>>> This is the 2nd version of the patch set to extend "counted_by" attribute
>>> to pointer fields of structures.
>>>
>>> the first version was submitted 3 months ago on 1/16/2025, and triggered
>>> a lot of discussion on whether we need a new syntax for counted_by
>>> attribute.
>>>
>>> https://gcc.gnu.org/pipermail/gcc-patches/2025-January/673837.html
>>>
>>> After a long discussion since then:
>>> (https://gcc.gnu.org/pipermail/gcc-patches/2025-March/677024.html)
>>>
>>> We agreed to the following compromised solution:
>>>
>>> 1. Keep the current syntax of counted_by for lone identifier;
>>> 2. Add a new attribute "counted_by_exp" for expressions.
>>>
>>> Although there are still some discussion going on for the new
>>> counted_by_exp attribute (In Clang community)
>>> https://discourse.llvm.org/t/rfc-bounds-safety-in-c-syntax-compatibility-with-gcc/85885
>>>
>>> The syntax for the lone identifier is kept the same as before.
>>>
>>> So, I'd like to resubmit my previous patch of extending "counted_by"
>>> to pointer fields of structures.
>>>
>>> The whole patch set has been rebased on the latest trunk, some testing case
>>> adjustment, bootstrapped and regression tested on both aarch64 and x86.
>>>
>>> There will be a seperate patch set for the new "counted_by_exp"
>>> attribute later to cover the expressions cases.
>>>
>>> The following are more details on this patch set:
>>>
>>> For example:
>>>
>>> struct PP {
>>> size_t count2;
>>> char other1;
>>> char *array2 __attribute__ ((counted_by (count2)));
>>> int other2;
>>> } *pp;
>>>
>>> specifies that the "array2" is an array that is pointed by the
>>> pointer field, and its number of elements is given by the field
>>> "count2" in the same structure.
>>>
>>> There are the following importand facts about "counted_by" on pointer
>>> fields compared to the "counted_by" on FAM fields:
>>>
>>> 1. one more new requirement for pointer fields with "counted_by" attribute:
>>> pp->array2 and pp->count2 can ONLY be changed by changing the whole
>>> structure
>>> at the same time.
>>>
>>> 2. the following feature for FAM field with "counted_by" attribute is NOT
>>> valid for the pointer field any more:
>>>
>>> " One important feature of the attribute is, a reference to the
>>> flexible array member field uses the latest value assigned to the
>>> field that represents the number of the elements before that
>>> reference. For example,
>>>
>>> p->count = val1;
>>> p->array[20] = 0; // ref1 to p->array
>>> p->count = val2;
>>> p->array[30] = 0; // ref2 to p->array
>>>
>>> in the above, 'ref1' uses 'val1' as the number of the elements in
>>> 'p->array', and 'ref2' uses 'val2' as the number of elements in
>>> 'p->array'. "
>>>
>>> This patch set includes 3 parts:
>>>
>>> 1.Extend "counted_by" attribute to pointer fields of structures.
>>> 2.Convert a pointer reference with counted_by attribute to .ACCESS_WITH_SIZE
>>> and use it in builtinin-object-size.
>>> 3.Use the counted_by attribute of pointers in array bound checker.
>>>
>>> In which, the patch 1 and 2 are simple and straightforward, however, the
>>> patch 3
>>> is a little complicate due to the following reason:
>>>
>>> Current array bound checker only instruments ARRAY_REF, and the INDEX
>>> information is the 2nd operand of the ARRAY_REF.
>>>
>>> When extending the array bound checker to pointer references with
>>> counted_by attributes, the hardest part is to get the INDEX of the
>>> corresponding array ref from the offset computation expression of
>>> the pointer ref.
>>>
>>> I do need some careful review on the 3rd part of the patch. And I do wonder
>>> for the access to pointer arrays:
>>>
>>> struct annotated {
>>> int b;
>>> int *c __attribute__ ((counted_by (b)));
>>> } *p_array_annotated;
>>>
>>> p_array_annotated->c[annotated_index] = 2;
>>>
>>> Is it possible to generate ARRAY_REF instead of INDIRECT_REF for the above
>>> p_array_annotated->c[annotated_index]
>>> in C FE? then we can keep the INDEX info in the IR and avoid all the hacks
>>> to get the index from the OFFSET computation expression.
>>>
>>> The whole patch set has been rebased on the latest trunk, bootstrapped
>>> and regression tested on both aarch64 and x86.
>>>
>>> Let me know any comments and suggestions.
>>>
>>> Thanks.
>>>
>>> Qing
>>
>
> --
> Univ.-Prof. Dr. rer. nat. Martin Uecker
> Graz University of Technology
> Institute of Biomedical Imaging
