[PATCH 0/6] Fix error messages from serial code

[PATCH 0/6] Fix error messages from serial code

[Tom Tromey]

Internal AdaCore testing on a Windows host with a target running on
qemu revealed that remote.c could sometimes display random error
messages.  For example, different runs of the same test might display:

    Remote communication error.  Target disconnected.: Arg list too long.
    Remote communication error.  Target disconnected.: No error.
    Remote communication error.  Target disconnected.: Not a directory.

I looked into this and this series is the result.  It's hard to be
sure that this completely solves the problem -- there are a lot of
code paths and the errors aren't very reproducible -- but it helps.

Regression tested using native-extended-gdbserver on x86-64 Fedora 36.

---
Tom Tromey (6):
      Fix latent bug in ser_windows_send_break
      Introduce throw_winerror_with_name
      Change serial_setbaudrate to throw exception
      Change serial "open" functions to throw exception
      Change serial_send_break and serial_write to throw
      Change serial_readchar to throw

 gdb/nat/windows-nat.c  |   6 +--
 gdb/remote.c           | 106 +++++++++++++++++++++++++++----------------------
 gdb/ser-base.c         |  12 +++---
 gdb/ser-base.h         |   6 +--
 gdb/ser-event.c        |   4 +-
 gdb/ser-mingw.c        |  86 ++++++++++++++++++---------------------
 gdb/ser-pipe.c         |  12 +++---
 gdb/ser-tcp.c          |  93 ++++++++++++++++++++++++-------------------
 gdb/ser-tcp.h          |   4 +-
 gdb/ser-uds.c          |  36 ++++++++---------
 gdb/ser-unix.c         |  57 ++++++++++++--------------
 gdb/serial.c           |  44 ++++++++------------
 gdb/serial.h           |  22 +++++-----
 gdb/windows-nat.c      |  31 +++++++++------
 gdbserver/win32-low.cc |  11 +++--
 gdbsupport/errors.cc   |   8 ++++
 gdbsupport/errors.h    |   7 ++++
 17 files changed, 279 insertions(+), 266 deletions(-)
---
base-commit: d1722abe60ca7c330210aa97c8ec52ff98644206
change-id: 20230912-serial-exceptions-07104644e711

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH 1/6] Fix latent bug in ser_windows_send_break

[Tom Tromey]

The ClearCommBreak documentation says:

    If the function fails, the return value is zero.

ser_windows_send_break inverts this check.  This has never been
noticed because the caller doesn't check the result.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30770
---
 gdb/ser-mingw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/ser-mingw.c b/gdb/ser-mingw.c
index c0aa5d8d0b9..806f3999385 100644
--- a/gdb/ser-mingw.c
+++ b/gdb/ser-mingw.c
@@ -137,7 +137,7 @@ ser_windows_send_break (struct serial *scb)
   /* Delay for 250 milliseconds.  */
   Sleep (250);
 
-  if (ClearCommBreak (h))
+  if (ClearCommBreak (h) == 0)
     return -1;
 
   return 0;

-- 
2.40.1

[PATCH 2/6] Introduce throw_winerror_with_name

[Tom Tromey]

This introduces throw_winerror_with_name, a Windows analog of
perror_with_name, and changes various places in gdb to call it.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30770
---
 gdb/nat/windows-nat.c  |  6 ++----
 gdb/windows-nat.c      | 31 +++++++++++++++++++------------
 gdbserver/win32-low.cc | 11 +++++------
 gdbsupport/errors.cc   |  8 ++++++++
 gdbsupport/errors.h    |  7 +++++++
 5 files changed, 41 insertions(+), 22 deletions(-)

diff --git a/gdb/nat/windows-nat.c b/gdb/nat/windows-nat.c
index 8028494de20..c004b64269b 100644
--- a/gdb/nat/windows-nat.c
+++ b/gdb/nat/windows-nat.c
@@ -206,8 +206,7 @@ windows_process_info::get_exec_module_filename (char *exe_name_ret,
     if (len == 0)
       {
 	unsigned err = (unsigned) GetLastError ();
-	error (_("Error getting executable filename (error %u): %s"),
-	       err, strwinerror (err));
+	throw_winerror_with_name (_("Error getting executable filename"), err);
       }
     if (cygwin_conv_path (CCP_WIN_W_TO_POSIX, pathbuf, exe_name_ret,
 			  exe_name_max_len) < 0)
@@ -219,8 +218,7 @@ windows_process_info::get_exec_module_filename (char *exe_name_ret,
   if (len == 0)
     {
       unsigned err = (unsigned) GetLastError ();
-      error (_("Error getting executable filename (error %u): %s"),
-	     err, strwinerror (err));
+      throw_winerror_with_name (_("Error getting executable filename"), err);
     }
 #endif
 
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index d8b33452f46..b26dec12a56 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -1346,9 +1346,9 @@ windows_nat_target::windows_continue (DWORD continue_status, int id,
     });
 
   if (err.has_value ())
-    error (_("Failed to resume program execution"
-	     " (ContinueDebugEvent failed, error %u: %s)"),
-	   *err, strwinerror (*err));
+    throw_winerror_with_name (_("Failed to resume program execution"
+				" - ContinueDebugEvent failed"),
+			      *err);
 
   return TRUE;
 }
@@ -1366,8 +1366,7 @@ windows_nat_target::fake_create_process ()
   else
     {
       unsigned err = (unsigned) GetLastError ();
-      error (_("OpenProcess call failed, GetLastError = %u: %s"),
-	     err, strwinerror (err));
+      throw_winerror_with_name (_("OpenProcess call failed"), err);
       /*  We can not debug anything in that case.  */
     }
   add_thread (ptid_t (windows_process.current_event.dwProcessId, 0,
@@ -2047,8 +2046,11 @@ windows_nat_target::attach (const char *args, int from_tty)
     });
 
   if (err.has_value ())
-    error (_("Can't attach to process %u (error %u: %s)"),
-	   (unsigned) pid, *err, strwinerror (*err));
+    {
+      std::string msg = string_printf (_("Can't attach to process %u"),
+				       (unsigned) pid);
+      throw_winerror_with_name (msg.c_str (), *err);
+    }
 
   DebugSetProcessKillOnExit (FALSE);
 
@@ -2085,9 +2087,12 @@ windows_nat_target::detach (inferior *inf, int from_tty)
     });
 
   if (err.has_value ())
-    error (_("Can't detach process %u (error %u: %s)"),
-	   (unsigned) windows_process.current_event.dwProcessId,
-	   *err, strwinerror (*err));
+    {
+      std::string msg
+	= string_printf (_("Can't detach process %u"),
+			 (unsigned) windows_process.current_event.dwProcessId);
+      throw_winerror_with_name (msg.c_str (), *err);
+    }
 
   target_announce_detach (from_tty);
 
@@ -2790,8 +2795,10 @@ windows_nat_target::create_inferior (const char *exec_file,
 #endif	/* !__CYGWIN__ */
 
   if (ret.has_value ())
-    error (_("Error creating process %s, (error %u: %s)"),
-	   exec_file, *ret, strwinerror (*ret));
+    {
+      std::string msg = _("Error creating process ") + std::string (exec_file);
+      throw_winerror_with_name (msg.c_str (), *ret);
+    }
 
 #ifdef __x86_64__
   BOOL wow64;
diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index 3246957ec44..d94f4a60bc0 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -324,8 +324,7 @@ do_initial_child_stuff (HANDLE proch, DWORD pid, int attached)
   if (!IsWow64Process (proch, &wow64))
     {
       DWORD err = GetLastError ();
-      error ("Check if WOW64 process failed (error %d): %s\n",
-	     (int) err, strwinerror (err));
+      throw_winerror_with_name ("Check if WOW64 process failed", err);
     }
   windows_process.wow64_process = wow64;
 
@@ -579,8 +578,9 @@ win32_process_target::create_inferior (const char *program,
 
   if (!ret)
     {
-      error ("Error creating process \"%s %s\", (error %d): %s\n",
-	     program, args, (int) err, strwinerror (err));
+      std::string msg = string_printf (_("Error creating process \"%s %s\""),
+				       program, args);
+      throw_winerror_with_name (msg.c_str (), err);
     }
   else
     {
@@ -627,8 +627,7 @@ win32_process_target::attach (unsigned long pid)
     }
 
   err = GetLastError ();
-  error ("Attach to process failed (error %d): %s\n",
-	 (int) err, strwinerror (err));
+  throw_winerror_with_name ("Attach to process failed", err);
 }
 
 /* See nat/windows-nat.h.  */
diff --git a/gdbsupport/errors.cc b/gdbsupport/errors.cc
index b48ce10eef8..59f934c6916 100644
--- a/gdbsupport/errors.cc
+++ b/gdbsupport/errors.cc
@@ -144,4 +144,12 @@ strwinerror (ULONGEST error)
   return buf;
 }
 
