From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 58480 invoked by alias); 13 Oct 2016 22:43:04 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 58458 invoked by uid 89); 13 Oct 2016 22:43:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=waited, 23,9, *command, lim X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 13 Oct 2016 22:42:53 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 792B0C057FAD; Thu, 13 Oct 2016 22:42:52 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9DMgoAH012090; Thu, 13 Oct 2016 18:42:51 -0400 Subject: Re: [RFA v2 09/17] Change command stats reporting to use class To: Tom Tromey , gdb-patches@sourceware.org References: <1476393012-29987-1-git-send-email-tom@tromey.com> <1476393012-29987-10-git-send-email-tom@tromey.com> From: Pedro Alves Message-ID: Date: Thu, 13 Oct 2016 22:43:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <1476393012-29987-10-git-send-email-tom@tromey.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-10/txt/msg00418.txt.bz2 On 10/13/2016 10:10 PM, Tom Tromey wrote: > This removes make_command_stats_cleanup in favor of an RAII class. > The patch is reasonably straightforward, but keeping the same > semantics without excessive reindentation required splitting > captured_main in two. > > 2016-09-26 Tom Tromey > > * maint.h (scoped_command_stats): New class. > (make_command_stats_cleanup): Don't declare. > * maint.c (struct cmd_stats): Remove. > (~scoped_command_stats): Rename from report_command_stats. Now a > destructor. > (scoped_command_stats): Rename from make_command_stats_cleanup. > Now a constructor. > * main.c (captured_main_1): New function. Use > scoped_command_stats. > (captured_main): Call captured_main_1. > * event-top.c (command_handler): Use scoped_command_stats. > --- > gdb/ChangeLog | 14 +++++++ > gdb/event-top.c | 5 +-- > gdb/main.c | 25 +++++++----- > gdb/maint.c | 118 ++++++++++++++++++-------------------------------------- > gdb/maint.h | 39 +++++++++++++++++-- > 5 files changed, 104 insertions(+), 97 deletions(-) > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index 7c072b5..f476299 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,5 +1,19 @@ > 2016-09-26 Tom Tromey > > + * maint.h (scoped_command_stats): New class. > + (make_command_stats_cleanup): Don't declare. > + * maint.c (struct cmd_stats): Remove. > + (~scoped_command_stats): Rename from report_command_stats. Now a > + destructor. > + (scoped_command_stats): Rename from make_command_stats_cleanup. > + Now a constructor. > + * main.c (captured_main_1): New function. Use > + scoped_command_stats. > + (captured_main): Call captured_main_1. > + * event-top.c (command_handler): Use scoped_command_stats. > + > +2016-09-26 Tom Tromey > + > * mi/mi-main.c (mi_cmd_data_read_memory): Use gdb::unique_ptr. > Remove some cleanups. > > diff --git a/gdb/event-top.c b/gdb/event-top.c > index 9b0ccbc..c452501 100644 > --- a/gdb/event-top.c > +++ b/gdb/event-top.c > @@ -562,13 +562,12 @@ void > command_handler (char *command) > { > struct ui *ui = current_ui; > - struct cleanup *stat_chain; > char *c; > > if (ui->instream == ui->stdin_stream) > reinitialize_more_filter (); > > - stat_chain = make_command_stats_cleanup (1); > + scoped_command_stats stat_reporter (1); > > /* Do not execute commented lines. */ > for (c = command; *c == ' ' || *c == '\t'; c++) > @@ -580,8 +579,6 @@ command_handler (char *command) > /* Do any commands attached to breakpoint we stopped at. */ > bpstat_do_actions (); > } > - > - do_cleanups (stat_chain); > } > > /* Append RL, an input line returned by readline or one of its > diff --git a/gdb/main.c b/gdb/main.c > index 2ea9466..420b4d3 100644 > --- a/gdb/main.c > +++ b/gdb/main.c > @@ -441,12 +441,12 @@ typedef struct cmdarg { > /* Define type VEC (cmdarg_s). */ > DEF_VEC_O (cmdarg_s); > > -static int > -captured_main (void *data) > +static void > +captured_main_1 (struct captured_main_args *context) > { > - struct captured_main_args *context = (struct captured_main_args *) data; > int argc = context->argc; > char **argv = context->argv; > + > static int quiet = 0; > static int set_args = 0; > static int inhibit_home_gdbinit = 0; > @@ -486,14 +486,14 @@ captured_main (void *data) > int save_auto_load; > struct objfile *objfile; > > - struct cleanup *pre_stat_chain; > + struct cleanup *chain; > > #ifdef HAVE_SBRK > - /* Set this before calling make_command_stats_cleanup. */ > + /* Set this before constructing scoped_command_stats. */ > lim_at_start = (char *) sbrk (0); > #endif > > - pre_stat_chain = make_command_stats_cleanup (0); > + scoped_command_stats stat_reporter (0); > > #if defined (HAVE_SETLOCALE) && defined (HAVE_LC_MESSAGES) > setlocale (LC_MESSAGES, ""); > @@ -510,7 +510,7 @@ captured_main (void *data) > notice_open_fds (); > save_original_signals_state (); > > - make_cleanup (VEC_cleanup (cmdarg_s), &cmdarg_vec); > + chain = make_cleanup (VEC_cleanup (cmdarg_s), &cmdarg_vec); > dirsize = 1; > dirarg = (char **) xmalloc (dirsize * sizeof (*dirarg)); > ndir = 0; > @@ -1139,8 +1139,15 @@ captured_main (void *data) > quit_force (NULL, 0); > } > > - /* Show time and/or space usage. */ > - do_cleanups (pre_stat_chain); > + do_cleanups (chain); > +} > + > +static void > +captured_main (void *data) > +{ > + struct captured_main_args *context = (struct captured_main_args *) data; > + > + captured_main_1 (context); > > /* NOTE: cagney/1999-11-07: There is probably no reason for not > moving this loop and the code found in captured_command_loop() > diff --git a/gdb/maint.c b/gdb/maint.c > index d2c9346..06e6766 100644 > --- a/gdb/maint.c > +++ b/gdb/maint.c > @@ -752,31 +752,6 @@ static int per_command_symtab; > static struct cmd_list_element *per_command_setlist; > static struct cmd_list_element *per_command_showlist; > > -/* Records a run time and space usage to be used as a base for > - reporting elapsed time or change in space. */ > - > -struct cmd_stats > -{ > - /* Zero if the saved time is from the beginning of GDB execution. > - One if from the beginning of an individual command execution. */ > - int msg_type; > - /* Track whether the stat was enabled at the start of the command > - so that we can avoid printing anything if it gets turned on by > - the current command. */ > - int time_enabled : 1; > - int space_enabled : 1; > - int symtab_enabled : 1; > - long start_cpu_time; > - struct timeval start_wall_time; > - long start_space; > - /* Total number of symtabs (over all objfiles). */ > - int start_nr_symtabs; > - /* A count of the compunits. */ > - int start_nr_compunit_symtabs; > - /* Total number of blocks. */ > - int start_nr_blocks; > -}; > - > /* Set whether to display time statistics to NEW_VALUE > (non-zero means true). */ > > @@ -827,31 +802,38 @@ count_symtabs_and_blocks (int *nr_symtabs_ptr, int *nr_compunit_symtabs_ptr, > *nr_blocks_ptr = nr_blocks; > } > > -/* As indicated by display_time and display_space, report GDB's elapsed time > - and space usage from the base time and space provided in ARG, which > - must be a pointer to a struct cmd_stat. This function is intended > - to be called as a cleanup. */ > +/* As indicated by display_time and display_space, report GDB's > + elapsed time and space usage from the base time and space recorded > + in this object. */ > > -static void > -report_command_stats (void *arg) > +scoped_command_stats::~scoped_command_stats () > { > - struct cmd_stats *start_stats = (struct cmd_stats *) arg; > - int msg_type = start_stats->msg_type; > + /* Early exit if we're not reporting any stats. It can be expensive to > + compute the pre-command values so don't collect them at all if we're > + not reporting stats. Alas this doesn't work in the startup case because > + we don't know yet whether we will be reporting the stats. For the > + startup case collect the data anyway (it should be cheap at this point), > + and leave it to the reporter to decide whether to print them. */ > + if (m_msg_type != 0 > + && !per_command_time > + && !per_command_space > + && !per_command_symtab) > + return; > > - if (start_stats->time_enabled && per_command_time) > + if (this->m_time_enabled && per_command_time) s/this->// ? > { > - long cmd_time = get_run_time () - start_stats->start_cpu_time; > + long cmd_time = get_run_time () - this->m_start_cpu_time; Ditto. > struct timeval now_wall_time, delta_wall_time, wait_time; > > gettimeofday (&now_wall_time, NULL); > timeval_sub (&delta_wall_time, > - &now_wall_time, &start_stats->start_wall_time); > + &now_wall_time, &this->m_start_wall_time); Ditto. > > /* Subtract time spend in prompt_for_continue from walltime. */ > wait_time = get_prompt_for_continue_wait_time (); > timeval_sub (&delta_wall_time, &delta_wall_time, &wait_time); > > - printf_unfiltered (msg_type == 0 > + printf_unfiltered (m_msg_type == 0 > ? _("Startup time: %ld.%06ld (cpu), %ld.%06ld (wall)\n") > : _("Command execution time: %ld.%06ld (cpu), %ld.%06ld (wall)\n"), > cmd_time / 1000000, cmd_time % 1000000, > @@ -859,15 +841,15 @@ report_command_stats (void *arg) > (long) delta_wall_time.tv_usec); > } > > - if (start_stats->space_enabled && per_command_space) > + if (this->m_space_enabled && per_command_space) > { > #ifdef HAVE_SBRK > char *lim = (char *) sbrk (0); > > long space_now = lim - lim_at_start; > - long space_diff = space_now - start_stats->start_space; > + long space_diff = space_now - this->m_start_space; > > - printf_unfiltered (msg_type == 0 > + printf_unfiltered (m_msg_type == 0 > ? _("Space used: %ld (%s%ld during startup)\n") > : _("Space used: %ld (%s%ld for this command)\n"), > space_now, > @@ -876,7 +858,7 @@ report_command_stats (void *arg) > #endif > } > > - if (start_stats->symtab_enabled && per_command_symtab) > + if (this->m_symtab_enabled && per_command_symtab) > { > int nr_symtabs, nr_compunit_symtabs, nr_blocks; > > @@ -885,54 +867,32 @@ report_command_stats (void *arg) > " #compunits: %d (+%d)," > " #blocks: %d (+%d)\n"), > nr_symtabs, > - nr_symtabs - start_stats->start_nr_symtabs, > + nr_symtabs - this->m_start_nr_symtabs, > nr_compunit_symtabs, > (nr_compunit_symtabs > - - start_stats->start_nr_compunit_symtabs), > + - this->m_start_nr_compunit_symtabs), > nr_blocks, > - nr_blocks - start_stats->start_nr_blocks); > + nr_blocks - this->m_start_nr_blocks); Several more. > } > } > > -/* Create a cleanup that reports time and space used since its creation. > - MSG_TYPE is zero for gdb startup, otherwise it is one(1) to report > - data for individual commands. */ > - > -struct cleanup * > -make_command_stats_cleanup (int msg_type) > +scoped_command_stats::scoped_command_stats (int msg_type) > +: m_msg_type (msg_type) > { > - struct cmd_stats *new_stat; > - > - /* Early exit if we're not reporting any stats. It can be expensive to > - compute the pre-command values so don't collect them at all if we're > - not reporting stats. Alas this doesn't work in the startup case because > - we don't know yet whether we will be reporting the stats. For the > - startup case collect the data anyway (it should be cheap at this point), > - and leave it to the reporter to decide whether to print them. */ > - if (msg_type != 0 > - && !per_command_time > - && !per_command_space > - && !per_command_symtab) > - return make_cleanup (null_cleanup, 0); > - > - new_stat = XCNEW (struct cmd_stats); > - > - new_stat->msg_type = msg_type; > - > - if (msg_type == 0 || per_command_space) > + if (m_msg_type == 0 || per_command_space) > { > #ifdef HAVE_SBRK > char *lim = (char *) sbrk (0); > - new_stat->start_space = lim - lim_at_start; > - new_stat->space_enabled = 1; > + this->m_start_space = lim - lim_at_start; > + this->m_space_enabled = 1; > #endif > } > > if (msg_type == 0 || per_command_time) > { > - new_stat->start_cpu_time = get_run_time (); > - gettimeofday (&new_stat->start_wall_time, NULL); > - new_stat->time_enabled = 1; > + this->m_start_cpu_time = get_run_time (); > + gettimeofday (&this->m_start_wall_time, NULL); > + this->m_time_enabled = 1; > } > > if (msg_type == 0 || per_command_symtab) > @@ -940,16 +900,14 @@ make_command_stats_cleanup (int msg_type) > int nr_symtabs, nr_compunit_symtabs, nr_blocks; > > count_symtabs_and_blocks (&nr_symtabs, &nr_compunit_symtabs, &nr_blocks); > - new_stat->start_nr_symtabs = nr_symtabs; > - new_stat->start_nr_compunit_symtabs = nr_compunit_symtabs; > - new_stat->start_nr_blocks = nr_blocks; > - new_stat->symtab_enabled = 1; > + this->m_start_nr_symtabs = nr_symtabs; > + this->m_start_nr_compunit_symtabs = nr_compunit_symtabs; > + this->m_start_nr_blocks = nr_blocks; > + this->m_symtab_enabled = 1; > } > Many more. > /* Initalize timer to keep track of how long we waited for the user. */ > reset_prompt_for_continue_wait_time (); > - > - return make_cleanup_dtor (report_command_stats, new_stat, xfree); > } > > /* Handle unknown "mt set per-command" arguments. > diff --git a/gdb/maint.h b/gdb/maint.h > index 841e790..ffbf0cb 100644 > --- a/gdb/maint.h > +++ b/gdb/maint.h > @@ -23,9 +23,40 @@ extern void set_per_command_time (int); > > extern void set_per_command_space (int); > > -/* Note: There's no set_per_command_symtab on purpose. > - Symtab stats aren't yet as useful for --statistics output. */ > - > -extern struct cleanup *make_command_stats_cleanup (int); > +/* Records a run time and space usage to be used as a base for > + reporting elapsed time or change in space. */ > + > +class scoped_command_stats > +{ > + public: > + > + scoped_command_stats (int msg_type); explicit? > + ~scoped_command_stats (); > + > + private: > + > + // No need for these. They are intentionally not defined anywhere. > + scoped_command_stats &operator= (const scoped_command_stats &); > + scoped_command_stats (const scoped_command_stats &); > + > + /* Zero if the saved time is from the beginning of GDB execution. > + One if from the beginning of an individual command execution. */ > + int m_msg_type; Bool? > + /* Track whether the stat was enabled at the start of the command > + so that we can avoid printing anything if it gets turned on by > + the current command. */ > + int m_time_enabled : 1; > + int m_space_enabled : 1; > + int m_symtab_enabled : 1; > + long m_start_cpu_time; > + struct timeval m_start_wall_time; > + long m_start_space; > + /* Total number of symtabs (over all objfiles). */ > + int m_start_nr_symtabs; > + /* A count of the compunits. */ > + int m_start_nr_compunit_symtabs; > + /* Total number of blocks. */ > + int m_start_nr_blocks; > +}; > > #endif /* MAINT_H */ > OK with those changes. Thanks, Pedro Alves