[PATCH] sim: add --map-info option

[PATCH] sim: add --map-info option

[Mike Frysinger]

There are options for listing the current device/hw tree and memory
regions, but no way to find out at run time all the current mappings.
So add a new --map-info option akin to the --memory-info option which
displays all the current mappings.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>

2010-12-01  Mike Frysinger  <vapier@gentoo.org>

	* sim-memopt.c (OPTION_MAP_INFO): Define.
	(memory_options): Handle --map-info.
	(memory_option_handler): Handle OPTION_MAP_INFO.
---
 sim/common/sim-memopt.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 50 insertions(+), 1 deletions(-)

diff --git a/sim/common/sim-memopt.c b/sim/common/sim-memopt.c
index 47b6c9e..9f58ec5 100644
--- a/sim/common/sim-memopt.c
+++ b/sim/common/sim-memopt.c
@@ -67,7 +67,8 @@ enum {
   OPTION_MEMORY_ALIAS,
   OPTION_MEMORY_CLEAR,
   OPTION_MEMORY_FILL,
-  OPTION_MEMORY_MAPFILE
+  OPTION_MEMORY_MAPFILE,
+  OPTION_MAP_INFO
 };
 
 static DECLARE_OPTION_HANDLER (memory_option_handler);
@@ -113,6 +114,9 @@ static const OPTION memory_options[] =
   { {"info-memory", no_argument, NULL, OPTION_MEMORY_INFO },
       '\0', NULL, NULL,
       memory_option_handler },
+  { {"map-info", no_argument, NULL, OPTION_MAP_INFO },
+      '\0', NULL, "List mapped regions",
+      memory_option_handler },
 
   { {NULL, no_argument, NULL, 0}, '\0', NULL, NULL, NULL }
 };
@@ -520,6 +524,51 @@ memory_option_handler (SIM_DESC sd, sim_cpu *cpu, int opt,
 	break;
       }
 
+    case OPTION_MAP_INFO:
+      {
+	sim_core *memory;
+	unsigned nr_map;
+
+	for (memory = STATE_CORE (sd), nr_map = 0; nr_map < nr_maps; ++nr_map)
+	  {
+	    sim_core_map *map = &memory->common.map[nr_map];
+	    sim_core_mapping *mapping = map->first;
+
+	    if (!mapping)
+	      continue;
+
+	    if (nr_map <= io_map)
+	      sim_io_printf (sd, "%s maps:\n",
+			     (nr_map == read_map) ? "read" :
+			     (nr_map == write_map) ? "write" :
+			     (nr_map == exec_map) ? "exec" :
+			     /*(nr_map == io_map) ?*/ "io");
+	    else
+	      sim_io_printf (sd, "??? (%u) maps:\n", nr_map);
+
+	    do
+	      {
+		unsigned modulo;
+		sim_io_printf (sd, " map ");
+		if (mapping->space != 0)
+		  sim_io_printf (sd, "0x%lx:", (long) mapping->space);
+		sim_io_printf (sd, "0x%08lx", (long) mapping->base);
+		if (mapping->level != 0)
+		  sim_io_printf (sd, "@0x%lx", (long) mapping->level);
+		sim_io_printf (sd, ",0x%lx", (long) mapping->nr_bytes);
+		modulo = mapping->mask + 1;
+		if (modulo != 0)
+		  sim_io_printf (sd, "%%0x%lx", (long) modulo);
+		sim_io_printf (sd, "\n");
+		mapping = mapping->next;
+	      }
+	    while (mapping);
+	  }
+
+	return SIM_RC_OK;
+	break;
+      }
+
     default:
       sim_io_eprintf (sd, "Unknown memory option %d\n", opt);
       return SIM_RC_FAIL;
-- 
1.7.3.1

Re: [PATCH] sim: add --map-info option

[Joel Brobecker]

> 2010-12-01  Mike Frysinger  <vapier@gentoo.org>
> 
> 	* sim-memopt.c (OPTION_MAP_INFO): Define.
> 	(memory_options): Handle --map-info.
> 	(memory_option_handler): Handle OPTION_MAP_INFO.

Some questions...

> +	for (memory = STATE_CORE (sd), nr_map = 0; nr_map < nr_maps; ++nr_map)

Wouldn't it make sense to hoist the assignment to memory out of
the for initial assignment? It's never changed during the loop...