+/* See errors.h.  */
+
+void
+throw_winerror_with_name (const char *string, ULONGEST err)
+{
+  error (_("%s (error %d): %s"), string, err, strwinerror (err));
+}
+
 #endif /* USE_WIN32API */
diff --git a/gdbsupport/errors.h b/gdbsupport/errors.h
index 20f9152b671..a94debf24e6 100644
--- a/gdbsupport/errors.h
+++ b/gdbsupport/errors.h
@@ -120,6 +120,13 @@ extern void flush_streams ();
 
 extern const char *strwinerror (ULONGEST error);
 
+/* Like perror_with_name, but for Windows errors.  Throw an exception
+   including STRING and the system text for the given error
+   number.  */
+
+extern void throw_winerror_with_name (const char *string, ULONGEST err)
+  ATTRIBUTE_NORETURN;
+
 #endif /* USE_WIN32API */
 
 #endif /* COMMON_ERRORS_H */

-- 
2.40.1

[PATCH 3/6] Change serial_setbaudrate to throw exception

[Tom Tromey]

remote.c has this code:

      if (serial_setbaudrate (rs->remote_desc, baud_rate))
	{
	  /* The requested speed could not be set.  Error out to
	     top level after closing remote_desc.  Take care to
	     set remote_desc to NULL to avoid closing remote_desc
	     more than once.  */
	  serial_close (rs->remote_desc);
	  rs->remote_desc = NULL;
	  perror_with_name (name);

The perror here cannot be correct, because if serial_setbaudrate did
set errno, it may be obscured by serial_close.

This patch changes serial_setbaudrate to throw an exception instead.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30770
---
 gdb/remote.c    |  8 ++++++--
 gdb/ser-base.c  |  4 ++--
 gdb/ser-base.h  |  2 +-
 gdb/ser-mingw.c |  7 ++++---
 gdb/ser-unix.c  | 33 ++++++++++++---------------------
 gdb/serial.c    |  4 ++--
 gdb/serial.h    |  8 ++++----
 7 files changed, 31 insertions(+), 35 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index ba81c5b0b6f..9346ae8b3a1 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5959,7 +5959,11 @@ remote_target::open_1 (const char *name, int from_tty, int extended_p)
 
   if (baud_rate != -1)
     {
-      if (serial_setbaudrate (rs->remote_desc, baud_rate))
+      try
+	{
+	  serial_setbaudrate (rs->remote_desc, baud_rate);
+	}
+      catch (const gdb_exception_error &)
 	{
 	  /* The requested speed could not be set.  Error out to
 	     top level after closing remote_desc.  Take care to
@@ -5967,7 +5971,7 @@ remote_target::open_1 (const char *name, int from_tty, int extended_p)
 	     more than once.  */
 	  serial_close (rs->remote_desc);
 	  rs->remote_desc = NULL;
-	  perror_with_name (name);
+	  throw;
 	}
     }
 
diff --git a/gdb/ser-base.c b/gdb/ser-base.c
index 0883305ac2b..072211df1ca 100644
--- a/gdb/ser-base.c
+++ b/gdb/ser-base.c
@@ -561,10 +561,10 @@ ser_base_print_tty_state (struct serial *scb,
   return;
 }
 
-int
+void
 ser_base_setbaudrate (struct serial *scb, int rate)
 {
-  return 0;			/* Never fails!  */
+  /* Never fails!  */
 }
 
 int
diff --git a/gdb/ser-base.h b/gdb/ser-base.h
index 4c6537f7adf..60f84e1f42e 100644
--- a/gdb/ser-base.h
+++ b/gdb/ser-base.h
@@ -40,7 +40,7 @@ extern int ser_base_set_tty_state (struct serial *scb,
 extern void ser_base_print_tty_state (struct serial *scb,
 				      serial_ttystate ttystate,
 				      struct ui_file *stream);
-extern int ser_base_setbaudrate (struct serial *scb, int rate);
+extern void ser_base_setbaudrate (struct serial *scb, int rate);
 extern int ser_base_setstopbits (struct serial *scb, int num);
 extern int ser_base_setparity (struct serial *scb, int parity);
 extern int ser_base_drain_output (struct serial *scb);
diff --git a/gdb/ser-mingw.c b/gdb/ser-mingw.c
index 806f3999385..65ee1697331 100644
--- a/gdb/ser-mingw.c
+++ b/gdb/ser-mingw.c
@@ -226,18 +226,19 @@ ser_windows_setparity (struct serial *scb, int parity)
   return (SetCommState (h, &state) != 0) ? 0 : -1;
 }
 
-static int
+static void
 ser_windows_setbaudrate (struct serial *scb, int rate)
 {
   HANDLE h = (HANDLE) _get_osfhandle (scb->fd);
   DCB state;
 
   if (GetCommState (h, &state) == 0)
-    return -1;
+    throw_winerror_with_name ("call to GetCommState failed", GetLastError ());
 
   state.BaudRate = rate;
 
-  return (SetCommState (h, &state) != 0) ? 0 : -1;
+  if (SetCommState (h, &state) == 0)
+    throw_winerror_with_name ("call to SetCommState failed", GetLastError ());
 }
 
 static void
diff --git a/gdb/ser-unix.c b/gdb/ser-unix.c
index cdc0cf98b7b..5bd15985d8c 100644
--- a/gdb/ser-unix.c
+++ b/gdb/ser-unix.c
@@ -53,7 +53,7 @@ show_serial_hwflow (struct ui_file *file, int from_tty,
 static int hardwire_open (struct serial *scb, const char *name);
 static void hardwire_raw (struct serial *scb);
 static int rate_to_code (int rate);
-static int hardwire_setbaudrate (struct serial *scb, int rate);
+static void hardwire_setbaudrate (struct serial *scb, int rate);
 static int hardwire_setparity (struct serial *scb, int parity);
 static void hardwire_close (struct serial *scb);
 static int get_tty_state (struct serial *scb,
@@ -417,47 +417,38 @@ rate_to_code (int rate)
 	    {
 	      if (i)
 		{
-		  warning (_("Invalid baud rate %d.  "
-			     "Closest values are %d and %d."),
-			   rate, baudtab[i - 1].rate, baudtab[i].rate);
+		  error (_("Invalid baud rate %d.  "
+			   "Closest values are %d and %d."),
+			 rate, baudtab[i - 1].rate, baudtab[i].rate);
 		}
 	      else
 		{
-		  warning (_("Invalid baud rate %d.  Minimum value is %d."),
-			   rate, baudtab[0].rate);
+		  error (_("Invalid baud rate %d.  Minimum value is %d."),
+			 rate, baudtab[0].rate);
 		}
-	      return -1;
 	    }
 	}
     }
  
   /* The requested speed was too large.  */
-  warning (_("Invalid baud rate %d.  Maximum value is %d."),
-	    rate, baudtab[i - 1].rate);
-  return -1;
+  error (_("Invalid baud rate %d.  Maximum value is %d."),
+	 rate, baudtab[i - 1].rate);
 }
 
