GDB / SIM 7.3.1 Cosmetic patch for profile title

GDB / SIM 7.3.1 Cosmetic patch for profile title

[John Wehle]

Currently doing:

  tomi-unknown-elf-run -v -p program

results in:

  Summary profiling results:

  Summary profiling results:

  Summary profiling results:

  Summary profiling results:

  Instruction Statistics

    Total: 6,212 insns

  ...

The issue being that "Summary profiling results" is displayed multiple
times in a row.  This patch fixes profile_info so that the title is
only displayed once.

The enclosed patch has been tested on FreeBSD with gdb configured for
tomi Borealis (a processor under development by Venray Technology).

ChangeLog:

Mon Oct 17 00:14:40 EDT 2011  John Wehle  (john@feith.com)

	* sim-profile.c (profile_info): Only print the title once.

-- John Wehle
------------------8<------------------------8<------------------------
--- gdb/sim/common/sim-profile.c	2011-03-14 23:16:17.000000000 -0400
+++ gdb/sim/common/sim-profile.c	2011-09-08 23:43:49.000000000 -0400
@@ -1132,7 +1122,7 @@ profile_info (SIM_DESC sd, int verbose)
   /* FIXME: If the number of processors can be selected on the command line,
      then MAX_NR_PROCESSORS will need to take an argument of `sd'.  */
 
-  for (c = 0; c < MAX_NR_PROCESSORS; ++c)
+  for (c = 0; c < MAX_NR_PROCESSORS && ! print_title_p; ++c)
     {
       sim_cpu *cpu = STATE_CPU (sd, c);
       PROFILE_DATA *data = CPU_PROFILE_DATA (cpu);
@@ -1142,6 +1132,7 @@ profile_info (SIM_DESC sd, int verbose)
 	  {
 	    profile_printf (sd, cpu, "Summary profiling results:\n\n");
 	    print_title_p = 1;
+	    break;
 	  }
     }
 
-------------------------------------------------------------------------

Re: GDB / SIM 7.3.1 Cosmetic patch for profile title

[Mike Frysinger]

On Monday 17 October 2011 00:28:22 John Wehle wrote:
> The issue being that "Summary profiling results" is displayed multiple
> times in a row.  This patch fixes profile_info so that the title is
> only displayed once.

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike

Re: GDB / SIM 7.3.1 Cosmetic patch for profile title

[Joel Brobecker]

> The issue being that "Summary profiling results" is displayed multiple
> times in a row.  This patch fixes profile_info so that the title is
> only displayed once.
[...]
> Mon Oct 17 00:14:40 EDT 2011  John Wehle  (john@feith.com)
> 
> 	* sim-profile.c (profile_info): Only print the title once.

This is OK with one little nit:


> -  for (c = 0; c < MAX_NR_PROCESSORS; ++c)
> +  for (c = 0; c < MAX_NR_PROCESSORS && ! print_title_p; ++c)
                                          ^^^^^^^^^^^^^^^
                                  No space after the '!'.

As far as I can tell, you do not have a copyright assignment in
place for GDB (it looks like you assigned some specific changes
many many years ago, but nothing that covers those changes).
I will commit them using the "small change" rule, but if you think
you might contribute other changes, I invite you to start the process
now, so that future patches do not stay held up just because of
paperwork.

As a side note, it took me a while to understand some of the logic.
It looks like `print_title_p' really means `we-have-some-profile-data'.

So, if it was me, I'd probably rename that variable to something
like: profile_data_collected_p, and then rewrite the code such that
the double-loop only sets that variable.  And then have a simple
if block:

    if (profile_data_collected_p)
      profile_printf (sd, cpu, "Summary profiling results:\n\n");

The only other place where this variable is reference would also benefit
from that renaming, since...

      if (print_title_p
          && (PROFILE_INSN_P (cpu)
              || PROFILE_MODEL_P (cpu)))
        profile_print_addr_ranges (cpu);

... would become:

      if (profile_data_collected_p
          && (PROFILE_INSN_P (cpu)
              || PROFILE_MODEL_P (cpu)))
        profile_print_addr_ranges (cpu);

-- 
Joel

Re: GDB / SIM 7.3.1 Cosmetic patch for profile title

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 347 bytes --]

> As a side note, it took me a while to understand some of the logic.
> It looks like `print_title_p' really means `we-have-some-profile-data'.
> 
> So, if it was me, [...]

In fact, here is what I meant. If someone can test it and confirm
that I didn't screw something up, we could commit that. I think
it's a lot clearer this way, no?

-- 
Joel

[-- Attachment #2: sim-common-profile.diff --]
[-- Type: text/x-diff, Size: 3049 bytes --]

commit 0e2091a0b6727c5ffb874df6c19bb08d7654f8d8
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Tue Oct 18 18:01:04 2011 -0700

    [sim] minor cleanup of sim-profile.c:profile_info
    
    Re-arange the code a little bit to make it clearer what the intent
    of each piece is.
    
    sim/common/ChangeLog:
    
            * sim-profile.c (sim_desc_has_profile_data): New function,
            mostly extracted out of profile_info.
            (profile_info): Rename `print_title_p' into
            `profile_data_collected_p'. Replace extracted-out code by
            call to sim_desc_has_profile_data.

diff --git a/sim/common/sim-profile.c b/sim/common/sim-profile.c
index 83e964d..3221ecf 100644
--- a/sim/common/sim-profile.c
+++ b/sim/common/sim-profile.c
@@ -1109,6 +1109,32 @@ profile_print_addr_ranges (sim_cpu *cpu)
 }
 #endif
 
