Fix inline memcpy ICE (PR tree-optimization/86526)

Hi!

builtin_memcpy_read_str is a function meant to be called just as a callback
and verifies that we don't cross a '\0' boundary in the string.  For
inline_string_cmp, we've checked that the length returned from c_getstr
is fine, so we can cross as many embedded NULs as there are within the
TREE_STRING_LENGTH.

The rest of the patch is just a temporary to avoid using as_a twice in
each loop iteration, and lots of formatting fixes, mostly to avoid trailing
whitespace.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2018-07-16  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/86526
	* builtins.c (expand_builtin_memcmp): Formatting fixes.
	(inline_expand_builtin_string_cmp): Likewise.
	(inline_string_cmp): Likewise.  Use c_readstr instead of
	builtin_memcpy_read_str.  Add unit_mode temporary.

	* gcc.c-torture/compile/pr86526.c: New test.


	Jakub

On 07/16/2018 02:41 PM, Jakub Jelinek wrote:
> Hi!
> 
> builtin_memcpy_read_str is a function meant to be called just as a callback
> and verifies that we don't cross a '\0' boundary in the string.  For
> inline_string_cmp, we've checked that the length returned from c_getstr
> is fine, so we can cross as many embedded NULs as there are within the
> TREE_STRING_LENGTH.
> 
> The rest of the patch is just a temporary to avoid using as_a twice in
> each loop iteration, and lots of formatting fixes, mostly to avoid trailing
> whitespace.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2018-07-16  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/86526
> 	* builtins.c (expand_builtin_memcmp): Formatting fixes.
> 	(inline_expand_builtin_string_cmp): Likewise.
> 	(inline_string_cmp): Likewise.  Use c_readstr instead of
> 	builtin_memcpy_read_str.  Add unit_mode temporary.
> 
> 	* gcc.c-torture/compile/pr86526.c: New test.
OK.

This looks like it'll fix the m68k linux kernel build failure I saw over
the weekend as well.

jeff

On Mon, 16 Jul 2018, Jakub Jelinek wrote:

> Hi!
> 
> builtin_memcpy_read_str is a function meant to be called just as a callback
> and verifies that we don't cross a '\0' boundary in the string.  For
> inline_string_cmp, we've checked that the length returned from c_getstr
> is fine, so we can cross as many embedded NULs as there are within the
> TREE_STRING_LENGTH.
> 
> The rest of the patch is just a temporary to avoid using as_a twice in
> each loop iteration, and lots of formatting fixes, mostly to avoid trailing
> whitespace.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

Richard.