-static int
+static void
 hardwire_setbaudrate (struct serial *scb, int rate)
 {
   struct hardwire_ttystate state;
   int baud_code = rate_to_code (rate);
   
-  if (baud_code < 0)
-    {
-      /* The baud rate was not valid.
-	 A warning has already been issued.  */
-      errno = EINVAL;
-      return -1;
-    }
-
   if (get_tty_state (scb, &state))
-    return -1;
+    perror_with_name ("could not get tty state");
 
   cfsetospeed (&state.termios, baud_code);
   cfsetispeed (&state.termios, baud_code);
 
-  return set_tty_state (scb, &state);
+  if (set_tty_state (scb, &state))
+    perror_with_name ("could not set tty state");
 }
 
 static int
diff --git a/gdb/serial.c b/gdb/serial.c
index 8a8bab46ead..122ab0bd10e 100644
--- a/gdb/serial.c
+++ b/gdb/serial.c
@@ -512,10 +512,10 @@ serial_print_tty_state (struct serial *scb,
   scb->ops->print_tty_state (scb, ttystate, stream);
 }
 
-int
+void
 serial_setbaudrate (struct serial *scb, int rate)
 {
-  return scb->ops->setbaudrate (scb, rate);
+  scb->ops->setbaudrate (scb, rate);
 }
 
 int
diff --git a/gdb/serial.h b/gdb/serial.h
index 3b861200302..9a51fdf7816 100644
--- a/gdb/serial.h
+++ b/gdb/serial.h
@@ -177,10 +177,10 @@ extern void serial_print_tty_state (struct serial *scb,
 				    serial_ttystate ttystate,
 				    struct ui_file *);
 
-/* Set the baudrate to the decimal value supplied.  Returns 0 for
-   success, -1 for failure.  */
+/* Set the baudrate to the decimal value supplied.  Throws exception
+   on error.  */
 
-extern int serial_setbaudrate (struct serial *scb, int rate);
+extern void serial_setbaudrate (struct serial *scb, int rate);
 
 /* Set the number of stop bits to the value specified.  Returns 0 for
    success, -1 for failure.  */
@@ -275,7 +275,7 @@ struct serial_ops
     int (*set_tty_state) (struct serial *, serial_ttystate);
     void (*print_tty_state) (struct serial *, serial_ttystate,
 			     struct ui_file *);
-    int (*setbaudrate) (struct serial *, int rate);
+    void (*setbaudrate) (struct serial *, int rate);
     int (*setstopbits) (struct serial *, int num);
     /* Set the value PARITY as parity setting for serial object.
        Return 0 in the case of success.  */

-- 
2.40.1

[PATCH 4/6] Change serial "open" functions to throw exception

[Tom Tromey]

remote.c assumes that a failure to open the serial connection will set
errno.  This is somewhat true, because the Windows code tries to set
errno appropriately -- but only somewhat, because it isn't clear that
the "pex" code sets it, and the tcp code seems to do the wrong thing.
It seems better to simply have the serial open functions throw on
error.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30770
---
 gdb/remote.c    |  2 --
 gdb/ser-event.c |  4 +--
 gdb/ser-mingw.c | 47 ++++++++++++----------------------
 gdb/ser-pipe.c  | 12 ++++-----
 gdb/ser-tcp.c   | 79 ++++++++++++++++++++++++++++++---------------------------
 gdb/ser-tcp.h   |  2 +-
 gdb/ser-uds.c   | 26 ++++++++-----------
 gdb/ser-unix.c  |  7 ++---
 gdb/serial.c    | 32 ++++++++---------------
 gdb/serial.h    |  2 +-
 10 files changed, 90 insertions(+), 123 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 9346ae8b3a1..646f869ef3a 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5954,8 +5954,6 @@ remote_target::open_1 (const char *name, int from_tty, int extended_p)
     rs->wait_forever_enabled_p = true;
 
   rs->remote_desc = remote_serial_open (name);
-  if (!rs->remote_desc)
-    perror_with_name (name);
 
   if (baud_rate != -1)
     {
diff --git a/gdb/ser-event.c b/gdb/ser-event.c
index 5865e847bb5..fb02327492a 100644
--- a/gdb/ser-event.c
+++ b/gdb/ser-event.c
@@ -48,7 +48,7 @@ struct serial_event_state
 
 /* Open a new serial event.  */
 
-static int
+static void
 serial_event_open (struct serial *scb, const char *name)
 {
   struct serial_event_state *state;
@@ -85,8 +85,6 @@ serial_event_open (struct serial *scb, const char *name)
     scb->fd = _open_osfhandle ((intptr_t) dummy_file, 0);
   }
 #endif
-
-  return 0;
 }
 
 static void
diff --git a/gdb/ser-mingw.c b/gdb/ser-mingw.c
index 65ee1697331..30d908576da 100644
--- a/gdb/ser-mingw.c
+++ b/gdb/ser-mingw.c
@@ -48,7 +48,7 @@ static CancelIo_ftype *CancelIo;
 
 /* Open up a real live device for serial I/O.  */
 
-static int
+static void
 ser_windows_open (struct serial *scb, const char *name)
 {
   HANDLE h;
@@ -59,22 +59,18 @@ ser_windows_open (struct serial *scb, const char *name)
 		  OPEN_EXISTING, FILE_FLAG_OVERLAPPED, NULL);
   if (h == INVALID_HANDLE_VALUE)
     {
-      errno = ENOENT;
-      return -1;
+      std::string msg = string_printf(_("could not open file: %s"),
+				      name);
+      throw_winerror_with_name (msg.c_str (), GetLastError ());
     }
 
   scb->fd = _open_osfhandle ((intptr_t) h, O_RDWR);
   if (scb->fd < 0)
-    {
-      errno = ENOENT;
-      return -1;
-    }
+    error (_("could not get underlying file descriptor"));
 
   if (!SetCommMask (h, EV_RXCHAR))
-    {
-      errno = EINVAL;
-      return -1;
-    }
+    throw_winerror_with_name (_("error calling SetCommMask"),
+			      GetLastError ());
 
   timeouts.ReadIntervalTimeout = MAXDWORD;
   timeouts.ReadTotalTimeoutConstant = 0;
@@ -82,10 +78,8 @@ ser_windows_open (struct serial *scb, const char *name)
   timeouts.WriteTotalTimeoutConstant = 0;
   timeouts.WriteTotalTimeoutMultiplier = 0;
   if (!SetCommTimeouts (h, &timeouts))
-    {
-      errno = EINVAL;
-      return -1;
-    }
+    throw_winerror_with_name (_("error calling SetCommTimeouts"),
+			      GetLastError ());
 
   state = XCNEW (struct ser_windows_state);
   scb->state = state;
@@ -95,8 +89,6 @@ ser_windows_open (struct serial *scb, const char *name)
 
   /* Create a (currently unused) handle to record exceptions.  */
   state->except_event = CreateEvent (0, TRUE, FALSE, 0);
-
-  return 0;
 }
 
 /* Wait for the output to drain away, as opposed to flushing (discarding)
@@ -860,7 +852,7 @@ struct pipe_state_destroyer
 
 typedef std::unique_ptr<pipe_state, pipe_state_destroyer> pipe_state_up;
 
-static int
+static void
 pipe_windows_open (struct serial *scb, const char *name)
 {
   FILE *pex_stderr;
@@ -883,10 +875,10 @@ pipe_windows_open (struct serial *scb, const char *name)
 
   ps->pex = pex_init (PEX_USE_PIPES, "target remote pipe", NULL);
   if (! ps->pex)
-    return -1;
+    error (_("could not start pipeline"));
   ps->input = pex_input_pipe (ps->pex, 1);
   if (! ps->input)
-    return -1;
+    error (_("could not find input pipe"));
 
   {
     int err;
@@ -913,17 +905,15 @@ pipe_windows_open (struct serial *scb, const char *name)
 
   ps->output = pex_read_output (ps->pex, 1);
   if (! ps->output)
-    return -1;
+    error (_("could not find output pipe"));
   scb->fd = fileno (ps->output);
 
   pex_stderr = pex_read_err (ps->pex, 1);
   if (! pex_stderr)
-    return -1;
+    error (_("could not find error pipe"));
   scb->error_fd = fileno (pex_stderr);
 
   scb->state = ps.release ();
-
-  return 0;
 }
 
 static int
@@ -1191,15 +1181,12 @@ net_windows_done_wait_handle (struct serial *scb)
   stop_select_thread (&state->base);
 }
 
-static int
+static void
 net_windows_open (struct serial *scb, const char *name)
 {
   struct net_windows_state *state;
-  int ret;
 
-  ret = net_open (scb, name);
-  if (ret != 0)
-    return ret;
+  net_open (scb, name);
 
   state = XCNEW (struct net_windows_state);
   scb->state = state;
@@ -1210,8 +1197,6 @@ net_windows_open (struct serial *scb, const char *name)
 
   /* Start the thread.  */
   create_select_thread (net_windows_select_thread, scb, &state->base);
