> On Jan 22, 2024, at 2:40 AM, Richard Biener <richard.guent...@gmail.com>
> wrote:
>
> On Fri, Jan 19, 2024 at 5:26 PM Qing Zhao <qing.z...@oracle.com> wrote:
>>
>>
>>
>>> On Jan 19, 2024, at 4:30 AM, Richard Biener <richard.guent...@gmail.com>
>>> wrote:
>>>
>>> On Thu, Jan 18, 2024 at 3:46 PM Qing Zhao <qing.z...@oracle.com> wrote:
>>>>
>>>>
>>>>
>>>>> On Jan 17, 2024, at 1:43 AM, Richard Biener <richard.guent...@gmail.com>
>>>>> wrote:
>>>>>
>>>>> On Wed, Jan 17, 2024 at 7:42 AM Richard Biener
>>>>> <richard.guent...@gmail.com> wrote:
>>>>>>
>>>>>> On Tue, Jan 16, 2024 at 9:26 PM Qing Zhao <qing.z...@oracle.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> On Jan 15, 2024, at 4:31 AM, Richard Biener
>>>>>>>> <richard.guent...@gmail.com> wrote:
>>>>>>>>
>>>>>>>>> All my questions for unshare_expr relate to a LTO bug that I
>>>>>>>>> currently stuck with
>>>>>>>>> when using .ACCESS_WITH_SIZE in bound sanitizer (only with -flto,
>>>>>>>>> without -flto, no issue):
>>>>>>>>>
>>>>>>>>> [opc@qinzhao-aarch64-ol8 gcc]$ sh t
>>>>>>>>> during IPA pass: modref
>>>>>>>>> t.c:20:1: internal compiler error: tree code ‘ssa_name’ is not
>>>>>>>>> supported in LTO streams
>>>>>>>>> 0x14c3993 lto_write_tree
>>>>>>>>> ../../latest-gcc-write/gcc/lto-streamer-out.cc:561
>>>>>>>>> 0x14c3aeb lto_output_tree_1
>>>>>>>>>
>>>>>>>>> And the value of the tree node that triggered the ICE is:
>>>>>>>>> (gdb) call debug_tree(expr)
>>>>>>>>> <ssa_name 0xfffff5761e60 type <error_mark 0xfffff56c0e58>
>>>>>>>>> nothrow
>>>>>>>>> def_stmt
>>>>>>>>> version:13 in-free-list>
>>>>>>>>>
>>>>>>>>> Is there any good way to debug LTO bug?
>>>>>>>>
>>>>>>>> This happens usually when you have a VLA type and its type fields are
>>>>>>>> not
>>>>>>>> properly gimplified which usually happens because the frontend fails to
>>>>>>>> insert a gimplification point for it (a DECL_EXPR).
>>>>>>>
>>>>>>> I found an old gcc bug
>>>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97172
>>>>>>> ICE: tree code ‘ssa_name’ is not supported in LTO streams since
>>>>>>> r11-3303-g6450f07388f9fe57
>>>>>>>
>>>>>>> Which is very similar to the bug I am having right now.
>>>>>>>
>>>>>>> After further study, I suspect that the issue I am having right now
>>>>>>> with the LTO streaming also
>>>>>>> relate to “unshare_expr”, “save_expr”, and the combination of these
>>>>>>> two, I suspect that
>>>>>>> the current gcc cannot handle the combination of these two correctly
>>>>>>> for my case.
>>>>>>>
>>>>>>> My testing case is:
>>>>>>>
>>>>>>> #include <stdlib.h>
>>>>>>> void __attribute__((__noinline__)) setup_and_test_vla (int n1, int n2,
>>>>>>> int m)
>>>>>>> {
>>>>>>> struct foo {
>>>>>>> int n;
>>>>>>> int p[][n2][n1] __attribute__((counted_by(n)));
>>>>>>> } *f;
>>>>>>>
>>>>>>> f = (struct foo *) malloc (sizeof(struct foo) + m*sizeof(int[n2][n1]));
>>>>>>> f->n = m;
>>>>>>> f->p[m][n2][n1]=1;
>>>>>>> return;
>>>>>>> }
>>>>>>>
>>>>>>> int main(int argc, char *argv[])
>>>>>>> {
>>>>>>> setup_and_test_vla (10, 11, 20);
>>>>>>> return 0;
>>>>>>> }
>>>>>>>
>>>>>>> Failed with
>>>>>>> my_gcc -Os -fsanitize=bounds -flto
>>>>>>>
>>>>>>> If changing either n1 or n2 to a constant, the testing passed.
>>>>>>> If deleting -flto, the testing passed too.
>>>>>>>
>>>>>>> I double checked my code per the suggestions provided by you and Jakub
>>>>>>> in this
>>>>>>> email thread, and I think the code should be fine.
>>>>>>>
>>>>>>> The code is following:
>>>>>>>
>>>>>>> =====
>>>>>>> 504 /* Instrument array bounds for INDIRECT_REFs whose pointers are
>>>>>>> 505 POINTER_PLUS_EXPRs of calls to .ACCESS_WITH_SIZE. We create
>>>>>>> special
>>>>>>> 506 builtins that gets expanded in the sanopt pass, and make an array
>>>>>>> 507 dimension of it. ARRAY is the pointer to the base of the array,
>>>>>>> 508 which is a call to .ACCESS_WITH_SIZE, *OFFSET is the offset to
>>>>>>> the
>>>>>>> 509 beginning of array.
>>>>>>> 510 Return NULL_TREE if no instrumentation is emitted. */
>>>>>>> 511
>>>>>>> 512 tree
>>>>>>> 513 ubsan_instrument_bounds_indirect_ref (location_t loc, tree array,
>>>>>>> tree *offset)
>>>>>>> 514 {
>>>>>>> 515 if (!is_access_with_size_p (array))
>>>>>>> 516 return NULL_TREE;
>>>>>>> 517 tree bound = get_bound_from_access_with_size (array);
>>>>>>> 518 /* The type of the call to .ACCESS_WITH_SIZE is a pointer type to
>>>>>>> 519 the element of the array. */
>>>>>>> 520 tree element_size = TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE
>>>>>>> (array)));
>>>>>>> 521 gcc_assert (bound);
>>>>>>> 522
>>>>>>> 523 /* Given the offset, and the size of each element, the index can
>>>>>>> be
>>>>>>> 524 computed as: offset/element_size. */
>>>>>>> 525 *offset = save_expr (*offset);
>>>>>>> 526 tree index = fold_build2 (EXACT_DIV_EXPR,
>>>>>>> 527 sizetype, *offset,
>>>>>>> 528 unshare_expr (element_size));
>>>>>>> 529 /* Create a "(T *) 0" tree node to describe the original array
>>>>>>> type.
>>>>>>> 530 We get the original array type from the first argument of the
>>>>>>> call to
>>>>>>> 531 .ACCESS_WITH_SIZE (REF, COUNTED_BY_REF, 1, num_bytes, -1).
>>>>>>> 532
>>>>>>> 533 Originally, REF is a COMPONENT_REF with the original array
>>>>>>> type,
>>>>>>> 534 it was converted to a pointer to an ADDR_EXPR, and the
>>>>>>> ADDR_EXPR's
>>>>>>> 535 first operand is the original COMPONENT_REF. */
>>>>>>> 536 tree ref = CALL_EXPR_ARG (array, 0);
>>>>>>> 537 tree array_type
>>>>>>> 538 = unshare_expr (TREE_TYPE (TREE_OPERAND (TREE_OPERAND(ref, 0),
>>>>>>> 0)));
>>>>>>> 539 tree zero_with_type = build_int_cst (build_pointer_type
>>>>>>> (array_type), 0);
>>>>>>> 540 return build_call_expr_internal_loc (loc, IFN_UBSAN_BOUNDS,
>>>>>>> 541 void_type_node, 3,
>>>>>>> zero_with_type,
>>>>>>> 542 index, bound);
>>>>>>> 543 }
>>>>>>>
>>>>>>> =====
>>>>>>>
>>>>>>> Inside gdb, the guilty IR failed in LTO streaming is from the above
>>>>>>> line 520:
>>>>>>> TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (array))),
>>>>>>>
>>>>>>> When I use this tree node as an operand of the expression at line 526,
>>>>>>> I added
>>>>>>> unshare_expr.
>>>>>>>
>>>>>>> However, I still see the guilty IR as in gdb:
>>>>>>>
>>>>>>> unit-size <mult_expr 0xfffff5aabf90 type <integer_type
>>>>>>> 0xfffff57c0000 sizetype>
>>>>>>> side-effects
>>>>>>> arg:0 <mult_expr 0xfffff5aabf68 type <integer_type
>>>>>>> 0xfffff57c0000 sizetype>
>>>>>>>
>>>>>>> arg:0 <ssa_name 0xfffff5761e18 type <error_mark
>>>>>>> 0xfffff56c0e58>
>>>>>>> nothrow
>>>>>>> def_stmt
>>>>>>> version:12 in-free-list>
>>>>>>> arg:1 <ssa_name 0xfffff5761e60 type <error_mark
>>>>>>> 0xfffff56c0e58>
>>>>>>> nothrow
>>>>>>> def_stmt
>>>>>>> version:13 in-free-list>>
>>>>>>> arg:1 <integer_cst 0xfffff56c10c8 constant 4>>
>>>>>>>
>>>>>>>
>>>>>>> I have been stuck with this bug for quite some time.
>>>>>>> Any help is helpful.
>>>>>>
>>>>>> The above hasn't been gimplified correctly, you'd instead see
>>>>>> a D.1234 in there, not an expression with SSA names. That happens
>>>>>> when the frontend fails to emit a DECL_EXPR for a decl with this
>>>>>> type.
>>>>>
>>>>> .. which then also results in missing unsharing of this expression
>>>>> (so the SSA names leak in)
>>>>
>>>> Thanks a lot for the hints.
>>>>
>>>> One correction first, the LTO bug is not related to -fsanitize=bounds.
>>>> Deleting -fsanitize=bounds still can
>>>> repeat the failure.
>>>>
>>>> After further debugging into the gimplification phase related with the
>>>> SAVE_EXPR, I finally locate the place
>>>> where the unshareing of the expression is missing. This is in the
>>>> routine “pointer_int_sum” of c-family/c-common.cc:
>>>>
>>>> 3330 {
>>>> 3331 if (!complain && !COMPLETE_TYPE_P (TREE_TYPE (result_type)))
>>>> 3332 return error_mark_node;
>>>> 3333 size_exp = size_in_bytes_loc (loc, TREE_TYPE (result_type));
>>>> 3334 /* Wrap the pointer expression in a SAVE_EXPR to make sure it
>>>> 3335 is evaluated first when the size expression may depend
>>>> 3336 on it for VM types. */
>>>> 3337 if (TREE_SIDE_EFFECTS (size_exp)
>>>> 3338 && TREE_SIDE_EFFECTS (ptrop)
>>>> 3339 && variably_modified_type_p (TREE_TYPE (ptrop), NULL))
>>>> 3340 {
>>>> 3341 ptrop = save_expr (ptrop);
>>>> 3342 size_exp = build2 (COMPOUND_EXPR, TREE_TYPE (intop), ptrop,
>>>> size_exp);
>>>> 3343 }
>>>> 3344 }
>>>>
>>>> In the above, at line 3333, the tree node, TYPE_SIZE_UNIT
>>>> (TREE_TYPE(result_type)), is returned directly as
>>>> the size_exp,
>>>>
>>>> (gdb) call debug_tree(size_exp)
>>>> <mult_expr 0xfffff5a6f910
>>>> type <integer_type 0xfffff57c0000 sizetype public unsigned DI
>>>> size <integer_cst 0xfffff56c0e70 constant 64>
>>>> unit-size <integer_cst 0xfffff56c0e88 constant 8>
>>>> align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
>>>> 0xfffff57c0000 precision:64 min <integer_cst 0xfffff56c0ea0 0> max
>>>> <integer_cst 0xfffff56d05e0 18446744073709551615>>
>>>> side-effects
>>>> arg:0 <mult_expr 0xfffff5a6f8e8 type <integer_type 0xfffff57c0000
>>>> sizetype>
>>>> side-effects
>>>> arg:0 <nop_expr 0xfffff56dc540 type <integer_type 0xfffff57c0000
>>>> sizetype>
>>>> side-effects
>>>> arg:0 <save_expr 0xfffff56dc4c0 type <integer_type
>>>> 0xfffff57c05e8 int>
>>>> side-effects arg:0 <parm_decl 0xfffff76b6f80 n1>>>
>>>> arg:1 <nop_expr 0xfffff56dc600 type <integer_type 0xfffff57c0000
>>>> sizetype>
>>>> side-effects
>>>> arg:0 <save_expr 0xfffff56dc580 type <integer_type
>>>> 0xfffff57c05e8 int>
>>>> side-effects arg:0 <parm_decl 0xfffff76b7000 n2>>>>
>>>> arg:1 <integer_cst 0xfffff56c10c8 type <integer_type 0xfffff57c0000
>>>> sizetype> constant 4>>
>>>>
>>>>
>>>> Without unshare_expr to this size_exp, the above TYPE_SIZE_UNIT node
>>>> containing SAVE_EXPRs
>>>> is gimpflified to expressions with SSA_NAME during gimplification. (This
>>>> is unaccepted by LTO).
>>>>
>>>> Adding an unshare_expr (size_exp) resolved this problem.
>>>>
>>>> Although I still think that there might be potential issue with the
>>>> gimpflication of SAVE_EXPRs, I dare not
>>>> to modify that part of the code.
>>>>
>>>> At this moment, I will add unshare_expr to the routine “pointer_int_sum”
>>>> to workaround this issue.
>>>
>>> It's only a workaround mind you. The bug is that the frontend fails
>>> to emit a DECL_EXPR which would
>>> trigger both unsharing and proper gimplification of the type size.
>>
>> For a simple testing case:
>>
>> $ cat test.c
>> struct annotated {
>> unsigned int foo;
>> char b;
>> int array[] __attribute__((counted_by (foo)));
>> };
>> extern struct annotated * alloc_buf (int index);
>>
>> static void bar ()
>> {
>> struct annotated *p2 = alloc_buf (10);
>> p2->array[11] = 0;
>> return;
>> }
>>
>> The C FE generates the following IR:
>>
>> [opc@qinzhao-ol8u3-x86 108896]$ cat test.c.005t.original
>> ;; Function bar (null)
>> ;; enabled by -tree-original
>>
>>
>> {
>> struct annotated * p2 = alloc_buf (10);
>>
>> struct annotated * p2 = alloc_buf (10);
>> *(.ACCESS_WITH_SIZE ((int *) &p2->array, &p2->foo, 1, 32, -1) + 44) = 0;
>> return;
>> }
>>
>> Do you see any obvious IR issue in the above? Do I miss to generate any
>> DECL_EXPR in the above IR?
>
> It's an interesting question - I don't see where the gimplifier would
> need to access DECL/TYPE_SIZE
My bad, the above simple example did not expose the LTO error. Just used to
show the IR generated for the array access.
The LTO error only happens with multi-dimension array, whose inner dimensions
are VLA. And the outermost dimension is flexible array member. Like following:
void __attribute__((__noinline__)) setup_and_test_vla (int n1, int n2, int m)
{
struct foo {
int n;
int p[][n2][n1] __attribute__((counted_by(n)));
} *f;
f = (struct foo *) malloc (sizeof(struct foo) + m*sizeof(int[n2][n1]));
f->n = m;
f->p[m][n2][n1]=1;
return;
}
And the IR for the routine “setup_and_test_vla” is:
{
typedef struct foo struct struct foo;
struct foo * f;
SAVE_EXPR <n1>;, SAVE_EXPR <n2>;;, {
typedef struct foo struct struct foo;
};
struct foo * f;
f = (struct foo *) malloc (0, SAVE_EXPR <n1>;, SAVE_EXPR <n2>;;, ((long
unsigned int) m * (long unsigned int) ((sizetype) SAVE_EXPR <n1> * (sizetype)
SAVE_EXPR <n2>) + 1) * 4;);
f->n = m;
(*(.ACCESS_WITH_SIZE ((int[0:(sizetype) ((long int) SAVE_EXPR <n2> +
-1)][0:(sizetype) ((long int) SAVE_EXPR <n1> + -1)] *) &f->p, &f->n, 1, 32, -1)
+ (sizetype) (((long unsigned int) m * (long unsigned int) ((sizetype)
SAVE_EXPR <n1> * (sizetype) SAVE_EXPR <n2>)) * 4)))[n2][n1] = 1;
return;
}
> so the mistake, if any, should be that you need to unshare the size
> expressions you are using as
> argument to .ACCESS_WITH_SIZE?
Hm, I tried that in the beginning, but didn’t work. I will check again.
> Mind, you are replacing an ARRAY_REF
> with a pointer indirection
> as well
Yes, this is for resolving a very early gimplification issue as I reported last
Nov:
https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638793.html
Since no-one responded at that time, I fixed the issue by replacing the
ARRAY_REF
With a pointer indirection:
https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639605.html
The reason for such change is: return a flexible array member TYPE is not
allowed
by C language (our gimplification follows this rule), so, we have to return a
pointer TYPE instead.
> - IMO we shouldn't replace accesses this way but instead make
> it possible for analysis to
> discover the base/size values?
Yes, if keeping the ARRAY_REF, then bound sanitizer instrumentation will be
much simpler since the INDEX
Information is kept in the TYPE NODE of the ARRAY_REF.
However, due to the above reason, we have to replace the ARRAY_REF with a
pointer INDIRECT_REF.
Such change made the bound sanitizer more difficult due to the INDEX was lost
when the ARRAY_REF was
Converted to the INDIRECT_REF.
I resolved this issue by adding a new argument to .ACCESS_WITH_SIZE to record
the INDEX when converting
The ARRAY_REF to INDIRECT_REF during C FE.
Let me know if you have any comment.
Thanks.
Qing
>
>> Thanks.
>>
>> Qing
>>
>>
>> I compared it with the following testing case without the “counted-by”
>> annotation
>> and use an user-defined “access_with_size” function, The IR looks like
>> exactly the same:
>>
>> $ cat test_1.c
>> struct annotated {
>> unsigned int foo;
>> char b;
>> int array[];
>> };
>> extern struct annotated *alloc_buf (int);
>> extern int *access_with_size (int * ref, unsigned int * size, int a, int b,
>> int c);
>>
>> static void bar ()
>> {
>> struct annotated *p2 = alloc_buf (10);
>> access_with_size ((int *) &p2->array, &p2->foo, 1, 32, -1)[11] = 0;
>> return;
>> }
>> [opc@qinzhao-ol8u3-x86 108896]$ cat test_1.c.005t.original
>>
>> ;; Function bar (null)
>> ;; enabled by -tree-original
>>
>>
>> {
>> struct annotated * p2 = alloc_buf (10);
>>
>> struct annotated * p2 = alloc_buf (10);
>> *(access_with_size ((int *) &p2->array, &p2->foo, 1, 32, -1) + 44) = 0;
>> return;
>> }
>>
>>
>>
>>>
>>>> Let me know if you have any comment and suggestion.
>>>>
>>>> Thanks a lot.
>>>>
>>>> Qing
>>>>
>>>>>>
>>>>>>>
>>>>>>> Qing
>>>>>>>
>>>>>>>>
>>>>>>>>> Thanks a lot for the help.
>>>>>>>>>
>>>>>>>>> Qing
