On Mon, Jul 14, 2025 at 10:58 PM Qing Zhao <qing.z...@oracle.com> wrote:
>
>
> > On Jul 7, 2025, at 13:07, Qing Zhao <qing.z...@oracle.com> wrote:
> >
> > As I mentioned in the latest email I replied to the thread, the original
> > implementation of the counted_by for pointer was implemented without the
> > additional indirection.
> > But that implementation has a fundamental bug during testing. then I
> > changed the implementation like the current.
> >
> > I need spending a little more time to find the details of that fundamental
> > bug with the original implementation.
> >
> > If the current bug is urgent to be fixed. and you are not comfortable with
> > the simple Patch Sid provided, then I am okay to back it out now and then
> > push it back with the fix to this current bug at a later time after
> > everyone is comfortable with the current implementation.
> >
> > Thanks a lot!
> >
> > Qing
>
>
> Hi, this is an update on the above fundamental issue I mentioned previously.
> (I finally located this issue and recorded it here)
>
> 1. Based on the previous discussion on how to resolve PR120929, we agreed the
> following solution:
>
> struct S {
> int n;
> int *p __attribute__((counted_by(n)));
> } *f;
>
> when generating a call to .ACCESS_WITH_SIZE for f->p, instead of generating
> *.ACCESS_WITH_SIZE (&f->p, &f->n,...)
>
> We should generate
> .ACCESS_WITH_SIZE (f->p, &f->n,...)
>
> i.e.,
> the return type and the type of the first argument of the call is the
> original pointer type in this version,
> instead of the pointer to the original pointer type in the 7th version;
>
> 2. I implemented this new .ACCESS_WITH_SIZE generation for pointers in my
> local workspace. It looked fine in the beginning,
> However, during testing, I finally located the _fundamental issue_ with this
> design.
>
> This issue can be shown clearly with the following simple testing case:
> (Note, the numbers on the left in the following testing case is the line #)
>
> $ cat t1.c
> 1 struct annotated {
> 2 int b;
> 3 int *c __attribute__ ((counted_by (b)));
> 4 } *p_array_annotated;
> 5
> 6 void __attribute__((__noinline__)) setup (int annotated_count)
> 7 {
> 8 p_array_annotated
> 9 = (struct annotated *) __builtin_malloc (sizeof (struct annotated));
> 10 p_array_annotated->c = (int *) __builtin_malloc (annotated_count *
> sizeof (int));
> 11 p_array_annotated->c[2] = 10;
> 12 p_array_annotated->b = annotated_count;
But isn't this bogus since you access c[2] while it's counted_by value
is still uninitialized?
I'd say by using counted_by you now invoke UB here.
Richard.
> 13 return;
> 14 }
> 15
> 16 int main(int argc, char *argv[])
> 17 {
> 18 setup (10);
> 19 return 0;
> 20 }
>
> $my-gcc t1.c -O0 -g -o ./t1.exe -fdump-tree-gimple
> $ ./t1.exe
> Segmentation fault (core dumped)
>
> 3. As I debugged, the segmentation fault happened at line 11:
> p_array_annotated->c[2] = 10;
> Since the value of the pointer "p_array_annotated->c” is 0x0.
>
> 4. Study the gimple dump t1.c.007t.gimple as following:
>
> 1 __attribute__((noinline))
> 2 void setup (int annotated_count)
> 3 {
> 4 int * D.2969;
> 5 6 _1 = __builtin_malloc (16);
> 7 p_array_annotated = _1;
> 8 _2 = (long unsigned int) annotated_count;
> 9 _3 = _2 * 4;
> 10 p_array_annotated.0_4 = p_array_annotated;
> 11 _5 = p_array_annotated.0_4->c;
> 12 p_array_annotated.1_6 = p_array_annotated;
> 13 _7 = &p_array_annotated.1_6->b;
> 14 D.2969 = .ACCESS_WITH_SIZE (_5, _7, 0B, 4);
> 15 _8 = __builtin_malloc (_3);
> 16 D.2969 = _8;
> 17 p_array_annotated.2_10 = p_array_annotated;
> 18 _11 = p_array_annotated.2_10->c;
> 19 p_array_annotated.3_12 = p_array_annotated;
> 20 _13 = &p_array_annotated.3_12->b;
> 21 _9 = .ACCESS_WITH_SIZE (_11, _13, 0B, 4);
> 22 _14 = _9 + 8;
> 23 *_14 = 10;
> 24 p_array_annotated.4_15 = p_array_annotated;
> 25 p_array_annotated.4_15->b = annotated_count;
> 26 return;
> 27 }
>
> We can see the root cause of this problem is because we passed the _value_ of
> “p_array_annotated->c”
> instead of the _address_ of “p_array_annotated->c” to .ACCESS_WITH_SIZE:
>
> At line 11, the value of “p_array_annotated.0_4->c” is 0x0 when it was
> assigned to “_5”;
> At line 14, the value of “_5” is passed to the call to .ACCESS_WITH_SIZE,
> which also is 0x0;
>
> And later when we expand .ACCESS_WITH_SIZE (_5, _7, 0B, 4), we replace it
> with its first argument “_5”,
> As a result, the IL after the expand will look like the following:
>
> 11 _5 = p_array_annotated.0_4->c;
> 14 D.2969 = _5;
> 15 _8 = __builtin_malloc (_3);
> 16 D.2969 = _8:
>
> We can clearly see that the above IL is wrong: p_array_annotated->c is
> initialized as 0x0, and this value is passed
> to the pointer D.2969, which is 0x0 too.
>
> 5. This is exactly the fundamental issue I met in the very beginning during
> my implementation of the counted_by for
> pointers. I should record this issue at that time and sent email to the alias
> at that time.
>
> 6. Based on this issue, I changed the .ACCESS_WITH_SIZE for pointers to
>
> *.ACCESS_WITH_SIZE (&f->p, &f->n,…)
>
> at that time.
>
> And this resolved this fundamental issue.
>
> In a summary:
>
> 1. We still need to keep the design of .ACCESS_WITH_SIZE for pointers as in
> patch version #7, i.e:
>
> *.ACCESS_WITH_SIZE (&f->p, &f->n,…)
>
> 2. As a side effect of the above IL , the change in gcc/tree-object-size.cc
> <http://tree-object-size.cc/> in patch version #7 is still needed:
>
> @@ -1854,6 +1856,17 @@ 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);
> + /* Handle the following stmt #2 to propagate the size from the
> + stmt #1 to #3:
> + 1 _1 = .ACCESS_WITH_SIZE (_3, _4, 1, 0, -1, 0B);
> + 2 _5 = *_1;
> + 3 _6 = __builtin_dynamic_object_size (_5, 1);
> + */
> + else if (TREE_CODE (rhs) == MEM_REF
> + && POINTER_TYPE_P (TREE_TYPE (rhs))
> + && TREE_CODE (TREE_OPERAND (rhs, 0)) == SSA_NAME
> + && integer_zerop (TREE_OPERAND (rhs, 1)))
> + reexamine = merge_object_sizes (osi, var, TREE_OPERAND (rhs,
> 0));
>
> But, we should add one more condition to the above in order to limit such
> propagation ONLY to
> the pointee is .ACCESS_WITH_SIZE, i.e (as Sid proposed)
>
> +/* Return true if PTR is defined with .ACCESS_WITH_SIZE. */
> +
> +static bool
> +is_access_with_size (tree ptr)
> +{
> + gcc_assert (TREE_CODE (ptr) == SSA_NAME);
> +
> + gimple *stmt = SSA_NAME_DEF_STMT (ptr);
> + if (gimple_code (stmt) != GIMPLE_CALL)
> + return false;
> +
> + return gimple_call_internal_p (as_a <gcall *> (stmt),
> IFN_ACCESS_WITH_SIZE);
> +}
> +
>
>
> + /* Handle the following stmt #2 to propagate the size from the
> + stmt #1 to #3:
> + 1 _1 = .ACCESS_WITH_SIZE (_3, _4, 1, 0, -1, 0B);
> + 2 _5 = *_1;
> + 3 _6 = __builtin_dynamic_object_size (_5, 1);
> + */
> + else if (TREE_CODE (rhs) == MEM_REF
> + && POINTER_TYPE_P (TREE_TYPE (rhs))
> + && TREE_CODE (TREE_OPERAND (rhs, 0)) == SSA_NAME
> + && integer_zerop (TREE_OPERAND (rhs, 1))
> + && is_access_with_size (TREE_OPERAND (rhs, 0)))
> + reexamine = merge_object_sizes (osi, var, TREE_OPERAND (rhs,
> 0));
>
> Let me know if you have comments or suggestions to this fundamental issue and
> whether my
> solution to it was reasonable.
>
> Thanks a lot.
>
> Qing
>
>
>
