Re: [commit] [gdbserver/linux] access memory of running processes

[Michael Snyder <msnyder@vmware.com>]

Hmmm, don't like "unprepare_to_access_memory".

What about "finish_accessing_memory"?


Pedro Alves wrote:
> Of all the debug API's we've added non-stop support to (we've added
> it to a few private targets), linux/ptrace is the only one so far that
> does not allow accessing memory when you pass it the PID of a process
> that is presently running to work with.  This affects the user experience
> much, as any command that touches memory, even breakpoint command fail with a
> hard error if you happen to have a running thread selected.  The workaround is
> to stop one thread, switch to it, do what you need, and re-resume the thread.
> You may leave all other threads running while doing that workaround.
> Obviously, this is cumbersome.  Particularly more so since random commands
> that weren't accessing target memory yesterday, may do so today.
> 
> I've been thinking of whether this should be addressed on the target
> side, or if it should be addressed by GDB.  Fixing this on the GDB side
> implies GDB interrupting a process, doing the access, and resuming it again.
> Since not all targets need this, it would have to be dependent on a target
> flag.  Fixing this on the target side means doing the temporarily-pause work,
> but it ends up resolving faster, because there's no inherent RSP latency
> involved.  The latter option is completely transparent to GDB, of course.
> 
> Thinking about it, I came up with the following points and conclusions:
> 
>  - Making sure that GDB has a consistent view of memory across a batch of
> memory accesses is something that is desired in some scenarios but not
> actually necessary in others, meaning, it's an independent issue from allowing
> a single access while threads are running.  The former always needs to be
> addressed by having GDB take the wheel.  The latter can be addressed in either
> way.
> 
>  - This is really a debug API issue.  The fact that we have to access an
> address space by passing to ptrace a stopped lwp is just a consequence of the
> fact that you always have to pass a stopped process to all ptrace API calls.
> 
>  - The current RSP memory access and breakpoint commands have synchronous
> semantics.  Doesn't sound like a strong argument, but, let me explain that I
> considered having gdbserver immediately reply OK to Z0 breakpoints if the
> target is running (and so GDB was happy), have gdbserver force an asynchronous
> stop on the current inferior, and finally insert the breakpoint when the
> inferior stops, and then transparently resume it.  The problem is, if the
> breakpoint fails to insert, because say, its address wasn't actually mapped in
> the address space, there's no way to inform GDB.  It's obviously worse with
> memory read/write packets.  Sure, we could come up with new RSP packets, but,
> as long as we have what we have, we might as well make them work (particularly
> since they work for most targets without extra fuss anyway) (synchronously) on
> linux too, and come up with new packets if we really find this way to be
> limiting.
> 
> There are few ways to be smart here, but I've chosen to be as dumb as
> possible, and then more dumb --- transparently stop all lwps at selected entry
> points of memory accesses corresponding to requests coming in from GDB.  Since
> the need for this is target specific, I've abstracted this out in new
> prepare_to_access_memory/unprepare_to_access_memory target methods.
> 
> Tested on x86_64-unknown-linux-gnu and applied.
> 
> --
> Pedro Alves
> 
> 2010-08-26  Pedro Alves  <pedro@codesourcery.com>
> 
>         gdbserver/
>         * linux-low.c (linux_prepare_to_access_memory): New.
>         (linux_unprepare_to_access_memory): New.
>         (linux_target_ops): Install them.
>         * server.c (read_memory): Rename to ...
>         (gdb_read_memory): ... this.  Use
>         prepare_to_access_memory/prepare_to_access_memory.
>         (write_memory): Rename to ...
>         (gdb_write_memory): ... this.  Use
>         prepare_to_access_memory/prepare_to_access_memory.
>         (handle_search_memory_1): Adjust.
>         (process_serial_event): Adjust.
>         * target.h (struct target_ops): New fields
>         prepare_to_access_memory and unprepare_to_access_memory.
>         (prepare_to_access_memory, unprepare_to_access_memory): New.
>         * linux-x86-low.c (x86_insert_point, x86_remove_point): Use
>         prepare_to_access_memory/prepare_to_access_memory.
>         * nto-low.c (nto_target_ops): Adjust.
>         * spu-low.c (spu_target_ops): Adjust.
>         * win32-low.c (win32_target_ops): Adjust.
> 
> ---
>  gdb/gdbserver/linux-low.c     |   21 +++++++++++++++++++++
>  gdb/gdbserver/linux-x86-low.c |   24 +++++++++++++++++++++---
>  gdb/gdbserver/nto-low.c       |    2 ++
>  gdb/gdbserver/server.c        |   39 +++++++++++++++++++++++++++++----------
>  gdb/gdbserver/spu-low.c       |    2 ++
>  gdb/gdbserver/target.h        |   29 +++++++++++++++++++++++++++++
>  gdb/gdbserver/win32-low.c     |    2 ++
>  7 files changed, 106 insertions(+), 13 deletions(-)
> 
> Index: src/gdb/gdbserver/linux-low.c
> ===================================================================
> --- src.orig/gdb/gdbserver/linux-low.c  2010-08-26 19:32:03.000000000 +0100
> +++ src/gdb/gdbserver/linux-low.c       2010-08-26 21:14:56.000000000 +0100
> @@ -5006,6 +5006,25 @@ linux_unpause_all (int unfreeze)
>  }
> 
>  static int
> +linux_prepare_to_access_memory (void)
> +{
> +  /* Neither ptrace nor /proc/PID/mem allow accessing memory through a
> +     running LWP.  */
> +  if (non_stop)
> +    linux_pause_all (1);
> +  return 0;
> +}
> +
> +static void
> +linux_unprepare_to_access_memory (void)
> +{
> +  /* Neither ptrace nor /proc/PID/mem allow accessing memory through a
> +     running LWP.  */
> +  if (non_stop)
> +    linux_unpause_all (1);
> +}
> +
> +static int
>  linux_install_fast_tracepoint_jump_pad (CORE_ADDR tpoint, CORE_ADDR tpaddr,
>                                         CORE_ADDR collector,
>                                         CORE_ADDR lockaddr,
> @@ -5043,6 +5062,8 @@ static struct target_ops linux_target_op
>    linux_wait,
>    linux_fetch_registers,
>    linux_store_registers,
> +  linux_prepare_to_access_memory,
> +  linux_unprepare_to_access_memory,
>    linux_read_memory,
>    linux_write_memory,
>    linux_look_up_symbols,
> Index: src/gdb/gdbserver/server.c
> ===================================================================
> --- src.orig/gdb/gdbserver/server.c     2010-08-26 16:37:50.000000000 +0100
> +++ src/gdb/gdbserver/server.c  2010-08-26 19:38:25.000000000 +0100
> @@ -539,8 +539,10 @@ monitor_show_help (void)
>  /* Read trace frame or inferior memory.  */
> 
>  static int
> -read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
> +gdb_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
>  {
> +  int ret;
> +
>    if (current_traceframe >= 0)
>      {
>        ULONGEST nbytes;
> @@ -558,19 +560,36 @@ read_memory (CORE_ADDR memaddr, unsigned
>        /* (assume no half-trace half-real blocks for now) */
>      }
> 
> -  return read_inferior_memory (memaddr, myaddr, len);
> +  ret = prepare_to_access_memory ();
> +  if (ret == 0)
> +    {
> +      ret = read_inferior_memory (memaddr, myaddr, len);
> +      unprepare_to_access_memory ();
> +    }
> +
> +  return ret;
>  }
> 
>  /* Write trace frame or inferior memory.  Actually, writing to trace
>     frames is forbidden.  */
> 
>  static int
> -write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
> +gdb_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
>  {
>    if (current_traceframe >= 0)
>      return EIO;
>    else
> -    return write_inferior_memory (memaddr, myaddr, len);
> +    {
> +      int ret;
> +
> +      ret = prepare_to_access_memory ();
> +      if (ret == 0)
> +       {
> +         ret = write_inferior_memory (memaddr, myaddr, len);
> +         unprepare_to_access_memory ();
> +       }
> +      return ret;
> +    }
>  }
> 
>  /* Subroutine of handle_search_memory to simplify it.  */
> @@ -584,7 +603,7 @@ handle_search_memory_1 (CORE_ADDR start_
>  {
>    /* Prime the search buffer.  */
> 
> -  if (read_memory (start_addr, search_buf, search_buf_size) != 0)
> +  if (gdb_read_memory (start_addr, search_buf, search_buf_size) != 0)
>      {
>        warning ("Unable to access target memory at 0x%lx, halting search.",
>                (long) start_addr);
> @@ -635,8 +654,8 @@ handle_search_memory_1 (CORE_ADDR start_
>                         ? search_space_len - keep_len
>                         : chunk_size);
> 
> -         if (read_memory (read_addr, search_buf + keep_len,
> -                                   nr_to_read) != 0)
> +         if (gdb_read_memory (read_addr, search_buf + keep_len,
> +                              nr_to_read) != 0)
>             {
>               warning ("Unable to access target memory at 0x%lx, halting search.",
>                        (long) read_addr);
> @@ -2924,7 +2943,7 @@ process_serial_event (void)
>      case 'm':
>        require_running (own_buf);
>        decode_m_packet (&own_buf[1], &mem_addr, &len);
> -      if (read_memory (mem_addr, mem_buf, len) == 0)
> +      if (gdb_read_memory (mem_addr, mem_buf, len) == 0)
>         convert_int_to_ascii (mem_buf, own_buf, len);
>        else
>         write_enn (own_buf);
> @@ -2932,7 +2951,7 @@ process_serial_event (void)
>      case 'M':
>        require_running (own_buf);
>        decode_M_packet (&own_buf[1], &mem_addr, &len, &mem_buf);
> -      if (write_memory (mem_addr, mem_buf, len) == 0)
> +      if (gdb_write_memory (mem_addr, mem_buf, len) == 0)
>         write_ok (own_buf);
>        else
>         write_enn (own_buf);
> @@ -2941,7 +2960,7 @@ process_serial_event (void)
>        require_running (own_buf);
>        if (decode_X_packet (&own_buf[1], packet_len - 1,
>                            &mem_addr, &len, &mem_buf) < 0
> -         || write_memory (mem_addr, mem_buf, len) != 0)
> +         || gdb_write_memory (mem_addr, mem_buf, len) != 0)
>         write_enn (own_buf);
>        else
>         write_ok (own_buf);
> Index: src/gdb/gdbserver/target.h
> ===================================================================
> --- src.orig/gdb/gdbserver/target.h     2010-08-26 16:37:50.000000000 +0100
> +++ src/gdb/gdbserver/target.h  2010-08-26 21:28:50.000000000 +0100
> @@ -181,6 +181,23 @@ struct target_ops
> 
>    void (*store_registers) (struct regcache *regcache, int regno);
> 
> +  /* Prepare to read or write memory from the inferior process.
> +     Targets use this to do what is necessary to get the state of the
> +     inferior such that it is possible to access memory.
> +
> +     This should generally only be called from client facing routines,
> +     such as gdb_read_memory/gdb_write_memory, or the insert_point
> +     callbacks.
> +
> +     Like `read_memory' and `write_memory' below, returns 0 on success
> +     and errno on failure.  */
> +
> +  int (*prepare_to_access_memory) (void);
> +
> +  /* Undo the effects of prepare_to_access_memory.  */
> +
> +  void (*unprepare_to_access_memory) (void);
> +
>    /* Read memory from the inferior process.  This should generally be
>       called through read_inferior_memory, which handles breakpoint shadowing.
> 
> @@ -469,6 +486,18 @@ int start_non_stop (int nonstop);
>  ptid_t mywait (ptid_t ptid, struct target_waitstatus *ourstatus, int options,
>                int connected_wait);
> 
> +#define prepare_to_access_memory()             \
> +  (the_target->prepare_to_access_memory                \
> +   ? (*the_target->prepare_to_access_memory) () \
> +   : 0)
> +
> +#define unprepare_to_access_memory()                   \
> +  do                                                   \
> +    {                                                  \
> +      if (the_target->unprepare_to_access_memory)      \
> +       (*the_target->unprepare_to_access_memory) ();   \
> +    } while (0)
> +
>  int read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len);
> 
>  int write_inferior_memory (CORE_ADDR memaddr, const unsigned char *myaddr,
> Index: src/gdb/gdbserver/linux-x86-low.c
> ===================================================================
> --- src.orig/gdb/gdbserver/linux-x86-low.c      2010-08-26 21:03:47.000000000 +0100
> +++ src/gdb/gdbserver/linux-x86-low.c   2010-08-26 21:04:19.000000000 +0100
> @@ -546,7 +546,7 @@ i386_dr_low_get_status (void)
>    return x86_linux_dr_get (ptid, DR_STATUS);
>  }
> 
> -/* Watchpoint support.  */
> +/* Breakpoint/Watchpoint support.  */
> 
>  static int
>  x86_insert_point (char type, CORE_ADDR addr, int len)
> @@ -555,7 +555,16 @@ x86_insert_point (char type, CORE_ADDR a
>    switch (type)
>      {
>      case '0':
> -      return set_gdb_breakpoint_at (addr);
> +      {
> +       int ret;
> +
> +       ret = prepare_to_access_memory ();
> +       if (ret)
> +         return -1;
> +       ret = set_gdb_breakpoint_at (addr);
> +       unprepare_to_access_memory ();
> +       return ret;
> +      }
>      case '2':
>      case '3':
>      case '4':
> @@ -574,7 +583,16 @@ x86_remove_point (char type, CORE_ADDR a
>    switch (type)
>      {
>      case '0':
> -      return delete_gdb_breakpoint_at (addr);
> +      {
> +       int ret;
> +
> +       ret = prepare_to_access_memory ();
> +       if (ret)
> +         return -1;
> +       ret = delete_gdb_breakpoint_at (addr);
> +       unprepare_to_access_memory ();
> +       return ret;
> +      }
>      case '2':
>      case '3':
>      case '4':
> Index: src/gdb/gdbserver/nto-low.c
> ===================================================================
> --- src.orig/gdb/gdbserver/nto-low.c    2010-06-16 10:55:22.000000000 +0100
> +++ src/gdb/gdbserver/nto-low.c 2010-08-26 21:07:28.000000000 +0100
> @@ -913,6 +913,8 @@ static struct target_ops nto_target_ops
>    nto_wait,
>    nto_fetch_registers,
>    nto_store_registers,
> +  NULL, /* prepare_to_access_memory */
> +  NULL, /* unprepare_to_access_memory */
>    nto_read_memory,
>    nto_write_memory,
>    NULL, /* nto_look_up_symbols */
> Index: src/gdb/gdbserver/spu-low.c
> ===================================================================
> --- src.orig/gdb/gdbserver/spu-low.c    2010-06-20 23:17:44.000000000 +0100
> +++ src/gdb/gdbserver/spu-low.c 2010-08-26 21:07:54.000000000 +0100
> @@ -653,6 +653,8 @@ static struct target_ops spu_target_ops
>    spu_wait,
>    spu_fetch_registers,
>    spu_store_registers,
> +  NULL, /* prepare_to_access_memory */
> +  NULL, /* unprepare_to_access_memory */
>    spu_read_memory,
>    spu_write_memory,
>    spu_look_up_symbols,
> Index: src/gdb/gdbserver/win32-low.c
> ===================================================================
> --- src.orig/gdb/gdbserver/win32-low.c  2010-06-16 10:58:30.000000000 +0100
> +++ src/gdb/gdbserver/win32-low.c       2010-08-26 21:08:16.000000000 +0100
> @@ -1781,6 +1781,8 @@ static struct target_ops win32_target_op
>    win32_wait,
>    win32_fetch_inferior_registers,
>    win32_store_inferior_registers,
> +  NULL, /* prepare_to_access_memory */
> +  NULL, /* unprepare_to_access_memory */
>    win32_read_inferior_memory,
>    win32_write_inferior_memory,
>    NULL, /* lookup_symbols */