Re: [PATCH] Fix various minor gimple-ssa-sprintf.c issues

[Martin Sebor <msebor@gmail.com>]

On 09/21/2016 09:09 AM, Jakub Jelinek wrote:
> Hi!
>
> When looking at PR77676, I've noticed various small formatting etc.
> issues, like not using is_gimple_* APIs where we have them, not using
> gimple_call_builtin_p/gimple_call_fndecl (this one actually can show up,
> if e.g. uses the builtin with incorrect arguments (fewer, different
> types etc.)), one pasto, 2 spaces in comments instead of 1 in the middle
> of sentences.  And, lastly 0 < var is very unusual ordering of the
> comparison operands, while we have a couple of such cases in the sources,
> usually it is when using 0 < var && var <= someotherconst, while
> var > 0 is used hundred times more often.

Thanks for correcting the uses of the gimple APIs!  I appreciate
your fixing the various typos as well, but I see no value in
changing the order of operands in inequality expressions or in
moving code around for no apparent reason.  However, I won't
object if you feel strongly about committing those changes(*).

What I would be even more grateful for is a review of the error
prone parts like those that caused the bootstrap failure.  I.e.,
any lingering assumptions about integer sizes between the host
and the target, and the code that controls the return value
range optimization.  (Similar to what you just did for the
PR77676 patch -- that was very helpful, thanks again.)

I'll look into the outstanding issues you noted below next.

Thanks
Martin

PS [*] I think it's unproductive to dwell on these details when
there are much bigger problems to worry about.  IME, insisting
on perfect formatting or on a particular order of operands only
takes time and energy away from focusing on the parts that could
actually cause serious issues.

