diff mbox

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

Message ID 20190504161753.15530-3-philippe.waroquiers@skynet.be
State New
Headers show

Commit Message

Philippe Waroquiers May 4, 2019, 4:17 p.m. UTC
gdb/ChangeLog
2019-05-04  Eli Zaretskii  <eliz@gnu.org>
	    Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* common/gdb_wait.h (WIFEXITED, WIFSIGNALED, WEXITSTATUS,
	WTERMSIG): Define better versions for MinGW.
	* windows-nat.c (xlate): Uncomment the definition.
	(windows_status_to_termsig): New function.
---
 gdb/common/gdb_wait.h | 27 +++++++++++++++++++++++++++
 gdb/windows-nat.c     | 18 ++++++++++++++----
 2 files changed, 41 insertions(+), 4 deletions(-)

Comments

Pedro Alves May 27, 2019, 5:33 p.m. UTC | #1
On 5/4/19 5:17 PM, Philippe Waroquiers wrote:
> gdb/ChangeLog
> 2019-05-04  Eli Zaretskii  <eliz@gnu.org>
> 	    Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* common/gdb_wait.h (WIFEXITED, WIFSIGNALED, WEXITSTATUS,
> 	WTERMSIG): Define better versions for MinGW.
> 	* windows-nat.c (xlate): Uncomment the definition.
> 	(windows_status_to_termsig): New function.

windows-nat.c looks like the wrong place to put this.

windows-nat.c is only included in the build if building a native
debugger.  But, you need this functionality on every Windows-hosted build
of GDB, even cross debuggers.  So I think you're breaking the build on
the Windows-hosted, non-native-debugger case.

E.g., --host=mingw --target=arm-none-eabi.

The right place would be mingw-hdep.c.

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.

Thanks,
Pedro Alves
Eli Zaretskii May 27, 2019, 6:37 p.m. UTC | #2
> From: Pedro Alves <palves@redhat.com>
> Date: Mon, 27 May 2019 18:33:11 +0100
> 
> 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.

We should probably do this everywhere where it matters whether the
inferior exited due to a fatal signal.
Pedro Alves May 29, 2019, 12:37 p.m. UTC | #3
On 5/27/19 7:37 PM, Eli Zaretskii wrote:
>> From: Pedro Alves <palves@redhat.com>
>> Date: Mon, 27 May 2019 18:33:11 +0100
>>
>> 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.
> 
> We should probably do this everywhere where it matters whether the
> inferior exited due to a fatal signal.

Wouldn't it better to leave that translation out of this patch series,
and do it everywhere it matters as a separate change, to avoid
creating inconsistencies?  Might be simpler for Philippe too, since
the current patch isn't OK as is.

Thanks,
Pedro Alves
Eli Zaretskii May 29, 2019, 3:03 p.m. UTC | #4
> Cc: philippe.waroquiers@skynet.be, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Wed, 29 May 2019 13:37:56 +0100
> 
> > We should probably do this everywhere where it matters whether the
> > inferior exited due to a fatal signal.
> 
> Wouldn't it better to leave that translation out of this patch series,
> and do it everywhere it matters as a separate change, to avoid
> creating inconsistencies?  Might be simpler for Philippe too, since
> the current patch isn't OK as is.

It's up to you guys, I don't have a strong opinion about that.
Philippe Waroquiers May 30, 2019, 10:26 a.m. UTC | #5
On Wed, 2019-05-29 at 13:37 +0100, Pedro Alves wrote:
> On 5/27/19 7:37 PM, Eli Zaretskii wrote:
> > > From: Pedro Alves <palves@redhat.com>
> > > Date: Mon, 27 May 2019 18:33:11 +0100
> > > 
> > > 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.
> > 
> > We should probably do this everywhere where it matters whether the
> > inferior exited due to a fatal signal.
> 
> Wouldn't it better to leave that translation out of this patch series,
> and do it everywhere it matters as a separate change, to avoid
> creating inconsistencies?  Might be simpler for Philippe too, since
> the current patch isn't OK as is.
Yes, that looks a better approach, in particular because I do not have
a platform where I can compile such builds ...

