[PATCH v3] x86: avoid i386_dis_printf()'s staging area for a fair part of output

[PATCH v3] x86: avoid i386_dis_printf()'s staging area for a fair part of output

[Jan Beulich]

While PR binutils/29483 has now been addressed differently, this
originally proposed change still has its merits: Avoiding vsnprintf()
for typically far more than half of the overall output results in a 2-3%
performance gain in my testing (with debug builds of objdump, libbfd,
and libopcodes).

With that part of output no longer using staging_area[], the array also
doesn't need to be quite as large anymore (the largest presently used
size is 27, from "64-bit address is disabled").

While limiting the scope of "res" it became apparent that
- no caller cares about the function's return value,
- the comment about the return value was wrong,
- a particular positive return value would have been meaningless to the
  caller.
Therefore convert the function to return "void" at the same time.
---
An alternative to the special casing would be to introduce something
like i386_dis_puts(), then to be used by all call sites which currently
pass "%s" or format strings without any format characters at all (plus,
of course, i386_dis_printf() itself).
---
v3: Change title and description. Re-base over PR binutils/29483 fix.
v2: Add testcase.

--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -9267,31 +9267,40 @@ oappend_register (instr_info *ins, const
    STYLE is the default style to use in the fprintf_styled_func calls,
    however, FMT might include embedded style markers (see oappend_style),
    these embedded markers are not printed, but instead change the style
-   used in the next fprintf_styled_func call.
+   used in the next fprintf_styled_func call.  */
 
-   Return non-zero to indicate the print call was a success.  */
-
-static int ATTRIBUTE_PRINTF_3
+static void ATTRIBUTE_PRINTF_3
 i386_dis_printf (instr_info *ins, enum disassembler_style style,
 		 const char *fmt, ...)
 {
   va_list ap;
   enum disassembler_style curr_style = style;
-  char *start, *curr;
-  char staging_area[MAX_OPERAND_BUFFER_SIZE];
-  int res;
+  const char *start, *curr;
+  char staging_area[40];
 
   va_start (ap, fmt);
-  res = vsnprintf (staging_area, sizeof (staging_area), fmt, ap);
-  va_end (ap);
+  /* In particular print_insn()'s processing of op_txt[] can hand rather long
+     strings here.  Bypass vsnprintf() in such cases to avoid capacity issues
+     with the staging area.  */
+  if (strcmp (fmt, "%s"))
+    {
+      int res = vsnprintf (staging_area, sizeof (staging_area), fmt, ap);
 
-  if (res < 0)
-    return res;
+      va_end (ap);
 
-  if ((size_t) res >= sizeof (staging_area))
-    abort ();
+      if (res < 0)
+	return;
 
-  start = curr = staging_area;
+      if ((size_t) res >= sizeof (staging_area))
+	abort ();
+
+      start = curr = staging_area;
+    }
+  else
+    {
+      start = curr = va_arg (ap, const char *);
+      va_end (ap);
+    }
 
   do
     {
@@ -9306,10 +9315,7 @@ i386_dis_printf (instr_info *ins, enum d
 						     curr_style,
 						     "%.*s", len, start);
 	  if (n < 0)
-	    {
-	      res = n;
-	      break;
-	    }
+	    break;
 
 	  if (*curr == '\0')
 	    break;
@@ -9343,8 +9349,6 @@ i386_dis_printf (instr_info *ins, enum d
 	++curr;
     }
   while (true);
-
-  return res;
 }
 
 static int

[PATCH v3] x86: avoid i386_dis_printf()'s staging area for a fair part of output

[Jan Beulich]

While PR binutils/29483 has now been addressed differently, this
originally proposed change still has its merits: Avoiding vsnprintf()
for typically far more than half of the overall output results in a 2-3%
performance gain in my testing (with debug builds of objdump, libbfd,
and libopcodes).

With that part of output no longer using staging_area[], the array also
doesn't need to be quite as large anymore (the largest presently used
size is 27, from "64-bit address is disabled").

While limiting the scope of "res" it became apparent that
- no caller cares about the function's return value,
- the comment about the return value was wrong,
- a particular positive return value would have been meaningless to the
  caller.
Therefore convert the function to return "void" at the same time.
---
An alternative to the special casing would be to introduce something
like i386_dis_puts(), then to be used by all call sites which currently
pass "%s" or format strings without any format characters at all (plus,
of course, i386_dis_printf() itself).
---
v3: Change title and description. Re-base over PR binutils/29483 fix.
v2: Add testcase.

--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -9267,31 +9267,40 @@ oappend_register (instr_info *ins, const
    STYLE is the default style to use in the fprintf_styled_func calls,
    however, FMT might include embedded style markers (see oappend_style),
    these embedded markers are not printed, but instead change the style
-   used in the next fprintf_styled_func call.
+   used in the next fprintf_styled_func call.  */
 
-   Return non-zero to indicate the print call was a success.  */
-
-static int ATTRIBUTE_PRINTF_3
+static void ATTRIBUTE_PRINTF_3
 i386_dis_printf (instr_info *ins, enum disassembler_style style,
 		 const char *fmt, ...)
 {
   va_list ap;
   enum disassembler_style curr_style = style;
-  char *start, *curr;
-  char staging_area[MAX_OPERAND_BUFFER_SIZE];
-  int res;
+  const char *start, *curr;
+  char staging_area[40];
 
   va_start (ap, fmt);
-  res = vsnprintf (staging_area, sizeof (staging_area), fmt, ap);
-  va_end (ap);
+  /* In particular print_insn()'s processing of op_txt[] can hand rather long
+     strings here.  Bypass vsnprintf() in such cases to avoid capacity issues
+     with the staging area.  */
+  if (strcmp (fmt, "%s"))
+    {
+      int res = vsnprintf (staging_area, sizeof (staging_area), fmt, ap);
 
-  if (res < 0)
-    return res;
+      va_end (ap);
 
-  if ((size_t) res >= sizeof (staging_area))
-    abort ();
+      if (res < 0)
+	return;
 
-  start = curr = staging_area;
+      if ((size_t) res >= sizeof (staging_area))
+	abort ();
+
+      start = curr = staging_area;
+    }
+  else
+    {
+      start = curr = va_arg (ap, const char *);
+      va_end (ap);
+    }
 
   do
     {
@@ -9306,10 +9315,7 @@ i386_dis_printf (instr_info *ins, enum d
 						     curr_style,
 						     "%.*s", len, start);
 	  if (n < 0)
-	    {
-	      res = n;
-	      break;
-	    }
+	    break;
 
 	  if (*curr == '\0')
 	    break;
@@ -9343,8 +9349,6 @@ i386_dis_printf (instr_info *ins, enum d
 	++curr;
     }
   while (true);
-
-  return res;
 }
 
 static int