From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12605 invoked by alias); 18 Dec 2019 17:42:51 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 12596 invoked by uid 89); 18 Dec 2019 17:42:51 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-11.9 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3 autolearn=ham version=3.3.1 spammy=target's X-HELO: us-smtp-delivery-1.mimecast.com Received: from us-smtp-2.mimecast.com (HELO us-smtp-delivery-1.mimecast.com) (207.211.31.81) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 18 Dec 2019 17:42:40 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1576690959; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=hpBLt9UIY5nHH1mFkVg2m1SMIPTmV6dlI5of33zcID8=; b=dinGzSHUOy51dKf4MYP9WCmop9hzQr7ieiKCkRqX4oqPmzpmoyiPWimSTbBGP6cvsMdm6M nqWtLzM4yB7IMKHiOXNzYdcvt2DX3mq4cgPXOGoZVIRDbbjTgdss7xhskytFTTWXJ7ZBkC fqsn/RGCbDZ5pjHOBz+lQgrmKFt4ps0= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-67-tzGxf7SzPgiNy_5NLP-mRg-1; Wed, 18 Dec 2019 12:42:32 -0500 Received: by mail-wm1-f70.google.com with SMTP id m133so747194wmf.2 for ; Wed, 18 Dec 2019 09:42:31 -0800 (PST) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:56ee:75ff:fe8d:232b? ([2001:8a0:f913:f700:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id 16sm3159120wmi.0.2019.12.18.09.42.28 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 18 Dec 2019 09:42:29 -0800 (PST) Subject: Re: [RFAv3 2/6] Improve process exit status macros on MinGW To: Eli Zaretskii References: <20190504161753.15530-1-philippe.waroquiers@skynet.be> <20190504161753.15530-3-philippe.waroquiers@skynet.be> <835zie51mf.fsf@gnu.org> <52c4ca33-ffc4-8e1e-fe08-a92123ef02aa@redhat.com> <83o8w536l6.fsf@gnu.org> Cc: philippe.waroquiers@skynet.be, gdb-patches@sourceware.org From: Pedro Alves Message-ID: <3aacf88f-212e-f11f-0688-4f8219dab4c3@redhat.com> Date: Wed, 18 Dec 2019 17:42:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <83o8w536l6.fsf@gnu.org> X-Mimecast-Spam-Score: 0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-12/txt/msg00775.txt.bz2 On 12/18/19 5:07 PM, Eli Zaretskii wrote: >> Cc: philippe.waroquiers@skynet.be, gdb-patches@sourceware.org >> From: Pedro Alves >> 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 . 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 /* 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 > + > > +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 # 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 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