Philippe
Eli Zaretskii Dec. 17, 2019, 4:59 p.m. UTC | #6
> From: Pedro Alves <palves@redhat.com>
> Date: Mon, 27 May 2019 18:33:11 +0100

To recap, back in May Philippe added the 'pipe' command, and we had a
brief discussion regarding the use of WIFEXITED, WIFSIGNALED, and
other related macros from <sys/wait.h>, on MS-Windows.  It was decided
back then to leave for later the translation of exit codes returned by
'pipe' in MS-Windows build of GDB.

I've now started to look at this issue, with the intent to provide ext
status to signal conversion for the MS-Windows ports of GDB, and I
have a few questions regarding the details.

In that discussion, Pedro commented on Philippe's proposed patch
(https://sourceware.org/ml/gdb-patches/2019-05/msg00131.html) which
added the definitions for WIF* and WEXIT* macros to gdb_wait.h and
their use in widnows-nat.c.  The comments are in
https://sourceware.org/ml/gdb-patches/2019-05/msg00590.html, and go
like this:

> > 	* common/gdb_wait.h (WIFEXITED, WIFSIGNALED, WEXITSTATUS,
> > 	WTERMSIG): Define better versions for MinGW.
> > 	* windows-nat.c (xlate): Uncomment the definition.
> > 	(windows_status_to_termsig): New function.
> 
> windows-nat.c looks like the wrong place to put this.
> 
> windows-nat.c is only included in the build if building a native
> debugger.  But, you need this functionality on every Windows-hosted build
> of GDB, even cross debuggers.  So I think you're breaking the build on
> the Windows-hosted, non-native-debugger case.
> 
> E.g., --host=mingw --target=arm-none-eabi.
> 
> The right place would be mingw-hdep.c.

I'm okay with doing this in mingw-hdep.c, but I'm a bit confused by
this comment.  The encoding of the fatal exception in the exit status
of a program is a feature of the native MS-Windows processes.  Does
"running cross-debugger" mentioned above allude to running an
MS-Windows program?  If so, which GDB component (that is presumably
not windows-nat.c) is involved in running such cross-debugged programs
and for translating the debug status to the likes of
TARGET_WAITKIND_EXITED?  And how does the 'pipe' command support these
cross-debugging use cases (as it uses the 'popen' C library function,
which AFAIU runs natively)?

> 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?

TIA
Pedro Alves Dec. 17, 2019, 5:51 p.m. UTC | #7
On 12/17/19 4:59 PM, Eli Zaretskii wrote:
>> From: Pedro Alves <palves@redhat.com>
>> Date: Mon, 27 May 2019 18:33:11 +0100
> 
> To recap, back in May Philippe added the 'pipe' command, and we had a
> brief discussion regarding the use of WIFEXITED, WIFSIGNALED, and
> other related macros from <sys/wait.h>, on MS-Windows.  It was decided
> back then to leave for later the translation of exit codes returned by
> 'pipe' in MS-Windows build of GDB.
> 
> I've now started to look at this issue, with the intent to provide ext
> status to signal conversion for the MS-Windows ports of GDB, and I
> have a few questions regarding the details.
> 
> In that discussion, Pedro commented on Philippe's proposed patch
> (https://sourceware.org/ml/gdb-patches/2019-05/msg00131.html) which
> added the definitions for WIF* and WEXIT* macros to gdb_wait.h and
> their use in widnows-nat.c.  The comments are in
> https://sourceware.org/ml/gdb-patches/2019-05/msg00590.html, and go
> like this:
> 
>>> 	* common/gdb_wait.h (WIFEXITED, WIFSIGNALED, WEXITSTATUS,
>>> 	WTERMSIG): Define better versions for MinGW.
>>> 	* windows-nat.c (xlate): Uncomment the definition.
>>> 	(windows_status_to_termsig): New function.
>>
>> windows-nat.c looks like the wrong place to put this.
>>
>> windows-nat.c is only included in the build if building a native
>> debugger.  But, you need this functionality on every Windows-hosted build
>> of GDB, even cross debuggers.  So I think you're breaking the build on
>> the Windows-hosted, non-native-debugger case.
>>
>> E.g., --host=mingw --target=arm-none-eabi.
>>
>> The right place would be mingw-hdep.c.
> 
> I'm okay with doing this in mingw-hdep.c, but I'm a bit confused by
> this comment.  The encoding of the fatal exception in the exit status
> of a program is a feature of the native MS-Windows processes.  Does
> "running cross-debugger" mentioned above allude to running an
> MS-Windows program?  

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.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/common/gdb_wait.h b/gdb/common/gdb_wait.h
index b3b752cf3a..ca95240009 100644
--- a/gdb/common/gdb_wait.h
+++ b/gdb/common/gdb_wait.h
@@ -40,13 +40,31 @@ 
    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 STATUS_* symbols in the Windows API headers.
+
+   The 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.  */
+
 #ifndef	WIFEXITED
+#if defined (__MINGW32__)
+#define WIFEXITED(stat_val)   (((stat_val) & 0xC0000000) == 0)
+#else
 #define WIFEXITED(w)	(((w)&0377) == 0)
 #endif
+#endif
 
 #ifndef	WIFSIGNALED
+#if defined (__MINGW32__)
+#define WIFSIGNALED(stat_val) (((stat_val) & 0xC0000000) == 0xC0000000)
+#else
 #define WIFSIGNALED(w)	(((w)&0377) != 0177 && ((w)&~0377) == 0)
 #endif
+#endif
 
 #ifndef	WIFSTOPPED
 #ifdef IBM6000
@@ -64,12 +82,21 @@ 
 #endif
 
 #ifndef	WEXITSTATUS
+#if defined (__MINGW32__)
+#define WEXITSTATUS(stat_val) ((stat_val) & 255)
+#else
 #define WEXITSTATUS(w)	(((w) >> 8) & 0377) /* same as WRETCODE */
 #endif
+#endif
 
 #ifndef	WTERMSIG
+#if defined (__MINGW32__)
+extern enum gdb_signal windows_status_to_termsig (int stat_val);
+#define WTERMSIG(stat_val)    windows_status_to_termsig (stat_val)
+#else
 #define WTERMSIG(w)	((w) & 0177)
 #endif
+#endif
 
 #ifndef	WSTOPSIG
 #define WSTOPSIG	WEXITSTATUS
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index ae4e3d55b3..c90caeda6a 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -280,9 +280,9 @@  static const int *mappings;
    a segment register or not.  */
 static segment_register_p_ftype *segment_register_p;
 
-/* See windows_nat_target::resume to understand why this is commented
-   out.  */
-#if 0
+/* See windows_nat_target::resume to understand why xlate is not used
+   to translate a signal into an exception.  */
+
 /* This vector maps the target's idea of an exception (extracted
    from the DEBUG_EVENT structure) to GDB's idea.  */
 
@@ -302,7 +302,17 @@  static const struct xlate_exception xlate[] =
   {STATUS_FLOAT_DIVIDE_BY_ZERO, GDB_SIGNAL_FPE}
 };
 
-#endif /* 0 */
+/* Translate a windows exception inside STAT_VAL into a gdb_signal.
+   This should only be called if WIFSIGNALED (stat_val).  */
+
+enum gdb_signal
+windows_status_to_termsig (int stat_val)
+{
+  for (const xlate_exception &x : xlate)
+    if (x.them == (stat_val & ~0xC0000000))
+      return x.us;
+  return GDB_SIGNAL_UNKNOWN;
+}
 
 struct windows_nat_target final : public x86_nat_target<inf_child_target>
 {