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

Pedro Alves
2019-12-18T17:42:51Z

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