[RFAv3,2/6] Improve process exit status macros on MinGW

  > Cc: philippe.waroquiers@skynet.be, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Tue, 17 Dec 2019 17:51:26 +0000
> 
> The issue pointed out was that by putting the windows_status_to_termsig
> function in windows-nat.c, and then calling it from gdb's common code
> (cli/cli-cmds.c, via WTERMSIG) would result in a build/link failure when
> you try to build a cross debugger hosted on mingw, because such a gdb
> build does not include the native Windows target support, i.e., does not
> build/link the windows-nat.o object.  Putting said function in mingw-hdep.c
> instead fixes that issue because that file is included as part of the build
> in all kinds of mingw-hosted GDBs, either native or cross-debugger.
> 
> >> I admit to being a bit confused about why we want to do this
> >> translation for this feature while we don't do it for the exit code
> >> of inferiors running under gdb, for example.  I mean, exit status
> >> with 0xc0000000 set don't cause $_exitsignal to be set instead of
> >> $_exitcode.
> > 
> > Yes, we should do this for exit code of inferiors as well.
> > 
> > Native MS-Windows debugging produces the TARGET_WAITKIND_* values in
> > windows-nat.c, so I think the conversion we are talking about will
> > have to be done there, perhaps _in_addition_to_ other places?  IOW,
> > the function that performs the conversion can be defined in
> > mingw-hdep.c, but it will have to be called from windows-nat.c at
> > least, right?  And I'm uncertain what other places will have to call
> > that conversion function for the use case of running a cross-debugger,
> > can someone please help me understand that?
> 
> You'll also want to call it in gdbserver's win32-low.c file, so that
> you get the translation too when debugging against gdbserver.
> This actually suggests putting the new function in some new
> shared file in gdb/gdbsupport/, since gdb/mingw-hdep.c is gdb-only.

A new file for just one function sounds too much to me.  Is it OK to
define an inline function in gdb_wait.h, as in the prototype change
below?  If this way is accepted, I will post a fully formatted patch.

Thanks.
  

On 12/18/19 5:07 PM, Eli Zaretskii wrote:
>> Cc: philippe.waroquiers@skynet.be, gdb-patches@sourceware.org
>> From: Pedro Alves <palves@redhat.com>
>> Date: Tue, 17 Dec 2019 17:51:26 +0000
>>
>> The issue pointed out was that by putting the windows_status_to_termsig
>> function in windows-nat.c, and then calling it from gdb's common code
>> (cli/cli-cmds.c, via WTERMSIG) would result in a build/link failure when
>> you try to build a cross debugger hosted on mingw, because such a gdb
>> build does not include the native Windows target support, i.e., does not
>> build/link the windows-nat.o object.  Putting said function in mingw-hdep.c
>> instead fixes that issue because that file is included as part of the build
>> in all kinds of mingw-hosted GDBs, either native or cross-debugger.
>>
>>>> I admit to being a bit confused about why we want to do this
>>>> translation for this feature while we don't do it for the exit code
>>>> of inferiors running under gdb, for example.  I mean, exit status
>>>> with 0xc0000000 set don't cause $_exitsignal to be set instead of
>>>> $_exitcode.
>>>
>>> Yes, we should do this for exit code of inferiors as well.
>>>
>>> Native MS-Windows debugging produces the TARGET_WAITKIND_* values in
>>> windows-nat.c, so I think the conversion we are talking about will
>>> have to be done there, perhaps _in_addition_to_ other places?  IOW,
>>> the function that performs the conversion can be defined in
>>> mingw-hdep.c, but it will have to be called from windows-nat.c at
>>> least, right?  And I'm uncertain what other places will have to call
>>> that conversion function for the use case of running a cross-debugger,
>>> can someone please help me understand that?
>>
>> You'll also want to call it in gdbserver's win32-low.c file, so that
>> you get the translation too when debugging against gdbserver.
>> This actually suggests putting the new function in some new
>> shared file in gdb/gdbsupport/, since gdb/mingw-hdep.c is gdb-only.
> 
> A new file for just one function sounds too much to me.  Is it OK to
> define an inline function in gdb_wait.h, as in the prototype change
> below?  If this way is accepted, I will post a fully formatted patch.

