From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16728 invoked by alias); 26 Aug 2010 23:29:09 -0000 Received: (qmail 16714 invoked by uid 22791); 26 Aug 2010 23:29:07 -0000 X-SWARE-Spam-Status: No, hits=-4.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,TW_EG,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from smtp-outbound-1.vmware.com (HELO smtp-outbound-1.vmware.com) (65.115.85.69) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 26 Aug 2010 23:29:00 +0000 Received: from mailhost2.vmware.com (mailhost2.vmware.com [10.16.67.167]) by smtp-outbound-1.vmware.com (Postfix) with ESMTP id 3F71C4D027; Thu, 26 Aug 2010 16:28:57 -0700 (PDT) Received: from msnyder-server.eng.vmware.com (promd-2s-dhcp138.eng.vmware.com [10.20.124.138]) by mailhost2.vmware.com (Postfix) with ESMTP id 2F1738E5A0; Thu, 26 Aug 2010 16:28:57 -0700 (PDT) Message-ID: <4C76F8B8.9050502@vmware.com> Date: Thu, 26 Aug 2010 23:29:00 -0000 From: Michael Snyder User-Agent: Thunderbird 2.0.0.24 (X11/20100702) MIME-Version: 1.0 To: Pedro Alves CC: "gdb-patches@sourceware.org" Subject: Re: [commit] [gdbserver/linux] access memory of running processes References: <201008270026.22929.pedro@codesourcery.com> In-Reply-To: <201008270026.22929.pedro@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-08/txt/msg00452.txt.bz2 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 > > 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 */