> On Apr 24, 2025, at 15:43, Bill Wendling <isanb...@gmail.com> wrote:
>
> On Thu, Apr 24, 2025 at 8:15 AM Qing Zhao <qing.z...@oracle.com> wrote:
>>
>> 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.
>>
> Clang supports __sized_by that can handle a 'void *', where it defaults to
> 'u8'.
Thanks for the info.
Qing
>
> -bw
>
>> 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
