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

[Pedro Alves <palves@redhat.com>]

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