> On Jun 19, 2025, at 12:16, Siddhesh Poyarekar <siddh...@gotplt.org> wrote:
>
> On 2025-06-19 12:07, Siddhesh Poyarekar wrote:
>> On 2025-06-16 18:08, Qing Zhao wrote:
>>> gcc/ChangeLog:
>>>
>>> * tree-object-size.cc (access_with_size_object_size): Handle pointers
>>> with counted_by.
>> This should probably just say "Update comment for .ACCESS_WITH_SIZE.".
>>> (collect_object_sizes_for): Likewise.
>
> Oh, and this should be updated accordingly as well.
Yes, will update.
>
>>> @@ -1854,6 +1856,10 @@ collect_object_sizes_for (struct object_size_info
>>> *osi, tree var)
>>> if (TREE_CODE (rhs) == SSA_NAME
>>> && POINTER_TYPE_P (TREE_TYPE (rhs)))
>>> reexamine = merge_object_sizes (osi, var, rhs);
>>> + else if (TREE_CODE (rhs) == MEM_REF
>>> + && POINTER_TYPE_P (TREE_TYPE (rhs))
>>> + && TREE_CODE (TREE_OPERAND (rhs, 0)) == SSA_NAME)
>>> + reexamine = merge_object_sizes (osi, var, TREE_OPERAND (rhs, 0));
>>> else
>>> expr_object_size (osi, var, rhs);
>>> }
>> Interesting, looks like this would improve coverage for more than just this
>> specific case of pointers within structs. Nice! Is this what
>> pointer-counted-by-7.c covers?
the mentioned code change:
+ else if (TREE_CODE (rhs) == MEM_REF
+ && POINTER_TYPE_P (TREE_TYPE (rhs))
+ && TREE_CODE (TREE_OPERAND (rhs, 0)) == SSA_NAME)
+ reexamine = merge_object_sizes (osi, var, TREE_OPERAND (rhs, 0));
is mainly for the following IR pattern that is common when the object size is
queried
for a pointer with “counted_by” attribute:
_1 = .ACCESS_WITH_SIZE (_3, _4, 1, 0, -1, 0B);
_5 = *_1; ===========> this is the GIMPLE_ASSIGN stmt that the above code
handles.
_6 = __builtin_dynamic_object_size (_5, 1);
Yes, this is a minimal and necessary change for using the counted_by of
pointers in __bdos.
Hope this is clear.
I will add comments to the above change.
>
> Actually, I wonder if this is incomplete. We should be able to get the
> object size even if TREE_OPERAND (rhs, 0) is not an SSA_NAME, like we do in
> addr_object_size:
>
> ```
> if (TREE_CODE (pt_var) == MEM_REF)
> {
> tree sz, wholesize;
>
> if (!osi || (object_size_type & OST_SUBOBJECT) != 0
> || TREE_CODE (TREE_OPERAND (pt_var, 0)) != SSA_NAME)
> {
> compute_builtin_object_size (TREE_OPERAND (pt_var, 0),
> object_size_type & ~OST_SUBOBJECT, &sz);
> wholesize = sz;
> }
> else
> {
> tree var = TREE_OPERAND (pt_var, 0);
> if (osi->pass == 0)
> collect_object_sizes_for (osi, var);
> if (bitmap_bit_p (computed[object_size_type],
> SSA_NAME_VERSION (var)))
> {
> sz = object_sizes_get (osi, SSA_NAME_VERSION (var));
> wholesize = object_sizes_get (osi, SSA_NAME_VERSION (var), true);
> }
> else
> sz = wholesize = size_unknown (object_size_type);
> }
> if (!size_unknown_p (sz, object_size_type))
> sz = size_for_offset (sz, TREE_OPERAND (pt_var, 1), wholesize);
>
> if (!size_unknown_p (sz, object_size_type)
> && (TREE_CODE (sz) != INTEGER_CST
> || compare_tree_int (sz, offset_limit) < 0))
> {
> pt_var_size = sz;
> pt_var_wholesize = wholesize;
> }
> }
> ```
>
> Maybe refactor this to handle MEM_REF and update addr_object_size (and
> collect_object_sizes_for for the gimple_assign_single_p case) to use it?
>
> However, that's a more general change and we'd probably need a new test case
> to validate it.
Yes, I guess that this might be a general improvement to the current __bdos. I
can do it later in a separate patch. Do you have a good testing case for this?
> I won't block on this though, your minimal change is a step forward, maybe
> just add a comment about this?
Yeah, I will add comments to the current change in tree-object-size.cc
<http://tree-object-size.cc/> and also update the testing cases per you
suggested.
Thanks a lot for your review.
Qing
>
> Thanks,
> Sid
