[PATCH] windows-nat: Decode system error numbers

[PATCH] windows-nat: Decode system error numbers

[Maciej W. Rozycki]

Hi,

 A while ago I have come across a problem while debugging a MinGW program 
natively where the backend reported an error numerically.  As quite 
obviously I couldn't figure out what the cause of the error was I went 
back to the sources and discovered that we have this piece of code in 
windows-nat that fails to decode Windows system errors.  With the patch 
below I was able to track down what the problem was.

 Now I don't have that setup available anymore (that was something weird 
anyway), so to validate this change before submission I simulated an error 
scenario by running GDB under GDB and deliberately corrupting data.  I 
chose windows_kill_inferior as it's easy to trigger by just killing the 
inferior from the debuggee GDB.

 So an example session looks like:

(gdb) break windows_kill_inferior
Breakpoint 1 at 0x519cf4: file .../gdb/windows-nat.c, line 2312.
(gdb) continue
Continuing.
[Switching to Thread 4700.0x1bb4]

Breakpoint 1, windows_kill_inferior (ops=0x953180)
    at .../gdb/windows-nat.c:2312
2312      CHECK (TerminateProcess (current_process_handle, 0));
(gdb) print current_process_handle
$1 = (HANDLE) 0x148
(gdb) set variable current_process_handle = 0
(gdb) continue
Continuing.

and as it stands this is printed by the debuggee GDB:

error return .../gdb/windows-nat.c:2312 was 6

but with the patch applied a proper error message is produced instead:

error return .../gdb/windows-nat.c:2332 was: The handle is invalid. (6)

As Windows tends to terminate its error messages with a CR+LF sequence, 
these characters are stripped by my code so that the result remains a 
single line (I decided not to strip the trailing full stop though and 
consequently added one to the fallback message for consistency).

 This code builds and works as manually verified above; these errors never 
trigger in the test suite so that doesn't really cover it -- my 
understanding is they are not meant to trigger unless GDB or the system is 
malfunctioning for some reason.

 OK to apply?

2011-11-09  Maciej W. Rozycki  <macro@codesourcery.com>

	gdb/
	* windows-nat.c (check): Decode the error number retrieved with 
	GetLastError.

  Maciej