-
-  return 0;
 }
 
 
diff --git a/gdb/ser-pipe.c b/gdb/ser-pipe.c
index 47ccd33cece..7bdaaa071dc 100644
--- a/gdb/ser-pipe.c
+++ b/gdb/ser-pipe.c
@@ -34,7 +34,6 @@
 
 #include <signal.h>
 
-static int pipe_open (struct serial *scb, const char *name);
 static void pipe_close (struct serial *scb);
 
 struct pipe_state
@@ -44,7 +43,7 @@ struct pipe_state
 
 /* Open up a raw pipe.  */
 
-static int
+static void
 pipe_open (struct serial *scb, const char *name)
 {
 #if !HAVE_SOCKETPAIR
@@ -69,12 +68,13 @@ pipe_open (struct serial *scb, const char *name)
     }
 
   if (gdb_socketpair_cloexec (AF_UNIX, SOCK_STREAM, 0, pdes) < 0)
-    return -1;
+    perror_with_name (_("could not open socket pair"));
   if (gdb_socketpair_cloexec (AF_UNIX, SOCK_STREAM, 0, err_pdes) < 0)
     {
+      int save = errno;
       close (pdes[0]);
       close (pdes[1]);
-      return -1;
+      perror_with_name (_("could not open socket pair"), save);
     }
 
   /* Create the child process to run the command in.  Note that the
@@ -86,11 +86,12 @@ pipe_open (struct serial *scb, const char *name)
   /* Error.  */
   if (pid == -1)
     {
+      int save = errno;
       close (pdes[0]);
       close (pdes[1]);
       close (err_pdes[0]);
       close (err_pdes[1]);
-      return -1;
+      perror_with_name (_("could not vfork"), save);
     }
 
   if (fcntl (err_pdes[0], F_SETFL, O_NONBLOCK) == -1)
@@ -148,7 +149,6 @@ pipe_open (struct serial *scb, const char *name)
 
   /* If we don't do this, GDB simply exits when the remote side dies.  */
   signal (SIGPIPE, SIG_IGN);
-  return 0;
 #endif
 }
 
diff --git a/gdb/ser-tcp.c b/gdb/ser-tcp.c
index 6083bd61e62..75ea4c80051 100644
--- a/gdb/ser-tcp.c
+++ b/gdb/ser-tcp.c
@@ -84,11 +84,11 @@ static unsigned int tcp_retry_limit = 15;
 
 /* Helper function to wait a while.  If SOCK is not -1, wait on its
    file descriptor.  Otherwise just wait on a timeout, updating
-   *POLLS.  Returns -1 on timeout or interrupt, otherwise the value of
-   select.  */
+   *POLLS.  Returns -1 on timeout or interrupt and set OUT_ERROR,
+   otherwise the value of select.  */
 
 static int
-wait_for_connect (int sock, unsigned int *polls)
+wait_for_connect (int sock, unsigned int *polls, ULONGEST *out_error)
 {
   struct timeval t;
   int n;
@@ -98,14 +98,14 @@ wait_for_connect (int sock, unsigned int *polls)
      interrupt.  */
   if (deprecated_ui_loop_hook && deprecated_ui_loop_hook (0))
     {
-      errno = EINTR;
+      *out_error = EINTR;
       return -1;
     }
 
   /* Check for timeout.  */
   if (*polls > tcp_retry_limit * POLL_INTERVAL)
     {
-      errno = ETIMEDOUT;
+      *out_error = ETIMEDOUT;
       return -1;
     }
 
@@ -155,19 +155,34 @@ wait_for_connect (int sock, unsigned int *polls)
   return n;
 }
 
+/* A helper to get the error number for either Windows or POSIX.  */
+static ULONGEST
+get_error ()
+{
+#ifdef USE_WIN32API
+  return WSAGetLastError ();
+#else
+  return errno;
+#endif
+}
+
 /* Try to connect to the host represented by AINFO.  If the connection
-   succeeds, return its socket.  Otherwise, return -1 and set ERRNO
+   succeeds, return its socket.  Otherwise, return -1 and set OUT_ERROR
    accordingly.  POLLS is used when 'connect' returns EINPROGRESS, and
    we need to invoke 'wait_for_connect' to obtain the status.  */
 
 static int
-try_connect (const struct addrinfo *ainfo, unsigned int *polls)
+try_connect (const struct addrinfo *ainfo, unsigned int *polls,
+	     ULONGEST *out_error)
 {
   int sock = gdb_socket_cloexec (ainfo->ai_family, ainfo->ai_socktype,
 				 ainfo->ai_protocol);
 
   if (sock < 0)
-    return -1;
+    {
+      *out_error = get_error ();
+      return -1;
+    }
 
   /* Set socket nonblocking.  */
 #ifdef USE_WIN32API
@@ -182,11 +197,7 @@ try_connect (const struct addrinfo *ainfo, unsigned int *polls)
      already.  */
   if (connect (sock, ainfo->ai_addr, ainfo->ai_addrlen) < 0)
     {
-#ifdef USE_WIN32API
-      int err = WSAGetLastError();
-#else
-      int err = errno;
-#endif
+      ULONGEST err = get_error ();
 
       /* If we've got a "connection refused" error, just return
 	 -1.  The caller will know what to do.  */
@@ -199,7 +210,7 @@ try_connect (const struct addrinfo *ainfo, unsigned int *polls)
 	  )
 	{
 	  close (sock);
-	  errno = err;
+	  *out_error = err;
 	  return -1;
 	}
 
@@ -218,7 +229,7 @@ try_connect (const struct addrinfo *ainfo, unsigned int *polls)
 	  )
 	{
 	  close (sock);
-	  errno = err;
+	  *out_error = err;
 	  return -1;
 	}
 
@@ -226,17 +237,15 @@ try_connect (const struct addrinfo *ainfo, unsigned int *polls)
       int n;
 
       do
-	n = wait_for_connect (sock, polls);
+	n = wait_for_connect (sock, polls, out_error);
       while (n == 0);
 
       if (n < 0)
 	{
-	  int saved_errno = errno;
-
 	  /* A negative value here means that we either timed out or
 	     got interrupted by the user.  Just return.  */
 	  close (sock);
-	  errno = saved_errno;
+	  /* OUT_ERROR was set by wait_for_connect, above.  */
 	  return -1;
 	}
     }
@@ -253,16 +262,14 @@ try_connect (const struct addrinfo *ainfo, unsigned int *polls)
 
   if (ret < 0)
     {
-      int saved_errno = errno;
-
+      *out_error = get_error ();
       close (sock);
-      errno = saved_errno;
       return -1;
     }
   else if (ret == 0 && err != 0)
     {
+      *out_error = err;
       close (sock);
-      errno = err;
       return -1;
     }
 
@@ -272,7 +279,7 @@ try_connect (const struct addrinfo *ainfo, unsigned int *polls)
 
 /* Open a tcp socket.  */
 
