public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH v2 0/6] Add vDefaultInferiorFd feature
@ 2024-01-19 11:56 Alexandra Hájková
  2024-01-19 11:56 ` [PATCH v2 1/6] gdb.server/non-existing-program.exp: Use gdbserver_start Alexandra Hájková
                   ` (7 more replies)
  0 siblings, 8 replies; 14+ messages in thread
From: Alexandra Hájková @ 2024-01-19 11:56 UTC (permalink / raw)
  To: gdb-patches

Currently, when GDBserver is run locally using stdio, the inferior
is unable to read from STDIN so we can't give it any input.
The main motivation to address this issue is to use GDB together
with Valgrind, using vgdb --multi feature which allows to run
Valgrind from inside GDB. Valgrind then acts as a locally run
GDBserver that uses stdio.

Add a new DefaultInferiorFd feature and the corresponding packet.
This feature allows GDB to send, to GDBserver, the file descriptor
numbers of the terminal to which GDB is connected. The inferior is
then started connected to the same terminal as GDB. This allows the
inferior run by local GDBserver to read from GDB's STDIN and write
its output to GDB's STOUT/ERR the same way as native target.

v2: - [2/6]:- add an error handling in a case dup would fail
            - hoist the calls to mark_fd_no_cloexec near the dup()s
      [3/6] - improve documentation formatting
            - rephrase and extend the documentation
            - cosmetics
            - add a new target method so that gdbserver doesn't advertise
              this new feauture on platforms it does not work at
      [4/3] - cosmetics

Alexandra Hájková (6):
  gdb.server/non-existing-program.exp: Use gdbserver_start.
  gdb/ser-pipe.c: Duplicate the file descriptors
  Add new vDefaultInferiorFd packet
  gdbserver/linux-low.cc: Connect the inferior to the terminal
  remote.c: Add terminal handling functions
  Add defaultinf.exp test to the testsuite

 gdb/doc/gdb.texinfo                           |  42 ++++++
 gdb/remote.c                                  |  83 +++++++++++
 gdb/ser-pipe.c                                |  40 ++++++
 gdb/serial.c                                  |   4 +
 gdb/serial.h                                  |   4 +
 gdb/testsuite/gdb.server/defaultinf.c         |  39 +++++
 gdb/testsuite/gdb.server/defaultinf.exp       |  59 ++++++++
 .../gdb.server/non-existing-program.exp       |  54 ++-----
 gdb/testsuite/lib/gdbserver-support.exp       |  62 +++++---
 gdbserver/linux-low.cc                        |  38 ++++-
 gdbserver/linux-low.h                         |   2 +
 gdbserver/server.cc                           | 135 +++++++++++++++++-
 gdbserver/server.h                            |  12 ++
 gdbserver/target.cc                           |   6 +
 gdbserver/target.h                            |   6 +
 15 files changed, 524 insertions(+), 62 deletions(-)
 create mode 100644 gdb/testsuite/gdb.server/defaultinf.c
 create mode 100644 gdb/testsuite/gdb.server/defaultinf.exp

-- 
2.43.0


