> Am 03.07.2025 um 16:11 schrieb Jakub Jelinek <ja...@redhat.com>:
>
> Hi!
>
> The following testcase is miscompiled with -fsanitize=undefined but we
> introduce UB into the IL even without that flag.
>
> The optimization ptr +- (expr +- cst) when expr/cst have undefined
> overflow into (ptr +- cst) +- expr is sometimes simply not valid,
> without careful analysis on what ptr points to we don't know if it
> is valid to do (ptr +- cst) pointer arithmetics.
> E.g. on the testcase, ptr points to start of an array (actually
> conditionally one or another) and cst is -1, so ptr - 1 is invalid
> pointer arithmetics, while ptr + (expr - 1) can be valid if expr
> is at runtime always > 1 and smaller than size of the array ptr points
> to + 1.
>
> Unfortunately, removing this 1992-ish optimization altogether causes
> FAIL: c-c++-common/restrict-2.c -Wc++-compat scan-tree-dump-times lim2
> "Moving statement" 11
> FAIL: gcc.dg/tree-ssa/copy-headers-5.c scan-tree-dump ch2 "is now do-while
> loop"
> FAIL: gcc.dg/tree-ssa/copy-headers-5.c scan-tree-dump-times ch2 " if " 3
> FAIL: gcc.dg/vect/pr57558-2.c scan-tree-dump vect "vectorized 1 loops"
> FAIL: gcc.dg/vect/pr57558-2.c -flto -ffat-lto-objects scan-tree-dump vect
> "vectorized 1 loops"
> regressions (restrict-2.c also for C++ in all std modes). I've been thinking
> about some match.pd optimization for signed integer addition/subtraction of
> constant followed by widening integral conversion followed by multiplication
> or left shift, but that wouldn't help 32-bit arches.
>
> So, instead at least for now, the following patch keeps doing the
> optimization, just doesn't perform it in pointer arithmetics.
> pointer_int_sum itself actually adds the multiplication by size_exp,
> so ptr + expr is turned into ptr p+ expr * size_exp,
> so this patch will try to optimize
> ptr + (expr +- cst)
> into
> ptr p+ ((sizetype)expr * size_exp +- (sizetype)cst * size_exp)
> and
> ptr - (expr +- cst)
> into
> ptr p+ -((sizetype)expr * size_exp +- (sizetype)cst * size_exp)
So the important part is the distribution of the multiplication, not the
(invalid) association?
OK then.
Thanks,
Richard
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2025-07-03 Jakub Jelinek <ja...@redhat.com>
>
> PR c/120837
> * c-common.cc (pointer_int_sum): Rewrite the intop PLUS_EXPR or
> MINUS_EXPR optimization into extension of both intop operands,
> their separate multiplication and then addition/subtraction followed
> by rest of pointer_int_sum handling after the multiplication.
>
> * gcc.dg/ubsan/pr120837.c: New test.
>
> --- gcc/c-family/c-common.cc.jj 2025-07-01 09:36:43.115908270 +0200
> +++ gcc/c-family/c-common.cc 2025-07-03 12:31:12.789367448 +0200
> @@ -3438,20 +3438,41 @@ pointer_int_sum (location_t loc, enum tr
> an overflow error if the constant is negative but INTOP is not. */
> && (TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (intop))
> || (TYPE_PRECISION (TREE_TYPE (intop))
> - == TYPE_PRECISION (TREE_TYPE (ptrop)))))
> + == TYPE_PRECISION (TREE_TYPE (ptrop))))
> + && TYPE_PRECISION (TREE_TYPE (intop)) <= TYPE_PRECISION (sizetype))
> {
> - enum tree_code subcode = resultcode;
> - tree int_type = TREE_TYPE (intop);
> - if (TREE_CODE (intop) == MINUS_EXPR)
> - subcode = (subcode == PLUS_EXPR ? MINUS_EXPR : PLUS_EXPR);
> - /* Convert both subexpression types to the type of intop,
> - because weird cases involving pointer arithmetic
> - can result in a sum or difference with different type args. */
> - ptrop = build_binary_op (EXPR_LOCATION (TREE_OPERAND (intop, 1)),
> - subcode, ptrop,
> - convert (int_type, TREE_OPERAND (intop, 1)),
> - true);
> - intop = convert (int_type, TREE_OPERAND (intop, 0));
> + tree intop0 = TREE_OPERAND (intop, 0);
> + tree intop1 = TREE_OPERAND (intop, 1);
> + if (TYPE_PRECISION (TREE_TYPE (intop)) != TYPE_PRECISION (sizetype)
> + || TYPE_UNSIGNED (TREE_TYPE (intop)) != TYPE_UNSIGNED (sizetype))
> + {
> + tree optype = c_common_type_for_size (TYPE_PRECISION (sizetype),
> + TYPE_UNSIGNED (sizetype));
> + intop0 = convert (optype, intop0);
> + intop1 = convert (optype, intop1);
> + }
> + tree t = fold_build2_loc (loc, MULT_EXPR, TREE_TYPE (intop0), intop0,
> + convert (TREE_TYPE (intop0), size_exp));
> + intop0 = convert (sizetype, t);
> + if (TREE_OVERFLOW_P (intop0) && !TREE_OVERFLOW (t))
> + intop0 = wide_int_to_tree (TREE_TYPE (intop0), wi::to_wide (intop0));
> + t = fold_build2_loc (loc, MULT_EXPR, TREE_TYPE (intop1), intop1,
> + convert (TREE_TYPE (intop1), size_exp));
> + intop1 = convert (sizetype, t);
> + if (TREE_OVERFLOW_P (intop1) && !TREE_OVERFLOW (t))
> + intop1 = wide_int_to_tree (TREE_TYPE (intop1), wi::to_wide (intop1));
> + intop = build_binary_op (EXPR_LOCATION (intop), TREE_CODE (intop),
> + intop0, intop1, true);
> +
> + /* Create the sum or difference. */
> + if (resultcode == MINUS_EXPR)
> + intop = fold_build1_loc (loc, NEGATE_EXPR, sizetype, intop);
> +
> + ret = fold_build_pointer_plus_loc (loc, ptrop, intop);
> +
> + fold_undefer_and_ignore_overflow_warnings ();
> +
> + return ret;
> }
>
> /* Convert the integer argument to a type the same size as sizetype
> --- gcc/testsuite/gcc.dg/ubsan/pr120837.c.jj 2025-07-03 12:12:55.392105677
> +0200
> +++ gcc/testsuite/gcc.dg/ubsan/pr120837.c 2025-07-03 12:12:48.058198048
> +0200
> @@ -0,0 +1,32 @@
> +/* PR c/120837 */
> +/* { dg-do run } */
> +/* { dg-options "-O1 -fsanitize=undefined -fno-sanitize-recover=undefined" }
> */
> +
> +[[gnu::noipa]] void
> +bar (void **x, void **y)
> +{
> + x[0] = 0;
> + x[1] = 0;
> + x[2] = 0;
> + y[0] = 0;
> + y[1] = 0;
> + y[2] = 0;
> + y[3] = 0;
> + y[4] = 0;
> +}
> +
> +[[gnu::noipa]] void *
> +foo (int x, int y)
> +{
> + void *a[3];
> + void *b[5];
> + bar (a, b);
> + return (x > y ? b : a)[y - 1];
> +}
> +
> +int
> +main ()
> +{
> + if (foo (2, 1) != 0)
> + __builtin_abort ();
> +}
>
> Jakub
>