-int
+void
 net_open (struct serial *scb, const char *name)
 {
   struct addrinfo hint;
@@ -295,12 +302,7 @@ net_open (struct serial *scb, const char *name)
 		       &hint, &ainfo);
 
   if (r != 0)
-    {
-      gdb_printf (gdb_stderr, _("%s: cannot resolve name: %s\n"),
-		  name, gai_strerror (r));
-      errno = ENOENT;
-      return -1;
-    }
+    error (_("%s: cannot resolve name: %s\n"), name, gai_strerror (r));
 
   scoped_free_addrinfo free_ainfo (ainfo);
 
@@ -311,6 +313,7 @@ net_open (struct serial *scb, const char *name)
      'struct addrinfo' that succeed.  */
   struct addrinfo *success_ainfo = NULL;
   unsigned int polls = 0;
+  ULONGEST last_error = 0;
 
   /* Assume the worst.  */
   scb->fd = -1;
@@ -324,7 +327,7 @@ net_open (struct serial *scb, const char *name)
 	  /* Iterate over the list of possible addresses to connect
 	     to.  For each, we'll try to connect and see if it
 	     succeeds.  */
-	  int sock = try_connect (iter, &polls);
+	  int sock = try_connect (iter, &polls, &last_error);
 
 	  if (sock >= 0)
 	    {
@@ -336,9 +339,9 @@ net_open (struct serial *scb, const char *name)
 	    }
 	  else if (
 #ifdef USE_WIN32API
-	  errno == WSAECONNREFUSED
+		   last_error == WSAECONNREFUSED
 #else
-	  errno == ECONNREFUSED
+		   last_error == ECONNREFUSED
 #endif
 		   )
 	    got_connrefused = true;
@@ -353,12 +356,16 @@ net_open (struct serial *scb, const char *name)
   while (tcp_auto_retry
 	 && success_ainfo == NULL
 	 && got_connrefused
-	 && wait_for_connect (-1, &polls) >= 0);
+	 && wait_for_connect (-1, &polls, &last_error) >= 0);
 
   if (success_ainfo == NULL)
     {
       net_close (scb);
-      return -1;
+#ifdef USE_WIN32API
+      throw_winerror_with_name (_("could not connect"), last_error);
+#else
+      perror_with_name (_("could not connect"), last_error);
+#endif
     }
 
   /* Turn off nonblocking.  */
@@ -384,8 +391,6 @@ net_open (struct serial *scb, const char *name)
      when the remote side dies.  */
   signal (SIGPIPE, SIG_IGN);
 #endif
-
-  return 0;
 }
 
 void
diff --git a/gdb/ser-tcp.h b/gdb/ser-tcp.h
index aeb4de04a3b..444d77c0b74 100644
--- a/gdb/ser-tcp.h
+++ b/gdb/ser-tcp.h
@@ -22,7 +22,7 @@
 
 struct serial;
 
-extern int net_open (struct serial *scb, const char *name);
+extern void net_open (struct serial *scb, const char *name);
 extern void net_close (struct serial *scb);
 extern int net_read_prim (struct serial *scb, size_t count);
 extern int net_write_prim (struct serial *scb, const void *buf, size_t count);
diff --git a/gdb/ser-uds.c b/gdb/ser-uds.c
index 665aab912da..e51058b534c 100644
--- a/gdb/ser-uds.c
+++ b/gdb/ser-uds.c
@@ -30,36 +30,30 @@
 
 /* Open an AF_UNIX socket.  */
 
