Fix ICE with statement frontiers (PR sanitizer/85213)

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?

2018-04-05  Jakub Jelinek  <jakub@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.


	Jakub

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  <jakub@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
> 
>