^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH v2] remote.c: Make packet_check_result return a structure
@ 2024-01-07 14:56 Alexandra Hájková
  2024-01-09 19:01 ` Tom Tromey
  0 siblings, 1 reply; 14+ messages in thread
From: Alexandra Hájková @ 2024-01-07 14:56 UTC (permalink / raw)
  To: gdb-patches

packet_check_result currently returns an packet_result enum.
If GDB will recieve an error in a format E.errtext, which
is possible for some q packets, such errtext is lost if
treated by packet_check_result.
Introduce a new structure which bundles enum packet_result
with possible error message or error code returned by
the GDBserver.
I plan to make more GDBserver response checking functions to use
packet_check_result to make remote.c code more consistent.
This will also allow to use E.errtext more in the future.

There's no infrastructure to test this with a test case so
I tested this by modifying store_registers_using_G function
to get an error message:

[remote] Sending packet: $GG00000000 ...

gdbserver: Wrong sized register packet (expected 1792 bytes, got 1793)
gdbserver: Invalid hex digit 71
Killing process(es): 1104002
[remote] Packet received: E01
Could not write registers; remote failure reply '01'

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
---
v2:
- Added a unit test for packet_check_result.
- Improved formatting and reorganised packet_result structure.
- Renamed pkt_status to pkt_result and result to status varariablese
where suitable.

 gdb/remote.c | 147 +++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 113 insertions(+), 34 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index dcc1a0d0639..8e51e588bce 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -149,13 +149,46 @@ get_target_type_name (bool target_connected)
 /* Analyze a packet's return value and update the packet config
    accordingly.  */
 
-enum packet_result
+enum packet_status
 {
   PACKET_ERROR,
   PACKET_OK,
   PACKET_UNKNOWN
 };
 
+/* Keeps packet's return value. If packet's return value is PACKET_ERROR,
+   err_msg contains an error message string from E.string or the number
+   stored as a string from E.num.  */
+struct packet_result
+{
+  packet_result (enum packet_status status, std::string err_msg):
+    m_status (status), m_err_msg (std::move (err_msg))
+  {
+    gdb_assert (status == PACKET_ERROR);
+  }
+
+  explicit packet_result (enum packet_status status):
+    m_status (status)
+  {
+    gdb_assert (status != PACKET_ERROR);
+  }
+
+  enum packet_status status () const
+    {
+      return this->m_status;
+    }
+
+  const char *err_msg () const
+    {
+      gdb_assert (this->m_status == PACKET_ERROR);
+      return this->m_err_msg.c_str ();
+    }
+
+private:
+  enum packet_status m_status;
+  std::string m_err_msg;
+};
+
 /* Enumeration of packets for a remote target.  */
 
 enum {
@@ -732,8 +765,8 @@ struct remote_features
 
 /* Check result value in BUF for packet WHICH_PACKET and update the packet's
    support configuration accordingly.  */
-  packet_result packet_ok (const char *buf, const int which_packet);
-  packet_result packet_ok (const gdb::char_vector &buf, const int which_packet);
+  packet_status packet_ok (const char *buf, const int which_packet);
+  packet_status packet_ok (const gdb::char_vector &buf, const int which_packet);
 
   /* Configuration of a remote target's memory read packet.  */
   memory_packet_config m_memory_read_packet_config;
@@ -1254,7 +1287,7 @@ class remote_target : public process_stratum_target
 					int unit_size,
 					ULONGEST *xfered_len);
 
-  packet_result remote_send_printf (const char *format, ...)
+  packet_status remote_send_printf (const char *format, ...)
     ATTRIBUTE_PRINTF (2, 3);
 
   target_xfer_status remote_flash_write (ULONGEST address,
@@ -2418,7 +2451,10 @@ add_packet_config_cmd (const unsigned int which_packet, const char *name,
     }
 }
 
-static enum packet_result
+/* Check GDBserver's reply packet. Return packet_result
+   structure which contains the packet_status enum
+   and an error message for the PACKET_ERROR case.  */
+static packet_result
 packet_check_result (const char *buf)
 {
   if (buf[0] != '\0')
@@ -2428,42 +2464,46 @@ packet_check_result (const char *buf)
       if (buf[0] == 'E'
 	  && isxdigit (buf[1]) && isxdigit (buf[2])
 	  && buf[3] == '\0')
+      {
 	/* "Enn"  - definitely an error.  */
-	return PACKET_ERROR;
+	return { PACKET_ERROR, buf + 1 };
+      }
 
       /* Always treat "E." as an error.  This will be used for
 	 more verbose error messages, such as E.memtypes.  */
       if (buf[0] == 'E' && buf[1] == '.')
-	return PACKET_ERROR;
+      {
+	return { PACKET_ERROR, buf + 2 };
+      }
 
       /* The packet may or may not be OK.  Just assume it is.  */
-      return PACKET_OK;
+      return packet_result ( PACKET_OK );
     }
   else
+  {
     /* The stub does not support the packet.  */
-    return PACKET_UNKNOWN;
+    return packet_result ( PACKET_UNKNOWN );
+  }
 }
 
-static enum packet_result
+static packet_result
 packet_check_result (const gdb::char_vector &buf)
 {
   return packet_check_result (buf.data ());
 }
 
-packet_result
+packet_status
 remote_features::packet_ok (const char *buf, const int which_packet)
 {
   packet_config *config = &m_protocol_packets[which_packet];
   packet_description *descr = &packets_descriptions[which_packet];
 
-  enum packet_result result;
-
   if (config->detect != AUTO_BOOLEAN_TRUE
       && config->support == PACKET_DISABLE)
     internal_error (_("packet_ok: attempt to use a disabled packet"));
 
-  result = packet_check_result (buf);
-  switch (result)
+  packet_result result = packet_check_result (buf);
+  switch (result.status ())
     {
     case PACKET_OK:
     case PACKET_ERROR:
@@ -2498,10 +2538,10 @@ remote_features::packet_ok (const char *buf, const int which_packet)
       break;
     }
 
-  return result;
+  return result.status ();
 }
 
-packet_result
+packet_status
 remote_features::packet_ok (const gdb::char_vector &buf, const int which_packet)
 {
   return packet_ok (buf.data (), which_packet);
@@ -3000,7 +3040,7 @@ remote_target::set_syscall_catchpoint (int pid, bool needed, int any_count,
 				       gdb::array_view<const int> syscall_counts)
 {
   const char *catch_packet;
-  enum packet_result result;
+  enum packet_status result;
   int n_sysno = 0;
 
   if (m_features.packet_support (PACKET_QCatchSyscalls) == PACKET_DISABLE)
@@ -8791,9 +8831,10 @@ remote_target::send_g_packet ()
   xsnprintf (rs->buf.data (), get_remote_packet_size (), "g");
   putpkt (rs->buf);
   getpkt (&rs->buf);
-  if (packet_check_result (rs->buf) == PACKET_ERROR)
+  packet_result result = packet_check_result (rs->buf);
+  if (result.status () == PACKET_ERROR)
     error (_("Could not read registers; remote failure reply '%s'"),
-	   rs->buf.data ());
+	   result.err_msg ());
 
   /* We can get out of synch in various cases.  If the first character
      in the buffer is not a hex character, assume that has happened
@@ -9099,9 +9140,10 @@ remote_target::store_registers_using_G (const struct regcache *regcache)
   bin2hex (regs, p, rsa->sizeof_g_packet);
   putpkt (rs->buf);
   getpkt (&rs->buf);
-  if (packet_check_result (rs->buf) == PACKET_ERROR)
+  packet_result pkt_status = packet_check_result (rs->buf);
+  if (pkt_status.status () == PACKET_ERROR)
     error (_("Could not write registers; remote failure reply '%s'"),
-	   rs->buf.data ());
+	   pkt_status.err_msg ());
 }
 
 /* Store register REGNUM, or all registers if REGNUM == -1, from the contents
@@ -9683,7 +9725,7 @@ remote_target::remote_read_bytes (CORE_ADDR memaddr,
    FORMAT and the remaining arguments, then gets the reply.  Returns
    whether the packet was a success, a failure, or unknown.  */
 
-packet_result
+packet_status
 remote_target::remote_send_printf (const char *format, ...)
 {
   struct remote_state *rs = get_remote_state ();
@@ -9705,8 +9747,9 @@ remote_target::remote_send_printf (const char *format, ...)
 
   rs->buf[0] = '\0';
   getpkt (&rs->buf);
+  packet_result pkt_status = packet_check_result (rs->buf);
 
-  return packet_check_result (rs->buf);
+  return pkt_status.status ();
 }
 
 /* Flash writing can take quite some time.  We'll set
@@ -9718,7 +9761,7 @@ void
 remote_target::flash_erase (ULONGEST address, LONGEST length)
 {
   int addr_size = gdbarch_addr_bit (current_inferior ()->arch ()) / 8;
-  enum packet_result ret;
+  enum packet_status ret;
   scoped_restore restore_timeout
     = make_scoped_restore (&remote_timeout, remote_flash_timeout);
 
@@ -11308,7 +11351,7 @@ remote_target::verify_memory (const gdb_byte *data, CORE_ADDR lma, ULONGEST size
   if (target_has_execution ()
       && m_features.packet_support (PACKET_qCRC) != PACKET_DISABLE)
     {
-      enum packet_result result;
+      enum packet_status status;
 
       /* Make sure the remote is pointing at the right process.  */
       set_general_process ();
@@ -11324,10 +11367,10 @@ remote_target::verify_memory (const gdb_byte *data, CORE_ADDR lma, ULONGEST size
 
       getpkt (&rs->buf);
 
-      result = m_features.packet_ok (rs->buf, PACKET_qCRC);
-      if (result == PACKET_ERROR)
+      status = m_features.packet_ok (rs->buf, PACKET_qCRC);
+      if (status == PACKET_ERROR)
 	return -1;
-      else if (result == PACKET_OK)
+      else if (status == PACKET_OK)
 	{
 	  for (target_crc = 0, tmp = &rs->buf[1]; *tmp; tmp++)
 	    target_crc = target_crc * 16 + fromhex (*tmp);
@@ -12210,7 +12253,7 @@ remote_target::get_thread_local_address (ptid_t ptid, CORE_ADDR lm,
       struct remote_state *rs = get_remote_state ();
       char *p = rs->buf.data ();
       char *endp = p + get_remote_packet_size ();
-      enum packet_result result;
+      enum packet_status result;
 
       strcpy (p, "qGetTLSAddr:");
       p += strlen (p);
@@ -12256,7 +12299,7 @@ remote_target::get_tib_address (ptid_t ptid, CORE_ADDR *addr)
       struct remote_state *rs = get_remote_state ();
       char *p = rs->buf.data ();
       char *endp = p + get_remote_packet_size ();
-      enum packet_result result;
+      enum packet_status result;
 
       strcpy (p, "qGetTIBAddr:");
       p += strlen (p);
@@ -13821,7 +13864,7 @@ remote_target::get_trace_status (struct trace_status *ts)
 {
   /* Initialize it just to avoid a GCC false warning.  */
   char *p = NULL;
-  enum packet_result result;
+  enum packet_status result;
   struct remote_state *rs = get_remote_state ();
 
   if (m_features.packet_support (PACKET_qTStatus) == PACKET_DISABLE)
@@ -14201,7 +14244,7 @@ remote_target::set_trace_buffer_size (LONGEST val)
       struct remote_state *rs = get_remote_state ();
       char *buf = rs->buf.data ();
       char *endbuf = buf + get_remote_packet_size ();
-      enum packet_result result;
+      enum packet_status result;
 
       gdb_assert (val >= 0 || val == -1);
       buf += xsnprintf (buf, endbuf - buf, "QTBuffer:size:");
@@ -15527,9 +15570,10 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
 
   putpkt (rs->buf);
   getpkt (&rs->buf);
+  packet_result pkt_status = packet_check_result (rs->buf);
 
   /* Verify if the request was successful.  */
-  return packet_check_result (rs->buf.data ()) == PACKET_OK;
+  return pkt_status.status () == PACKET_OK;
 }
 
 /* Return true if remote target T is non-stop.  */
@@ -15626,6 +15670,39 @@ test_memory_tagging_functions ()
 		      expected.length ()) == 0);
 }
 
+static void
+test_packet_check_result ()
+{
+  std::string buf = "E01";
+
+  packet_result result = packet_check_result (buf.data ());
+  SELF_CHECK (result.status () == PACKET_ERROR);
+
+  buf.assign("E1");
+  result = packet_check_result (buf.data ());
+  SELF_CHECK (result.status () == PACKET_OK);
+
+  buf.assign("E000");
+  result = packet_check_result (buf.data ());
+  SELF_CHECK (result.status () == PACKET_OK);
+
+  buf.assign("E.msg");
+  result = packet_check_result (buf.data ());
+  SELF_CHECK (result.status () == PACKET_ERROR);
+
+  buf.assign("E.");
+  result = packet_check_result (buf.data ());
+  SELF_CHECK (result.status () == PACKET_ERROR);
+
+  buf.assign("some response");
+  result = packet_check_result (buf.data ());
+  SELF_CHECK (result.status () == PACKET_OK);
+
+  buf.assign("");
+  result = packet_check_result (buf.data ());
+  SELF_CHECK (result.status () == PACKET_UNKNOWN);
+}
+
 } // namespace selftests
 #endif /* GDB_SELF_TEST */
 
@@ -16125,5 +16202,7 @@ from the target."),
 #if GDB_SELF_TEST
   selftests::register_test ("remote_memory_tagging",
 			    selftests::test_memory_tagging_functions);
+  selftests::register_test ("packet_check_result",
+			    selftests::test_packet_check_result);
 #endif
 }
-- 
2.43.0


^ permalink raw reply	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-02-23 16:19 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-19 11:56 [PATCH v2 0/6] Add vDefaultInferiorFd feature Alexandra Hájková
2024-01-19 11:56 ` [PATCH v2 1/6] gdb.server/non-existing-program.exp: Use gdbserver_start Alexandra Hájková
2024-01-19 11:56 ` [PATCH v2] remote.c: Make packet_check_result return a structure Alexandra Hájková
2024-02-22 14:37   ` Alexandra Petlanova Hajkova
2024-02-23 16:19     ` Tom Tromey
2024-01-19 11:56 ` [PATCH v2 2/6] gdb/ser-pipe.c: Duplicate the file descriptors Alexandra Hájková
2024-01-19 11:56 ` [PATCH v2 3/6] Add new vDefaultInferiorFd packet Alexandra Hájková
2024-01-19 12:04   ` Eli Zaretskii
2024-01-19 11:56 ` [PATCH v2 4/6] gdbserver/linux-low.cc: Connect the inferior to the terminal Alexandra Hájková
2024-01-19 11:56 ` [PATCH v2 5/6] remote.c: Add terminal handling functions Alexandra Hájková
2024-01-19 11:56 ` [PATCH v2 6/6] Add defaultinf.exp test to the testsuite Alexandra Hájková
2024-02-22 14:38 ` [PATCH v2 0/6] Add vDefaultInferiorFd feature Alexandra Petlanova Hajkova
  -- strict thread matches above, loose matches on Subject: below --
2024-01-07 14:56 [PATCH v2] remote.c: Make packet_check_result return a structure Alexandra Hájková
2024-01-09 19:01 ` Tom Tromey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).