> 2018-07-16  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/86526
> 	* builtins.c (expand_builtin_memcmp): Formatting fixes.
> 	(inline_expand_builtin_string_cmp): Likewise.
> 	(inline_string_cmp): Likewise.  Use c_readstr instead of
> 	builtin_memcpy_read_str.  Add unit_mode temporary.
> 
> 	* gcc.c-torture/compile/pr86526.c: New test.
> 
> --- gcc/builtins.c.jj	2018-07-16 09:42:25.743985945 +0200
> +++ gcc/builtins.c	2018-07-16 11:47:46.535014889 +0200
> @@ -4454,19 +4454,19 @@ expand_builtin_memcmp (tree exp, rtx tar
>    no_overflow = check_access (exp, /*dst=*/NULL_TREE, /*src=*/NULL_TREE,
>  			      len, /*maxread=*/NULL_TREE, size,
>  			      /*objsize=*/NULL_TREE);
> -  if (no_overflow) 
> +  if (no_overflow)
>      {
>        size = compute_objsize (arg2, 0);
>        no_overflow = check_access (exp, /*dst=*/NULL_TREE, /*src=*/NULL_TREE,
>  				  len,  /*maxread=*/NULL_TREE, size,
>  				  /*objsize=*/NULL_TREE);
> -    } 
> +    }
>  
> -  /* Due to the performance benefit, always inline the calls first 
> +  /* Due to the performance benefit, always inline the calls first
>       when result_eq is false.  */
>    rtx result = NULL_RTX;
> -   
> -  if (!result_eq && fcode != BUILT_IN_BCMP && no_overflow) 
> +
> +  if (!result_eq && fcode != BUILT_IN_BCMP && no_overflow)
>      {
>        result = inline_expand_builtin_string_cmp (exp, target, true);
>        if (result)
> @@ -6748,7 +6748,7 @@ expand_builtin_goacc_parlevel_id_size (t
>    return target;
>  }
>  
> -/* Expand a string compare operation using a sequence of char comparison 
> +/* Expand a string compare operation using a sequence of char comparison
>     to get rid of the calling overhead, with result going to TARGET if
>     that's convenient.
>  
> @@ -6757,7 +6757,7 @@ expand_builtin_goacc_parlevel_id_size (t
>     LENGTH is the number of chars to compare;
>     CONST_STR_N indicates which source string is the constant string;
>     IS_MEMCMP indicates whether it's a memcmp or strcmp.
> -   
> +  
>     to: (assume const_str_n is 2, i.e., arg2 is a constant string)
>  
>     target = var_str[0] - const_str[0];
> @@ -6772,41 +6772,38 @@ expand_builtin_goacc_parlevel_id_size (t
>    */
>  
>  static rtx
> -inline_string_cmp (rtx target, tree var_str, const char* const_str, 
> +inline_string_cmp (rtx target, tree var_str, const char *const_str,
>  		   unsigned HOST_WIDE_INT length,
>  		   int const_str_n, machine_mode mode,
> -		   bool is_memcmp) 
> +		   bool is_memcmp)
>  {
>    HOST_WIDE_INT offset = 0;
> -  rtx var_rtx_array 
> +  rtx var_rtx_array
>      = get_memory_rtx (var_str, build_int_cst (unsigned_type_node,length));
>    rtx var_rtx = NULL_RTX;
> -  rtx const_rtx = NULL_RTX; 
> -  rtx result = target ? target : gen_reg_rtx (mode); 
> -  rtx_code_label *ne_label = gen_label_rtx ();  
> +  rtx const_rtx = NULL_RTX;
> +  rtx result = target ? target : gen_reg_rtx (mode);
> +  rtx_code_label *ne_label = gen_label_rtx ();
>    tree unit_type_node = is_memcmp ? unsigned_char_type_node : char_type_node;
> +  scalar_int_mode unit_mode
> +    = as_a <scalar_int_mode> TYPE_MODE (unit_type_node);
>  
>    start_sequence ();
>  
>    for (unsigned HOST_WIDE_INT i = 0; i < length; i++)
>      {
> -      var_rtx 
> +      var_rtx
>  	= adjust_address (var_rtx_array, TYPE_MODE (unit_type_node), offset);
> -      const_rtx 
> -	= builtin_memcpy_read_str (CONST_CAST (char *, const_str),
> -				   offset,
> -    				   as_a <scalar_int_mode> 
> -				   TYPE_MODE (unit_type_node));
> +      const_rtx = c_readstr (const_str + offset, unit_mode);
>        rtx op0 = (const_str_n == 1) ? const_rtx : var_rtx;
>        rtx op1 = (const_str_n == 1) ? var_rtx : const_rtx;
> -  
> -      result = expand_simple_binop (mode, MINUS, op0, op1, 
> -			            result, is_memcmp ? 1 : 0, OPTAB_WIDEN);
> -      if (i < length - 1) 
> -        emit_cmp_and_jump_insns (result, CONST0_RTX (mode), NE, NULL_RTX,
> -            		         mode, true, ne_label);
> -      offset 
> -	+= GET_MODE_SIZE (as_a <scalar_int_mode> TYPE_MODE (unit_type_node));
> +
> +      result = expand_simple_binop (mode, MINUS, op0, op1,
> +				    result, is_memcmp ? 1 : 0, OPTAB_WIDEN);
> +      if (i < length - 1)
> +	emit_cmp_and_jump_insns (result, CONST0_RTX (mode), NE, NULL_RTX,
> +	    			 mode, true, ne_label);
> +      offset += GET_MODE_SIZE (unit_mode);
>      }
>  
>    emit_label (ne_label);
> @@ -6817,7 +6814,7 @@ inline_string_cmp (rtx target, tree var_
>    return result;
>  }
>  
> -/* Inline expansion a call to str(n)cmp, with result going to 
> +/* Inline expansion a call to str(n)cmp, with result going to
>     TARGET if that's convenient.
>     If the call is not been inlined, return NULL_RTX.  */
>  static rtx
> @@ -6829,7 +6826,7 @@ inline_expand_builtin_string_cmp (tree e
>    bool is_ncmp = (fcode == BUILT_IN_STRNCMP || fcode == BUILT_IN_MEMCMP);
>  
>    gcc_checking_assert (fcode == BUILT_IN_STRCMP
> -		       || fcode == BUILT_IN_STRNCMP 
> +		       || fcode == BUILT_IN_STRNCMP
>  		       || fcode == BUILT_IN_MEMCMP);
>  
>    tree arg1 = CALL_EXPR_ARG (exp, 0);
> @@ -6842,7 +6839,7 @@ inline_expand_builtin_string_cmp (tree e
>  
>    const char *src_str1 = c_getstr (arg1, &len1);
>    const char *src_str2 = c_getstr (arg2, &len2);
> - 
> +
>    /* If neither strings is constant string, the call is not qualify.  */
>    if (!src_str1 && !src_str2)
>      return NULL_RTX;
> @@ -6867,16 +6864,16 @@ inline_expand_builtin_string_cmp (tree e
>    if (is_ncmp && (len3 = tree_to_uhwi (len3_tree)) < length)
>      length = len3;
>  
> -  /* If the length of the comparision is larger than the threshold, 
> +  /* If the length of the comparision is larger than the threshold,
>       do nothing.  */
> -  if (length > (unsigned HOST_WIDE_INT) 
> +  if (length > (unsigned HOST_WIDE_INT)
>  	       PARAM_VALUE (BUILTIN_STRING_CMP_INLINE_LENGTH))
>      return NULL_RTX;
>  
>    machine_mode mode = TYPE_MODE (TREE_TYPE (exp));
>  
>    /* Now, start inline expansion the call.  */
> -  return inline_string_cmp (target, (const_str_n == 1) ? arg2 : arg1, 
> +  return inline_string_cmp (target, (const_str_n == 1) ? arg2 : arg1,
>  			    (const_str_n == 1) ? src_str1 : src_str2, length,
>  			    const_str_n, mode, is_memcmp);
>  }
> @@ -7282,7 +7279,7 @@ expand_builtin (tree exp, rtx target, rt
>  	return target;
>        break;
>  
> -    /* Expand it as BUILT_IN_MEMCMP_EQ first. If not successful, change it 
> +    /* Expand it as BUILT_IN_MEMCMP_EQ first. If not successful, change it
>         back to a BUILT_IN_STRCMP. Remember to delete the 3rd paramater
>         when changing it to a strcmp call.  */
>      case BUILT_IN_STRCMP_EQ:
> @@ -7291,7 +7288,7 @@ expand_builtin (tree exp, rtx target, rt
>  	return target;
>  
>        /* Change this call back to a BUILT_IN_STRCMP.  */
> -      TREE_OPERAND (exp, 1) 
> +      TREE_OPERAND (exp, 1)
>  	= build_fold_addr_expr (builtin_decl_explicit (BUILT_IN_STRCMP));
>  
>        /* Delete the last parameter.  */
> @@ -7317,7 +7314,7 @@ expand_builtin (tree exp, rtx target, rt
>  	return target;
>  
>        /* Change it back to a BUILT_IN_STRNCMP.  */
> -      TREE_OPERAND (exp, 1) 
> +      TREE_OPERAND (exp, 1)
>  	= build_fold_addr_expr (builtin_decl_explicit (BUILT_IN_STRNCMP));
>        /* FALLTHROUGH */
>  
> --- gcc/testsuite/gcc.c-torture/compile/pr86526.c.jj	2018-07-16 11:50:30.756223270 +0200
> +++ gcc/testsuite/gcc.c-torture/compile/pr86526.c	2018-07-16 11:50:16.205203998 +0200
> @@ -0,0 +1,8 @@
> +/* PR tree-optimization/86526 */
> +
> +void
> +foo (char *x)
> +{
> +  if (__builtin_memcmp (x, "\0a", 3))
> +    __builtin_abort ();
> +}
> 
> 	Jakub
> 
>