>
> I see various other issues, which I'll happily defer to Martin, e.g.
>   /* These are available as macros in the C and C++ front ends but,
>      sadly, not here.  */
>   static tree intmax_type_node;
>   static tree uintmax_type_node;
>
>   /* Initialize the intmax nodes above the first time through here.  */
>   if (!intmax_type_node)
>     build_intmax_type_nodes (&intmax_type_node, &uintmax_type_node);
> This is a big no no, the trees aren't registered with GC this way, so
> they could be garbage collected (you'd need to move those to file-scope to
> be able to GC register them).
> Or
>         /* Compute the maximum just once.  */
>         static const int a_max[] = {
>           format_floating_max (double_type_node, 'a'),
>           format_floating_max (long_double_type_node, 'a')
>         };
> etc. (used several time), do we really need to mess up with the
> (thread-safe) local static locking/guards etc.?  Can't you initialize
> the cache say to -1 and compute first if you need it and it is still -1?
> PR77676 shows various other issues in the pass.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2016-09-21  Jakub Jelinek  <jakub@redhat.com>
>
> 	* gimple-ssa-sprintf.c: Fix comment formatting.
> 	(pass_sprintf_length::gate): Use x > 0 instead of 0 < x.
> 	(format_integer): Use is_gimple_assign.
> 	(format_floating, format_string, format_directive,
> 	get_destination_size): Use x > 0 instead of 0 < x.
> 	(pass_sprintf_length::handle_gimple_call): Likewise.  Use
> 	gimple_call_builtin_p and gimple_call_fndecl.  Reorder
> 	case BUILT_IN_SPRINTF_CHK.  Fix up BUILT_IN_SNPRINTF_CHK comment.
> 	Replace "to to" with "to" in comment.
> 	(pass_sprintf_length::execute): Use is_gimple_call.
>
> --- gcc/gimple-ssa-sprintf.c.jj	2016-09-21 08:54:15.000000000 +0200
> +++ gcc/gimple-ssa-sprintf.c	2016-09-21 15:09:02.209261013 +0200
> @@ -37,7 +37,7 @@ along with GCC; see the file COPYING3.
>
>     The pass handles all forms standard sprintf format directives,
>     including character, integer, floating point, pointer, and strings,
> -   with  the standard C flags, widths, and precisions.  For integers
> +   with the standard C flags, widths, and precisions.  For integers
>     and strings it computes the length of output itself.  For floating
>     point it uses MPFR to fornmat known constants with up and down
>     rounding and uses the resulting range of output lengths.  For
> @@ -130,8 +130,8 @@ pass_sprintf_length::gate (function *)
>       not optimizing and the pass is being invoked early, or when
>       optimizing and the pass is being invoked during optimization
>       (i.e., "late").  */
> -  return ((0 < warn_format_length || flag_printf_return_value)
> -	  && (0 < optimize) == fold_return_value);
> +  return ((warn_format_length > 0 || flag_printf_return_value)
> +	  && (optimize > 0) == fold_return_value);
>  }
>
>  /* The result of a call to a formatted function.  */
> @@ -464,7 +464,7 @@ struct conversion_spec
>
>    /* Format conversion function that given a conversion specification
>       and an argument returns the formatting result.  */
> -  fmtresult  (*fmtfunc) (const conversion_spec &, tree);
> +  fmtresult (*fmtfunc) (const conversion_spec &, tree);
>
>    /* Return True when a the format flag CHR has been used.  */
>    bool get_flag (char chr) const
> @@ -1032,10 +1032,10 @@ format_integer (const conversion_spec &s
>  	{
>  	  /* The argument here may be the result of promoting the actual
>  	     argument to int.  Try to determine the type of the actual
> -	     argument before promotion and  narrow down its range that
> +	     argument before promotion and narrow down its range that
>  	     way.  */
>  	  gimple *def = SSA_NAME_DEF_STMT (arg);
> -	  if (gimple_code (def) == GIMPLE_ASSIGN)
> +	  if (is_gimple_assign (def))
>  	    {
>  	      tree_code code = gimple_assign_rhs_code (def);
>  	      if (code == NOP_EXPR)
> @@ -1188,7 +1188,7 @@ format_floating (const conversion_spec &
>      case 'a':
>        {
>  	/* The minimum output is "0x.p+0".  */
> -	res.range.min = 6 + (0 < prec ? prec : 0);
> +	res.range.min = 6 + (prec > 0 ? prec : 0);
>
>  	/* Compute the maximum just once.  */
>  	static const int a_max[] = {
> @@ -1249,7 +1249,7 @@ format_floating (const conversion_spec &
>        gcc_unreachable ();
>      }
>
> -  if (0 < width)
> +  if (width > 0)
>      {
>        if (res.range.min < (unsigned)width)
>  	res.range.min = width;
> @@ -1440,7 +1440,7 @@ get_string_length (tree str)
>  static fmtresult
>  format_string (const conversion_spec &spec, tree arg)
>  {
> -  unsigned width = spec.have_width && 0 < spec.width ? spec.width : 0;
> +  unsigned width = spec.have_width && spec.width > 0 ? spec.width : 0;
>    int prec = spec.have_precision ? spec.precision : -1;
>
>    if (spec.star_width)
> @@ -1756,7 +1756,7 @@ format_directive (const pass_sprintf_len
>      }
>    else
>      {
> -      if (!res->warned && 0 < fmtres.range.min && navail < fmtres.range.min)
> +      if (!res->warned && fmtres.range.min > 0 && navail < fmtres.range.min)
>  	{
>  	  const char* fmtstr
>  	    = (info.bounded
> @@ -2332,7 +2332,7 @@ get_destination_size (tree dest)
>       a member array as opposed to the whole enclosing object), otherwise
>       use type-zero object size to determine the size of the enclosing
>       object (the function fails without optimization in this type).  */
> -  int ost = 0 < optimize;
> +  int ost = optimize > 0;
>    unsigned HOST_WIDE_INT size;
>    if (compute_builtin_object_size (dest, ost, &size))
>      return size;
> @@ -2449,18 +2449,10 @@ pass_sprintf_length::handle_gimple_call
>    call_info info = call_info ();
>
>    info.callstmt = gsi_stmt (gsi);
> -  info.func = gimple_call_fn (info.callstmt);
> -  if (!info.func)
> -    return;
> -
> -  if (TREE_CODE (info.func) == ADDR_EXPR)
> -    info.func = TREE_OPERAND (info.func, 0);
> -
> -  if (TREE_CODE (info.func) != FUNCTION_DECL
> -      || !DECL_BUILT_IN(info.func)
> -      || DECL_BUILT_IN_CLASS (info.func) != BUILT_IN_NORMAL)
> +  if (!gimple_call_builtin_p (info.callstmt, BUILT_IN_NORMAL))
>      return;
>
> +  info.func = gimple_call_fndecl (info.callstmt);
>    info.fncode = DECL_FUNCTION_CODE (info.func);
>
>    /* The size of the destination as in snprintf(dest, size, ...).  */
> @@ -2487,6 +2479,14 @@ pass_sprintf_length::handle_gimple_call
>        info.argidx = 2;
>        break;
>
> +    case BUILT_IN_SPRINTF_CHK:
> +      // Signature:
> +      //   __builtin___sprintf_chk (dst, ost, objsize, format, ...)
> +      idx_objsize = 2;
> +      idx_format = 3;
> +      info.argidx = 4;
> +      break;
> +
>      case BUILT_IN_SNPRINTF:
>        // Signature:
>        //   __builtin_snprintf (dst, size, format, ...)
> @@ -2498,7 +2498,7 @@ pass_sprintf_length::handle_gimple_call
>
>      case BUILT_IN_SNPRINTF_CHK:
>        // Signature:
> -      //   __builtin___sprintf_chk (dst, size, ost, objsize, format, ...)
> +      //   __builtin___snprintf_chk (dst, size, ost, objsize, format, ...)
>        idx_dstsize = 1;
>        idx_objsize = 3;
>        idx_format = 4;
> @@ -2506,14 +2506,6 @@ pass_sprintf_length::handle_gimple_call
>        info.bounded = true;
>        break;
>
> -    case BUILT_IN_SPRINTF_CHK:
> -      // Signature:
> -      //   __builtin___sprintf_chk (dst, ost, objsize, format, ...)
> -      idx_objsize = 2;
> -      idx_format = 3;
> -      info.argidx = 4;
> -      break;
> -
>      case BUILT_IN_VSNPRINTF:
>        // Signature:
>        //   __builtin_vsprintf (dst, size, format, va)
> @@ -2556,7 +2548,7 @@ pass_sprintf_length::handle_gimple_call
>
>    if (idx_dstsize == HOST_WIDE_INT_M1U)
>      {
> -      // For non-bounded functions like sprintf, to to determine
> +      // For non-bounded functions like sprintf, to determine
>        // the size of the destination from the object or pointer
>        // passed to it as the first argument.
>        dstsize = get_destination_size (gimple_call_arg (info.callstmt, 0));
> @@ -2648,7 +2640,8 @@ pass_sprintf_length::handle_gimple_call
>       attempt to substitute the computed result for the return value of
>       the call.  Avoid this optimization when -frounding-math is in effect
>       and the format string contains a floating point directive.  */
> -  if (0 < optimize && flag_printf_return_value
> +  if (optimize > 0
> +      && flag_printf_return_value
>        && (!flag_rounding_math || !res.floating))
>      try_substitute_return_value (gsi, info, res);
>  }
> @@ -2667,7 +2660,7 @@ pass_sprintf_length::execute (function *
>  	  /* Iterate over statements, looking for function calls.  */
>  	  gimple *stmt = gsi_stmt (si);
>
> -	  if (gimple_code (stmt) == GIMPLE_CALL)
> +	  if (is_gimple_call (stmt))
>  	    handle_gimple_call (si);
>  	}
>      }
>
> 	Jakub
>