diff mbox

Re: Regression for GDB global --statistics

Message ID 21437.45024.997531.453750@ruffy.mtv.corp.google.com
State New
Headers show

Commit Message

Doug Evans July 9, 2014, 9:10 p.m. UTC
Jan Kratochvil writes:
 > On Fri, 15 Mar 2013 23:29:09 +0100, Doug Evans wrote:
 > > This patch adds a new option to display some simple symtab stats
 > > akin to how "mt time|space 1" work.  For consistency with the
 > > rest of gdb I named it:
 > > 
 > > maint set per-command symtab on|off
 > > maint show per-command symtab
 > 
 > bd712aed2f88ab824d403c55a212c2be3f41a335 is the first bad commit
 > commit bd712aed2f88ab824d403c55a212c2be3f41a335
 > Author: Doug Evans <dje@google.com>
 > Date:   Thu Mar 21 17:37:30 2013 +0000
 >         New commands "mt set per-command {space,time,symtab} {on,off}".
 > 
 > before (gdb-7.6):
 > gdb -nx -q -statistics  </dev/null
 > Startup time: 0.031000 (cpu), 0.033828 (wall)
 > Space used: 4005888 (+4005888 during startup)
 > (gdb) quit
 > 
 > after (gdb-7.7+):
 > gdb -nx -q -statistics  </dev/null
 > (gdb) quit
 > 
 > Commands stats work now but no longer the global stats.  Command stats look
 > differently ("for this command" vs. "during startup"):
 > echo echo|gdb -nx -q -statistics 
 > (gdb) Command execution time: 0.000000 (cpu), 0.000008 (wall)
 > Space used: 4206592 (+0 for this command)
 > (gdb) quit
 > 
 > It has been found by Miroslav Franc.

Thanks.

Here's a patch.

2014-07-09  Doug Evans  <dje@google.com>

	* maint.c (count_symtabs_and_blocks): Handle NULL
	current_program_space.
	(report_command_stats): Check global enabled flag in addition to
	recorded enabled flag.
	(make_command_stats_cleanup): Handle msg_type == 0, startup.

	testsuite/
	* gdb.base/maint.exp: Update testing of per-command stats.

Comments

Pedro Alves July 10, 2014, 4:42 p.m. UTC | #1
Hi Doug,

On 07/09/2014 10:10 PM, Doug Evans wrote:
> Here's a patch.
> 
> 2014-07-09  Doug Evans  <dje@google.com>
> 
> 	* maint.c (count_symtabs_and_blocks): Handle NULL
> 	current_program_space.

It's not obvious to me how this can be NULL, given we initialize
it so early.  If there's a good reason, could you please add
a comment here mentioning it?

