> 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;
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