-static int
+static void
 uds_open (struct serial *scb, const char *name)
 {
   struct sockaddr_un addr;
 
   if (strlen (name) > UNIX_PATH_MAX - 1)
-    {
-      warning
-	(_("The socket name is too long.  It may be no longer than %s bytes."),
-	 pulongest (UNIX_PATH_MAX - 1L));
-      return -1;
-    }
+    error (_("The socket name is too long.  It may be no longer than %s bytes."),
+	   pulongest (UNIX_PATH_MAX - 1L));
 
   memset (&addr, 0, sizeof addr);
   addr.sun_family = AF_UNIX;
   strncpy (addr.sun_path, name, UNIX_PATH_MAX - 1);
 
-  int sock = socket (AF_UNIX, SOCK_STREAM, 0);
+  scb->fd = socket (AF_UNIX, SOCK_STREAM, 0);
+  if (scb->fd < 0)
+    perror_with_name (_("could not open socket"));
 
-  if (connect (sock, (struct sockaddr *) &addr,
+  if (connect (scb->fd, (struct sockaddr *) &addr,
 	       sizeof (struct sockaddr_un)) < 0)
     {
-      close (sock);
-      scb->fd = -1;
-      return -1;
+      int saved = errno;
+      close (scb->fd);
+      perror_with_name (_("could not connect to remote"), saved);
     }
-
-  scb->fd = sock;
-
-  return 0;
 }
 
 static void
diff --git a/gdb/ser-unix.c b/gdb/ser-unix.c
index 5bd15985d8c..020b41e563d 100644
--- a/gdb/ser-unix.c
+++ b/gdb/ser-unix.c
@@ -50,7 +50,6 @@ show_serial_hwflow (struct ui_file *file, int from_tty,
 }
 #endif
 
-static int hardwire_open (struct serial *scb, const char *name);
 static void hardwire_raw (struct serial *scb);
 static int rate_to_code (int rate);
 static void hardwire_setbaudrate (struct serial *scb, int rate);
@@ -72,14 +71,12 @@ static int hardwire_setstopbits (struct serial *, int);
 
 /* Open up a real live device for serial I/O.  */
 
-static int
+static void
 hardwire_open (struct serial *scb, const char *name)
 {
   scb->fd = gdb_open_cloexec (name, O_RDWR, 0).release ();
   if (scb->fd < 0)
-    return -1;
-
-  return 0;
+    perror_with_name ("could not open device");
 }
 
 static int
diff --git a/gdb/serial.c b/gdb/serial.c
index 122ab0bd10e..720af1a356d 100644
--- a/gdb/serial.c
+++ b/gdb/serial.c
@@ -173,12 +173,10 @@ serial_for_fd (int fd)
 
 /* Create a new serial for OPS.  */
 
-static struct serial *
+static gdb::unique_xmalloc_ptr<struct serial>
 new_serial (const struct serial_ops *ops)
 {
-  struct serial *scb;
-
-  scb = XCNEW (struct serial);
+  gdb::unique_xmalloc_ptr<struct serial> scb (XCNEW (struct serial));
 
   scb->ops = ops;
 
@@ -221,7 +219,7 @@ serial_open (const char *name)
     }
 
   if (!ops)
-    return NULL;
+    error (_("could not find serial handler for '%s'"), name);
 
   return serial_open_ops_1 (ops, open_name);
 }
@@ -231,20 +229,14 @@ serial_open (const char *name)
 static struct serial *
 serial_open_ops_1 (const struct serial_ops *ops, const char *open_name)
 {
-  struct serial *scb;
-
-  scb = new_serial (ops);
+  gdb::unique_xmalloc_ptr<struct serial> scb = new_serial (ops);
 
   /* `...->open (...)' would get expanded by the open(2) syscall macro.  */
-  if ((*scb->ops->open) (scb, open_name))
-    {
-      xfree (scb);
-      return NULL;
-    }
+  (*scb->ops->open) (scb.get (), open_name);
 
   scb->name = open_name != NULL ? xstrdup (open_name) : NULL;
   scb->next = scb_base;
-  scb_base = scb;
+  scb_base = scb.get ();
 
   if (!serial_logfile.empty ())
     {
@@ -256,7 +248,7 @@ serial_open_ops_1 (const struct serial_ops *ops, const char *open_name)
       serial_logfp = file.release ();
     }
 
-  return scb;
+  return scb.release ();
 }
 
 /* See serial.h.  */
@@ -273,8 +265,6 @@ serial_open_ops (const struct serial_ops *ops)
 static struct serial *
 serial_fdopen_ops (const int fd, const struct serial_ops *ops)
 {
-  struct serial *scb;
-
   if (!ops)
     {
       ops = serial_interface_lookup ("terminal");
@@ -285,18 +275,18 @@ serial_fdopen_ops (const int fd, const struct serial_ops *ops)
   if (!ops)
     return NULL;
 
-  scb = new_serial (ops);
+  gdb::unique_xmalloc_ptr<struct serial> scb = new_serial (ops);
 
   scb->name = NULL;
   scb->next = scb_base;
-  scb_base = scb;
+  scb_base = scb.get ();
 
   if ((ops->fdopen) != NULL)
-    (*ops->fdopen) (scb, fd);
+    (*ops->fdopen) (scb.get (), fd);
   else
     scb->fd = fd;
 
-  return scb;
+  return scb.release ();
 }
 
 struct serial *
diff --git a/gdb/serial.h b/gdb/serial.h
index 9a51fdf7816..9ae7f4f13f1 100644
--- a/gdb/serial.h
+++ b/gdb/serial.h
@@ -259,7 +259,7 @@ struct serial
 struct serial_ops
   {
     const char *name;
-    int (*open) (struct serial *, const char *name);
+    void (*open) (struct serial *, const char *name);
     void (*close) (struct serial *);
     int (*fdopen) (struct serial *, int fd);
     int (*readchar) (struct serial *, int timeout);

-- 
2.40.1

[PATCH 5/6] Change serial_send_break and serial_write to throw

[Tom Tromey]

This changes serial_send_break and serial_write to throw exceptions
rather than attempt to set errno and return an error indicator.  This
lets us correctly report failures on Windows.

Both functions had to be converted in a single patch because one
implementation of send_break works via write.

This also introduces remote_serial_send_break to handle error checking
when attempting to send a break.  This was previously ignored.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30770
---
 gdb/remote.c    | 40 ++++++++++++++++++++++++++++++++--------
 gdb/ser-base.c  |  8 +++-----
 gdb/ser-base.h  |  4 ++--
 gdb/ser-mingw.c | 18 +++++++++---------
 gdb/ser-tcp.c   |  9 ++++++---
 gdb/ser-tcp.h   |  2 +-
 gdb/ser-uds.c   |  5 ++++-
 gdb/ser-unix.c  | 12 ++++++++----
 gdb/serial.c    |  8 ++++----
 gdb/serial.h    | 12 ++++++------
 10 files changed, 75 insertions(+), 43 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 646f869ef3a..23046beeb2e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1202,6 +1202,7 @@ class remote_target : public process_stratum_target
   int readchar (int timeout);
 
   void remote_serial_write (const char *str, int len);
+  void remote_serial_send_break ();
 
   int putpkt (const char *buf);
   int putpkt_binary (const char *buf, int cnt);
@@ -4553,15 +4554,13 @@ remote_target::get_offsets ()
 void
 remote_target::send_interrupt_sequence ()
 {
-  struct remote_state *rs = get_remote_state ();
-
   if (interrupt_sequence_mode == interrupt_sequence_control_c)
     remote_serial_write ("\x03", 1);
   else if (interrupt_sequence_mode == interrupt_sequence_break)
-    serial_send_break (rs->remote_desc);
+    remote_serial_send_break ();
   else if (interrupt_sequence_mode == interrupt_sequence_break_g)
     {
-      serial_send_break (rs->remote_desc);
+      remote_serial_send_break ();
       remote_serial_write ("g", 1);
     }
   else
@@ -4569,7 +4568,6 @@ remote_target::send_interrupt_sequence ()
 		    interrupt_sequence_mode);
 }
 
-
 /* If STOP_REPLY is a T stop reply, look for the "thread" register,
    and extract the PTID.  Returns NULL_PTID if not found.  */
 
@@ -9672,16 +9670,42 @@ remote_target::remote_serial_write (const char *str, int len)
 
   rs->got_ctrlc_during_io = 0;
 
-  if (serial_write (rs->remote_desc, str, len))
+  try
     {
-      unpush_and_perror (this, _("Remote communication error.  "
-				 "Target disconnected"));
+      serial_write (rs->remote_desc, str, len);
+    }
+  catch (const gdb_exception_error &ex)
+    {
+      remote_unpush_target (this);
+      throw_error (TARGET_CLOSE_ERROR,
+		   _("Remote communication error.  "
+		     "Target disconnected: %s"),
+		   ex.what ());
     }
 
   if (rs->got_ctrlc_during_io)
     set_quit_flag ();
 }
 
+void
+remote_target::remote_serial_send_break ()
+{
+  struct remote_state *rs = get_remote_state ();
+
+  try
+    {
+      serial_send_break (rs->remote_desc);
+    }
+  catch (const gdb_exception_error &ex)
+    {
+      remote_unpush_target (this);
+      throw_error (TARGET_CLOSE_ERROR,
+		   _("Remote communication error.  "
+		     "Target disconnected: %s"),
+		   ex.what ());
+    }
+}
+
 /* Return a string representing an escaped version of BUF, of len N.
    E.g. \n is converted to \\n, \t to \\t, etc.  */
 
diff --git a/gdb/ser-base.c b/gdb/ser-base.c
index 072211df1ca..d83003ebf96 100644
--- a/gdb/ser-base.c
+++ b/gdb/ser-base.c
@@ -471,7 +471,7 @@ ser_base_readchar (struct serial *scb, int timeout)
   return generic_readchar (scb, timeout, do_ser_base_readchar);
 }
 
-int
+void
 ser_base_write (struct serial *scb, const void *buf, size_t count)
 {
   const char *str = (const char *) buf;
@@ -487,12 +487,11 @@ ser_base_write (struct serial *scb, const void *buf, size_t count)
 	{
 	  if (errno == EINTR)
 	    continue;
-	  return 1;
+	  perror_with_name ("error while writing");
 	}
       count -= cc;
       str += cc;
     }
-  return 0;
 }
 
 int
@@ -514,10 +513,9 @@ ser_base_flush_input (struct serial *scb)
     return SERIAL_ERROR;
 }
 
-int
+void
 ser_base_send_break (struct serial *scb)
 {
-  return 0;
 }
 
 int
diff --git a/gdb/ser-base.h b/gdb/ser-base.h
index 60f84e1f42e..6fcdf724413 100644
--- a/gdb/ser-base.h
+++ b/gdb/ser-base.h
@@ -30,7 +30,7 @@ extern int generic_readchar (struct serial *scb, int timeout,
 						 int timeout));
 extern int ser_base_flush_output (struct serial *scb);
 extern int ser_base_flush_input (struct serial *scb);
-extern int ser_base_send_break (struct serial *scb);
+extern void ser_base_send_break (struct serial *scb);
 extern void ser_base_raw (struct serial *scb);
 extern serial_ttystate ser_base_get_tty_state (struct serial *scb);
 extern serial_ttystate ser_base_copy_tty_state (struct serial *scb,
@@ -45,7 +45,7 @@ extern int ser_base_setstopbits (struct serial *scb, int num);
 extern int ser_base_setparity (struct serial *scb, int parity);
 extern int ser_base_drain_output (struct serial *scb);
 
-extern int ser_base_write (struct serial *scb, const void *buf, size_t count);
+extern void ser_base_write (struct serial *scb, const void *buf, size_t count);
 
 extern void ser_base_async (struct serial *scb, int async_p);
 extern int ser_base_readchar (struct serial *scb, int timeout);
diff --git a/gdb/ser-mingw.c b/gdb/ser-mingw.c
index 30d908576da..4607a10fde8 100644
--- a/gdb/ser-mingw.c
+++ b/gdb/ser-mingw.c
@@ -118,21 +118,21 @@ ser_windows_flush_input (struct serial *scb)
   return (PurgeComm (h, PURGE_RXCLEAR) != 0) ? 0 : -1;
 }
 