Thanks,
Pedro Alves
Jan Kratochvil July 11, 2014, 9:25 p.m. UTC | #2
On Wed, 09 Jul 2014 23:10:56 +0200, Doug Evans wrote:
> --- a/gdb/testsuite/gdb.base/maint.exp
> +++ b/gdb/testsuite/gdb.base/maint.exp
> @@ -130,10 +130,11 @@ gdb_expect  {
>  # tests here!!
>  gdb_test_no_output "maint check-symtabs"
>  
> +# Test per-command stats.
>  gdb_test_no_output "maint set per-command on"
> -
> -gdb_test "maint set per-command off" \
> +gdb_test "pwd" \
>      "Command execution time: \[0-9.\]+ \\(cpu\\), \[0-9.\]+ \\(wall\\)\[\r\n\]+Space used: $decimal \\(\\+$decimal for this command\\)\[\r\n\]+#symtabs: $decimal \\(\\+$decimal\\), #primary symtabs: $decimal \\(\\+$decimal\\), #blocks: $decimal \\(\\+$decimal\\)"
> +gdb_test_no_output "maint set per-command off"

Is it needed to change this behavior?  It may break some user scripts working
fine with previous GDB stable releases.

Besides that the startup statistics regression is not tested here but I can
post then a testcase for it.


Thanks,
Jan
Doug Evans July 11, 2014, 9:33 p.m. UTC | #3
On Fri, Jul 11, 2014 at 2:25 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Wed, 09 Jul 2014 23:10:56 +0200, Doug Evans wrote:
>> --- a/gdb/testsuite/gdb.base/maint.exp
>> +++ b/gdb/testsuite/gdb.base/maint.exp
>> @@ -130,10 +130,11 @@ gdb_expect  {
>>  # tests here!!
>>  gdb_test_no_output "maint check-symtabs"
>>
>> +# Test per-command stats.
>>  gdb_test_no_output "maint set per-command on"
>> -
>> -gdb_test "maint set per-command off" \
>> +gdb_test "pwd" \
>>      "Command execution time: \[0-9.\]+ \\(cpu\\), \[0-9.\]+ \\(wall\\)\[\r\n\]+Space used: $decimal \\(\\+$decimal for this command\\)\[\r\n\]+#symtabs: $decimal \\(\\+$decimal\\), #primary symtabs: $decimal \\(\\+$decimal\\), #blocks: $decimal \\(\\+$decimal\\)"
>> +gdb_test_no_output "maint set per-command off"
>
> Is it needed to change this behavior?  It may break some user scripts working
> fine with previous GDB stable releases.

Change in behaviour of something related to a maintenance command?
Or were you referring to something else?

One problem I wish to avoid is not doing anything expensive unnecessarily.
And here that translates to not computing the values of statistics
from before a command executes (so that one can then print the delta
after it completes) if said statistics will not be printed.  That then
translates to any turning on of a statistics-reporting command (mt set
per-command foo) will not take effect until the next command.  How
will that be a problem?

> Besides that the startup statistics regression is not tested here but I can
> post then a testcase for it.

As you wish.  Thanks.
diff mbox

Patch

diff --git a/gdb/maint.c b/gdb/maint.c
index 5f34af3..5b11f24 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -830,13 +830,16 @@  count_symtabs_and_blocks (int *nr_symtabs_ptr, int *nr_primary_symtabs_ptr,
   int nr_primary_symtabs = 0;
   int nr_blocks = 0;
 
-  ALL_SYMTABS (o, s)
+  if (current_program_space != NULL)
     {
-      ++nr_symtabs;
-      if (s->primary)
+      ALL_SYMTABS (o, s)
 	{
-	  ++nr_primary_symtabs;
-	  nr_blocks += BLOCKVECTOR_NBLOCKS (BLOCKVECTOR (s));
+	  ++nr_symtabs;
+	  if (s->primary)
+	    {
+	      ++nr_primary_symtabs;
+	      nr_blocks += BLOCKVECTOR_NBLOCKS (BLOCKVECTOR (s));
+	    }
 	}
     }
 
@@ -856,7 +859,7 @@  report_command_stats (void *arg)
   struct cmd_stats *start_stats = (struct cmd_stats *) arg;
   int msg_type = start_stats->msg_type;
 
-  if (start_stats->time_enabled)
+  if (start_stats->time_enabled && per_command_time)
     {
       long cmd_time = get_run_time () - start_stats->start_cpu_time;
       struct timeval now_wall_time, delta_wall_time, wait_time;
@@ -877,7 +880,7 @@  report_command_stats (void *arg)
 			 (long) delta_wall_time.tv_usec);
     }
 
-  if (start_stats->space_enabled)
+  if (start_stats->space_enabled && per_command_space)
     {
 #ifdef HAVE_SBRK
       char *lim = (char *) sbrk (0);
@@ -894,7 +897,7 @@  report_command_stats (void *arg)
 #endif
     }
 
-  if (start_stats->symtab_enabled)
+  if (start_stats->symtab_enabled && per_command_symtab)
     {
       int nr_symtabs, nr_primary_symtabs, nr_blocks;
 
@@ -920,8 +923,14 @@  make_command_stats_cleanup (int msg_type)
 {
   struct cmd_stats *new_stat;
 
-  /* Early exit if we're not reporting any stats.  */
-  if (!per_command_time
+  /* 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);
@@ -930,7 +939,7 @@  make_command_stats_cleanup (int msg_type)
 
   new_stat->msg_type = msg_type;
 
-  if (per_command_space)
+  if (msg_type == 0 || per_command_space)
     {
 #ifdef HAVE_SBRK
       char *lim = (char *) sbrk (0);
@@ -939,14 +948,14 @@  make_command_stats_cleanup (int msg_type)
 #endif
     }
 
-  if (per_command_time)
+  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;
     }
 
-  if (per_command_symtab)
+  if (msg_type == 0 || per_command_symtab)
     {
       int nr_symtabs, nr_primary_symtabs, nr_blocks;
 
diff --git a/gdb/testsuite/gdb.base/maint.exp b/gdb/testsuite/gdb.base/maint.exp
index 64753b7..21d0a31 100644
--- a/gdb/testsuite/gdb.base/maint.exp
+++ b/gdb/testsuite/gdb.base/maint.exp
@@ -130,10 +130,11 @@  gdb_expect  {
 # tests here!!
 gdb_test_no_output "maint check-symtabs"
 
+# Test per-command stats.
 gdb_test_no_output "maint set per-command on"
-
-gdb_test "maint set per-command off" \
+gdb_test "pwd" \
     "Command execution time: \[0-9.\]+ \\(cpu\\), \[0-9.\]+ \\(wall\\)\[\r\n\]+Space used: $decimal \\(\\+$decimal for this command\\)\[\r\n\]+#symtabs: $decimal \\(\\+$decimal\\), #primary symtabs: $decimal \\(\\+$decimal\\), #blocks: $decimal \\(\\+$decimal\\)"
+gdb_test_no_output "maint set per-command off"
 
 gdb_test "maint demangle" \
     "\"maintenance demangle\" takes an argument to demangle\\."