I don't think it's too much.  As a static inline function means that
you end up with multiple versions of the function, including
the mapping array, in the gdb binary.  And also make gdb_wait.h
expose <windows.h>.  You could add a new gdbsupport/gdb_wait.c
file, with the function wrapped in #ifdef __MINGW32__ to keep it simple
and avoid host/target checks in the configure.ac files.

> 
> Thanks.
> 
> --- gdb/gdbsupport/gdb_wait.h~0	2019-09-21 00:58:17.000000000 +0300
> +++ gdb/gdbsupport/gdb_wait.h	2019-12-18 17:59:28.434097000 +0200
> @@ -40,18 +40,84 @@
>     NOTE exception for GNU/Linux below).  We also fail to declare
>     wait() and waitpid().  */
>  
> +/* For MINGW, the underlying idea is that when a Windows program is
> +   terminated by a fatal exception, its exit code is the value of that
> +   exception, as defined by the various EXCEPTION_* symbols in the
> +   Windows API headers.
> +
> +   The translation below is not perfect, because a program could
> +   legitimately exit normally with a status whose value happens to
> +   have the high bits set, but that's extremely rare, to say the
> +   least, and it is deemed such a negligibly small probability of
> +   false positives is justified by the utility of reporting the
> +   terminating signal in the "normal" cases.  */
> +
> +#ifdef __MINGW32__
> +
> +# define WIN32_LEAN_AND_MEAN
> +# include <windows.h>		/* for EXCEPTION_* constants */
> +# include "gdb/signals.h"	/* for enum gdb_signal */
> +
> +struct xlate_status
> +{
> +  DWORD status;		/* exit status (actually, fatal exception code) */
> +  enum gdb_signal sig;	/* corresponding GDB signal value */
> +};
> +
> +static inline enum gdb_signal
> +windows_status_to_termsig (DWORD status)
> +{
> +  static const xlate_status status_xlate_tbl[] =
> +    {
> +     {EXCEPTION_ACCESS_VIOLATION, 	  GDB_SIGNAL_SEGV},
> +     {EXCEPTION_IN_PAGE_ERROR,		  GDB_SIGNAL_SEGV},
> +     {EXCEPTION_INVALID_HANDLE, 	  GDB_SIGNAL_SEGV},
> +     {EXCEPTION_ILLEGAL_INSTRUCTION, 	  GDB_SIGNAL_ILL},
> +     {EXCEPTION_NONCONTINUABLE_EXCEPTION, GDB_SIGNAL_ILL},
> +     {EXCEPTION_ARRAY_BOUNDS_EXCEEDED, 	  GDB_SIGNAL_SEGV},
> +     {EXCEPTION_FLT_DENORMAL_OPERAND, 	  GDB_SIGNAL_FPE},
> +     {EXCEPTION_FLT_DIVIDE_BY_ZERO, 	  GDB_SIGNAL_FPE},
> +     {EXCEPTION_FLT_INEXACT_RESULT, 	  GDB_SIGNAL_FPE},
> +     {EXCEPTION_FLT_INVALID_OPERATION, 	  GDB_SIGNAL_FPE},
> +     {EXCEPTION_FLT_OVERFLOW, 		  GDB_SIGNAL_FPE},
> +     {EXCEPTION_FLT_STACK_CHECK, 	  GDB_SIGNAL_FPE},
> +     {EXCEPTION_FLT_UNDERFLOW, 		  GDB_SIGNAL_FPE},
> +     {EXCEPTION_INT_DIVIDE_BY_ZERO, 	  GDB_SIGNAL_FPE},
> +     {EXCEPTION_INT_OVERFLOW, 		  GDB_SIGNAL_FPE},
> +     {EXCEPTION_PRIV_INSTRUCTION, 	  GDB_SIGNAL_ILL},
> +     {EXCEPTION_STACK_OVERFLOW, 	  GDB_SIGNAL_SEGV},
> +     {CONTROL_C_EXIT, 			  GDB_SIGNAL_TERM}
> +    };
> +
> +  for (const xlate_status &x : status_xlate_tbl)
> +    if (x.status == status)
> +      return x.sig;
> +
> +  return GDB_SIGNAL_UNKNOWN;
> +}
> +
> +#endif	/* __MINGW32__ */
> +
>  #ifndef	WIFEXITED
> -#define WIFEXITED(w)	(((w)&0377) == 0)
> +# ifdef __MINGW32__
> +#  define WIFEXITED(w)	(((w) & 0xC0000000) == 0)
> +# else
> +#  define WIFEXITED(w)	(((w)&0377) == 0)
> +# endif
>  #endif
>  
>  #ifndef	WIFSIGNALED
> -#define WIFSIGNALED(w)	(((w)&0377) != 0177 && ((w)&~0377) == 0)
> +# ifdef __MINGW32__
> +#  define WIFSIGNALED(w)	(((w) & 0xC0000000) == 0xC0000000)
> +# else
> +#  define WIFSIGNALED(w)	(((w)&0377) != 0177 && ((w)&~0377) == 0)
> +# endif
>  #endif
>  
>  #ifndef	WIFSTOPPED
>  #ifdef IBM6000
>  
> -/* Unfortunately, the above comment (about being compatible in all Unix 
> +/* Unfortunately, the above comment (about being compatible in all Unix
>     systems) is not quite correct for AIX, sigh.  And AIX 3.2 can generate
>     status words like 0x57c (sigtrap received after load), and gdb would
>     choke on it.  */
> @@ -64,11 +130,19 @@
>  #endif
>  
>  #ifndef	WEXITSTATUS
> -#define WEXITSTATUS(w)	(((w) >> 8) & 0377) /* same as WRETCODE */
> +# ifdef __MINGW32__
> +#  define WEXITSTATUS(w)	((w) & ~0xC0000000)
> +# else
> +#  define WEXITSTATUS(w)	(((w) >> 8) & 0377) /* same as WRETCODE */
> +# endif
>  #endif
>  
>  #ifndef	WTERMSIG
> -#define WTERMSIG(w)	((w) & 0177)
> +# ifdef __MINGW32__
> +#  define WTERMSIG(w)	windows_status_to_termsig (w)
> +# else
> +#  define WTERMSIG(w)	((w) & 0177)
> +# endif
>  #endif
>  
>  #ifndef	WSTOPSIG
> --- gdb/windows-nat.c~0	2019-12-11 22:24:51.000000000 +0200
> +++ gdb/windows-nat.c	2019-12-18 18:21:00.264558400 +0200
> @@ -68,6 +68,7 @@
>  #include "inf-child.h"
>  #include "gdbsupport/gdb_tilde_expand.h"
>  #include "gdbsupport/pathstuff.h"
> +#include "gdbsupport/gdb_wait.h"
>  
>  #define AdjustTokenPrivileges		dyn_AdjustTokenPrivileges
>  #define DebugActiveProcessStop		dyn_DebugActiveProcessStop
> @@ -1620,8 +1621,17 @@ get_windows_debug_event (struct target_o
>  	  windows_delete_thread (ptid_t (current_event.dwProcessId, 0,
>  					 current_event.dwThreadId),
>  				 0, true /* main_thread_p */);
> -	  ourstatus->kind = TARGET_WAITKIND_EXITED;
> -	  ourstatus->value.integer = current_event.u.ExitProcess.dwExitCode;
> +	  DWORD exit_status = current_event.u.ExitProcess.dwExitCode;
> +	  if (WIFEXITED (exit_status))
> +	    {
> +	      ourstatus->kind = TARGET_WAITKIND_EXITED;
> +	      ourstatus->value.integer = WEXITSTATUS (exit_status);
> +	    }
> +	  else
> +	    {
> +	      ourstatus->kind = TARGET_WAITKIND_SIGNALLED;
> +	      ourstatus->value.sig = WTERMSIG (exit_status);
> +	    }
>  	  thread_id = current_event.dwThreadId;
>  	}
>        break;
> --- gdb/windows-tdep.c~0	2019-09-21 00:58:17.000000000 +0300
> +++ gdb/windows-tdep.c	2019-12-18 17:49:52.360580700 +0200
> @@ -35,6 +35,8 @@
>  #include "solib-target.h"
>  #include "gdbcore.h"
>  
> +#include <signal.h>
> +
>  
> +static int
> +windows_gdb_signal_to_target (struct gdbarch *gdbarch, enum gdb_signal signal)
> +{

We should not rely on signal.h constants for this hook.  See gdbarch.sh:

 # Signal translation: translate the GDB's internal signal number into
 # the inferior's signal (target's) representation.  The implementation
 # of this method must be host independent.  IOW, don't rely on symbols
 # of the NAT_FILE header (the nm-*.h files), the host <signal.h>
 # header, or similar headers.
 # Return the target signal number if found, or -1 if the GDB internal
 # signal number is invalid.
 M;int;gdb_signal_to_target;enum gdb_signal signal;signal

Say you're debugging against a mingw gdbserver from a Linux host.
If you rely on <signal.h> constants here, this function is going to
return the Linux (or whatever the host) numbers instead of the
Windows/mingw numbers.  For Linux, we define the LINUX_SIGxxx numbers
in linux-tdep.c, around line 125.  The patch should add a similar
enum.

Some overall comments:

With this change, the user no longer has access to the original
$_exitcode, for the cases that match one of the known exceptions.
I don't know whether anyone is relying on those, though I wouldn't
be surprised if so.  I assume you've pondered about this and consider 
that the change is still worth it anyhow.  This should at least be
documented.  I wonder whether we should provide both the exit
code in $_exitcode and the translated signal number in $_exitsignal,
though that would require more changes.

Related, when windows_status_to_termsig doesn't recognize the
exception number, we end up with GDB_SIGNAL_UNKNOWN, and then
$_exitsignal == -1.  I.e., we lose information in that case.  Again,
something to ponder about that information loss is OK, or whether
we should do something about it.

Thanks,
Pedro Alves
  

> Cc: philippe.waroquiers@skynet.be, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Wed, 18 Dec 2019 17:42:28 +0000
> 
> > A new file for just one function sounds too much to me.  Is it OK to
> > define an inline function in gdb_wait.h, as in the prototype change
> > below?  If this way is accepted, I will post a fully formatted patch.
> 
> I don't think it's too much.  As a static inline function means that
> you end up with multiple versions of the function, including
> the mapping array, in the gdb binary.  And also make gdb_wait.h
> expose <windows.h>.  You could add a new gdbsupport/gdb_wait.c
> file, with the function wrapped in #ifdef __MINGW32__ to keep it simple
> and avoid host/target checks in the configure.ac files.

Sorry, I don't understand what host/target checks might be needed in
configure.ac, can you explain?

> We should not rely on signal.h constants for this hook.  See gdbarch.sh:
> 
>  # Signal translation: translate the GDB's internal signal number into
>  # the inferior's signal (target's) representation.  The implementation
>  # of this method must be host independent.  IOW, don't rely on symbols
>  # of the NAT_FILE header (the nm-*.h files), the host <signal.h>
>  # header, or similar headers.
>  # Return the target signal number if found, or -1 if the GDB internal
>  # signal number is invalid.
>  M;int;gdb_signal_to_target;enum gdb_signal signal;signal
> 
> Say you're debugging against a mingw gdbserver from a Linux host.
> If you rely on <signal.h> constants here, this function is going to
> return the Linux (or whatever the host) numbers instead of the
> Windows/mingw numbers.  For Linux, we define the LINUX_SIGxxx numbers
> in linux-tdep.c, around line 125.  The patch should add a similar
> enum.

So we need to have 2 different sets of explicit signal numbers, one
for MinGW64, the other for mingw.org's MinGW (no, they are not
identical)?  And perhaps one more for Cygwin?  Or should we just
support the signals common to all 3 environments?

