public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jakub Jelinek <jakub@redhat.com>
To: Jeff Law <law@redhat.com>, Martin Sebor <msebor@gmail.com>
Cc: gcc-patches@gcc.gnu.org
Subject: [PATCH] Fix various minor gimple-ssa-sprintf.c issues
Date: Wed, 21 Sep 2016 15:19:00 -0000	[thread overview]
Message-ID: <20160921150954.GC7282@tucnak.redhat.com> (raw)

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.

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

             reply	other threads:[~2016-09-21 15:10 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-09-21 15:19 Jakub Jelinek [this message]
2016-09-22  4:54 ` Martin Sebor
2016-09-22  7:40   ` Jakub Jelinek
2016-09-22  8:35     ` Marek Polacek
2016-09-23 17:59       ` Martin Sebor
2016-09-22 15:29     ` Martin Sebor
2016-09-26  9:21   ` Gerald Pfeifer
2016-09-27 17:33     ` Martin Sebor
2016-09-27 17:52       ` Martin Sebor
2016-09-28  4:43       ` Jeff Law

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160921150954.GC7282@tucnak.redhat.com \
    --to=jakub@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=law@redhat.com \
    --cc=msebor@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).