This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[PATCH] C++-ify parse_format_string
- From: Simon Marchi <simon dot marchi at polymtl dot ca>
- To: gdb-patches at sourceware dot org
- Cc: Tom Tromey <tom at tromey dot com>, Pedro Alves <palves at redhat dot com>, Simon Marchi <simon dot marchi at ericsson dot com>
- Date: Sat, 2 Dec 2017 15:31:33 -0500
- Subject: [PATCH] C++-ify parse_format_string
- Authentication-results: sourceware.org; auth=none
- References: <87h8tivyo4.fsf@tromey.com>
From: Simon Marchi <simon.marchi@ericsson.com>
I finally got around to polish this patch. Let me know what you think.
This patch C++ifies the parse_format_string function and its return
value, allowing to get rid of free_format_pieces_cleanup. Currently,
format_piece::string points into a big buffer shared by all format
pieces resulting of the format string parse. To make the code and
allocation scheme simpler to understand, this patch makes each piece own
its string with an std::string.
I've tried to measure the impact of this change on the time taken by
parse_format_string in an optimized build. I ran
$ perf record -g -- ./gdb -nx -batch -x test.gdb
with test.gdb:
set $a = 0
while $a < 20000
printf "%daaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa<16 times>\n", 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16
set $a = $a + 1
end
The purpose of the long string of a's is to have some format pieces for
which the substring doesn't fit in the std::string local buffer. The
command "perf report" shows this for current master:
1.81% 1.81% gdb gdb [.] parse_format_string
and this with the patch:
1.69% 1.69% gdb gdb [.] parse_format_string
So it doesn't seem like it makes much of a difference. If you have tips
on how to use perf to get more precise measurements, I would be glad to
hear them.
gdb/ChangeLog:
* common/format.h (struct format_piece): Add constructor.
<string>: Change type to std::string.
(parse_format_string): Return an std::vector of format_piece.
(free_format_pieces): Remove.
(free_format_pieces_cleanup): Remove.
* common/format.c (parse_format_string): Return an std::vector of
format_piece. Don't allocate a big buffer, adjust to those
changes.
(free_format_pieces): Remove.
(free_format_pieces_cleanup): Remove.
* ax-gdb.c (gen_printf): Remove format_piece * parameter.
(maint_agent_printf_command): Adjust to parse_format_string
changes.
* ax-gdb.h (struct format_piece): Don't forward-declare.
(gen_printf): Remove format_piece * parameter.
* breakpoint.c (parse_cmd_to_aexpr): Adjust to
parse_format_string chagnes.
* printcmd.c (ui_printf): Likewise.
gdb/gdbserver/ChangeLog:
* ax.c (ax_printf): Adjust to parse_format_string changes.
---
gdb/ax-gdb.c | 22 ++++--------
gdb/ax-gdb.h | 2 --
gdb/breakpoint.c | 24 ++++---------
gdb/common/format.c | 99 +++++++----------------------------------------------
gdb/common/format.h | 19 ++++------
gdb/gdbserver/ax.c | 34 +++++++++---------
gdb/printcmd.c | 41 ++++++++++------------
7 files changed, 67 insertions(+), 174 deletions(-)
diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
index 5027f6a464..6c0d0f29f9 100644
--- a/gdb/ax-gdb.c
+++ b/gdb/ax-gdb.c
@@ -2541,7 +2541,6 @@ agent_expr_up
gen_printf (CORE_ADDR scope, struct gdbarch *gdbarch,
CORE_ADDR function, LONGEST channel,
const char *format, int fmtlen,
- struct format_piece *frags,
int nargs, struct expression **exprs)
{
agent_expr_up ax (new agent_expr (gdbarch, scope));
@@ -2681,12 +2680,7 @@ agent_eval_command (const char *exp, int from_tty)
static void
maint_agent_printf_command (const char *cmdrest, int from_tty)
{
- struct cleanup *old_chain = 0;
- struct expression *argvec[100];
struct frame_info *fi = get_current_frame (); /* need current scope */
- const char *format_start, *format_end;
- struct format_piece *fpieces;
- int nargs;
/* We don't deal with overlay debugging at the moment. We need to
think more carefully about this. If you copy this code into
@@ -2703,13 +2697,11 @@ maint_agent_printf_command (const char *cmdrest, int from_tty)
if (*cmdrest++ != '"')
error (_("Must start with a format string."));
- format_start = cmdrest;
+ const char *format_start = cmdrest;
- fpieces = parse_format_string (&cmdrest);
+ parse_format_string (&cmdrest);
- old_chain = make_cleanup (free_format_pieces_cleanup, &fpieces);
-
- format_end = cmdrest;
+ const char *format_end = cmdrest;
if (*cmdrest++ != '"')
error (_("Bad format string, non-terminated '\"'."));
@@ -2723,15 +2715,14 @@ maint_agent_printf_command (const char *cmdrest, int from_tty)
cmdrest++;
cmdrest = skip_spaces (cmdrest);
- nargs = 0;
+ std::vector<struct expression *> argvec;
while (*cmdrest != '\0')
{
const char *cmd1;
cmd1 = cmdrest;
expression_up expr = parse_exp_1 (&cmd1, 0, (struct block *) 0, 1);
- argvec[nargs] = expr.release ();
- ++nargs;
+ argvec.push_back (expr.release ());
cmdrest = cmd1;
if (*cmdrest == ',')
++cmdrest;
@@ -2742,14 +2733,13 @@ maint_agent_printf_command (const char *cmdrest, int from_tty)
agent_expr_up agent = gen_printf (get_frame_pc (fi), get_current_arch (),
0, 0,
format_start, format_end - format_start,
- fpieces, nargs, argvec);
+ argvec.size (), argvec.data ());
ax_reqs (agent.get ());
ax_print (gdb_stdout, agent.get ());
/* It would be nice to call ax_reqs here to gather some general info
about the expression, and then print out the result. */
- do_cleanups (old_chain);
dont_repeat ();
}
diff --git a/gdb/ax-gdb.h b/gdb/ax-gdb.h
index 8b5ab46c66..834ddffe05 100644
--- a/gdb/ax-gdb.h
+++ b/gdb/ax-gdb.h
@@ -120,10 +120,8 @@ extern void gen_expr (struct expression *exp, union exp_element **pc,
extern void require_rvalue (struct agent_expr *ax, struct axs_value *value);
-struct format_piece;
extern agent_expr_up gen_printf (CORE_ADDR, struct gdbarch *,
CORE_ADDR, LONGEST, const char *, int,
- struct format_piece *,
int, struct expression **);
#endif /* AX_GDB_H */
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index d4d095d87e..065ac002f7 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2230,12 +2230,7 @@ build_target_condition_list (struct bp_location *bl)
static agent_expr_up
parse_cmd_to_aexpr (CORE_ADDR scope, char *cmd)
{
- struct cleanup *old_cleanups = 0;
- struct expression **argvec;
const char *cmdrest;
- const char *format_start, *format_end;
- struct format_piece *fpieces;
- int nargs;
struct gdbarch *gdbarch = get_current_arch ();
if (cmd == NULL)
@@ -2250,13 +2245,11 @@ parse_cmd_to_aexpr (CORE_ADDR scope, char *cmd)
if (*cmdrest++ != '"')
error (_("No format string following the location"));
- format_start = cmdrest;
+ const char *format_start = cmdrest;
- fpieces = parse_format_string (&cmdrest);
+ parse_format_string (&cmdrest);
- old_cleanups = make_cleanup (free_format_pieces_cleanup, &fpieces);
-
- format_end = cmdrest;
+ const char *format_end = cmdrest;
if (*cmdrest++ != '"')
error (_("Bad format string, non-terminated '\"'."));
@@ -2272,17 +2265,14 @@ parse_cmd_to_aexpr (CORE_ADDR scope, char *cmd)
/* For each argument, make an expression. */
- argvec = (struct expression **) alloca (strlen (cmd)
- * sizeof (struct expression *));
-
- nargs = 0;
+ std::vector<struct expression *> argvec;
while (*cmdrest != '\0')
{
const char *cmd1;
cmd1 = cmdrest;
expression_up expr = parse_exp_1 (&cmd1, scope, block_for_pc (scope), 1);
- argvec[nargs++] = expr.release ();
+ argvec.push_back (expr.release ());
cmdrest = cmd1;
if (*cmdrest == ',')
++cmdrest;
@@ -2296,7 +2286,7 @@ parse_cmd_to_aexpr (CORE_ADDR scope, char *cmd)
{
aexpr = gen_printf (scope, gdbarch, 0, 0,
format_start, format_end - format_start,
- fpieces, nargs, argvec);
+ argvec.size (), argvec.data ());
}
CATCH (ex, RETURN_MASK_ERROR)
{
@@ -2306,8 +2296,6 @@ parse_cmd_to_aexpr (CORE_ADDR scope, char *cmd)
}
END_CATCH
- do_cleanups (old_cleanups);
-
/* We have a valid agent expression, return it. */
return aexpr;
}
diff --git a/gdb/common/format.c b/gdb/common/format.c
index 8cb15511fa..b0c7268c8f 100644
--- a/gdb/common/format.c
+++ b/gdb/common/format.c
@@ -20,17 +20,14 @@
#include "common-defs.h"
#include "format.h"
-struct format_piece *
+std::vector<format_piece>
parse_format_string (const char **arg)
{
const char *s;
char *f, *string;
const char *prev_start;
const char *percent_loc;
- char *sub_start, *current_substring;
- struct format_piece *pieces;
- int next_frag;
- int max_pieces;
+ std::vector<format_piece> pieces;
enum argclass this_argclass;
s = *arg;
@@ -97,16 +94,6 @@ parse_format_string (const char **arg)
done with it; it's up to callers to complain about syntax. */
*arg = s;
- /* Need extra space for the '\0's. Doubling the size is sufficient. */
-
- current_substring = (char *) xmalloc (strlen (string) * 2 + 1000);
-
- max_pieces = strlen (string) + 2;
-
- pieces = XNEWVEC (struct format_piece, max_pieces);
-
- next_frag = 0;
-
/* Now scan the string for %-specs and see what kinds of args they want.
argclass classifies the %-specs so we can give printf-type functions
something of the right size. */
@@ -129,15 +116,8 @@ parse_format_string (const char **arg)
continue;
}
- sub_start = current_substring;
-
- strncpy (current_substring, prev_start, f - 1 - prev_start);
- current_substring += f - 1 - prev_start;
- *current_substring++ = '\0';
-
- pieces[next_frag].string = sub_start;
- pieces[next_frag].argclass = literal_piece;
- next_frag++;
+ pieces.emplace_back (std::string (prev_start, f - 1 - prev_start),
+ literal_piece);
percent_loc = f - 1;
@@ -309,7 +289,7 @@ parse_format_string (const char **arg)
f++;
- sub_start = current_substring;
+ std::string piece_string;
if (lcount > 1 && USE_PRINTF_I64)
{
@@ -317,11 +297,9 @@ parse_format_string (const char **arg)
Convert %lld to %I64d. */
int length_before_ll = f - percent_loc - 1 - lcount;
- strncpy (current_substring, percent_loc, length_before_ll);
- strcpy (current_substring + length_before_ll, "I64");
- current_substring[length_before_ll + 3] =
- percent_loc[length_before_ll + lcount];
- current_substring += length_before_ll + 4;
+ piece_string.assign (percent_loc, length_before_ll);
+ piece_string += "I64";
+ piece_string += percent_loc[length_before_ll + lcount];
}
else if (this_argclass == wide_string_arg
|| this_argclass == wide_char_arg)
@@ -329,71 +307,20 @@ parse_format_string (const char **arg)
/* Convert %ls or %lc to %s. */
int length_before_ls = f - percent_loc - 2;
- strncpy (current_substring, percent_loc, length_before_ls);
- strcpy (current_substring + length_before_ls, "s");
- current_substring += length_before_ls + 2;
+ piece_string.assign (percent_loc, length_before_ls);
+ piece_string += 's';
}
else
- {
- strncpy (current_substring, percent_loc, f - percent_loc);
- current_substring += f - percent_loc;
- }
-
- *current_substring++ = '\0';
+ piece_string.assign (percent_loc, f - percent_loc);
prev_start = f;
- pieces[next_frag].string = sub_start;
- pieces[next_frag].argclass = this_argclass;
- next_frag++;
+ pieces.emplace_back (std::move (piece_string), this_argclass);
}
/* Record the remainder of the string. */
- sub_start = current_substring;
-
- strncpy (current_substring, prev_start, f - prev_start);
- current_substring += f - prev_start;
- *current_substring++ = '\0';
-
- pieces[next_frag].string = sub_start;
- pieces[next_frag].argclass = literal_piece;
- next_frag++;
-
- /* Record an end-of-array marker. */
-
- pieces[next_frag].string = NULL;
- pieces[next_frag].argclass = literal_piece;
+ pieces.emplace_back (std::string (prev_start, f - prev_start), literal_piece);
return pieces;
}
-
-void
-free_format_pieces (struct format_piece *pieces)
-{
- if (!pieces)
- return;
-
- /* We happen to know that all the string pieces are in the block
- pointed to by the first string piece. */
- if (pieces[0].string)
- xfree (pieces[0].string);
-
- xfree (pieces);
-}
-
-void
-free_format_pieces_cleanup (void *ptr)
-{
- struct format_piece **location = (struct format_piece **) ptr;
-
- if (location == NULL)
- return;
-
- if (*location != NULL)
- {
- free_format_pieces (*location);
- *location = NULL;
- }
-}
-
diff --git a/gdb/common/format.h b/gdb/common/format.h
index f3a94b8bbb..558f719efe 100644
--- a/gdb/common/format.h
+++ b/gdb/common/format.h
@@ -48,22 +48,17 @@ enum argclass
struct format_piece
{
- char *string;
+ format_piece (std::string &&string_, enum argclass argclass_)
+ : string (std::move (string_)), argclass (argclass_)
+ {}
+
+ std::string string;
enum argclass argclass;
};
-/* Return an array of printf fragments found at the given string, and
+/* Return a vector of printf fragments found at the given string, and
rewrite ARG with a pointer to the end of the format string. */
-extern struct format_piece *parse_format_string (const char **arg);
-
-/* Given a pointer to an array of format pieces, free any memory that
- would have been allocated by parse_format_string. */
-
-extern void free_format_pieces (struct format_piece *frags);
-
-/* Freeing, cast as a cleanup. */
-
-extern void free_format_pieces_cleanup (void *);
+extern std::vector<format_piece> parse_format_string (const char **arg);
#endif /* COMMON_FORMAT_H */
diff --git a/gdb/gdbserver/ax.c b/gdb/gdbserver/ax.c
index 35ed2c69ad..46a1399e0b 100644
--- a/gdb/gdbserver/ax.c
+++ b/gdb/gdbserver/ax.c
@@ -816,30 +816,29 @@ ax_printf (CORE_ADDR fn, CORE_ADDR chan, const char *format,
int nargs, ULONGEST *args)
{
const char *f = format;
- struct format_piece *fpieces;
- int i, fp;
- char *current_substring;
+ int i;
int nargs_wanted;
ax_debug ("Printf of \"%s\" with %d args", format, nargs);
- fpieces = parse_format_string (&f);
+ std::vector<format_piece> fpieces = parse_format_string (&f);
nargs_wanted = 0;
- for (fp = 0; fpieces[fp].string != NULL; fp++)
- if (fpieces[fp].argclass != literal_piece)
+ for (const format_piece &piece : fpieces)
+ if (piece.argclass != literal_piece)
++nargs_wanted;
if (nargs != nargs_wanted)
error (_("Wrong number of arguments for specified format-string"));
i = 0;
- for (fp = 0; fpieces[fp].string != NULL; fp++)
+ for (const format_piece &piece : fpieces)
{
- current_substring = fpieces[fp].string;
+ const char *fmt = piece.string.c_str ();
+
ax_debug ("current substring is '%s', class is %d",
- current_substring, fpieces[fp].argclass);
- switch (fpieces[fp].argclass)
+ fmt, piece.argclass);
+ switch (piece.argclass)
{
case string_arg:
{
@@ -865,7 +864,7 @@ ax_printf (CORE_ADDR fn, CORE_ADDR chan, const char *format,
read_inferior_memory (tem, str, j);
str[j] = 0;
- printf (current_substring, (char *) str);
+ printf (fmt, (char *) str);
}
break;
@@ -874,7 +873,7 @@ ax_printf (CORE_ADDR fn, CORE_ADDR chan, const char *format,
{
long long val = args[i];
- printf (current_substring, val);
+ printf (fmt, val);
break;
}
#else
@@ -884,7 +883,7 @@ ax_printf (CORE_ADDR fn, CORE_ADDR chan, const char *format,
{
int val = args[i];
- printf (current_substring, val);
+ printf (fmt, val);
break;
}
@@ -892,7 +891,7 @@ ax_printf (CORE_ADDR fn, CORE_ADDR chan, const char *format,
{
long val = args[i];
- printf (current_substring, val);
+ printf (fmt, val);
break;
}
@@ -905,20 +904,19 @@ ax_printf (CORE_ADDR fn, CORE_ADDR chan, const char *format,
have modified GCC to include -Wformat-security by
default, which will warn here if there is no
argument. */
- printf (current_substring, 0);
+ printf (fmt, 0);
break;
default:
error (_("Format directive in '%s' not supported in agent printf"),
- current_substring);
+ fmt);
}
/* Maybe advance to the next argument. */
- if (fpieces[fp].argclass != literal_piece)
+ if (piece.argclass != literal_piece)
++i;
}
- free_format_pieces (fpieces);
fflush (stdout);
}
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 5d4417fa06..850d5ec9de 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -2427,7 +2427,6 @@ printf_pointer (struct ui_file *stream, const char *format,
static void
ui_printf (const char *arg, struct ui_file *stream)
{
- struct format_piece *fpieces;
const char *s = arg;
struct value **val_args;
int allocated_args = 20;
@@ -2447,9 +2446,7 @@ ui_printf (const char *arg, struct ui_file *stream)
if (*s++ != '"')
error (_("Bad format string, missing '\"'."));
- fpieces = parse_format_string (&s);
-
- make_cleanup (free_format_pieces_cleanup, &fpieces);
+ std::vector<format_piece> fpieces = parse_format_string (&s);
if (*s++ != '"')
error (_("Bad format string, non-terminated '\"'."));
@@ -2466,12 +2463,11 @@ ui_printf (const char *arg, struct ui_file *stream)
{
int nargs = 0;
int nargs_wanted;
- int i, fr;
- char *current_substring;
+ int i;
nargs_wanted = 0;
- for (fr = 0; fpieces[fr].string != NULL; fr++)
- if (fpieces[fr].argclass != literal_piece)
+ for (const format_piece &piece : fpieces)
+ if (piece.argclass != literal_piece)
++nargs_wanted;
/* Now, parse all arguments and evaluate them.
@@ -2499,16 +2495,17 @@ ui_printf (const char *arg, struct ui_file *stream)
/* Now actually print them. */
i = 0;
- for (fr = 0; fpieces[fr].string != NULL; fr++)
+ for (const format_piece &piece : fpieces)
{
- current_substring = fpieces[fr].string;
- switch (fpieces[fr].argclass)
+ const char *fmt = piece.string.c_str ();
+
+ switch (piece.argclass)
{
case string_arg:
- printf_c_string (stream, current_substring, val_args[i]);
+ printf_c_string (stream, fmt, val_args[i]);
break;
case wide_string_arg:
- printf_wide_c_string (stream, current_substring, val_args[i]);
+ printf_wide_c_string (stream, fmt, val_args[i]);
break;
case wide_char_arg:
{
@@ -2535,7 +2532,7 @@ ui_printf (const char *arg, struct ui_file *stream)
&output, translit_char);
obstack_grow_str0 (&output, "");
- fprintf_filtered (stream, current_substring,
+ fprintf_filtered (stream, fmt,
obstack_base (&output));
}
break;
@@ -2544,7 +2541,7 @@ ui_printf (const char *arg, struct ui_file *stream)
{
long long val = value_as_long (val_args[i]);
- fprintf_filtered (stream, current_substring, val);
+ fprintf_filtered (stream, fmt, val);
break;
}
#else
@@ -2554,14 +2551,14 @@ ui_printf (const char *arg, struct ui_file *stream)
{
int val = value_as_long (val_args[i]);
- fprintf_filtered (stream, current_substring, val);
+ fprintf_filtered (stream, fmt, val);
break;
}
case long_arg:
{
long val = value_as_long (val_args[i]);
- fprintf_filtered (stream, current_substring, val);
+ fprintf_filtered (stream, fmt, val);
break;
}
/* Handles floating-point values. */
@@ -2570,11 +2567,11 @@ ui_printf (const char *arg, struct ui_file *stream)
case dec32float_arg:
case dec64float_arg:
case dec128float_arg:
- printf_floating (stream, current_substring, val_args[i],
- fpieces[fr].argclass);
+ printf_floating (stream, fmt, val_args[i],
+ piece.argclass);
break;
case ptr_arg:
- printf_pointer (stream, current_substring, val_args[i]);
+ printf_pointer (stream, fmt, val_args[i]);
break;
case literal_piece:
/* Print a portion of the format string that has no
@@ -2585,14 +2582,14 @@ ui_printf (const char *arg, struct ui_file *stream)
have modified GCC to include -Wformat-security by
default, which will warn here if there is no
argument. */
- fprintf_filtered (stream, current_substring, 0);
+ fprintf_filtered (stream, fmt, 0);
break;
default:
internal_error (__FILE__, __LINE__,
_("failed internal consistency check"));
}
/* Maybe advance to the next argument. */
- if (fpieces[fr].argclass != literal_piece)
+ if (piece.argclass != literal_piece)
++i;
}
}
--
2.15.0