And what do we do when the signal numbers in the system's signal.h
header change in some future version of MinGW/Cygwin?  This sounds
like a very fragile arrangement, at least for non-Posix systems where
signals are emulated and aren't part of the kernel definitions set in
stone.  I'm okay with doing a bunch of defines, but it seems to me a
maintenance headache in the long run, FWIW.

> With this change, the user no longer has access to the original
> $_exitcode, for the cases that match one of the known exceptions.
> I don't know whether anyone is relying on those, though I wouldn't
> be surprised if so.  I assume you've pondered about this and consider 
> that the change is still worth it anyhow.  This should at least be
> documented.  I wonder whether we should provide both the exit
> code in $_exitcode and the translated signal number in $_exitsignal,
> though that would require more changes.

I think this is a micro-optimization that is way in the area of the
diminishing returns.  I've never seen a program to return an exit
status with the high 2 bits set.  I'd definitely not recommend to
tweak the current assumption in the GDB code that if a program exited
due to a signal, its exit code is irrelevant, based on such a
theoretical possibility.  In most cases, the fact that the inferior
got a fatal exception will be noted before it exits anyway.

> Related, when windows_status_to_termsig doesn't recognize the
> exception number, we end up with GDB_SIGNAL_UNKNOWN, and then
> $_exitsignal == -1.  I.e., we lose information in that case.  Again,
> something to ponder about that information loss is OK, or whether
> we should do something about it.