-static int
+static void
 ser_windows_send_break (struct serial *scb)
 {
   HANDLE h = (HANDLE) _get_osfhandle (scb->fd);
 
   if (SetCommBreak (h) == 0)
-    return -1;
+    throw_winerror_with_name ("error calling SetCommBreak",
+			      GetLastError ());
 
   /* Delay for 250 milliseconds.  */
   Sleep (250);
 
   if (ClearCommBreak (h) == 0)
-    return -1;
-
-  return 0;
+    throw_winerror_with_name ("error calling ClearCommBreak",
+			      GetLastError ());
 }
 
 static void
@@ -354,7 +354,7 @@ ser_windows_write_prim (struct serial *scb, const void *buf, size_t len)
     {
       if (GetLastError () != ERROR_IO_PENDING
 	  || !GetOverlappedResult (h, &ov, &bytes_written, TRUE))
-	bytes_written = -1;
+	throw_winerror_with_name ("error while writing", GetLastError ());
     }
 
   CloseHandle (ov.hEvent);
@@ -986,14 +986,14 @@ pipe_windows_write (struct serial *scb, const void *buf, size_t count)
 
   int pipeline_in_fd = fileno (ps->input);
   if (pipeline_in_fd < 0)
-    return -1;
+    error (_("could not find file number for pipe"));
 
   pipeline_in = (HANDLE) _get_osfhandle (pipeline_in_fd);
   if (pipeline_in == INVALID_HANDLE_VALUE)
-    return -1;
+    error (_("could not find handle for pipe"));
 
   if (! WriteFile (pipeline_in, buf, count, &written, NULL))
-    return -1;
+    throw_winerror_with_name (_("could not write to pipe"), GetLastError ());
 
   return written;
 }
diff --git a/gdb/ser-tcp.c b/gdb/ser-tcp.c
index 75ea4c80051..62f8a519782 100644
--- a/gdb/ser-tcp.c
+++ b/gdb/ser-tcp.c
@@ -419,14 +419,17 @@ net_write_prim (struct serial *scb, const void *buf, size_t count)
      UNIX systems it is generally "const void *".  The cast to "const
      char *" is OK everywhere, since in C++ any data pointer type can
      be implicitly converted to "const void *".  */
-  return send (scb->fd, (const char *) buf, count, 0);
+  int result = send (scb->fd, (const char *) buf, count, 0);
+  if (result == -1 && errno != EINTR)
+    perror_with_name ("error while writing");
+  return result;
 }
 
-int
+void
 ser_tcp_send_break (struct serial *scb)
 {
   /* Send telnet IAC and BREAK characters.  */
-  return (serial_write (scb, "\377\363", 2));
+  serial_write (scb, "\377\363", 2);
 }
 
 #ifndef USE_WIN32API
diff --git a/gdb/ser-tcp.h b/gdb/ser-tcp.h
index 444d77c0b74..c22fd878d6d 100644
--- a/gdb/ser-tcp.h
+++ b/gdb/ser-tcp.h
@@ -26,6 +26,6 @@ extern void net_open (struct serial *scb, const char *name);
 extern void net_close (struct serial *scb);
 extern int net_read_prim (struct serial *scb, size_t count);
 extern int net_write_prim (struct serial *scb, const void *buf, size_t count);
-extern int ser_tcp_send_break (struct serial *scb);
+extern void ser_tcp_send_break (struct serial *scb);
 
 #endif
diff --git a/gdb/ser-uds.c b/gdb/ser-uds.c
index e51058b534c..baa660be21d 100644
--- a/gdb/ser-uds.c
+++ b/gdb/ser-uds.c
@@ -75,7 +75,10 @@ uds_read_prim (struct serial *scb, size_t count)
 static int
 uds_write_prim (struct serial *scb, const void *buf, size_t count)
 {
-  return send (scb->fd, buf, count, 0);
+  int result = send (scb->fd, buf, count, 0);
+  if (result == -1 && errno != EINTR)
+    perror_with_name ("error while writing");
+  return result;
 }
 
 /* The local socket ops.  */
diff --git a/gdb/ser-unix.c b/gdb/ser-unix.c
index 020b41e563d..a2a897d4442 100644
--- a/gdb/ser-unix.c
+++ b/gdb/ser-unix.c
@@ -66,7 +66,7 @@ static void hardwire_print_tty_state (struct serial *, serial_ttystate,
 static int hardwire_drain_output (struct serial *);
 static int hardwire_flush_output (struct serial *);
 static int hardwire_flush_input (struct serial *);
-static int hardwire_send_break (struct serial *);
+static void hardwire_send_break (struct serial *);
 static int hardwire_setstopbits (struct serial *, int);
 
 /* Open up a real live device for serial I/O.  */
@@ -182,10 +182,11 @@ hardwire_flush_input (struct serial *scb)
   return tcflush (scb->fd, TCIFLUSH);
 }
 
-static int
+static void
 hardwire_send_break (struct serial *scb)
 {
-  return tcsendbreak (scb->fd, 0);
+  if (tcsendbreak (scb->fd, 0) == -1)
+    perror_with_name ("sending break");
 }
 
 static void
@@ -579,5 +580,8 @@ ser_unix_read_prim (struct serial *scb, size_t count)
 int
 ser_unix_write_prim (struct serial *scb, const void *buf, size_t len)
 {
-  return write (scb->fd, buf, len);
+  int result = write (scb->fd, buf, len);
+  if (result == -1 && errno != EINTR)
+    perror_with_name ("error while writing");
+  return result;
 }
diff --git a/gdb/serial.c b/gdb/serial.c
index 720af1a356d..e8ad339aad8 100644
--- a/gdb/serial.c
+++ b/gdb/serial.c
@@ -399,7 +399,7 @@ serial_readchar (struct serial *scb, int timeout)
   return (ch);
 }
 
-int
+void
 serial_write (struct serial *scb, const void *buf, size_t count)
 {
   if (serial_logfp != NULL)
@@ -428,7 +428,7 @@ serial_write (struct serial *scb, const void *buf, size_t count)
       gdb_flush (gdb_stdlog);
     }
 
-  return (scb->ops->write (scb, buf, count));
+  scb->ops->write (scb, buf, count);
 }
 
 void
@@ -461,13 +461,13 @@ serial_flush_input (struct serial *scb)
   return scb->ops->flush_input (scb);
 }
 
-int
+void
 serial_send_break (struct serial *scb)
 {
   if (serial_logfp != NULL)
     serial_logchar (serial_logfp, 'w', SERIAL_BREAK, 0);
 
-  return (scb->ops->send_break (scb));
+  scb->ops->send_break (scb);
 }
 
 void
