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
so the mistake, if any, should be that you need to unshare the size
expressions you are using as
argument to .ACCESS_WITH_SIZE? Mind, you are replacing an ARRAY_REF
with a pointer indirection
as well - IMO we shouldn't replace accesses this way but instead make
it possible for analysis to
discover the base/size values?
> 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
>
>