I don't think we should do anything about it, it's highly theoretical
situation I've never seen in real life.  And we already lose
information in windows-nat.c, when we return GDB_SIGNAL_UNKNOWN for
any exception we don't recognize explicitly.  In any case, the way
this stuff currently works, I see no simple way of returning an
arbitrary value of a signal.

Maybe we should abandon the idea of doing this in windows-nat.c and
win32-low.c, and only translate the exit code in cli-cmds.c for the
'pipe' command?  It's much more important in that case (since we don't
intercept the signals as we do from the inferior), and most of those
complications don't apply there.  There's also no backward
compatibility problem, since 'pipe' is a new feature in GDB 9, with 2
new convenience variables to hold the exit status and the signal
value.  WDYT?

Thanks.
  

On 12/18/19 6:32 PM, Eli Zaretskii wrote:
>> Cc: philippe.waroquiers@skynet.be, gdb-patches@sourceware.org
>> From: Pedro Alves <palves@redhat.com>
>> Date: Wed, 18 Dec 2019 17:42:28 +0000
>>
>>> A new file for just one function sounds too much to me.  Is it OK to
>>> define an inline function in gdb_wait.h, as in the prototype change
>>> below?  If this way is accepted, I will post a fully formatted patch.
>>
>> I don't think it's too much.  As a static inline function means that
>> you end up with multiple versions of the function, including
>> the mapping array, in the gdb binary.  And also make gdb_wait.h
>> expose <windows.h>.  You could add a new gdbsupport/gdb_wait.c
>> file, with the function wrapped in #ifdef __MINGW32__ to keep it simple
>> and avoid host/target checks in the configure.ac files.
> 
> Sorry, I don't understand what host/target checks might be needed in
> configure.ac, can you explain?

