[PATCH] Use strwinerror in gdb/windows-nat.c

[PATCH] Use strwinerror in gdb/windows-nat.c

[Tom Tromey]

When working on windows-nat.c, it's useful to see an error message in
addition to the error number given by GetLastError.  This patch moves
strwinerror from gdbserver to gdbsupport, and then updates
windows-nat.c to use it.  A couple of minor changes to strwinerror
(constify the return type and use the ARRAY_SIZE macro) are also
included.
---
 gdb/nat/windows-nat.c  | 22 ++++++++++------
 gdb/windows-nat.c      | 51 ++++++++++++++++++++++++------------
 gdbserver/win32-low.cc | 59 ------------------------------------------
 gdbserver/win32-low.h  | 11 --------
 gdbsupport/errors.cc   | 54 ++++++++++++++++++++++++++++++++++++++
 gdbsupport/errors.h    | 16 ++++++++++++
 6 files changed, 119 insertions(+), 94 deletions(-)

diff --git a/gdb/nat/windows-nat.c b/gdb/nat/windows-nat.c
index ea6e2980f59..ba60b585e85 100644
--- a/gdb/nat/windows-nat.c
+++ b/gdb/nat/windows-nat.c
@@ -95,8 +95,8 @@ windows_thread_info::suspend ()
 	 We can get Invalid Handle (6) if the main thread
 	 has exited.  */
       if (err != ERROR_INVALID_HANDLE && err != ERROR_ACCESS_DENIED)
-	warning (_("SuspendThread (tid=0x%x) failed. (winerr %u)"),
-		 (unsigned) tid, (unsigned) err);
+	warning (_("SuspendThread (tid=0x%x) failed. (winerr %u: %s)"),
+		 (unsigned) tid, (unsigned) err, strwinerror (err));
       suspended = -1;
     }
   else
@@ -113,8 +113,8 @@ windows_thread_info::resume ()
       if (ResumeThread (h) == (DWORD) -1)
 	{
 	  DWORD err = GetLastError ();
-	  warning (_("warning: ResumeThread (tid=0x%x) failed. (winerr %u)"),
-		   (unsigned) tid, (unsigned) err);
+	  warning (_("warning: ResumeThread (tid=0x%x) failed. (winerr %u: %s)"),
+		   (unsigned) tid, (unsigned) err, strwinerror (err));
 	}
     }
   suspended = 0;
