From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6730 invoked by alias); 23 Feb 2012 21:01:44 -0000 Received: (qmail 6718 invoked by uid 22791); 23 Feb 2012 21:01:42 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,KAM_MX3,KAM_STOCKGEN,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_CP,TW_QT,TW_XS,T_RP_MATCHES_RCVD,URIBL_BLACK 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, 23 Feb 2012 21:01:28 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q1NL1PwS000737 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 23 Feb 2012 16:01:25 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q1NL1NYD024216; Thu, 23 Feb 2012 16:01:24 -0500 Message-ID: <4F46A923.1090005@redhat.com> Date: Thu, 23 Feb 2012 21:05: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/9] move agent related code from gdbserver to common/agent.c References: <1329447300-18841-1-git-send-email-yao@codesourcery.com> <1329447300-18841-2-git-send-email-yao@codesourcery.com> In-Reply-To: <1329447300-18841-2-git-send-email-yao@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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/msg00517.txt.bz2 On 02/17/2012 02:54 AM, Yao Qi wrote: > 2012-02-13 Yao Qi > > * common/agent.c: New. > * common/agent.h: New. > * configure.ac: Add `sys/socket.h' and `linux/un.h' to > AC_CHECK_HEADERS. > * configure, configh.in: Regenerated. > > gdb/gdbserver: > > 2012-02-13 Yao Qi > > * Makefile.in (OBS): Add agent.o. > Add new rule for agent.o. > Track dependence of tracepoint.c on agent.h. > * 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_ust_socket_init): Renamed to ... > (gdb_agent_socket_init): ... New. > (gdb_ust_thread): Renamed to ... > (gdb_agent_helper_thread): ... New. > (gdb_ust_init): Move some code to ... > (gdb_agent_init): ... here. New. > [HAVE_UST]: Call gdb_ust_init. > (initialize_tracepoint_ftlib): Call gdb_agent_init. > * configure.ac: Add `linux/un.h' to AC_CHECK_HEADERS. > * config.in, configure: Regenerated. Something fishy with the indentation here. Make entries and indented with one tab. > --- > gdb/common/agent.c | 297 ++++++++++++++++++++++++++++++++++++++++++++ > gdb/common/agent.h | 37 ++++++ > gdb/config.in | 6 + > gdb/configure | 20 +++- > gdb/configure.ac | 9 ++- > gdb/gdbserver/Makefile.in | 8 +- > gdb/gdbserver/config.in | 3 + > gdb/gdbserver/configure | 19 +++ > gdb/gdbserver/configure.ac | 8 ++ > gdb/gdbserver/tracepoint.c | 285 +++++++++--------------------------------- > 10 files changed, 464 insertions(+), 228 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..657bba6 > --- /dev/null > +++ b/gdb/common/agent.c > @@ -0,0 +1,297 @@ > +/* Shared utility routines for GDB to interact with agent. > + > + Copyright (C) 2009-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 . */ > + > +#ifdef GDBSERVER > +#include "server.h" > +#else > +#include "defs.h" > +#include "target.h" > +#include "inferior.h" /* for non_stop */ > +#endif > + > +#include > +#include > +#include "agent.h" > + > +int debug_agent = 0; > + > +#ifdef GDBSERVER > +#define DEBUG_AGENT(fmt, args...) \ > + if (debug_agent) \ > + fprintf (stderr, fmt, ##args); > +#else > +#define DEBUG_AGENT(fmt, args...) \ > + if (debug_agent) \ > + fprintf_unfiltered (gdb_stdlog, fmt, ##args); > +#endif > + > +/* 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. FIXME: this global should be made > + per-process. */ > +static unsigned int helper_thread_id = 0; > + > +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; > + > +/* Look up all symbols needed by agent. Return 0 if all the symbols are > + found, return non-zero otherwise. */ > + > +int > +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 > + > + 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 > + { > + DEBUG_AGENT ("symbol `%s' not found\n", symbol_list[i].name); > + return -1; > + } > + } > + > + return 0; > +} > + > +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); > + else > +#endif > + { > + warning ("Error reading helper thread's id in lib"); > + } > + } > + > + return helper_thread_id; > +} > + > +#ifdef HAVE_LINUX_UN_H > +#include > +#include > +#define SOCK_DIR P_tmpdir > +#endif > + > +/* Connects to synchronization socket. PID is the pid of inferior, which is > + used to set up connection socket. */ set up the connection socket. > + > +static int > +gdb_connect_sync_socket (int pid) > +{ > +#ifdef HAVE_LINUX_UN_H > + 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; > + } > + > + return fd; > +#else > + return -1; > +#endif > +} > + > +/* Execute an agent command in inferior. PID is the value of pid of inferior. in the inferior. of the inferior. > + CMD is the buffer for command. GDB or GDBserver will store command into store the command. > + it and fetch return result from CMD. The interaction between GDB/GDBserver fetch the return result. > + and agent is synchronized by synchronization socket. Return zeor if success, and the agent. by a synchronization socket. zero. > + otherwise return non-zero. */ > + > +int > +agent_run_command (int pid, const char *cmd) > +{ The code at the caller in gdbserver also stops all threads (try with non-stop), and removes breakpoints , so that the helper thread doesn't trip on any breakpoint (e.g., a breakpoint at write/read, or a catch system), so this is easy to break on gdb as is, but okay. > + int fd; > + int tid = agent_get_helper_thread_id (); > + ptid_t ptid = ptid_build (pid, tid, 0); > + int len = strlen (cmd) + 1; > + > +#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 -1; > + } > + > + DEBUG_AGENT ("agent: resumed helper thread\n"); > + > + /* 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_connect_sync_socket (pid); > + if (fd >= 0) > + { > + char buf[1] = ""; > + int ret; > + > + DEBUG_AGENT ("agent: signalling helper thread\n"); > + > + do > + { > + ret = write (fd, buf, 1); > + } while (ret == -1 && errno == EINTR); > + > + DEBUG_AGENT ("agent:waiting for helper thread's response\n"); missing space after colon. > + > + do > + { > + ret = read (fd, buf, 1); > + } while (ret == -1 && errno == EINTR); > + > + close (fd); > + > + DEBUG_AGENT ("agent:helper thread's response received\n"); missing space after colon. > + } > + else > + return -1; > + > + /* Need to read response with the inferior stopped. */ > + if (!ptid_equal (ptid, null_ptid)) > + { > + struct target_waitstatus status; > + int was_non_stop = non_stop; > + /* Stop thread PTID. */ > + DEBUG_AGENT ("agent: stop helper thread\n"); > +#ifdef GDBSERVER > + { > + 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); > + } > + > + non_stop = 1; > + mywait (ptid, &status, 0, 0); > +#else > + non_stop = 1; > + target_stop (ptid); > + > + memset (&status, 0, sizeof (status)); > + target_wait (ptid, &status, 0); > +#endif > + non_stop = was_non_stop; > + } > + > + if (fd >= 0) > + { > +#ifdef GDBSERVER > + if (read_inferior_memory (ipa_sym_addrs.addr_cmd_buf, > + (unsigned char *) cmd, IPA_CMD_BUF_SIZE)) > +#else > + if (target_read_memory (ipa_sym_addrs.addr_cmd_buf, (gdb_byte *) cmd, > + IPA_CMD_BUF_SIZE)) > +#endif > + { > + warning ("Error reading command response"); > + return -1; > + } > + } > + > + return 0; > +} > @@ -7746,24 +7572,22 @@ run_inferior_command (char *cmd) > return err; > } > > -#else /* HAVE_UST */ > +#else /* !IN_PROCESS_AGENT */ > > -static int > -run_inferior_command (char *cmd) > -{ > - return -1; > -} > +#include > +#include > Hard to read, but are these now outside of HAVE_UST? That'll break mingw builds. > -#endif /* HAVE_UST */ > +#ifndef UNIX_PATH_MAX > +#define UNIX_PATH_MAX sizeof(((struct sockaddr_un *) NULL)->sun_path) > +#endif > > -#else /* !IN_PROCESS_AGENT */ > +/* Where we put the socked used for synchronization. */ > +#define SOCK_DIR P_tmpdir > @@ -8107,7 +7946,12 @@ gdb_ust_thread (void *arg) > > if (cmd_buf[0]) > { > - if (strcmp ("qTfSTM", cmd_buf) == 0) > + if (strcmp (cmd_buf, "help") == 0) > + { > + strcpy (cmd_buf, "for help, press F1\n"); > + } LOL, I remember writing that as a joke, but I didn't remember that it had ended up in the tree. > +#ifdef HAVE_UST > + else if (strcmp ("qTfSTM", cmd_buf) == 0) > { > cmd_qtfstm (cmd_buf); > } > @@ -8133,10 +7977,7 @@ gdb_ust_thread (void *arg) > { > cmd_qtstmat (cmd_buf); > } > - else if (strcmp (cmd_buf, "help") == 0) > - { > - strcpy (cmd_buf, "for help, press F1\n"); > - } > +#endif /* HAVE_UST */ > else > strcpy (cmd_buf, ""); > } > @@ -8151,18 +7992,16 @@ gdb_ust_thread (void *arg) > } Otherwise okay. -- Pedro Alves