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

[Pedro Alves <palves@redhat.com>]

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