From patchwork Tue Sep 19 05:45:11 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Klaus Gerlicher X-Patchwork-Id: 76343 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 408E03858410 for ; Tue, 19 Sep 2023 05:46:06 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 408E03858410 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1695102366; bh=h2yT5+pqSsEviwBI9O5cn4MKQUi/+D3iOJgIusPdfL0=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=mjQED3a1LNbyJFtADHtfKSy+nBFhjzzF4D7oOv6RUHH5K9qeAyO8EE4L2XeGyO0S6 HlZ5kksL3X7LZSh9eBUjd8GWxTkaGL0icRKV+PbxUqS6iPwFeawDLbsvsNiF9l1ArC eqVR7fDc/b6SRFbx7iX6933Np8oF5IUq1JyzFVkw= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.88]) by sourceware.org (Postfix) with ESMTPS id F364938582A3 for ; Tue, 19 Sep 2023 05:45:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org F364938582A3 X-IronPort-AV: E=McAfee;i="6600,9927,10837"; a="410789458" X-IronPort-AV: E=Sophos;i="6.02,158,1688454000"; d="scan'208";a="410789458" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Sep 2023 22:45:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10837"; a="861391556" X-IronPort-AV: E=Sophos;i="6.02,158,1688454000"; d="scan'208";a="861391556" Received: from dut1027pvc.igk.intel.com (HELO localhost) ([10.211.177.252]) by fmsmga002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Sep 2023 22:45:34 -0700 To: gdb-patches@sourceware.org Subject: [PATCH 1/1] gdb, gdbserver: replace PBUFSIZ with a target op Date: Tue, 19 Sep 2023 05:45:11 +0000 Message-Id: <20230919054511.17998-2-klaus.gerlicher@intel.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20230919054511.17998-1-klaus.gerlicher@intel.com> References: <20230919054511.17998-1-klaus.gerlicher@intel.com> MIME-Version: 1.0 X-Spam-Status: No, score=-10.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Klaus Gerlicher via Gdb-patches From: Klaus Gerlicher Reply-To: Klaus Gerlicher Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" From: "Gerlicher, Klaus" PBUFSIZ is a hardcoded value that needs to be as least twice as big as the register set. This means that this could be tailored to the target register size as not all targets need a big buffer. For SIMD targets the buffer will obviously be much bigger than for regular scalar targets. This patch changes this and adds a target op query_pbuf_size (). It also moves the static global client_state into the get_client_state () function and lazily dynamically allocates storage for its own_buf. --- gdb/remote.h | 8 +++--- gdb/target-delegates.c | 31 +++++++++++++++++++++-- gdb/target.h | 7 ++++++ gdbserver/hostio.cc | 54 ++++++++++++++++++++++++---------------- gdbserver/notif.cc | 8 +++--- gdbserver/server.cc | 26 +++++++++++-------- gdbserver/server.h | 31 ++++++++++++++++------- gdbserver/target.cc | 9 +++++++ gdbserver/target.h | 9 +++++++ gdbserver/tdesc.cc | 11 +++++--- gdbserver/tracepoint.cc | 4 +-- gdbsupport/common-defs.h | 5 ++++ 12 files changed, 146 insertions(+), 57 deletions(-) diff --git a/gdb/remote.h b/gdb/remote.h index 74366cdbcfb..b09b8c89663 100644 --- a/gdb/remote.h +++ b/gdb/remote.h @@ -56,10 +56,10 @@ extern void getpkt (remote_target *remote, char **buf, long *sizeof_buf, int forever); /* Send a packet to the remote machine, with error checking. The data - of the packet is in BUF. The string in BUF can be at most PBUFSIZ - - 5 to account for the $, # and checksum, and for a possible /0 if - we are debugging (remote_debug) and want to print the sent packet - as a string. */ + of the packet is in BUF. The string in BUF can be at most + target_query_pbuf_size () - 5 to account for the $, # and checksum, + and for a possible /0 if we are debugging (remote_debug) and want to + print the sent packet as a string. */ extern int putpkt (remote_target *remote, const char *buf); diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c index 8b066249624..bbb9614fcb2 100644 --- a/gdb/target-delegates.c +++ b/gdb/target-delegates.c @@ -197,6 +197,7 @@ struct dummy_target : public target_ops bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override; bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override; x86_xsave_layout fetch_x86_xsave_layout () override; + int query_pbuf_size () override; }; struct debug_target : public target_ops @@ -372,6 +373,7 @@ struct debug_target : public target_ops bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override; bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override; x86_xsave_layout fetch_x86_xsave_layout () override; + int query_pbuf_size () override; }; void @@ -4553,12 +4555,37 @@ dummy_target::fetch_x86_xsave_layout () x86_xsave_layout debug_target::fetch_x86_xsave_layout () { - x86_xsave_layout result; gdb_printf (gdb_stdlog, "-> %s->fetch_x86_xsave_layout (...)\n", this->beneath ()->shortname ()); - result = this->beneath ()->fetch_x86_xsave_layout (); + x86_xsave_layout result + = this->beneath ()->fetch_x86_xsave_layout (); gdb_printf (gdb_stdlog, "<- %s->fetch_x86_xsave_layout (", this->beneath ()->shortname ()); gdb_puts (") = ", gdb_stdlog); target_debug_print_x86_xsave_layout (result); gdb_puts ("\n", gdb_stdlog); return result; } + +int +target_ops::query_pbuf_size () +{ + return this->beneath ()->query_pbuf_size (); +} + +int +dummy_target::query_pbuf_size () +{ + return PBUFSIZ; +} + +int +debug_target::query_pbuf_size () +{ + gdb_printf (gdb_stdlog, "-> %s->query_pbuf_size (...)\n", this->beneath ()->shortname ()); + int result + = this->beneath ()->query_pbuf_size (); + gdb_printf (gdb_stdlog, "<- %s->query_pbuf_size (", this->beneath ()->shortname ()); + gdb_puts (") = ", gdb_stdlog); + target_debug_print_int (result); + gdb_puts ("\n", gdb_stdlog); + return result; +} diff --git a/gdb/target.h b/gdb/target.h index aa07075f9f2..8a2cc0a8b24 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -1330,6 +1330,10 @@ struct target_ops /* Return the x86 XSAVE extended state area layout. */ virtual x86_xsave_layout fetch_x86_xsave_layout () TARGET_DEFAULT_RETURN (x86_xsave_layout ()); + + /* Query PBUFSIZ from target instead of hard-coded #define'ing it. */ + virtual int query_pbuf_size () + TARGET_DEFAULT_RETURN (PBUFSIZ); }; /* Deleter for std::unique_ptr. See comments in @@ -2573,4 +2577,7 @@ extern void target_prepare_to_generate_core (void); /* See to_done_generating_core. */ extern void target_done_generating_core (void); +/* See target_ops::query_pbuf_size. */ +extern int target_query_pbuf_size (); + #endif /* !defined (TARGET_H) */ diff --git a/gdbserver/hostio.cc b/gdbserver/hostio.cc index ea70c26da0f..6092a33ec02 100644 --- a/gdbserver/hostio.cc +++ b/gdbserver/hostio.cc @@ -28,6 +28,7 @@ #include #include #include "gdbsupport/fileio.h" +#include struct fd_list { @@ -54,11 +55,17 @@ safe_fromhex (char a, int *nibble) /* Filenames are hex encoded, so the maximum we can handle is half the packet buffer size. Cap to PATH_MAX, if it is shorter. */ -#if !defined (PATH_MAX) || (PATH_MAX > (PBUFSIZ / 2 + 1)) -# define HOSTIO_PATH_MAX (PBUFSIZ / 2 + 1) +static int +hostio_path_max () +{ +#if !defined (PATH_MAX) + return target_query_pbuf_size () / 2 + 1; #else -# define HOSTIO_PATH_MAX PATH_MAX + return (PATH_MAX > (target_query_pbuf_size () / 2 + 1)) + ? target_query_pbuf_size () / 2 + 1 + : PATH_MAX; #endif +} static int require_filename (char **pp, char *filename) @@ -74,7 +81,7 @@ require_filename (char **pp, char *filename) int nib1, nib2; /* Don't allow overflow. */ - if (count >= HOSTIO_PATH_MAX - 1) + if (count >= hostio_path_max () - 1) return -1; if (safe_fromhex (p[0], &nib1) @@ -222,7 +229,7 @@ hostio_reply_with_data (char *own_buf, char *buffer, int len, sprintf (own_buf, "F%x;", len); output_index = strlen (own_buf); - out_maxlen = PBUFSIZ; + out_maxlen = target_query_pbuf_size (); for (input_index = 0; input_index < len; input_index++) { @@ -298,7 +305,7 @@ handle_setfs (char *own_buf) static void handle_open (char *own_buf) { - char filename[HOSTIO_PATH_MAX]; + std::vector filename (hostio_path_max ()); char *p; int fileio_flags, fileio_mode, flags, fd; mode_t mode; @@ -306,7 +313,7 @@ handle_open (char *own_buf) p = own_buf + strlen ("vFile:open:"); - if (require_filename (&p, filename) + if (require_filename (&p, filename.data ()) || require_comma (&p) || require_int (&p, &fileio_flags) || require_comma (&p) @@ -322,9 +329,10 @@ handle_open (char *own_buf) /* We do not need to convert MODE, since the fileio protocol uses the standard values. */ if (hostio_fs_pid != 0) - fd = the_target->multifs_open (hostio_fs_pid, filename, flags, mode); + fd = the_target->multifs_open (hostio_fs_pid, filename.data (), flags, + mode); else - fd = open (filename, flags, mode); + fd = open (filename.data (), flags, mode); if (fd == -1) { @@ -367,8 +375,8 @@ handle_pread (char *own_buf, int *new_packet_len) too much because of escaping, but this is handled below. */ if (max_reply_size == -1) { - sprintf (own_buf, "F%x;", PBUFSIZ); - max_reply_size = PBUFSIZ - strlen (own_buf); + sprintf (own_buf, "F%x;", target_query_pbuf_size ()); + max_reply_size = target_query_pbuf_size () - strlen (own_buf); } if (len > max_reply_size) len = max_reply_size; @@ -527,13 +535,13 @@ handle_close (char *own_buf) static void handle_unlink (char *own_buf) { - char filename[HOSTIO_PATH_MAX]; + std::vector filename (hostio_path_max ()); char *p; int ret; p = own_buf + strlen ("vFile:unlink:"); - if (require_filename (&p, filename) + if (require_filename (&p, filename.data ()) || require_end (p)) { hostio_packet_error (own_buf); @@ -541,9 +549,9 @@ handle_unlink (char *own_buf) } if (hostio_fs_pid != 0) - ret = the_target->multifs_unlink (hostio_fs_pid, filename); + ret = the_target->multifs_unlink (hostio_fs_pid, filename.data ()); else - ret = unlink (filename); + ret = unlink (filename.data ()); if (ret == -1) { @@ -557,13 +565,14 @@ handle_unlink (char *own_buf) static void handle_readlink (char *own_buf, int *new_packet_len) { - char filename[HOSTIO_PATH_MAX], linkname[HOSTIO_PATH_MAX]; + std::vector filename (hostio_path_max ()); + std::vector linkname (hostio_path_max ()); char *p; int ret, bytes_sent; p = own_buf + strlen ("vFile:readlink:"); - if (require_filename (&p, filename) + if (require_filename (&p, filename.data ()) || require_end (p)) { hostio_packet_error (own_buf); @@ -571,11 +580,11 @@ handle_readlink (char *own_buf, int *new_packet_len) } if (hostio_fs_pid != 0) - ret = the_target->multifs_readlink (hostio_fs_pid, filename, - linkname, - sizeof (linkname) - 1); + ret = the_target->multifs_readlink (hostio_fs_pid, filename.data (), + linkname.data (), + linkname.size () - 1); else - ret = readlink (filename, linkname, sizeof (linkname) - 1); + ret = readlink (filename.data (), linkname.data (), linkname.size ()); if (ret == -1) { @@ -583,7 +592,8 @@ handle_readlink (char *own_buf, int *new_packet_len) return; } - bytes_sent = hostio_reply_with_data (own_buf, linkname, ret, new_packet_len); + bytes_sent = hostio_reply_with_data (own_buf, linkname.data (), ret, + new_packet_len); /* If the response does not fit into a single packet, do not attempt to return a partial response, but simply fail. */ diff --git a/gdbserver/notif.cc b/gdbserver/notif.cc index 9b323b4a869..3fc93ac2e69 100644 --- a/gdbserver/notif.cc +++ b/gdbserver/notif.cc @@ -140,13 +140,13 @@ notif_push (struct notif_server *np, struct notif_event *new_event) about it, by sending a corresponding notification. */ if (is_first_event) { - char buf[PBUFSIZ]; - char *p = buf; + std::vector buf (target_query_pbuf_size ()); + char *p = buf.data (); - xsnprintf (p, PBUFSIZ, "%s:", np->notif_name); + xsnprintf (p, target_query_pbuf_size (), "%s:", np->notif_name); p += strlen (p); np->write (new_event, p); - putpkt_notif (buf); + putpkt_notif (buf.data ()); } } diff --git a/gdbserver/server.cc b/gdbserver/server.cc index c57270175b4..1314db953c3 100644 --- a/gdbserver/server.cc +++ b/gdbserver/server.cc @@ -161,13 +161,17 @@ static struct btrace_config current_btrace_conf; /* The client remote protocol state. */ -static client_state g_client_state; - client_state & get_client_state () { - client_state &cs = g_client_state; - return cs; + static client_state g_client_state; + + /* Lazily allocate ownbuf as clientstate is only ever used once a target is + available. */ + if (the_target != nullptr && g_client_state.own_buf == nullptr) + g_client_state.alloc_own_buf (); + + return g_client_state; } @@ -398,7 +402,7 @@ write_qxfer_response (char *buf, const gdb_byte *data, int len, int is_more) buf[0] = 'l'; return remote_escape_output (data, len, 1, (unsigned char *) buf + 1, - &out_len, PBUFSIZ - 2) + 1; + &out_len, target_query_pbuf_size () - 2) + 1; } /* Handle btrace enabling in BTS format. */ @@ -573,7 +577,7 @@ create_fetch_memtags_reply (char *reply, const gdb::byte_vector &tags) packet += bin2hex (tags.data (), tags.size ()); /* Check if the reply is too big for the packet to handle. */ - if (PBUFSIZ < packet.size ()) + if (target_query_pbuf_size () < packet.size ()) return false; strcpy (reply, packet.c_str ()); @@ -2018,8 +2022,8 @@ handle_qxfer (char *own_buf, int packet_len, int *new_packet_len_p) /* Read one extra byte, as an indicator of whether there is more. */ - if (len > PBUFSIZ - 2) - len = PBUFSIZ - 2; + if (len > target_query_pbuf_size () - 2) + len = target_query_pbuf_size () - 2; data = (unsigned char *) malloc (len + 1); if (data == NULL) { @@ -2376,7 +2380,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p) "QStartupWithShell+;QEnvironmentHexEncoded+;" "QEnvironmentReset+;QEnvironmentUnset+;" "QSetWorkingDir+", - PBUFSIZ - 1); + target_query_pbuf_size () - 1); if (target_supports_catch_syscall ()) strcat (own_buf, ";QCatchSyscalls+"); @@ -2578,7 +2582,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p) /* Handle "monitor" commands. */ if (startswith (own_buf, "qRcmd,")) { - char *mon = (char *) malloc (PBUFSIZ); + char *mon = (char *) malloc (target_query_pbuf_size ()); int len = strlen (own_buf + 6); if (mon == NULL) @@ -3894,7 +3898,7 @@ captured_main (int argc, char *argv[]) if (target_supports_tracepoints ()) initialize_tracepoint (); - mem_buf = (unsigned char *) xmalloc (PBUFSIZ); + mem_buf = (unsigned char *) xmalloc (target_query_pbuf_size ()); if (selftest) { diff --git a/gdbserver/server.h b/gdbserver/server.h index fde7dfe7060..2122c9e9429 100644 --- a/gdbserver/server.h +++ b/gdbserver/server.h @@ -101,11 +101,6 @@ extern int in_queued_stop_replies (ptid_t ptid); is chosen to fill up a packet (the headers account for the 32). */ #define MAXBUFBYTES(N) (((N)-32)/2) -/* Buffer sizes for transferring memory, registers, etc. Set to a constant - value to accommodate multiple register formats. This value must be at least - as large as the largest register set supported by gdbserver. */ -#define PBUFSIZ 18432 - /* Definition for an unknown syscall, used basically in error-cases. */ #define UNKNOWN_SYSCALL (-1) @@ -129,9 +124,27 @@ extern unsigned long signal_pid; struct client_state { - client_state (): - own_buf ((char *) xmalloc (PBUFSIZ + 1)) - {} + client_state () = default; + + client_state (const client_state& other) = delete; + client_state (const client_state&& other) = delete; + client_state& operator=(const client_state& other) = delete; + client_state& operator=(const client_state&& other) noexcept = delete; + + virtual ~client_state () + { + xfree (own_buf); + } + + /* Lazily allocate own_buf. This buffer is allocated one time only as soon + as we can query the target for the size information. */ + void alloc_own_buf () + { + /* We are asserting because we initially expect no allocation. */ + gdb_assert (own_buf == nullptr); + own_buf = (char *) xmalloc (target_query_pbuf_size () + 1); + gdb_assert (own_buf != nullptr); + } /* The thread set with an `Hc' packet. `Hc' is deprecated in favor of `vCont'. Note the multi-process extensions made `vCont' a @@ -177,7 +190,7 @@ struct client_state struct target_waitstatus last_status; ptid_t last_ptid; - char *own_buf; + char *own_buf = nullptr; /* If true, then GDB has requested noack mode. */ int noack_mode = 0; diff --git a/gdbserver/target.cc b/gdbserver/target.cc index f8e592d20c3..1de513aca9c 100644 --- a/gdbserver/target.cc +++ b/gdbserver/target.cc @@ -832,3 +832,12 @@ process_stratum_target::get_ipa_tdesc_idx () { return 0; } + +int +process_stratum_target::query_pbuf_size () +{ + /* Buffer sizes for transferring memory, registers, etc. The target decides + how big this needs to be but this value must be at least as large as the + largest register set supported by gdbserver. */ + return PBUFSIZ; +} diff --git a/gdbserver/target.h b/gdbserver/target.h index d993e361b76..7815be7f53c 100644 --- a/gdbserver/target.h +++ b/gdbserver/target.h @@ -508,6 +508,9 @@ class process_stratum_target Returns true if successful and false otherwise. */ virtual bool store_memtags (CORE_ADDR address, size_t len, const gdb::byte_vector &tags, int type); + + /* Query packet buffer size. */ + virtual int query_pbuf_size (); }; extern process_stratum_target *the_target; @@ -699,6 +702,12 @@ target_thread_pending_child (thread_info *thread) return the_target->thread_pending_child (thread); } +static inline int +target_query_pbuf_size () +{ + return the_target->query_pbuf_size (); +} + /* Read LEN bytes from MEMADDR in the buffer MYADDR. Return 0 if the read is successful, otherwise, return a non-zero error code. */ diff --git a/gdbserver/tdesc.cc b/gdbserver/tdesc.cc index 2c7257c458f..0f48a43e643 100644 --- a/gdbserver/tdesc.cc +++ b/gdbserver/tdesc.cc @@ -16,6 +16,7 @@ along with this program. If not, see . */ #include "server.h" +#include "target.h" #include "tdesc.h" #include "regdef.h" @@ -84,12 +85,16 @@ init_target_desc (struct target_desc *tdesc, tdesc->registers_size = offset / 8; - /* Make sure PBUFSIZ is large enough to hold a full register +#ifndef IN_PROCESS_AGENT + /* Make sure target packet buffer is large enough to hold a full register packet. */ - gdb_assert (2 * tdesc->registers_size + 32 <= PBUFSIZ); + gdb_assert (2 * tdesc->registers_size + 32 <= target_query_pbuf_size ()); -#ifndef IN_PROCESS_AGENT tdesc->expedite_regs = expedite_regs; +#else + /* Make sure PBUFSIZ is large enough to hold a full register + packet. */ + gdb_assert (2 * tdesc->registers_size + 32 <= PBUFSIZ); #endif } diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc index 609d49a87ef..b7ec9e1bf25 100644 --- a/gdbserver/tracepoint.cc +++ b/gdbserver/tracepoint.cc @@ -4018,8 +4018,8 @@ cmd_qtbuffer (char *own_buf) num = tot - offset; /* Trim to available packet size. */ - if (num >= (PBUFSIZ - 16) / 2 ) - num = (PBUFSIZ - 16) / 2; + if (num >= (target_query_pbuf_size () - 16) / 2) + num = (target_query_pbuf_size () - 16) / 2; bin2hex (tbp, own_buf, num); } diff --git a/gdbsupport/common-defs.h b/gdbsupport/common-defs.h index 72c1c5144f3..462ce00ed04 100644 --- a/gdbsupport/common-defs.h +++ b/gdbsupport/common-defs.h @@ -230,4 +230,9 @@ #define HAVE_USEFUL_SBRK 1 #endif +/* The default packet buffer size for transferring memory, registers, etc. If a + * target needs a bigger (or smaller) buffer, it should implement the target op + * query_pbuf_size (). */ +#define PBUFSIZ 18432 + #endif /* COMMON_COMMON_DEFS_H */