This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] sim: add --map-info option
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Mike Frysinger <vapier at gentoo dot org>
- Cc: gdb-patches at sourceware dot org, Stephen dot Kilbane at analog dot com, Stuart dot Henderson at analog dot com, David dot Gibson at analog dot com
- Date: Tue, 14 Dec 2010 11:35:58 +0400
- Subject: Re: [PATCH] sim: add --map-info option
- References: <1291219863-18458-1-git-send-email-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.
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