diff --git a/gdb/serial.h b/gdb/serial.h
index 9ae7f4f13f1..50726bee2be 100644
--- a/gdb/serial.h
+++ b/gdb/serial.h
@@ -119,10 +119,10 @@ enum serial_rc {
 
 extern int serial_readchar (struct serial *scb, int timeout);
 
-/* Write COUNT bytes from BUF to the port SCB.  Returns 0 for
-   success, non-zero for failure.  */
+/* Write COUNT bytes from BUF to the port SCB.  Throws exception on
+   error.  */
 
-extern int serial_write (struct serial *scb, const void *buf, size_t count);
+extern void serial_write (struct serial *scb, const void *buf, size_t count);
 
 /* Write a printf style string onto the serial port.  */
 
@@ -145,7 +145,7 @@ extern int serial_flush_input (struct serial *);
 
 /* Send a break between 0.25 and 0.5 seconds long.  */
 
-extern int serial_send_break (struct serial *scb);
+extern void serial_send_break (struct serial *scb);
 
 /* Turn the port into raw mode.  */
 
@@ -263,12 +263,12 @@ struct serial_ops
     void (*close) (struct serial *);
     int (*fdopen) (struct serial *, int fd);
     int (*readchar) (struct serial *, int timeout);
-    int (*write) (struct serial *, const void *buf, size_t count);
+    void (*write) (struct serial *, const void *buf, size_t count);
     /* Discard pending output */
     int (*flush_output) (struct serial *);
     /* Discard pending input */
     int (*flush_input) (struct serial *);
-    int (*send_break) (struct serial *);
+    void (*send_break) (struct serial *);
     void (*go_raw) (struct serial *);
     serial_ttystate (*get_tty_state) (struct serial *);
     serial_ttystate (*copy_tty_state) (struct serial *, serial_ttystate);

-- 
2.40.1

[PATCH 6/6] Change serial_readchar to throw

[Tom Tromey]

This changes serial_readchar to throw an exception rather than trying
to set and preserve errno.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30770
---
 gdb/remote.c    | 56 +++++++++++++++++++++-----------------------------------
 gdb/ser-mingw.c | 12 ++++++++----
 gdb/ser-tcp.c   |  5 ++++-
 gdb/ser-uds.c   |  5 ++++-
 gdb/ser-unix.c  |  5 ++++-
 5 files changed, 41 insertions(+), 42 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 23046beeb2e..fe5bd74b9fd 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -9592,22 +9592,6 @@ remote_target::flash_done ()
 /* Stuff for dealing with the packets which are part of this protocol.
    See comment at top of file for details.  */
 
-/* Close/unpush the remote target, and throw a TARGET_CLOSE_ERROR
-   error to higher layers.  Called when a serial error is detected.
-   The exception message is STRING, followed by a colon and a blank,
-   the system error message for errno at function entry and final dot
-   for output compatibility with throw_perror_with_name.  */
-
-static void
-unpush_and_perror (remote_target *target, const char *string)
-{
-  int saved_errno = errno;
-
-  remote_unpush_target (target);
-  throw_error (TARGET_CLOSE_ERROR, "%s: %s.", string,
-	       safe_strerror (saved_errno));
-}
-
 /* Read a single character from the remote end.  The current quit
    handler is overridden to avoid quitting in the middle of packet
    sequence, as that would break communication with the remote server.
@@ -9619,36 +9603,38 @@ remote_target::readchar (int timeout)
   int ch;
   struct remote_state *rs = get_remote_state ();
 
-  {
-    scoped_restore restore_quit_target
-      = make_scoped_restore (&curr_quit_handler_target, this);
-    scoped_restore restore_quit
-      = make_scoped_restore (&quit_handler, ::remote_serial_quit_handler);
+  try
+    {
+      scoped_restore restore_quit_target
+	= make_scoped_restore (&curr_quit_handler_target, this);
+      scoped_restore restore_quit
+	= make_scoped_restore (&quit_handler, ::remote_serial_quit_handler);
 
-    rs->got_ctrlc_during_io = 0;
+      rs->got_ctrlc_during_io = 0;
 
-    ch = serial_readchar (rs->remote_desc, timeout);
+      ch = serial_readchar (rs->remote_desc, timeout);
 
-    if (rs->got_ctrlc_during_io)
-      set_quit_flag ();
-  }
+      if (rs->got_ctrlc_during_io)
+	set_quit_flag ();
+    }
+  catch (const gdb_exception_error &ex)
+    {
+      remote_unpush_target (this);
+      throw_error (TARGET_CLOSE_ERROR,
+		   _("Remote communication error.  "
+		     "Target disconnected: %s"),
+		   ex.what ());
+    }
 
   if (ch >= 0)
     return ch;
 
-  switch ((enum serial_rc) ch)
+  if (ch == SERIAL_EOF)
     {
-    case SERIAL_EOF:
       remote_unpush_target (this);
       throw_error (TARGET_CLOSE_ERROR, _("Remote connection closed"));
-      /* no return */
-    case SERIAL_ERROR:
-      unpush_and_perror (this, _("Remote communication error.  "
-				 "Target disconnected"));
-      /* no return */
-    case SERIAL_TIMEOUT:
-      break;
     }
+
   return ch;
 }
 
diff --git a/gdb/ser-mingw.c b/gdb/ser-mingw.c
index 4607a10fde8..25016aebee5 100644
--- a/gdb/ser-mingw.c
+++ b/gdb/ser-mingw.c
@@ -333,7 +333,11 @@ ser_windows_read_prim (struct serial *scb, size_t count)
     {
       if (GetLastError () != ERROR_IO_PENDING
 	  || !GetOverlappedResult (h, &ov, &bytes_read, TRUE))
-	bytes_read = -1;
+	{
+	  ULONGEST err = GetLastError ();
+	  CloseHandle (ov.hEvent);
+	  throw_winerror_with_name (_("error while reading"), err);
+	}
     }
 
   CloseHandle (ov.hEvent);
@@ -962,16 +966,16 @@ pipe_windows_read (struct serial *scb, size_t count)
   DWORD bytes_read;
 
   if (pipeline_out == INVALID_HANDLE_VALUE)
-    return -1;
+    error (_("could not find file number for pipe"));
 
   if (! PeekNamedPipe (pipeline_out, NULL, 0, NULL, &available, NULL))
-    return -1;
+    throw_winerror_with_name (_("could not peek into pipe"), GetLastError ());
 
   if (count > available)
     count = available;
 
   if (! ReadFile (pipeline_out, scb->buf, count, &bytes_read, NULL))
-    return -1;
+    throw_winerror_with_name (_("could not read from pipe"), GetLastError ());
 
   return bytes_read;
 }
diff --git a/gdb/ser-tcp.c b/gdb/ser-tcp.c
index 62f8a519782..b31b030461b 100644
--- a/gdb/ser-tcp.c
+++ b/gdb/ser-tcp.c
@@ -409,7 +409,10 @@ net_read_prim (struct serial *scb, size_t count)
   /* Need to cast to silence -Wpointer-sign on MinGW, as Winsock's
      'recv' takes 'char *' as second argument, while 'scb->buf' is
      'unsigned char *'.  */
-  return recv (scb->fd, (char *) scb->buf, count, 0);
+  int result = recv (scb->fd, (char *) scb->buf, count, 0);
+  if (result == -1 && errno != EINTR)
+    perror_with_name ("error while reading");
+  return result;
 }
 
 int
diff --git a/gdb/ser-uds.c b/gdb/ser-uds.c
index baa660be21d..ad1f79e0800 100644
--- a/gdb/ser-uds.c
+++ b/gdb/ser-uds.c
@@ -69,7 +69,10 @@ uds_close (struct serial *scb)
 static int
 uds_read_prim (struct serial *scb, size_t count)
 {
-  return recv (scb->fd, scb->buf, count, 0);
+  int result = recv (scb->fd, scb->buf, count, 0);
+  if (result == -1 && errno != EINTR)
+    perror_with_name ("error while reading");
+  return result;
 }
 
 static int
diff --git a/gdb/ser-unix.c b/gdb/ser-unix.c
index a2a897d4442..07cd8b7b5b4 100644
--- a/gdb/ser-unix.c
+++ b/gdb/ser-unix.c
@@ -574,7 +574,10 @@ when debugging using remote targets."),
 int
 ser_unix_read_prim (struct serial *scb, size_t count)
 {
-  return read (scb->fd, scb->buf, count);
+  int result = recv (scb->fd, scb->buf, count, 0);
+  if (result == -1 && errno != EINTR)
+    perror_with_name ("error while reading");
+  return result;
 }
 
 int

-- 
2.40.1

Re: [PATCH 0/6] Fix error messages from serial code

[Tom Tromey]

>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> Internal AdaCore testing on a Windows host with a target running on
Tom> qemu revealed that remote.c could sometimes display random error
Tom> messages.  For example, different runs of the same test might display:

Tom>     Remote communication error.  Target disconnected.: Arg list too long.
Tom>     Remote communication error.  Target disconnected.: No error.
Tom>     Remote communication error.  Target disconnected.: Not a directory.

Tom> I looked into this and this series is the result.  It's hard to be
Tom> sure that this completely solves the problem -- there are a lot of
Tom> code paths and the errors aren't very reproducible -- but it helps.

Tom> Regression tested using native-extended-gdbserver on x86-64 Fedora 36.

I'm checking these in.

Tom