> On Jul 15, 2025, at 02:32, Richard Biener <richard.guent...@gmail.com> wrote: > > 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?
Sorry for my mistake in the testing case. If we switch the line 11 and 12 in the above testing case to make the counted_by value is initialized before p_array_annotated->c is used, the same issue. 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->b = annotated_count; 12 p_array_annotated->c[2] = 10; 13 return; 14 } 15 16 int main(int argc, char *argv[]) 17 { 18 setup (10); 19 return 0; 20 } > I'd say by using counted_by you now invoke UB here. The UB is because the way we generate the call to .ACCESS_WITH_SIZE: (for the above updated testing case, the gimple dump is): 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_9 = p_array_annotated; 18 p_array_annotated.2_9->b = annotated_count; 19 p_array_annotated.3_11 = p_array_annotated; 20 _12 = p_array_annotated.3_11->c; 21 p_array_annotated.4_13 = p_array_annotated; 22 _14 = &p_array_annotated.4_13->b; 23 _10 = .ACCESS_WITH_SIZE (_12, _14, 0B, 4); 24 _15 = _10 + 8; 25 *_15 = 10; 26 return; 27 } 28 In the above, for the source code at line 10: 10 p_array_annotated->c = (int *) __builtin_malloc (annotated_count * sizeof (int)); The ILs are: 11 _5 = p_array_annotated.0_4->c; 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; Note, at line 11, the value of the pointer “p_array_annotated->c” is assigned to _5, and this _5 is passed as the first argument to the call to .ACCESS_WITH_SIZE. Later when .ACCESS_WITH_SIZE is expand to its first argument, the IL is: 11 _5 = p_array_annotated.0_4->c; 14 D.2969 = _5; 15 _8 = __builtin_malloc (_3); 16 D.2969 = _8: Since the value of p_array_annotated->c at line 11 is uninitialized, yes, you are right, such implementation bring UB into the program. So, for struct S { int n; int *p __attribute__((counted_by(n))); } *f; When we generate a call to .ACCESS_WITH_SIZE for the above f->p as following (the agreed solution to PR120929) .ACCESS_WITH_SIZE (f->p, &f->n,…) Will bring UB into the program, therefore is not correct. On the other hand, generating the call to .ACCESS_WITH_SIZE as: *.ACCESS_WITH_SIZE (&f->p, &f->n,…) to pass the address of the original pointer should be the correct design as following (As I did in the previous implementation) 9 _6 = &p_array_annotated.0_5->c; 11 _8 = &p_array_annotated.1_7->b; 12 _4 = .ACCESS_WITH_SIZE (_6, _8, 1, 0, -1, 0B); 13 _9 = __builtin_malloc (_3); 14 *_4 = _9; Do I missing anything here? Thanks. Qing > > 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 > > > >