@@ -204,8 +204,11 @@ windows_process_info::get_exec_module_filename (char *exe_name_ret,
     len = GetModuleFileNameEx (handle,
 			       dh_buf, pathbuf, exe_name_max_len);
     if (len == 0)
-      error (_("Error getting executable filename: %u."),
-	     (unsigned) GetLastError ());
+      {
+	unsigned err = (unsigned) GetLastError ();
+	error (_("Error getting executable filename (error %u): %s"),
+	       err, strwinerror (err));
+      }
     if (cygwin_conv_path (CCP_WIN_W_TO_POSIX, pathbuf, exe_name_ret,
 			  exe_name_max_len) < 0)
       error (_("Error converting executable filename to POSIX: %d."), errno);
@@ -214,8 +217,11 @@ windows_process_info::get_exec_module_filename (char *exe_name_ret,
   len = GetModuleFileNameEx (handle,
 			     dh_buf, exe_name_ret, exe_name_max_len);
   if (len == 0)
-    error (_("Error getting executable filename: %u."),
-	   (unsigned) GetLastError ());
+    {
+      unsigned err = (unsigned) GetLastError ();
+      error (_("Error getting executable filename (error %u): %s"),
+	     err, strwinerror (err));
+    }
 #endif
 
     return 1;	/* success */
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 9265ed9e632..1e310371e47 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -325,8 +325,11 @@ static void
 check (BOOL ok, const char *file, int line)
 {
   if (!ok)
-    gdb_printf ("error return %s:%d was %u\n", file, line,
-		(unsigned) GetLastError ());
+    {
+      unsigned err = (unsigned) GetLastError ();
+      gdb_printf ("error return %s:%d was %u: %s\n", file, line,
+		  err, strwinerror (err));
+    }
 }
 
 /* See nat/windows-nat.h.  */
@@ -1160,9 +1163,12 @@ windows_continue (DWORD continue_status, int id, int killed)
   res = continue_last_debug_event (continue_status, debug_events);
 
   if (!res)
-    error (_("Failed to resume program execution"
-	     " (ContinueDebugEvent failed, error %u)"),
-	   (unsigned int) GetLastError ());
+    {
+      unsigned err = (unsigned) GetLastError ();
+      error (_("Failed to resume program execution"
+	       " (ContinueDebugEvent failed, error %u: %s)"),
+	     err, strwinerror (err));
+    }
 
   return res;
 }
@@ -1179,8 +1185,9 @@ windows_nat_target::fake_create_process ()
     windows_process.open_process_used = 1;
   else
     {
-      error (_("OpenProcess call failed, GetLastError = %u"),
-       (unsigned) GetLastError ());
+      unsigned err = (unsigned) GetLastError ();
+      error (_("OpenProcess call failed, GetLastError = %u: %s"),
+	     err, strwinerror (err));
       /*  We can not debug anything in that case.  */
     }
   add_thread (ptid_t (windows_process.current_event.dwProcessId, 0,
@@ -1887,8 +1894,11 @@ windows_nat_target::attach (const char *args, int from_tty)
 #endif
 
   if (!ok)
-    error (_("Can't attach to process %u (error %u)"),
-	   (unsigned) pid, (unsigned) GetLastError ());
+    {
+      unsigned err = (unsigned) GetLastError ();
+      error (_("Can't attach to process %u (error %u: %s)"),
+	     (unsigned) pid, err, strwinerror (err));
+    }
 
   DebugSetProcessKillOnExit (FALSE);
 
@@ -1916,9 +1926,12 @@ windows_nat_target::detach (inferior *inf, int from_tty)
   resume (ptid, 0, GDB_SIGNAL_0);
 
   if (!DebugActiveProcessStop (windows_process.current_event.dwProcessId))
-    error (_("Can't detach process %u (error %u)"),
-	   (unsigned) windows_process.current_event.dwProcessId,
-	   (unsigned) GetLastError ());
+    {
+      unsigned err = (unsigned) GetLastError ();
+      error (_("Can't detach process %u (error %u: %s)"),
+	     (unsigned) windows_process.current_event.dwProcessId,
+	     err, strwinerror (err));
+    }
   DebugSetProcessKillOnExit (FALSE);
 
   target_announce_detach (from_tty);
@@ -2526,8 +2539,11 @@ windows_nat_target::create_inferior (const char *exec_file,
       tty = CreateFileA (inferior_tty.c_str (), GENERIC_READ | GENERIC_WRITE,
 			 0, &sa, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, 0);
       if (tty == INVALID_HANDLE_VALUE)
-	warning (_("Warning: Failed to open TTY %s, error %#x."),
-		 inferior_tty.c_str (), (unsigned) GetLastError ());
+	{
+	  unsigned err = (unsigned) GetLastError ();
+	  warning (_("Warning: Failed to open TTY %s, error %#x: %s"),
+		   inferior_tty.c_str (), err, strwinerror (err));
+	}
     }
   if (redirected || tty != INVALID_HANDLE_VALUE)
     {
@@ -2608,8 +2624,11 @@ windows_nat_target::create_inferior (const char *exec_file,
 #endif	/* !__CYGWIN__ */
 
   if (!ret)
-    error (_("Error creating process %s, (error %u)."),
-	   exec_file, (unsigned) GetLastError ());
+    {
+      unsigned err = (unsigned) GetLastError ();
+      error (_("Error creating process %s, (error %u: %s)"),
+	     exec_file, err, strwinerror (err));
+    }
 
 #ifdef __x86_64__
   BOOL wow64;
diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index 27224077cea..5e2c028d683 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -61,10 +61,6 @@ gdbserver_windows_process windows_process;
 #define _T(x) TEXT (x)
 #endif
 
-#ifndef COUNTOF
-#define COUNTOF(STR) (sizeof (STR) / sizeof ((STR)[0]))
-#endif
-
 int using_threads = 1;
 
 const struct target_desc *win32_tdesc;
@@ -483,61 +479,6 @@ child_store_inferior_registers (struct regcache *regcache, int r)
       (*the_low_target.store_inferior_register) (regcache, th, regno);
 }
 
-/* Map the Windows error number in ERROR to a locale-dependent error
-   message string and return a pointer to it.  Typically, the values
-   for ERROR come from GetLastError.
-
-   The string pointed to shall not be modified by the application,
-   but may be overwritten by a subsequent call to strwinerror
-
-   The strwinerror function does not change the current setting
-   of GetLastError.  */
-
-char *
-strwinerror (DWORD error)
-{
-  static char buf[1024];
-  TCHAR *msgbuf;
-  DWORD lasterr = GetLastError ();
-  DWORD chars = FormatMessage (FORMAT_MESSAGE_FROM_SYSTEM
-			       | FORMAT_MESSAGE_ALLOCATE_BUFFER,
-			       NULL,
-			       error,
-			       0, /* Default language */
-			       (LPTSTR) &msgbuf,
-			       0,
-			       NULL);
-  if (chars != 0)
-    {
-      /* If there is an \r\n appended, zap it.  */
-      if (chars >= 2
-	  && msgbuf[chars - 2] == '\r'
-	  && msgbuf[chars - 1] == '\n')
-	{
-	  chars -= 2;
-	  msgbuf[chars] = 0;
-	}
-
-      if (chars > ((COUNTOF (buf)) - 1))
-	{
-	  chars = COUNTOF (buf) - 1;
-	  msgbuf [chars] = 0;
-	}
-
-#ifdef UNICODE
-      wcstombs (buf, msgbuf, chars + 1);
-#else
-      strncpy (buf, msgbuf, chars + 1);
-#endif
-      LocalFree (msgbuf);
-    }
-  else
-    sprintf (buf, "unknown win32 error (%u)", (unsigned) error);
-
-  SetLastError (lasterr);
-  return buf;
-}
-
 static BOOL
 create_process (const char *program, char *args,
 		DWORD flags, PROCESS_INFORMATION *pi)
diff --git a/gdbserver/win32-low.h b/gdbserver/win32-low.h
index 58bb105b283..d7bb76b6a98 100644
--- a/gdbserver/win32-low.h
+++ b/gdbserver/win32-low.h
@@ -211,15 +211,4 @@ extern gdbserver_windows_process windows_process;
 /* Retrieve the context for this thread, if not already retrieved.  */
 extern void win32_require_context (windows_nat::windows_thread_info *th);
 
-/* Map the Windows error number in ERROR to a locale-dependent error
-   message string and return a pointer to it.  Typically, the values
-   for ERROR come from GetLastError.
-
-   The string pointed to shall not be modified by the application,
-   but may be overwritten by a subsequent call to strwinerror
-
-   The strwinerror function does not change the current setting
-   of GetLastError.  */
-extern char * strwinerror (DWORD error);
-
 #endif /* GDBSERVER_WIN32_LOW_H */
diff --git a/gdbsupport/errors.cc b/gdbsupport/errors.cc
index 703b2213756..b98d0154eb3 100644
--- a/gdbsupport/errors.cc
+++ b/gdbsupport/errors.cc
@@ -19,6 +19,9 @@
 
 #include "common-defs.h"
 #include "errors.h"
+#ifdef USE_WIN32API
+#include <windows.h>
+#endif /* USE_WIN32API */
 
 /* See gdbsupport/errors.h.  */
 
@@ -67,3 +70,54 @@ internal_warning (const char *file, int line, const char *fmt, ...)
   internal_vwarning (file, line, fmt, ap);
   va_end (ap);
 }
+
+#ifdef USE_WIN32API
+
+/* See errors.h.  */
+
+const char *
+strwinerror (ULONGEST error)
+{
+  static char buf[1024];
+  TCHAR *msgbuf;
+  DWORD lasterr = GetLastError ();
+  DWORD chars = FormatMessage (FORMAT_MESSAGE_FROM_SYSTEM
+			       | FORMAT_MESSAGE_ALLOCATE_BUFFER,
+			       NULL,
+			       error,
+			       0, /* Default language */
+			       (LPTSTR) &msgbuf,
+			       0,
+			       NULL);
+  if (chars != 0)
+    {
+      /* If there is an \r\n appended, zap it.  */
+      if (chars >= 2
+	  && msgbuf[chars - 2] == '\r'
+	  && msgbuf[chars - 1] == '\n')
+	{
+	  chars -= 2;
+	  msgbuf[chars] = 0;
+	}
+
+      if (chars > ARRAY_SIZE (buf) - 1)
+	{
+	  chars = ARRAY_SIZE (buf) - 1;
+	  msgbuf [chars] = 0;
+	}
+
+#ifdef UNICODE
+      wcstombs (buf, msgbuf, chars + 1);
+#else
+      strncpy (buf, msgbuf, chars + 1);
+#endif
+      LocalFree (msgbuf);
+    }
+  else
+    sprintf (buf, "unknown win32 error (%u)", (unsigned) error);
+
+  SetLastError (lasterr);
+  return buf;
+}
+
+#endif /* USE_WIN32API */
diff --git a/gdbsupport/errors.h b/gdbsupport/errors.h
index 47965ea8a3c..9a671d3e289 100644
--- a/gdbsupport/errors.h
+++ b/gdbsupport/errors.h
@@ -91,4 +91,20 @@ extern void malloc_failure (long size) ATTRIBUTE_NORETURN;
 
 extern void flush_streams ();
 
+#ifdef USE_WIN32API
+
+/* Map the Windows error number in ERROR to a locale-dependent error
+   message string and return a pointer to it.  Typically, the values
+   for ERROR come from GetLastError.
+
+   The string pointed to shall not be modified by the application,
+   but may be overwritten by a subsequent call to strwinerror
+
+   The strwinerror function does not change the current setting
+   of GetLastError.  */
+
+extern const char *strwinerror (ULONGEST error);
+
+#endif /* USE_WIN32API */
+
 #endif /* COMMON_ERRORS_H */
-- 
2.34.1

Re: [PATCH] Use strwinerror in gdb/windows-nat.c

[Tom Tromey]

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

Tom> When working on windows-nat.c, it's useful to see an error message in
Tom> addition to the error number given by GetLastError.  This patch moves
Tom> strwinerror from gdbserver to gdbsupport, and then updates
Tom> windows-nat.c to use it.  A couple of minor changes to strwinerror
Tom> (constify the return type and use the ARRAY_SIZE macro) are also
Tom> included.

I'm checking this in now.

Tom

Re: [PATCH] Use strwinerror in gdb/windows-nat.c

[Jon Turney]

[-- Attachment #1: Type: text/plain, Size: 786 bytes --]

On 16/08/2022 14:57, Tom Tromey via Gdb-patches wrote:
>>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> When working on windows-nat.c, it's useful to see an error message in
> Tom> addition to the error number given by GetLastError.  This patch moves
> Tom> strwinerror from gdbserver to gdbsupport, and then updates
> Tom> windows-nat.c to use it.  A couple of minor changes to strwinerror
> Tom> (constify the return type and use the ARRAY_SIZE macro) are also
> Tom> included.
> 
> I'm checking this in now.

As is traditional by now, this breaks the Cygwin build.

I'd suggest the attached to fix this, but I don't think this qualifies 
as obvious, since it's not obvious what USE_WIN32API is supposed to 
mean, so a review would be useful.

[-- Attachment #2: 0001-Fix-Cygwin-build-after-02d04eac.patch --]
[-- Type: text/plain, Size: 1676 bytes --]

From 4e6b72a98f1f03c66801af4f368718784f809bb4 Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Wed, 2 Nov 2022 13:16:16 +0000
Subject: [PATCH] Fix Cygwin build after 02d04eac

Commit 02d04eac "Use strwinerror in gdb/windows-nat.c" also moves
strwinerror() under the USE_WIN32API conditional, which is not defined
for Cygwin (and looks like it shouldn't be, as appears to imply
non-POSIX and MiNGW and WinSock...)

Also enable the declaration and definition of strwinerror() when
__CYGWIN__ is defined.
---
 gdbsupport/errors.cc | 4 ++--
 gdbsupport/errors.h  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdbsupport/errors.cc b/gdbsupport/errors.cc
index 7464df3d067..93fc70ad296 100644
--- a/gdbsupport/errors.cc
+++ b/gdbsupport/errors.cc
@@ -19,7 +19,7 @@
 
 #include "common-defs.h"
 #include "errors.h"
-#ifdef USE_WIN32API
+#if defined (USE_WIN32API) || defined(__CYGWIN__)
 #include <windows.h>
 #endif /* USE_WIN32API */
 
@@ -71,7 +71,7 @@ internal_warning_loc (const char *file, int line, const char *fmt, ...)
   va_end (ap);
 }
 
-#ifdef USE_WIN32API
+#if defined (USE_WIN32API) || defined(__CYGWIN__)
 
 /* See errors.h.  */
 
diff --git a/gdbsupport/errors.h b/gdbsupport/errors.h
index 5a925789893..d67d692e4cb 100644
--- a/gdbsupport/errors.h
+++ b/gdbsupport/errors.h
@@ -99,7 +99,7 @@ extern void malloc_failure (long size) ATTRIBUTE_NORETURN;
 
 extern void flush_streams ();
 
-#ifdef USE_WIN32API
+#if defined(USE_WIN32API) || defined(__CYGWIN__)
 
 /* Map the Windows error number in ERROR to a locale-dependent error
    message string and return a pointer to it.  Typically, the values
-- 
2.38.1

Re: [PATCH] Use strwinerror in gdb/windows-nat.c

[Tom Tromey]

>>>>> "Jon" == Jon Turney <jon.turney@dronecode.org.uk> writes:

>> I'm checking this in now.

Jon> As is traditional by now, this breaks the Cygwin build.

Sorry about that.  I don't break Cygwin on purpose, but I also don't
have any way to do a test build.

Jon> I'd suggest the attached to fix this, but I don't think this qualifies
Jon> as obvious, since it's not obvious what USE_WIN32API is supposed to 
Jon> mean, so a review would be useful.

I don't fully know either, but this is what gdbsupport/common.m4 says:

  case ${host} in
    *mingw32*)
      AC_DEFINE(USE_WIN32API, 1,
		[Define if we should use the Windows API, instead of the
		 POSIX API.  On Windows, we use the Windows API when
		 building for MinGW, but the POSIX API when building
		 for Cygwin.])
      WIN32APILIBS="-lws2_32"

Jon> Commit 02d04eac "Use strwinerror in gdb/windows-nat.c" also moves
Jon> strwinerror() under the USE_WIN32API conditional, which is not defined
Jon> for Cygwin (and looks like it shouldn't be, as appears to imply
Jon> non-POSIX and MiNGW and WinSock...)

Jon> Also enable the declaration and definition of strwinerror() when
Jon> __CYGWIN__ is defined.

I'm not sure how this area ought to work on Cygwin.  The goal here is to
get a better error message when some Windows API fails.  If Cygwin can
use strwinerror, then this seems fine.  If not, I could write a patch to
stub this out for Cygwin, which would be the status quo ante.

Tom

Re: [PATCH] Use strwinerror in gdb/windows-nat.c

[Jon Turney]

On 08/11/2022 18:24, Tom Tromey via Gdb-patches wrote:
>>>>>> "Jon" == Jon Turney <jon.turney@dronecode.org.uk> writes:
> 
> Jon> I'd suggest the attached to fix this, but I don't think this qualifies
> Jon> as obvious, since it's not obvious what USE_WIN32API is supposed to
> Jon> mean, so a review would be useful.
> 
> I don't fully know either, but this is what gdbsupport/common.m4 says:
> 
>    case ${host} in
>      *mingw32*)
>        AC_DEFINE(USE_WIN32API, 1,
> 		[Define if we should use the Windows API, instead of the
> 		 POSIX API.  On Windows, we use the Windows API when
> 		 building for MinGW, but the POSIX API when building
> 		 for Cygwin.])
>        WIN32APILIBS="-lws2_32"

Well, yes.  But this seems to mash up a few different things, e.g.:

- using the Windows API for interfacing with the debuggee
- not using any POSIX API
- target is MinGW

> Jon> Commit 02d04eac "Use strwinerror in gdb/windows-nat.c" also moves
> Jon> strwinerror() under the USE_WIN32API conditional, which is not defined
> Jon> for Cygwin (and looks like it shouldn't be, as appears to imply
> Jon> non-POSIX and MiNGW and WinSock...)
> 
> Jon> Also enable the declaration and definition of strwinerror() when
> Jon> __CYGWIN__ is defined.
> 
> I'm not sure how this area ought to work on Cygwin.  The goal here is to
> get a better error message when some Windows API fails.  If Cygwin can
> use strwinerror, then this seems fine.  If not, I could write a patch to

Thanks for reviewing this change.

Cygwin currently uses the Windows Debug API to control the inferior, so 
it's getting Windows error codes back, so turning those into text like 
this is indeed useful.

I'm going to take this as an approval and apply.

(There is a locale issue, in that the Windows idea of the current 
locale, which FormatMessage() uses, is not necessary the same, or even 
in a 1:1 mapping with Cygwin's POSIX-like idea of it, but I think I'll 
ignore that for the moment...)

> stub this out for Cygwin, which would be the status quo ante.

I think the status quo includes that these function would be used in 
gdbserver, if anyone built it for Cygwin, because there weren't under 
USE_WIN32API before.