From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27477 invoked by alias); 9 Feb 2012 19:21:20 -0000 Received: (qmail 27465 invoked by uid 22791); 9 Feb 2012 19:21:16 -0000 X-SWARE-Spam-Status: No, hits=-4.7 required=5.0 tests=AWL,BAYES_50,KAM_STOCKGEN,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_EG,TW_XS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 09 Feb 2012 19:20:56 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q19JKh8D004003 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 9 Feb 2012 14:20:43 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q19JKfq0007611; Thu, 9 Feb 2012 14:20:42 -0500 Message-ID: <4F341C89.40501@redhat.com> Date: Thu, 09 Feb 2012 19:21:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20120131 Thunderbird/10.0 MIME-Version: 1.0 To: Yao Qi CC: gdb-patches@sourceware.org Subject: Re: [patch 1/8] Generalize interaction with agent in gdb/gdbserver References: <4F1D55D7.7030506@codesourcery.com> <4F1D62D3.2080805@codesourcery.com> In-Reply-To: <4F1D62D3.2080805@codesourcery.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: 2012-02/txt/msg00156.txt.bz2 On 01/23/2012 01:38 PM, Yao Qi wrote: > Existing gdbserver is able to communicate with agent via socket and > command buffer. We want to grow this way for a more general purpose -- > for gdb/gdbserver to communicate with agent for different types of > operations. This patch is to move this bits of code to common/agent.c, > so that these routines can be used by gdb and gdbserver > > Since this part of code needs to query symbol and access inferior's > memory, these APIs are different between gdb and gdbserver. This causes > some #ifdef/#endif blocks in the code. > > Previously, the communication channel is quite specific to gdbserver and > tracepoint. With this patch, the communication channel is more generic, > so that gdb and gdbserver can use this channel to talk with agent. > > -- Yao (齐尧) > > > 0001-move-gdbserver-tracepoint-to-common-agent.patch > > > gdb: > > 2012-01-23 Yao Qi > > * common/agent.c: New. > * common/agent.h: New. > > gdb/gdbserver: > > 2012-01-23 Yao Qi > > * Makefile.in (OBS): Add agent.o. > Add new rule for agent.o. > * tracepoint.c (run_inferior_command_1): > (run_inferior_command): Call agent_run_command. > (gdb_ust_connect_sync_socket): Deleted. Move it to > common/agent.c. > (resume_thread, stop_thread): Likewise. > --- > gdb/common/agent.c | 262 ++++++++++++++++++++++++++++++++++++++++++++ > gdb/common/agent.h | 38 +++++++ > gdb/gdbserver/Makefile.in | 6 +- > gdb/gdbserver/tracepoint.c | 171 +---------------------------- > 4 files changed, 311 insertions(+), 166 deletions(-) > create mode 100644 gdb/common/agent.c > create mode 100644 gdb/common/agent.h > > diff --git a/gdb/common/agent.c b/gdb/common/agent.c > new file mode 100644 > index 0000000..5c66071 > --- /dev/null > +++ b/gdb/common/agent.c > @@ -0,0 +1,262 @@ > +/* Shared utility routines for GDB to interact with agent. > + > + Copyright (C) 2012 Free Software Foundation, Inc. I'm still not clear on if when we move code to a new file, we can just drop the previous copyright years. The way I think of it is, if that were possible, then e.g., renaming a file would produce a file with just the current year as copyright. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +#ifdef GDBSERVER > +#include "server.h" > +#else > +#include "defs.h" > +#include "target.h" > +#endif > + > +#include > +#include > +#include > +#include > +#include "agent.h" > + > +int debug_agent = 0; > + > +/* Global flag to determine using agent or not. */ > +int use_agent = 0; > + > +#define SOCK_DIR P_tmpdir > + > +/* Addresses of in-process agent's symbols both GDB and GDBserver cares > + about. */ > + > +struct ipa_sym_addresses > +{ > + CORE_ADDR addr_helper_thread_id; > + CORE_ADDR addr_cmd_buf; > +}; > + > +/* Cache of the helper thread id. */ > +static unsigned int helper_thread_id = 0; All these globals should be make per-process at some point. > + > +static struct > +{ > + const char *name; > + int offset; > + int required; > +} symbol_list[] = { > + IPA_SYM(helper_thread_id), > + IPA_SYM(cmd_buf), > +}; > + > +static struct ipa_sym_addresses ipa_sym_addrs; > + > +void > +agent_look_up_symbols (void) > +{ > + int i; > + > + for (i = 0; i < sizeof (symbol_list) / sizeof (symbol_list[0]); i++) > + { > + CORE_ADDR *addrp = > + (CORE_ADDR *) ((char *) &ipa_sym_addrs + symbol_list[i].offset); > +#ifdef GDBSERVER > + Spurious space. > + if (look_up_one_symbol (symbol_list[i].name, addrp, 1) == 0) > +#else > + struct minimal_symbol *sym = lookup_minimal_symbol (symbol_list[i].name, > + NULL, NULL); > + > + if (sym != NULL) > + *addrp = SYMBOL_VALUE_ADDRESS (sym); > + else > +#endif > + { > + if (debug_agent) > + fprintf (stderr, "symbol `%s' not found\n", symbol_list[i].name); > + } > + } > +} > + > +static unsigned int > +agent_get_helper_thread_id (void) > +{ > + if (helper_thread_id == 0) > + { > +#ifdef GDBSERVER > + if (read_inferior_memory (ipa_sym_addrs.addr_helper_thread_id, > + (unsigned char *) &helper_thread_id, > + sizeof helper_thread_id)) > +#else > + enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch); > + gdb_byte buf[4]; > + > + if (target_read_memory (ipa_sym_addrs.addr_helper_thread_id, > + buf, sizeof buf) == 0) > + helper_thread_id = extract_unsigned_integer (buf, sizeof buf, > + byte_order); > +#endif > + { > + warning ("Error reading helper thread's id in lib"); > + } > + } > + > + return helper_thread_id; > +} > + > +/* Connects to synchronization socket. PID is the pid of inferior, which is > + used to set up connection socket. */ > + > +static int > +gdb_ust_connect_sync_socket (int pid) "ust" no longer makes sense. > +{ > + struct sockaddr_un addr; > + int res, fd; > + char path[UNIX_PATH_MAX]; > + > + res = xsnprintf (path, UNIX_PATH_MAX, "%s/gdb_ust%d", P_tmpdir, pid); > + if (res >= UNIX_PATH_MAX) > + return -1; > + > + res = fd = socket (PF_UNIX, SOCK_STREAM, 0); > + if (res == -1) > + { > + warning ("error opening sync socket: %s\n", strerror (errno)); > + return -1; > + } > + > + addr.sun_family = AF_UNIX; > + > + res = xsnprintf (addr.sun_path, UNIX_PATH_MAX, "%s", path); > + if (res >= UNIX_PATH_MAX) > + { > + warning ("string overflow allocating socket name\n"); > + close (fd); > + return -1; > + } > + > + res = connect (fd, (struct sockaddr *) &addr, sizeof (addr)); > + if (res == -1) > + { > + warning ("error connecting sync socket (%s): %s. " > + "Make sure the directory exists and that it is writable.", > + path, strerror (errno)); > + close (fd); > + return -1; > + } All UNIX_PATH_MAX, PF_UNIX, P_tmpdir, etc business, doesn't compile or work on all supported GDB hosts... This used to be wrapped on HAVE_UST, so it wasn't a problem when it lived in gdbserver. > + > + return fd; > +} > + > +/* Execute an agent command in inferior. PID is the value of pid of inferior. > + CMD is the buffer for command, and LEN is length of command. GDB or > + GDBserver will store command into it and fetch return result from CMD. > + The interaction between GDB/GDBserver and agent is synchronized by > + synchronization socket. */ > + > +void > +agent_run_command (int pid, const char *cmd, int len) > +{ > + int fd; > + int tid = agent_get_helper_thread_id (); > + ptid_t ptid = ptid_build (pid, tid, 0); > + > +#ifdef GDBSERVER > + int ret = write_inferior_memory (ipa_sym_addrs.addr_cmd_buf, > + (const unsigned char *) cmd, len); > +#else > + int ret = target_write_memory (ipa_sym_addrs.addr_cmd_buf, cmd, len); > +#endif > + > + if (ret != 0) > + { > + warning ("unable to write"); > + return; > + } > + > + /* Resume helper thread. */ > +#ifdef GDBSERVER > +{ > + struct thread_resume resume_info; > + > + resume_info.thread = ptid; > + resume_info.kind = resume_continue; > + resume_info.sig = TARGET_SIGNAL_0; > + (*the_target->resume) (&resume_info, 1); > +} > +#else > + target_resume (ptid, 0, TARGET_SIGNAL_0); > +#endif > + > + fd = gdb_ust_connect_sync_socket (pid); > + if (fd >= 0) > + { > + char buf[1] = ""; > + int ret; > + > + if (debug_agent) > + fprintf (stderr, "agent: signalling helper thread\n"); "fprintf (stderr, " is not the proper way to output log messages on the gdb side. > + > + do > + { > + ret = write (fd, buf, 1); > + } while (ret == -1 && errno == EINTR); > + > + if (debug_agent) > + fprintf (stderr, "agent:waiting for helper thread's response\n"); > + > + do > + { > + ret = read (fd, buf, 1); > + } while (ret == -1 && errno == EINTR); > + > + close (fd); > + > + if (debug_agent) > + fprintf (stderr, "agent:helper thread's response received\n"); > + } > + > +#ifdef GDBSERVER > + /* Need to read response with the inferior stopped. */ > + if (!ptid_equal (ptid, null_ptid)) > + { > + int was_non_stop = non_stop; > + struct target_waitstatus status; > + struct thread_resume resume_info; > + > + /* Stop thread PTID. */ > + resume_info.thread = ptid; > + resume_info.kind = resume_stop; > + resume_info.sig = TARGET_SIGNAL_0; > + (*the_target->resume) (&resume_info, 1); > + > + non_stop = 1; > + mywait (ptid, &status, 0, 0); > + non_stop = was_non_stop; > + } > +#endif Since there's no #else, I take it you haven't really tried using this on GDB, without gdbserver? > + > + if (fd >= 0) > + { > +#ifdef GDBSERVER > + if (read_inferior_memory (ipa_sym_addrs.addr_cmd_buf, > + (unsigned char *) cmd, CMD_BUF_SIZE)) > +#else > + if (target_read_memory (ipa_sym_addrs.addr_cmd_buf, (gdb_byte *) cmd, > + CMD_BUF_SIZE)) > +#endif > + { > + warning ("Error reading command response"); > + } > + } > +} > diff --git a/gdb/common/agent.h b/gdb/common/agent.h > new file mode 100644 > index 0000000..079b65e > --- /dev/null > +++ b/gdb/common/agent.h > @@ -0,0 +1,38 @@ > +/* Shared utility routines for GDB to interact with agent. > + > + Copyright (C) 2012 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +void agent_run_command (int pid, const char *cmd, int len); > + > +void agent_look_up_symbols (void); > + > +#define STRINGIZE_1(STR) #STR > +#define STRINGIZE(STR) STRINGIZE_1(STR) These should probably move somewhere more central. > +#define IPA_SYM(SYM) \ > + { \ > + STRINGIZE (gdb_agent_ ## SYM), \ > + offsetof (struct ipa_sym_addresses, addr_ ## SYM) \ > + } > + > +/* The size in bytes of the buffer used to talk to the IPA helper > + thread. */ > +#define CMD_BUF_SIZE 1024 Would be a good idea to prefix this. IPA_CMD_BUF_SIZE ? > + > +extern int debug_agent ; Spurious space before `;'. > + > +extern int use_agent; This doesn't seem to be used anywhere in this patch. It'd be best if only added in the patch that actually introduces uses. > diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in > index 6ccf5ae..ddca704 100644 > --- a/gdb/gdbserver/Makefile.in > +++ b/gdb/gdbserver/Makefile.in > @@ -135,7 +135,7 @@ SOURCES = $(SFILES) > TAGFILES = $(SOURCES) ${HFILES} ${ALLPARAM} ${POSSLIBS} > > OBS = inferiors.o regcache.o remote-utils.o server.o signals.o target.o \ > - utils.o version.o \ > + utils.o version.o agent.o \ > mem-break.o hostio.o event-loop.o tracepoint.o \ > xml-utils.o common-utils.o ptid.o buffer.o \ > $(XML_BUILTIN) \ > @@ -335,6 +335,7 @@ regcache_h = $(srcdir)/regcache.h > signals_def = $(srcdir)/../../include/gdb/signals.def > signals_h = $(srcdir)/../../include/gdb/signals.h $(signals_def) > ptid_h = $(srcdir)/../common/ptid.h > +agent_h = $(srcdir)/../common/agent.h > linux_osdata_h = $(srcdir)/../common/linux-osdata.h > server_h = $(srcdir)/server.h $(regcache_h) config.h $(srcdir)/target.h \ > $(srcdir)/mem-break.h $(srcdir)/../common/gdb_signals.h \ > @@ -423,6 +424,9 @@ ptid.o: ../common/ptid.c $(ptid_h) > buffer.o: ../common/buffer.c $(server_h) > $(CC) -c $(CPPFLAGS) $(INTERNAL_CFLAGS) $< -DGDBSERVER > > +agent.o: ../common/agent.c $(server_h) $(agent_h) > + $(CC) -c $(CPPFLAGS) $(INTERNAL_CFLAGS) $< -DGDBSERVER > + > # We build memmem.c without -Werror because this file is not under > # our control. On LynxOS, the compiler generates some warnings > # because str-two-way.h uses a constant (MAX_SIZE) whose definition > diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c > index 3e6dac0..7f462bc 100644 > --- a/gdb/gdbserver/tracepoint.c > +++ b/gdb/gdbserver/tracepoint.c > @@ -17,6 +17,8 @@ > along with this program. If not, see . */ > > #include "server.h" > +#include "agent.h" Needs tracepoint.o's dependencies updated. > + > #include > #include > #include > @@ -185,18 +187,8 @@ struct ipa_sym_addresses > CORE_ADDR addr_get_trace_state_variable_value; > CORE_ADDR addr_set_trace_state_variable_value; > CORE_ADDR addr_ust_loaded; > - CORE_ADDR addr_helper_thread_id; > - CORE_ADDR addr_cmd_buf; > }; > > -#define STRINGIZE_1(STR) #STR > -#define STRINGIZE(STR) STRINGIZE_1(STR) > -#define IPA_SYM(SYM) \ > - { \ > - STRINGIZE (gdb_agent_ ## SYM), \ > - offsetof (struct ipa_sym_addresses, addr_ ## SYM) \ > - } > - > static struct > { > const char *name; > @@ -232,11 +224,9 @@ static struct > IPA_SYM(get_trace_state_variable_value), > IPA_SYM(set_trace_state_variable_value), > IPA_SYM(ust_loaded), > - IPA_SYM(helper_thread_id), > - IPA_SYM(cmd_buf), > }; > > -struct ipa_sym_addresses ipa_sym_addrs; > +static struct ipa_sym_addresses ipa_sym_addrs; > > int all_tracepoint_symbols_looked_up; > > @@ -355,6 +345,8 @@ tracepoint_look_up_symbols (void) > } > } > > + agent_look_up_symbols (); So what if some of the symbols required by agent_look_up_symbols aren't found (meaning, if we have an old agent loaded? > + > all_tracepoint_symbols_looked_up = 1; > } > > @@ -1312,10 +1304,6 @@ static LONGEST get_timestamp (void); > #define cmpxchg(mem, oldval, newval) \ > __sync_val_compare_and_swap (mem, oldval, newval) > > -/* The size in bytes of the buffer used to talk to the IPA helper > - thread. */ > -#define CMD_BUF_SIZE 1024 > - > /* Record that an error occurred during expression evaluation. */ > > static void > @@ -7566,76 +7554,6 @@ static struct ltt_available_probe gdb_ust_probe = > > #ifdef HAVE_UST > > -static int > -gdb_ust_connect_sync_socket (int pid) > -{ > - struct sockaddr_un addr; > - int res, fd; > - char path[UNIX_PATH_MAX]; > - > - res = xsnprintf (path, UNIX_PATH_MAX, "%s/gdb_ust%d", SOCK_DIR, pid); > - if (res >= UNIX_PATH_MAX) > - { > - trace_debug ("string overflow allocating socket name"); > - return -1; > - } > - > - res = fd = socket (PF_UNIX, SOCK_STREAM, 0); > - if (res == -1) > - { > - warning ("error opening sync socket: %s\n", strerror (errno)); > - return -1; > - } > - > - addr.sun_family = AF_UNIX; > - > - res = xsnprintf (addr.sun_path, UNIX_PATH_MAX, "%s", path); > - if (res >= UNIX_PATH_MAX) > - { > - warning ("string overflow allocating socket name\n"); > - close (fd); > - return -1; > - } > - > - res = connect (fd, (struct sockaddr *) &addr, sizeof (addr)); > - if (res == -1) > - { > - warning ("error connecting sync socket (%s): %s. " > - "Make sure the directory exists and that it is writable.", > - path, strerror (errno)); > - close (fd); > - return -1; > - } > - > - return fd; > -} > - > -/* Resume thread PTID. */ > - > -static void > -resume_thread (ptid_t ptid) > -{ > - struct thread_resume resume_info; > - > - resume_info.thread = ptid; > - resume_info.kind = resume_continue; > - resume_info.sig = TARGET_SIGNAL_0; > - (*the_target->resume) (&resume_info, 1); > -} > - > -/* Stop thread PTID. */ > - > -static void > -stop_thread (ptid_t ptid) > -{ > - struct thread_resume resume_info; > - > - resume_info.thread = ptid; > - resume_info.kind = resume_stop; > - resume_info.sig = TARGET_SIGNAL_0; > - (*the_target->resume) (&resume_info, 1); > -} > - > /* Ask the in-process agent to run a command. Since we don't want to > have to handle the IPA hitting breakpoints while running the > command, we pause all threads, remove all breakpoints, and then set > @@ -7647,91 +7565,14 @@ static int > run_inferior_command (char *cmd) > { > int err = -1; > - int fd = -1; > int pid = ptid_get_pid (current_inferior->entry.id); > - int tid; > - ptid_t ptid = null_ptid; > > trace_debug ("run_inferior_command: running: %s", cmd); > > pause_all (0); > uninsert_all_breakpoints (); > > - if (read_inferior_integer (ipa_sym_addrs.addr_helper_thread_id, &tid)) > - { > - warning ("Error reading helper thread's id in lib"); > - goto out; > - } > - > - if (tid == 0) > - { > - warning ("helper thread not initialized yet"); > - goto out; > - } > - > - if (write_inferior_memory (ipa_sym_addrs.addr_cmd_buf, > - (unsigned char *) cmd, strlen (cmd) + 1)) > - { > - warning ("Error writing command"); > - goto out; > - } > - > - ptid = ptid_build (pid, tid, 0); > - > - resume_thread (ptid); > - > - fd = gdb_ust_connect_sync_socket (pid); > - if (fd >= 0) > - { > - char buf[1] = ""; > - int ret; > - > - trace_debug ("signalling helper thread"); > - > - do > - { > - ret = write (fd, buf, 1); > - } while (ret == -1 && errno == EINTR); > - > - trace_debug ("waiting for helper thread's response"); > - > - do > - { > - ret = read (fd, buf, 1); > - } while (ret == -1 && errno == EINTR); > - > - close (fd); > - > - trace_debug ("helper thread's response received"); > - } > - > - out: > - > - /* Need to read response with the inferior stopped. */ > - if (!ptid_equal (ptid, null_ptid)) > - { > - int was_non_stop = non_stop; > - struct target_waitstatus status; > - > - stop_thread (ptid); > - non_stop = 1; > - mywait (ptid, &status, 0, 0); > - non_stop = was_non_stop; > - } > - > - if (fd >= 0) > - { > - if (read_inferior_memory (ipa_sym_addrs.addr_cmd_buf, > - (unsigned char *) cmd, CMD_BUF_SIZE)) > - { > - warning ("Error reading command response"); > - } > - else > - { > - err = 0; > - trace_debug ("run_inferior_command: response: %s", cmd); > - } > - } > + agent_run_command (pid, (const char *) cmd, len); > > reinsert_all_breakpoints (); > unpause_all (0); > -- 1.7.0.4 -- Pedro Alves