> +	    if (nr_map <= io_map)
> +	      sim_io_printf (sd, "%s maps:\n",
> +			     (nr_map == read_map) ? "read" :
> +			     (nr_map == write_map) ? "write" :
> +			     (nr_map == exec_map) ? "exec" :
> +			     /*(nr_map == io_map) ?*/ "io");
> +	    else
> +	      sim_io_printf (sd, "??? (%u) maps:\n", nr_map);

I rather you used a case statement or series of if's, in this case
than assume that there are only 4 values <= io_map and thus if it's
not read_map or write_map or exec_map, then it's io_map.  How about:


    /* Return "read", or "write", or ... if valid nr_map.
       Otherwise return null;  */
    
    char *
    io_map_to_str (unsigned nr_map)
    {
      [...]

And then you can use that function to do:

        map_str = io_map_to_str (nr_map);
        if (map_str)
          sim_io_printf ("%s maps:\n", map_str);
        else
          sim_io_printf ("??? (%u) maps:\n", nr_map);

> +	    do
> +	      {
> +		unsigned modulo;

Empty line after local variable declarations.

> +		sim_io_printf (sd, " map ");
> +		if (mapping->space != 0)
> +		  sim_io_printf (sd, "0x%lx:", (long) mapping->space);
> +		sim_io_printf (sd, "0x%08lx", (long) mapping->base);
> +		if (mapping->level != 0)
> +		  sim_io_printf (sd, "@0x%lx", (long) mapping->level);
> +		sim_io_printf (sd, ",0x%lx", (long) mapping->nr_bytes);
> +		modulo = mapping->mask + 1;
> +		if (modulo != 0)
> +		  sim_io_printf (sd, "%%0x%lx", (long) modulo);

I don't understand the necessity to cast everything to long. Can you
explain?

Thanks,
-- 
Joel

Re: [PATCH] sim: add --map-info option

[Mike Frysinger]

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

On Tuesday, December 14, 2010 02:35:58 Joel Brobecker wrote:
> > +	for (memory = STATE_CORE (sd), nr_map = 0; nr_map < nr_maps; 
++nr_map)
> 
> Wouldn't it make sense to hoist the assignment to memory out of
> the for initial assignment? It's never changed during the loop...

i dont understand ... isnt this the whole purpose of the first clause of the 
for statement ?  initializing things once that dont change inside of the loop.  
by this same logic, you could say i should hoist the nr_map assignment out 
too.  or is the multiple initializing assignments undesirable ?

> > +	    if (nr_map <= io_map)
> > +	      sim_io_printf (sd, "%s maps:\n",
> > +			     (nr_map == read_map) ? "read" :
> > +			     (nr_map == write_map) ? "write" :
> > +			     (nr_map == exec_map) ? "exec" :
> > +			     /*(nr_map == io_map) ?*/ "io");
> > +	    else
> > +	      sim_io_printf (sd, "??? (%u) maps:\n", nr_map);
> 
> I rather you used a case statement or series of if's, in this case
> than assume that there are only 4 values <= io_map and thus if it's
> not read_map or write_map or exec_map, then it's io_map.

i dont really like that idea

> How about:
> 
>     /* Return "read", or "write", or ... if valid nr_map.
>        Otherwise return null;  */
> 
>     char *
>     io_map_to_str (unsigned nr_map)
>     {
>       [...]
> 
> And then you can use that function to do:
> 
>         map_str = io_map_to_str (nr_map);
>         if (map_str)
>           sim_io_printf ("%s maps:\n", map_str);
>         else
>           sim_io_printf ("??? (%u) maps:\n", nr_map);

but this idea is good, so i'll implement it

> > +		sim_io_printf (sd, " map ");
> > +		if (mapping->space != 0)
> > +		  sim_io_printf (sd, "0x%lx:", (long) mapping->space);
> > +		sim_io_printf (sd, "0x%08lx", (long) mapping->base);
> > +		if (mapping->level != 0)
> > +		  sim_io_printf (sd, "@0x%lx", (long) mapping->level);
> > +		sim_io_printf (sd, ",0x%lx", (long) mapping->nr_bytes);
> > +		modulo = mapping->mask + 1;
> > +		if (modulo != 0)
> > +		  sim_io_printf (sd, "%%0x%lx", (long) modulo);
> 
> I don't understand the necessity to cast everything to long. Can you
> explain?

it's taken largely unchanged from the OPTION_MEMORY_INFO case block just above 
my new block.  i guess my new code could review the types and do it right.
-mike

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

Re: [PATCH] sim: add --map-info option

[Mike Frysinger]

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

On Tuesday, December 14, 2010 10:16:21 Mike Frysinger wrote:
> On Tuesday, December 14, 2010 02:35:58 Joel Brobecker wrote:
> > How about:
> >     
> >     char *
> >     io_map_to_str (unsigned nr_map)
> >     {
> 
> but this idea is good, so i'll implement it

ah, someone already beat us to it.  there's a map_to_str() func.

> > > +		sim_io_printf (sd, " map ");
> > > +		if (mapping->space != 0)
> > > +		  sim_io_printf (sd, "0x%lx:", (long) mapping->space);
> > > +		sim_io_printf (sd, "0x%08lx", (long) mapping->base);
> > > +		if (mapping->level != 0)
> > > +		  sim_io_printf (sd, "@0x%lx", (long) mapping->level);
> > > +		sim_io_printf (sd, ",0x%lx", (long) mapping->nr_bytes);
> > > +		modulo = mapping->mask + 1;
> > > +		if (modulo != 0)
> > > +		  sim_io_printf (sd, "%%0x%lx", (long) modulo);
> > 
> > I don't understand the necessity to cast everything to long. Can you
> > explain?
> 
> it's taken largely unchanged from the OPTION_MEMORY_INFO case block just
> above my new block.  i guess my new code could review the types and do it
> right.

ok, some of the (long) casts are necessary.  specifically, the ones who have a 
type of "unsigned_word" as those depend on the target bitness (which could be 
16, 32, or 64).
-mike

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

[PATCH v2] sim: add --map-info option

[Mike Frysinger]

There are options for listing the current device/hw tree and memory
regions, but no way to find out at run time all the current mappings.
So add a new --map-info option akin to the --memory-info option which
displays all the current mappings.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>

2010-12-14  Mike Frysinger  <vapier@gentoo.org>

	* sim-memopt.c (OPTION_MAP_INFO): Define.
	(memory_options): Handle --map-info.
	(memory_option_handler): Handle OPTION_MAP_INFO.
---
v2
	- convert to map_to_str
	- drop long casts where unnecessary

 sim/common/sim-memopt.c |   45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 44 insertions(+), 1 deletions(-)

diff --git a/sim/common/sim-memopt.c b/sim/common/sim-memopt.c
index 47b6c9e..26b57a5 100644
--- a/sim/common/sim-memopt.c
+++ b/sim/common/sim-memopt.c
@@ -67,7 +67,8 @@ enum {
   OPTION_MEMORY_ALIAS,
   OPTION_MEMORY_CLEAR,
   OPTION_MEMORY_FILL,
-  OPTION_MEMORY_MAPFILE
+  OPTION_MEMORY_MAPFILE,
+  OPTION_MAP_INFO
 };
 
 static DECLARE_OPTION_HANDLER (memory_option_handler);
@@ -113,6 +114,9 @@ static const OPTION memory_options[] =
   { {"info-memory", no_argument, NULL, OPTION_MEMORY_INFO },
       '\0', NULL, NULL,
       memory_option_handler },
+  { {"map-info", no_argument, NULL, OPTION_MAP_INFO },
+      '\0', NULL, "List mapped regions",
+      memory_option_handler },
 
   { {NULL, no_argument, NULL, 0}, '\0', NULL, NULL, NULL }
 };
@@ -520,6 +524,45 @@ memory_option_handler (SIM_DESC sd, sim_cpu *cpu, int opt,
 	break;
       }
 
+    case OPTION_MAP_INFO:
+      {
+	sim_core *memory;
+	unsigned nr_map;
+
+	for (memory = STATE_CORE (sd), nr_map = 0; nr_map < nr_maps; ++nr_map)
+	  {
+	    sim_core_map *map = &memory->common.map[nr_map];
+	    sim_core_mapping *mapping = map->first;
+
+	    if (!mapping)
+	      continue;
+
+	    sim_io_printf (sd, "%s maps:\n", map_to_str (nr_map));
+	    do
+	      {
+		unsigned modulo;
+
+		sim_io_printf (sd, " map ");
+		if (mapping->space != 0)
+		  sim_io_printf (sd, "0x%x:", mapping->space);
+		sim_io_printf (sd, "0x%08lx", (long) mapping->base);
+		if (mapping->level != 0)
+		  sim_io_printf (sd, "@0x%x", mapping->level);
+		sim_io_printf (sd, ",0x%lx", (long) mapping->nr_bytes);
+		modulo = mapping->mask + 1;
+		if (modulo != 0)
+		  sim_io_printf (sd, "%%0x%x", modulo);
+		sim_io_printf (sd, "\n");
+
+		mapping = mapping->next;
+	      }
+	    while (mapping);
+	  }
+
+	return SIM_RC_OK;
+	break;
+      }
+
     default:
       sim_io_eprintf (sd, "Unknown memory option %d\n", opt);
       return SIM_RC_FAIL;
-- 
1.7.3.1

Re: [PATCH] sim: add --map-info option

[Pedro Alves]

On Tuesday 14 December 2010 18:33:09, Mike Frysinger wrote:
> ok, some of the (long) casts are necessary.  specifically, the ones who have a 
> type of "unsigned_word" as those depend on the target bitness (which could be 
> 16, 32, or 64).

long is 32-bit on Win64.

-- 
Pedro Alves

Re: [PATCH] sim: add --map-info option

[Mike Frysinger]

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

On Tuesday, December 14, 2010 13:50:27 Pedro Alves wrote:
> On Tuesday 14 December 2010 18:33:09, Mike Frysinger wrote:
> > ok, some of the (long) casts are necessary.  specifically, the ones who
> > have a type of "unsigned_word" as those depend on the target bitness
> > (which could be 16, 32, or 64).
> 
> long is 32-bit on Win64.

so what you're saying is that all the sim code that casts unsigned_word to a 
long is broken and not just the code in my patch.  sounds like a bigger 
problem that should be fixed independently and by someone who is interested in 
testing on systems dumb enough to do LLP64.
-mike

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

Re: [PATCH] sim: add --map-info option

[Pedro Alves]

On Tuesday 14 December 2010 19:03:02, Mike Frysinger wrote:
> On Tuesday, December 14, 2010 13:50:27 Pedro Alves wrote:
> > On Tuesday 14 December 2010 18:33:09, Mike Frysinger wrote:
> > > ok, some of the (long) casts are necessary.  specifically, the ones who
> > > have a type of "unsigned_word" as those depend on the target bitness
> > > (which could be 16, 32, or 64).
> > 
> > long is 32-bit on Win64.
> 
> so what you're saying is that all the sim code that casts unsigned_word to a 
> long is broken and not just the code in my patch.  sounds like a bigger 
> problem that should be fixed independently and by someone who is interested in 
> testing on systems dumb enough to do LLP64.

No, I just said "long is 32-bit on Win64".

I just noticed src/README-HACKING:

C Language Assumptions
======================

The programmer may assume that the simulator is being built using an
ANSI C compiler that supports a 64 bit data type.  Consequently:

	o	prototypes can be used (although using
		PARAMS() and K&R declarations wouldn't
		go astray).

	o	If sim-types.h is included, the two
		types signed64 and unsigned64 are
		available.

	o	The type `unsigned' is valid.

However, the user should be aware of the following:

	o	GCC's `<number>LL' is NOT acceptable.
		Microsoft-C doesn't reconize it.

	o	MSC's `<number>i64' is NOT acceptable.
		GCC doesn't reconize it.

	o	GCC's `long long' MSC's `_int64' can
		NOT be used to define 64 bit integer data
		types.

	o	An empty array (eg int a[0]) is not valid.


but if the sim is already not following its own rules, let's leave it.
For GDB code proper, long would be unacceptible.

-- 
Pedro Alves

Re: [PATCH] sim: add --map-info option

[Mike Frysinger]

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

On Tuesday, December 14, 2010 14:11:34 Pedro Alves wrote:
> but if the sim is already not following its own rules, let's leave it.

it is not, so i wont worry about it
-mike

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

Re: [PATCH] sim: add --map-info option

[Joel Brobecker]

> > Wouldn't it make sense to hoist the assignment to memory out of
> > the for initial assignment? It's never changed during the loop...
> 
> i dont understand ... isnt this the whole purpose of the first clause
> of the for statement ?  initializing things once that dont change
> inside of the loop.  

The code works, and I'm fine if you really want to keep it that way.
But you said "initializing things once that dont change inside of the
loop" and that is not true.  You would typically initialize the
variables used in the looping condition, and these variables change
at each iteration.

But that's beside the point, and I don't think there is anything in
the C language that says what should should do, or should not do.  All
I am saying is that you are doing something unusual: You are initializing
in that clause a variable that is never changed during the loop, nor
referenced in the other two clauses of your for statement. If I was
maintaining the code, I would find it much clearer to initialize
the variable before the loop, and then write a typical "for (...)"
statement.

-- 
Joel

Re: [PATCH v2] sim: add --map-info option

[Joel Brobecker]

> 2010-12-14  Mike Frysinger  <vapier@gentoo.org>
> 
> 	* sim-memopt.c (OPTION_MAP_INFO): Define.
> 	(memory_options): Handle --map-info.
> 	(memory_option_handler): Handle OPTION_MAP_INFO.
> ---
> v2
> 	- convert to map_to_str
> 	- drop long casts where unnecessary

This looks OK to me.  If I read your discussion with Pedro correctly,
he's OK too, so OK to commit.

One last request: Can you send a patch adding a note in the gdb/NEWS file?

Thank you,
-- 
Joel

Re: [PATCH v2] sim: add --map-info option

[Mike Frysinger]

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

On Wednesday, December 15, 2010 00:02:26 Joel Brobecker wrote:
> One last request: Can you send a patch adding a note in the gdb/NEWS file?

to mention the new sim option --map-info ?  i didnt think new sim options were 
announced in the gdb/NEWS file as i didnt see any examples of this ...
-mike

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

Re: [PATCH v2] sim: add --map-info option

[Joel Brobecker]

> > One last request: Can you send a patch adding a note in the gdb/NEWS file?
> 
> to mention the new sim option --map-info ?  i didnt think new sim
> options were announced in the gdb/NEWS file as i didnt see any
> examples of this ...

Yes. I think it's NEWS-worthy, regardless of what we have (or not)
been doing in the past.

Also, since there does not seem to be any sim documentation in the GDB
Manual, the only way I could think of to give this new option a tiny bit
of exposure is the NEWS file. 

-- 
Joel

Re: [PATCH] sim: add --map-info option

[Mike Frysinger]

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

On Tuesday, December 14, 2010 23:54:07 Joel Brobecker wrote:
> > > Wouldn't it make sense to hoist the assignment to memory out of
> > > the for initial assignment? It's never changed during the loop...
> > 
> > i dont understand ... isnt this the whole purpose of the first clause
> > of the for statement ?  initializing things once that dont change
> > inside of the loop.
> 
> The code works, and I'm fine if you really want to keep it that way.
> But you said "initializing things once that dont change inside of the
> loop" and that is not true.  You would typically initialize the
> variables used in the looping condition, and these variables change
> at each iteration.

*shrug* i guess we view the purpose of the first clause slightly differently.  
i'm ok with changing the style as you suggest since that seems to be the more 
common method used in the rest of the code base.

so i've committed this now with the memory init moved to the memory decl.
-mike

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

Re: [toolchain-devel] [PATCH v2] sim: add --map-info option

[Mike Frysinger]

On Wed, Dec 15, 2010 at 06:47, Joel Brobecker wrote:
>> > One last request: Can you send a patch adding a note in the gdb/NEWS file?
>>
>> to mention the new sim option --map-info ?  i didnt think new sim
>> options were announced in the gdb/NEWS file as i didnt see any
>> examples of this ...
>
> Yes. I think it's NEWS-worthy, regardless of what we have (or not)
> been doing in the past.
>
> Also, since there does not seem to be any sim documentation in the GDB
> Manual, the only way I could think of to give this new option a tiny bit
> of exposure is the NEWS file.

hows this ?

--- NEWS        10 Dec 2010 20:33:44 -0000      1.414
+++ NEWS        15 Dec 2010 12:01:10 -0000
@@ -115,6 +115,10 @@

 * Guile support was removed.

+* New features in the GNU simulator
+
+  ** The --map-info flag lists all known core mappings.
+
 *** Changes in GDB 7.2

 * Shared library support for remote targets by default
-mike

Re: [toolchain-devel] [PATCH v2] sim: add --map-info option

[Eli Zaretskii]

> From: Mike Frysinger <vapier@gentoo.org>
> Date: Wed, 15 Dec 2010 07:01:30 -0500
> Cc: toolchain-devel@blackfin.uclinux.org, gdb-patches@sourceware.org
> 
> hows this ?
> 
> --- NEWS        10 Dec 2010 20:33:44 -0000      1.414
> +++ NEWS        15 Dec 2010 12:01:10 -0000
> @@ -115,6 +115,10 @@
> 
>  * Guile support was removed.
> 
> +* New features in the GNU simulator
> +
> +  ** The --map-info flag lists all known core mappings.
> +
>  *** Changes in GDB 7.2

OK.

Thanks.