+/* Return nonzero if some profile data has been collected, zero
+   otherwise.  */
+
+static int
+sim_desc_has_profile_data (SIM_DESC sd)
+{
+  int i, c;
+
+  /* FIXME: If the number of processors can be selected on the command line,
+     then MAX_NR_PROCESSORS will need to take an argument of `sd'.  */
+
+  for (c = 0; c < MAX_NR_PROCESSORS; ++c)
+    {
+      sim_cpu *cpu = STATE_CPU (sd, c);
+      PROFILE_DATA *data = CPU_PROFILE_DATA (cpu);
+
+      for (i = 0; i < MAX_PROFILE_VALUES; ++i)
+	if (PROFILE_FLAGS (data) [i])
+	  return 1;
+    }
+
+  /* No profile data found.  Return zero.  */
+  return 0;
+
+}
+
 /* Top level function to print all summary profile information.
    It is [currently] intended that all such data is printed by this function.
    I'd rather keep it all in one place for now.  To that end, MISC_CPU and
@@ -1124,27 +1150,14 @@ profile_print_addr_ranges (sim_cpu *cpu)
 static void
 profile_info (SIM_DESC sd, int verbose)
 {
-  int i,c;
-  int print_title_p = 0;
+  int c;
+  const int profile_data_collected_p = sim_desc_has_profile_data (sd);
 
-  /* Only print the title if some data has been collected.  */
   /* ??? Why don't we just exit if no data collected?  */
-  /* FIXME: If the number of processors can be selected on the command line,
-     then MAX_NR_PROCESSORS will need to take an argument of `sd'.  */
-
-  for (c = 0; c < MAX_NR_PROCESSORS && !print_title_p; ++c)
-    {
-      sim_cpu *cpu = STATE_CPU (sd, c);
-      PROFILE_DATA *data = CPU_PROFILE_DATA (cpu);
 
-      for (i = 0; i < MAX_PROFILE_VALUES; ++i)
-	if (PROFILE_FLAGS (data) [i])
-	  {
-	    profile_printf (sd, cpu, "Summary profiling results:\n\n");
-	    print_title_p = 1;
-	    break;
-	  }
-    }
+  /* Only print the title if some data has been collected.  */
+  if (profile_data_collected_p)
+    profile_printf (sd, cpu, "Summary profiling results:\n\n");
 
   /* Loop, cpu by cpu, printing results.  */
 
@@ -1179,7 +1192,7 @@ profile_info (SIM_DESC sd, int verbose)
 	}
 
 #ifdef SIM_HAVE_ADDR_RANGE
-      if (print_title_p
+      if (profile_data_collected_p
 	  && (PROFILE_INSN_P (cpu)
 	      || PROFILE_MODEL_P (cpu)))
 	profile_print_addr_ranges (cpu);

Re: GDB / SIM 7.3.1 Cosmetic patch for profile title

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1204 bytes --]

On Tuesday 18 October 2011 21:05:22 Joel Brobecker wrote:
> > As a side note, it took me a while to understand some of the logic.
> > It looks like `print_title_p' really means `we-have-some-profile-data'.
> > 
> > So, if it was me, [...]
> 
> In fact, here is what I meant. If someone can test it and confirm
> that I didn't screw something up, we could commit that. I think
> it's a lot clearer this way, no?

since profile_printf() takes a cpu arg, this fails to build once the header 
printf is pulled out from the loop where "cpu" is declared.

i think the intention might have been for the header to be printed once per 
cpu since the output log file exists per cpu.  so if you want to pull out the 
inner loop into like sim_cpu_has_profile_data (SIM_DESC sd, sim_cpu *cpu), that 
would probably work, but we'd still want the outer loop that walks all the 
cpu's in the profile_info() func.  maybe we can merge that loop with the cpu 
loop right below ...

> --- a/sim/common/sim-profile.c
> +++ b/sim/common/sim-profile.c
> 
> +  /* No profile data found.  Return zero.  */
> +  return 0;
> +
> +}

don't think you want that blank line before the closing brace
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: GDB / SIM 7.3.1 Cosmetic patch for profile title

[Joel Brobecker]

> since profile_printf() takes a cpu arg, this fails to build once the
> header printf is pulled out from the loop where "cpu" is declared.

Ah, correct. A quick look at that function reveals that it only needs
the "cpu" to get access to the associaed PROFILE_FILE. Not sure what
this is and why it is necessary, rather than doing a simple printf.

In any case, sounded like a good idea, but maybe it wasn't. I'll leave
it to you guys if you want to pursue it.

-- 
Joel

Re: GDB / SIM 7.3.1 Cosmetic patch for profile title

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1458 bytes --]

On Thursday 20 October 2011 11:59:18 Joel Brobecker wrote:
> > since profile_printf() takes a cpu arg, this fails to build once the
> > header printf is pulled out from the loop where "cpu" is declared.
> 
> Ah, correct. A quick look at that function reveals that it only needs
> the "cpu" to get access to the associaed PROFILE_FILE. Not sure what
> this is and why it is necessary, rather than doing a simple printf.
> 
> In any case, sounded like a good idea, but maybe it wasn't. I'll leave
> it to you guys if you want to pursue it.

the sim profile code has a --profile-file flag so that you can send the profile 
output to a dedicated file.  this is really useful when you're trying to pick 
apart all the other things the sim may output -- tracing, output from the 
simulated program itself, etc...  when you've got like 4+ different sources of 
information sent to one or two fd's, you can see how dedicated file flags are a 
hard necessity :).

fundamentally, the profile code operates on a per-cpu basis which makes sense 
in that the execution/profile of each cpu looks very different.  and the profile 
output code follows this convention.

atm, in practice, all output is sent to the same file handle, and most (all?) 
sims i know of only support one cpu.  so it's a bit of a wash, but i think we 
should stick to supporting the multi-cpu concept everywhere so that future 
work is much easier.

</braindump>
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]