What I meant is that files like mingw-hdep.o are conditionally added
to the set of files to build by configure.ac, depending on the host
triplet.  An alternative mechanism would be to merge all of 
mingw-hdep.c posix-hdep.c in a single .c file, and then use #ifdef
within.  I was suggesting the latter mechanism for this new file.


> 
>> We should not rely on signal.h constants for this hook.  See gdbarch.sh:
>>
>>  # Signal translation: translate the GDB's internal signal number into
>>  # the inferior's signal (target's) representation.  The implementation
>>  # of this method must be host independent.  IOW, don't rely on symbols
>>  # of the NAT_FILE header (the nm-*.h files), the host <signal.h>
>>  # header, or similar headers.
>>  # Return the target signal number if found, or -1 if the GDB internal
>>  # signal number is invalid.
>>  M;int;gdb_signal_to_target;enum gdb_signal signal;signal
>>
>> Say you're debugging against a mingw gdbserver from a Linux host.
>> If you rely on <signal.h> constants here, this function is going to
>> return the Linux (or whatever the host) numbers instead of the
>> Windows/mingw numbers.  For Linux, we define the LINUX_SIGxxx numbers
>> in linux-tdep.c, around line 125.  The patch should add a similar
>> enum.
> 
> So we need to have 2 different sets of explicit signal numbers, one
> for MinGW64, the other for mingw.org's MinGW (no, they are not
> identical)?  And perhaps one more for Cygwin?  Or should we just
> support the signals common to all 3 environments?
> 
> And what do we do when the signal numbers in the system's signal.h
> header change in some future version of MinGW/Cygwin?  