gdb-windows-nat-check.patch
Index: gdb-fsf-trunk-quilt/gdb/windows-nat.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/windows-nat.c	2011-11-09 11:38:15.000000000 +0000
+++ gdb-fsf-trunk-quilt/gdb/windows-nat.c	2011-11-09 11:48:36.765865140 +0000
@@ -274,9 +274,29 @@ windows_set_context_register_offsets (co
 static void
 check (BOOL ok, const char *file, int line)
 {
-  if (!ok)
-    printf_filtered ("error return %s:%d was %lu\n", file, line,
-		     GetLastError ());
+  const char *msg = "Unspecified error.";
+  unsigned long err;
+  char buf[1025];
+  size_t size;
+
+  if (ok)
+    return;
+
+  err = GetLastError();
+  size = FormatMessage (FORMAT_MESSAGE_FROM_SYSTEM
+			| FORMAT_MESSAGE_IGNORE_INSERTS,
+			NULL,
+			err,
+			MAKELANGID (LANG_NEUTRAL, SUBLANG_DEFAULT),
+			buf, (sizeof (buf) - 1) / sizeof (TCHAR), NULL);
+  if (size > 0 && buf[size - 1] == '\n')
+    buf[--size] = '\0';
+  if (size > 0 && buf[size - 1] == '\r')
+    buf[--size] = '\0';
+  if (size > 0)
+    msg = buf;
+
+  printf_filtered ("error return %s:%d was: %s (%lu)\n", file, line, msg, err);
 }
 
 /* Find a thread record given a thread id.  If GET_CONTEXT is not 0,

Re: [PATCH] windows-nat: Decode system error numbers

[Joel Brobecker]

> and as it stands this is printed by the debuggee GDB:
> 
> error return .../gdb/windows-nat.c:2312 was 6
> 
> but with the patch applied a proper error message is produced instead:
> 
> error return .../gdb/windows-nat.c:2332 was: The handle is invalid. (6)

This is nice, I like it.

>  This code builds and works as manually verified above; these errors
>  never trigger in the test suite so that doesn't really cover it -- my
>  understanding is they are not meant to trigger unless GDB or the
>  system is malfunctioning for some reason.

I don't know of any way to force the triggering of these errors under
"normal" circumstances either.  But I did notice that we get them
occasionally during our testsuite nightly run, with an error number
5 (permissing denied kind of thing?).

This happened when trying to kill the inferior, for instance. It's very
hard to reproduce, very rare and random, so I've never been able to get
to the bottom of it. And the only hints I got on the web was that
Borland had the same problem, and that they "fixed" it. Since I could
not observe any anomaly besides the error message (inferior was killed
as expected, for instance), I was left wondering whether Borland just
disabled the error message...

> 2011-11-09  Maciej W. Rozycki  <macro@codesourcery.com>
> 
> 	gdb/
> 	* windows-nat.c (check): Decode the error number retrieved with 
> 	GetLastError.

A Windows maintainer can approve your code.  I looked at the code, and
nothing really jumped at me, except maybe the fact that you decrement
size and use it change one element in buf at the same time. Personally,
I'm not fond of these practices in general, but I can live with that in
this case.

-- 
Joel

Re: [PATCH] windows-nat: Decode system error numbers

[Eli Zaretskii]

> Date: Wed, 9 Nov 2011 12:02:40 +0000
> From: "Maciej W. Rozycki" <macro@codesourcery.com>
> 
> +  const char *msg = "Unspecified error.";
> +  unsigned long err;
> +  char buf[1025];
> +  size_t size;
> +
> +  if (ok)
> +    return;
> +
> +  err = GetLastError();
> +  size = FormatMessage (FORMAT_MESSAGE_FROM_SYSTEM
> +			| FORMAT_MESSAGE_IGNORE_INSERTS,
> +			NULL,
> +			err,
> +			MAKELANGID (LANG_NEUTRAL, SUBLANG_DEFAULT),
> +			buf, (sizeof (buf) - 1) / sizeof (TCHAR), NULL);

Will this DTRT with Cygwin, which AFAIK wants the wide versions of the
APIs?  Is, for example, "char buf[1025];" appropriate in that case?

Re: [PATCH] windows-nat: Decode system error numbers

[Pedro Alves]

On Wednesday 09 November 2011 16:42:50, Eli Zaretskii wrote:
> > Date: Wed, 9 Nov 2011 12:02:40 +0000
> > From: "Maciej W. Rozycki" <macro@codesourcery.com>
> > 
> > +  const char *msg = "Unspecified error.";
> > +  unsigned long err;
> > +  char buf[1025];
> > +  size_t size;
> > +
> > +  if (ok)
> > +    return;
> > +
> > +  err = GetLastError();
> > +  size = FormatMessage (FORMAT_MESSAGE_FROM_SYSTEM
> > +			| FORMAT_MESSAGE_IGNORE_INSERTS,
> > +			NULL,
> > +			err,
> > +			MAKELANGID (LANG_NEUTRAL, SUBLANG_DEFAULT),
> > +			buf, (sizeof (buf) - 1) / sizeof (TCHAR), NULL);
> 
> Will this DTRT with Cygwin, which AFAIK wants the wide versions of the
> APIs?  Is, for example, "char buf[1025];" appropriate in that case?

Yeah, if UNICODE is defined, FormatMessage maps to FormatMessageW and 
buf will be filled with a wide string, though I'm not sure
if __USEWIDE implies UNICODE.

gdbserver/win32-low.c has strwinerror for this.  It'd be nice if
windows-nat.c also abstracted this equally or similarly.

-- 
Pedro Alves

Re: [PATCH] windows-nat: Decode system error numbers

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Wed, 9 Nov 2011 16:58:00 +0000
> Cc: "Maciej W. Rozycki" <macro@codesourcery.com>
> 
> > Will this DTRT with Cygwin, which AFAIK wants the wide versions of the
> > APIs?  Is, for example, "char buf[1025];" appropriate in that case?
> 
> Yeah, if UNICODE is defined, FormatMessage maps to FormatMessageW and 
> buf will be filled with a wide string, though I'm not sure
> if __USEWIDE implies UNICODE.

But buf[] should be TCHAR then, shouldn't it?

Re: [PATCH] windows-nat: Decode system error numbers

[Pedro Alves]

On Wednesday 09 November 2011 17:00:50, Eli Zaretskii wrote:
> > From: Pedro Alves <pedro@codesourcery.com>
> > Date: Wed, 9 Nov 2011 16:58:00 +0000
> > Cc: "Maciej W. Rozycki" <macro@codesourcery.com>
> > 
> > > Will this DTRT with Cygwin, which AFAIK wants the wide versions of the
> > > APIs?  Is, for example, "char buf[1025];" appropriate in that case?
> > 
> > Yeah, if UNICODE is defined, FormatMessage maps to FormatMessageW and 
> > buf will be filled with a wide string, though I'm not sure
> > if __USEWIDE implies UNICODE.
> 
> But buf[] should be TCHAR then, shouldn't it?

Right, and that's what strwinerror does.  But looking at the windows-nat.c
code, indeed __USEWISE does not seem to imply UNICODE.  Otherwise e.g.,
this

  HANDLE hconsole = CreateFile ("CONOUT$", GENERIC_READ | GENERIC_WRITE,
				FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, 0);

wouldn't compile on cygwin.  So I don't think we support building
windows-nat.c with UNICODE on currently (gdbserver/win32-low.c does
build that way though, since it builds on Windows CE too, and that
is always UNICODE), and char would be fine.

-- 
Pedro Alves

Re: [PATCH] windows-nat: Decode system error numbers

[Corinna Vinschen]

On Nov  9 16:58, Pedro Alves wrote:
> On Wednesday 09 November 2011 16:42:50, Eli Zaretskii wrote:
> > > Date: Wed, 9 Nov 2011 12:02:40 +0000
> > > From: "Maciej W. Rozycki" <macro@codesourcery.com>
> > > 
> > > +  const char *msg = "Unspecified error.";
> > > +  unsigned long err;
> > > +  char buf[1025];
> > > +  size_t size;
> > > +
> > > +  if (ok)
> > > +    return;
> > > +
> > > +  err = GetLastError();
> > > +  size = FormatMessage (FORMAT_MESSAGE_FROM_SYSTEM
> > > +			| FORMAT_MESSAGE_IGNORE_INSERTS,
> > > +			NULL,
> > > +			err,
> > > +			MAKELANGID (LANG_NEUTRAL, SUBLANG_DEFAULT),
> > > +			buf, (sizeof (buf) - 1) / sizeof (TCHAR), NULL);
> > 
> > Will this DTRT with Cygwin, which AFAIK wants the wide versions of the
> > APIs?  Is, for example, "char buf[1025];" appropriate in that case?
> 
> Yeah, if UNICODE is defined, FormatMessage maps to FormatMessageW and 
> buf will be filled with a wide string, though I'm not sure
> if __USEWIDE implies UNICODE.

Then again, buf is defined as char, not wchar_t or WCHAR.  It would be
better to make buf a wchar_t, always call FormatMessageW, and then call
printf_filtered with %ls as string format.  That should work on all
supported platforms.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [PATCH] windows-nat: Decode system error numbers

[Christopher Faylor]

On Wed, Nov 09, 2011 at 06:06:52PM +0100, Corinna Vinschen wrote:
>On Nov  9 16:58, Pedro Alves wrote:
>> On Wednesday 09 November 2011 16:42:50, Eli Zaretskii wrote:
>> > > Date: Wed, 9 Nov 2011 12:02:40 +0000
>> > > From: "Maciej W. Rozycki" <macro@codesourcery.com>
>> > > 
>> > > +  const char *msg = "Unspecified error.";
>> > > +  unsigned long err;
>> > > +  char buf[1025];
>> > > +  size_t size;
>> > > +
>> > > +  if (ok)
>> > > +    return;
>> > > +
>> > > +  err = GetLastError();
>> > > +  size = FormatMessage (FORMAT_MESSAGE_FROM_SYSTEM
>> > > +			| FORMAT_MESSAGE_IGNORE_INSERTS,
>> > > +			NULL,
>> > > +			err,
>> > > +			MAKELANGID (LANG_NEUTRAL, SUBLANG_DEFAULT),
>> > > +			buf, (sizeof (buf) - 1) / sizeof (TCHAR), NULL);
>> > 
>> > Will this DTRT with Cygwin, which AFAIK wants the wide versions of the
>> > APIs?  Is, for example, "char buf[1025];" appropriate in that case?
>> 
>> Yeah, if UNICODE is defined, FormatMessage maps to FormatMessageW and 
>> buf will be filled with a wide string, though I'm not sure
>> if __USEWIDE implies UNICODE.
>
>Then again, buf is defined as char, not wchar_t or WCHAR.  It would be
>better to make buf a wchar_t, always call FormatMessageW, and then call
>printf_filtered with %ls as string format.  That should work on all
>supported platforms.

With that type of change, this would likely be approved.

cgf