Thank you, Sid!
I just sent the 6th version of the patch based on all your suggestions and
comments.
(Also fixed the typo in doc/extend.texi per Joseph’s comments)
Could you lease take a look at it (patch #2 includes all the changes per your
suggestions)
and let me know whether it’s good to go?
(Patch #1 and #3 have been approved by Joseph with the typo fix in
doc/extend.texi)
thanks.
Qing
> On Jun 21, 2025, at 07:54, Siddhesh Poyarekar <siddh...@gotplt.org> wrote:
>
> On 2025-06-20 11:26, Qing Zhao wrote:
>> 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.
>
> Thanks for the clarification.
>
>>>
>>> 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 don't, it was just a theoretical possibility I came up with while looking
> at your patch.
>
>>> 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.
>
> Thank you!
>
> Sid
