[PATCH] gdb/sim: add support for exporting memory map

[Andrew Burgess]

* Mike Frysinger via Gdb-patches <gdb-patches@sourceware.org> [2021-01-06 01:04:33 -0500]:

> This allows gdb to quickly dump & process the memory map that the sim
> knows about.  This isn't fully accurate, but is largely limited by the
> gdb memory map format.  While the sim supports RWX bits, gdb can only
> handle RW or RO regions.
> ---
>  gdb/remote-sim.c         | 16 +++++++++++
>  include/gdb/remote-sim.h |  9 +++++++
>  sim/common/sim-core.c    | 58 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 83 insertions(+)
> 
> gdb/:
> 
> 	* remote-sim.c: Include memory-map.h.
> 	(gdbsim_target): Define memory_map override.
> 	(gdbsim_target::memory_map): Define.
> 
> include/gdb/:
> 
> 	* remote-sim.h (sim_memory_map): Define.
> 
> sim/common/:
> 
> 	* sim-core.c (sim_memory_map): Define.
> 
> diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c
> index c4f3913edbad..e44add538afb 100644
> --- a/gdb/remote-sim.c
> +++ b/gdb/remote-sim.c
> @@ -42,6 +42,7 @@
>  #include "readline/readline.h"
>  #include "gdbthread.h"
>  #include "gdbsupport/byte-vector.h"
> +#include "memory-map.h"
>  
>  /* Prototypes */
>  
> @@ -164,6 +165,7 @@ struct gdbsim_target final
>  
>    bool has_all_memory ()  override;
>    bool has_memory ()  override;
> +  std::vector<mem_region> memory_map () override;
>  
>  private:
>    sim_inferior_data *get_inferior_data_by_ptid (ptid_t ptid,
> @@ -1270,6 +1272,20 @@ gdbsim_target::has_memory ()
>    return true;
>  }
>  
> +std::vector<mem_region>
> +gdbsim_target::memory_map ()

This should have a header comment, even if its just:

  /* Implement target_ops::memory_map.  */

> +{
> +  struct sim_inferior_data *sim_data
> +    = get_sim_inferior_data (current_inferior (), SIM_INSTANCE_NEEDED);
> +  std::vector<mem_region> result;
> +  gdb::unique_xmalloc_ptr<char> text(sim_memory_map (sim_data->gdbsim_desc));

Whitespace missing after 'text'.

> +
> +  if (text)

GDB prefers 'if (text != nullptr)'.

> +    result = parse_memory_map (text.get ());
> +
> +  return result;
> +}
> +
>  void _initialize_remote_sim ();
>  void
>  _initialize_remote_sim ()
> diff --git a/include/gdb/remote-sim.h b/include/gdb/remote-sim.h
> index 73fb670c17e5..a3ba3aa36cd2 100644
> --- a/include/gdb/remote-sim.h
> +++ b/include/gdb/remote-sim.h
> @@ -213,6 +213,15 @@ int sim_store_register (SIM_DESC sd, int regno, unsigned char *buf, int length);
>  void sim_info (SIM_DESC sd, int verbose);
>  
>  
> +/* Return a memory map in XML format.
> +
> +   The caller must free the returned string.
> +
> +   For details on the format, see GDB's Memory Map Format documentation.  */
> +
> +char *sim_memory_map (SIM_DESC sd);
> +
> +
>  /* Run (or resume) the simulated program.
>  
>     STEP, when non-zero indicates that only a single simulator cycle
> diff --git a/sim/common/sim-core.c b/sim/common/sim-core.c
> index 74369aa99fe2..eb40d2588f58 100644
> --- a/sim/common/sim-core.c
> +++ b/sim/common/sim-core.c
> @@ -452,6 +452,64 @@ sim_core_translate (sim_core_mapping *mapping,
>  }
>  
>  
> +#if EXTERN_SIM_CORE_P
> +char *
> +sim_memory_map (SIM_DESC sd)
> +{

I would like to start following the GDB convention of requiring a
header comment on all functions, so here we'd just say:

  /* See include/gdb/remote-sim.h.  */

> +  sim_core *core = STATE_CORE (sd);
> +  unsigned map;
> +  char *s1, *s2, *entry;
> +
> +  s1 = xstrdup (
> +    "<?xml version='1.0'?>\n"
> +    "<!DOCTYPE memory-map PUBLIC '+//IDN gnu.org//DTD GDB Memory Map V1.0//EN'"
> +    " 'http://sourceware.org/gdb/gdb-memory-map.dtd'>\n"
> +    "<memory-map>\n");
> +
> +  for (map = 0;
> +       map < nr_maps;
> +       map++)
> +    {
> +      sim_core_mapping *mapping = core->common.map[map].first;
> +
> +      while (mapping != NULL)

Instead of using while here and then 'goto next' inside, could we not
say:

  for (mapping = core->common.map[map].first;
       mapping != NULL;
       mapping = mapping->next)

and then replace the 'goto next' with 'continue' ?

> +	{
> +	  if (mapping->level != 0)
> +	    goto next;
> +
> +	  entry = xasprintf ("<memory type='ram' start='%#x' length='%#x'/>\n",
> +			     mapping->base, mapping->nr_bytes);
> +	  /* The sim memory map is organized by access, not by addresses.
> +	     So a RWX memory map will have three independent mappings.
> +	     GDB's format cannot support overlapping regions, so we have
> +	     to filter those out.
> +
> +	     Further, GDB can only handle RX ("rom") or RWX ("ram") mappings.
> +	     We just emit "ram" everywhere to keep it simple.  If GDB ever
> +	     gains support for more stuff, we can expand this.
> +
> +	     Using strstr is kind of hacky, but as long as the map is not huge
> +	     (we're talking <10K), should be fine.  */
> +	  if (strstr (s1, entry) == NULL)
> +	    {
> +	      s2 = concat (s1, entry, NULL);
> +	      free (s1);
> +	      s1 = s2;
> +	    }
> +	  free (entry);
> +
> + next:
> +	  mapping = mapping->next;
> +	}
> +    }
> +
> +  s2 = concat (s1, "</memory-map>", NULL);
> +  free (s1);
> +  return s2;
> +}
> +#endif
> +
> +
>  #if EXTERN_SIM_CORE_P
>  unsigned
>  sim_core_read_buffer (SIM_DESC sd,
> -- 
> 2.28.0
>