From patchwork Sat Dec 2 20:31:33 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 24693 Received: (qmail 61857 invoked by alias); 2 Dec 2017 20:31:53 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 61840 invoked by uid 89); 2 Dec 2017 20:31:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy=sk:pieces, 946, doubling, 90419 X-HELO: barracuda.ebox.ca Received: from barracuda.ebox.ca (HELO barracuda.ebox.ca) (96.127.255.19) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 02 Dec 2017 20:31:48 +0000 X-ASG-Debug-ID: 1512246695-0c856e65d53e2a420001-fS2M51 Received: from smtp.ebox.ca (smtp.electronicbox.net [96.127.255.82]) by barracuda.ebox.ca with ESMTP id 9jmOGLH3AOxw7m1M (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 02 Dec 2017 15:31:35 -0500 (EST) X-Barracuda-Envelope-From: simon.marchi@polymtl.ca X-Barracuda-RBL-Trusted-Forwarder: 96.127.255.82 Received: from simark.lan (192-222-251-162.qc.cable.ebox.net [192.222.251.162]) by smtp.ebox.ca (Postfix) with ESMTP id EBC67441D64; Sat, 2 Dec 2017 15:31:34 -0500 (EST) From: Simon Marchi X-Barracuda-Effective-Source-IP: 192-222-251-162.qc.cable.ebox.net[192.222.251.162] X-Barracuda-Apparent-Source-IP: 192.222.251.162 X-Barracuda-RBL-IP: 192.222.251.162 To: gdb-patches@sourceware.org Cc: Tom Tromey , Pedro Alves , Simon Marchi Subject: [PATCH] C++-ify parse_format_string Date: Sat, 2 Dec 2017 15:31:33 -0500 X-ASG-Orig-Subj: [PATCH] C++-ify parse_format_string Message-Id: <20171202203133.10770-1-simon.marchi@polymtl.ca> In-Reply-To: <87h8tivyo4.fsf@tromey.com> References: <87h8tivyo4.fsf@tromey.com> X-Barracuda-Connect: smtp.electronicbox.net[96.127.255.82] X-Barracuda-Start-Time: 1512246695 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://96.127.255.19:443/cgi-mod/mark.cgi X-Barracuda-Scan-Msg-Size: 21171 X-Barracuda-BRTS-Status: 1 X-Barracuda-Spam-Score: 0.50 X-Barracuda-Spam-Status: No, SCORE=0.50 using global scores of TAG_LEVEL=1000.0 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=8.0 tests=BSF_RULE7568M X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.45456 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- 0.50 BSF_RULE7568M Custom Rule 7568M X-IsSubscribed: yes From: Simon Marchi 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. : 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 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 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 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 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 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 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 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; } }