On Thu, 5 Apr 2018, Jakub Jelinek wrote:
> Hi!
>
> As mentioned in the PR, on the following testcase we cp_save_expr in order
> to avoid evaluating -1 * __builtin_expect (({ ... }), ...) twice and use
> the SAVE_EXPR in the original shift as well as in a comparison, but then
> we attempt to optimize the comparison by grabbing parts of the SAVE_EXPR,
> create comparison out of it and building another SAVE_EXPR around it.
> As unshare_expr/mostly_copy_tree_r doesn't unshare STATEMENT_LISTs, we end
> up with the STATEMENT_LIST containing DEBUG_BEGIN_STMT and x != 0 multiple
> times in the IL and as gimplification is destructive, we destroy the
> STATEMENT_LIST the first time and second time ICE on it.
>
> I don't see other way how to resolve this than to avoid this weirdo
> optimization, I believe the optimization is from pre-GIMPLE era where we
> didn't have hundreds of passes that can optimize it before expansion.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, haven't seen any
> regressions, ok for trunk?
OK.
Thanks,
Richard.
> 2018-04-05 Jakub Jelinek <ja...@redhat.com>
>
> PR sanitizer/85213
> * fold-const.c (twoval_comparison_p): Remove SAVE_P argument and don't
> look through SAVE_EXPRs with non-side-effects argument. Adjust
> recursive calls.
> (fold_comparison): Adjust twoval_comparison_p caller, don't handle
> save_p here.
>
> * c-c++-common/ubsan/pr85213.c: New test.
>
> --- gcc/fold-const.c.jj 2018-03-29 12:37:23.887476367 +0200
> +++ gcc/fold-const.c 2018-04-05 11:30:28.996962481 +0200
> @@ -115,7 +115,7 @@ static tree negate_expr (tree);
> static tree associate_trees (location_t, tree, tree, enum tree_code, tree);
> static enum comparison_code comparison_to_compcode (enum tree_code);
> static enum tree_code compcode_to_comparison (enum comparison_code);
> -static int twoval_comparison_p (tree, tree *, tree *, int *);
> +static int twoval_comparison_p (tree, tree *, tree *);
> static tree eval_subst (location_t, tree, tree, tree, tree, tree);
> static tree optimize_bit_field_compare (location_t, enum tree_code,
> tree, tree, tree);
> @@ -3549,13 +3549,12 @@ operand_equal_for_comparison_p (tree arg
> two different values, which will be stored in *CVAL1 and *CVAL2; if
> they are nonzero it means that some operands have already been found.
> No variables may be used anywhere else in the expression except in the
> - comparisons. If SAVE_P is true it means we removed a SAVE_EXPR around
> - the expression and save_expr needs to be called with CVAL1 and CVAL2.
> + comparisons.
>
> If this is true, return 1. Otherwise, return zero. */
>
> static int
> -twoval_comparison_p (tree arg, tree *cval1, tree *cval2, int *save_p)
> +twoval_comparison_p (tree arg, tree *cval1, tree *cval2)
> {
> enum tree_code code = TREE_CODE (arg);
> enum tree_code_class tclass = TREE_CODE_CLASS (code);
> @@ -3568,39 +3567,23 @@ twoval_comparison_p (tree arg, tree *cva
> || code == COMPOUND_EXPR))
> tclass = tcc_binary;
>
> - else if (tclass == tcc_expression && code == SAVE_EXPR
> - && ! TREE_SIDE_EFFECTS (TREE_OPERAND (arg, 0)))
> - {
> - /* If we've already found a CVAL1 or CVAL2, this expression is
> - two complex to handle. */
> - if (*cval1 || *cval2)
> - return 0;
> -
> - tclass = tcc_unary;
> - *save_p = 1;
> - }
> -
> switch (tclass)
> {
> case tcc_unary:
> - return twoval_comparison_p (TREE_OPERAND (arg, 0), cval1, cval2,
> save_p);
> + return twoval_comparison_p (TREE_OPERAND (arg, 0), cval1, cval2);
>
> case tcc_binary:
> - return (twoval_comparison_p (TREE_OPERAND (arg, 0), cval1, cval2,
> save_p)
> - && twoval_comparison_p (TREE_OPERAND (arg, 1),
> - cval1, cval2, save_p));
> + return (twoval_comparison_p (TREE_OPERAND (arg, 0), cval1, cval2)
> + && twoval_comparison_p (TREE_OPERAND (arg, 1), cval1, cval2));
>
> case tcc_constant:
> return 1;
>
> case tcc_expression:
> if (code == COND_EXPR)
> - return (twoval_comparison_p (TREE_OPERAND (arg, 0),
> - cval1, cval2, save_p)
> - && twoval_comparison_p (TREE_OPERAND (arg, 1),
> - cval1, cval2, save_p)
> - && twoval_comparison_p (TREE_OPERAND (arg, 2),
> - cval1, cval2, save_p));
> + return (twoval_comparison_p (TREE_OPERAND (arg, 0), cval1, cval2)
> + && twoval_comparison_p (TREE_OPERAND (arg, 1), cval1, cval2)
> + && twoval_comparison_p (TREE_OPERAND (arg, 2), cval1, cval2));
> return 0;
>
> case tcc_comparison:
> @@ -8781,9 +8764,8 @@ fold_comparison (location_t loc, enum tr
> if (TREE_CODE (arg1) == INTEGER_CST && TREE_CODE (arg0) != INTEGER_CST)
> {
> tree cval1 = 0, cval2 = 0;
> - int save_p = 0;
>
> - if (twoval_comparison_p (arg0, &cval1, &cval2, &save_p)
> + if (twoval_comparison_p (arg0, &cval1, &cval2)
> /* Don't handle degenerate cases here; they should already
> have been handled anyway. */
> && cval1 != 0 && cval2 != 0
> @@ -8856,12 +8838,6 @@ fold_comparison (location_t loc, enum tr
> return omit_one_operand_loc (loc, type, integer_one_node,
> arg0);
> }
>
> - if (save_p)
> - {
> - tem = save_expr (build2 (code, type, cval1, cval2));
> - protected_set_expr_location (tem, loc);
> - return tem;
> - }
> return fold_build2_loc (loc, code, type, cval1, cval2);
> }
> }
> --- gcc/testsuite/c-c++-common/ubsan/pr85213.c.jj 2018-04-05
> 11:43:53.157045995 +0200
> +++ gcc/testsuite/c-c++-common/ubsan/pr85213.c 2018-04-05
> 11:43:33.954043995 +0200
> @@ -0,0 +1,9 @@
> +/* PR sanitizer/85213 */
> +/* { dg-do compile } */
> +/* { dg-options "-O1 -fsanitize=undefined -fcompare-debug" } */
> +
> +int
> +foo (int x)
> +{
> + return (__builtin_expect (({ x != 0; }) ? 0 : 1, 3) == 0) * -1 << 0;
> +}
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