I would think signal numbers changing would be unlikely, since it
would be an ABI break.

> This sounds
> like a very fragile arrangement, at least for non-Posix systems where
> signals are emulated and aren't part of the kernel definitions set in
> stone.  I'm okay with doing a bunch of defines, but it seems to me a
> maintenance headache in the long run, FWIW.

The alternative is that the cross debugging scenario
doesn't work.  It doesn't seem better to me.  A potential alternative
would be to offload the mapping/conversions to the remote/server
side somehow, but that can't work for cross-core debugging, so it's
not ideal either.

> 
>> With this change, the user no longer has access to the original
>> $_exitcode, for the cases that match one of the known exceptions.
>> I don't know whether anyone is relying on those, though I wouldn't
>> be surprised if so.  I assume you've pondered about this and consider 
>> that the change is still worth it anyhow.  This should at least be
>> documented.  I wonder whether we should provide both the exit
>> code in $_exitcode and the translated signal number in $_exitsignal,
>> though that would require more changes.
> 
> I think this is a micro-optimization that is way in the area of the
> diminishing returns.  I've never seen a program to return an exit
> status with the high 2 bits set.  I'd definitely not recommend to
> tweak the current assumption in the GDB code that if a program exited
> due to a signal, its exit code is irrelevant, based on such a
> theoretical possibility.  In most cases, the fact that the inferior
> got a fatal exception will be noted before it exits anyway.

We're writing a debugger, and it doesn't seem odd to me that people
would like for the debugger to provide all information available
in order to debug the problem, rather than hide it.
You may be right that nobody actually cares about it, but I thought
I'd point it out so we can make an informed decision.

> 
>> Related, when windows_status_to_termsig doesn't recognize the
>> exception number, we end up with GDB_SIGNAL_UNKNOWN, and then
>> $_exitsignal == -1.  I.e., we lose information in that case.  Again,
>> something to ponder about that information loss is OK, or whether
>> we should do something about it.
> 
> I don't think we should do anything about it, it's highly theoretical
> situation I've never seen in real life.  And we already lose
> information in windows-nat.c, when we return GDB_SIGNAL_UNKNOWN for
> any exception we don't recognize explicitly.  

OK.  We do print the raw Win32 exception before converting to 
GDB_SIGNAL_UNKNOWN, so the user sees it, but I get your point.

> In any case, the way
> this stuff currently works, I see no simple way of returning an
> arbitrary value of a signal.

Yeah, I think we'd have to change struct target_waitstatus.

> 
> Maybe we should abandon the idea of doing this in windows-nat.c and
> win32-low.c, and only translate the exit code in cli-cmds.c for the
> 'pipe' command?  It's much more important in that case (since we don't
> intercept the signals as we do from the inferior), and most of those
> complications don't apply there.  There's also no backward
> compatibility problem, since 'pipe' is a new feature in GDB 9, with 2
> new convenience variables to hold the exit status and the signal
> value.  WDYT?

I don't know, I don't have a strong opinion.  As I mentioned, I'm
only pointing out the issues so we hash it all out and make an
informed decision.

I'll take a look at your new patch.

Thanks,
Pedro Alves