Message ID | 831s1murm2.fsf@gnu.org |
---|---|
State | New, archived |
Headers |
Received: (qmail 47624 invoked by alias); 28 Apr 2019 14:33:41 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 47616 invoked by uid 89); 28 Apr 2019 14:33:41 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-8.1 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_2, GIT_PATCH_3, SPF_PASS autolearn=ham version=3.3.1 spammy=HTo:U*palves, stdio.h, UD:stdio.h, UD:stdlib.h X-HELO: eggs.gnu.org Received: from eggs.gnu.org (HELO eggs.gnu.org) (209.51.188.92) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 28 Apr 2019 14:33:40 +0000 Received: from fencepost.gnu.org ([2001:470:142:3::e]:36331) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from <eliz@gnu.org>) id 1hKks6-0001hh-Bo; Sun, 28 Apr 2019 10:33:38 -0400 Received: from [176.228.60.248] (port=1463 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from <eliz@gnu.org>) id 1hKks4-0003AK-KH; Sun, 28 Apr 2019 10:33:37 -0400 Date: Sun, 28 Apr 2019 17:33:25 +0300 Message-Id: <831s1murm2.fsf@gnu.org> From: Eli Zaretskii <eliz@gnu.org> To: Pedro Alves <palves@redhat.com> CC: gdb-patches@sourceware.org In-reply-to: <93ccb0fa-8a05-60ff-d1a8-85d5663b8d16@redhat.com> (message from Pedro Alves on Fri, 19 Apr 2019 12:33:37 +0100) Subject: Re: Fix compilation using mingw.org's MinGW References: <835zrbe36c.fsf@gnu.org> <c63e5c22-9069-4d06-c10b-1c906e74e7d6@redhat.com> <250801eb-14f6-5a35-0556-cf5797dd8a7b@redhat.com> <83y347cfbu.fsf@gnu.org> <556cefd7-47ce-54ab-a228-2c727aab4179@redhat.com> <83d0lick7o.fsf@gnu.org> <93ccb0fa-8a05-60ff-d1a8-85d5663b8d16@redhat.com> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-IsSubscribed: yes |
Commit Message
Eli Zaretskii
April 28, 2019, 2:33 p.m. UTC
> Cc: gdb-patches@sourceware.org > From: Pedro Alves <palves@redhat.com> > Date: Fri, 19 Apr 2019 12:33:37 +0100 > > > I will try doing this in common-defs.h, but for the branch, we could > > set _WIN32_WINNT only in windows-nat.c, as that's the only file that > > currently cares, which should be safer. WDYT? > > Sounds good to me. Below please find 2 patches: the first one for the 8.3 branch, the second one for master. OK to commit (after writing ChangeLog entries)?
Comments
On 4/28/19 3:33 PM, Eli Zaretskii wrote: >> Cc: gdb-patches@sourceware.org >> From: Pedro Alves <palves@redhat.com> >> Date: Fri, 19 Apr 2019 12:33:37 +0100 >> >>> I will try doing this in common-defs.h, but for the branch, we could >>> set _WIN32_WINNT only in windows-nat.c, as that's the only file that >>> currently cares, which should be safer. WDYT? >> >> Sounds good to me. > > Below please find 2 patches: the first one for the 8.3 branch, the > second one for master. OK to commit (after writing ChangeLog > entries)? Sure, LGTM. A few remarks: - The override in _WIN32_WINNT is no longer necessary. It could be removed at the same time. - IIRC, Cygwin uses the same w32api headers as mingw.org. I don't know whether Cygwin sets _WIN32_WINNT to a higher number already; we haven't heard any complaints, so I guess it does. - We could remove the dynamic loading of GetConsoleFontSize & friends now, right? (for a separate patch/rainy day, not in this patch.) Thanks, Pedro Alves
On 30.04.2019 15:56, Pedro Alves wrote: > - IIRC, Cygwin uses the same w32api headers as mingw.org. I don't know > whether Cygwin sets _WIN32_WINNT to a higher number already; we > haven't heard any complaints, so I guess it does. > Cygwin uses mingw-w64.
On 4/30/19 1:56 PM, Pedro Alves wrote: > Sure, LGTM. > > A few remarks: > > - The override in _WIN32_WINNT is no longer necessary. It could > be removed at the same time. I meant the _WIN32_WINNT override in common/netstuff.c. I see now that there are more of those in other files too. Thanks, Pedro Alves
> Cc: gdb-patches@sourceware.org > From: Pedro Alves <palves@redhat.com> > Date: Tue, 30 Apr 2019 14:10:53 +0100 > > > - The override in _WIN32_WINNT is no longer necessary. It could > > be removed at the same time. > > I meant the _WIN32_WINNT override in common/netstuff.c. > > I see now that there are more of those in other files too. I will review them for the changeset on master, but I don't think we should change them on the 8.3 branch. OK? Thanks for the review.
> From: LRN <lrn1986@gmail.com> > Date: Tue, 30 Apr 2019 16:05:55 +0300 > > > - IIRC, Cygwin uses the same w32api headers as mingw.org. I don't know > > whether Cygwin sets _WIN32_WINNT to a higher number already; we > > haven't heard any complaints, so I guess it does. > > > > Cygwin uses mingw-w64. Maybe I'm missing something, but I don't think this is relevant. _WIN32_WINNT is not a MinGW invention, it's an official symbol in Windows headers, so all Windows compilers need to support it in the same way and with the same semantics. The only difference is the default value. Am I missing something?
On 4/30/19 4:23 PM, Eli Zaretskii wrote: >> Cc: gdb-patches@sourceware.org >> From: Pedro Alves <palves@redhat.com> >> Date: Tue, 30 Apr 2019 14:10:53 +0100 >> >>> - The override in _WIN32_WINNT is no longer necessary. It could >>> be removed at the same time. >> >> I meant the _WIN32_WINNT override in common/netstuff.c. >> >> I see now that there are more of those in other files too. > > I will review them for the changeset on master, but I don't think we > should change them on the 8.3 branch. OK? Yes, my comments were for master, since changing removing the overrides in the other files only makes sense with the _WIN32_WINNT definition in common-defs.h. Thanks, Pedro Alves
On 4/30/19 4:25 PM, Eli Zaretskii wrote: >> From: LRN <lrn1986@gmail.com> >> Date: Tue, 30 Apr 2019 16:05:55 +0300 >> >>> - IIRC, Cygwin uses the same w32api headers as mingw.org. I don't know >>> whether Cygwin sets _WIN32_WINNT to a higher number already; we >>> haven't heard any complaints, so I guess it does. >>> >> >> Cygwin uses mingw-w64. > > Maybe I'm missing something, but I don't think this is relevant. > _WIN32_WINNT is not a MinGW invention, it's an official symbol in > Windows headers, so all Windows compilers need to support it in the > same way and with the same semantics. The only difference is the > default value. > > Am I missing something? > The issue is where is that default set? On my Fedora mingw-w64 cross, it is not set by default by the compiler: $ x86_64-w64-mingw32-gcc -x c /dev/null -dM -E | grep _WIN32_WINNT $ (empty) It seems to be set instead in the headers, in include/sdkddkver.h. I guess that if Cygwin indeed uses mingw-w64 headers nowadays, it won't need the fix as long as the headers default to a higher version than gdb requires. Pedantically, I think we should tweak the "#ifdef __MINGW32__" to consider Cygwin too, since the Cygwin ports also pulls in w32api headers, but if it doesn't matter in practice, and it's not convenient to test, then we can simply forget about it until a time when someone notices something odd. Thanks, Pedro Alves
> Cc: gdb-patches@sourceware.org > From: Pedro Alves <palves@redhat.com> > Date: Tue, 30 Apr 2019 18:03:57 +0100 > > The issue is where is that default set? On some internal header file (it differs between various flavors of MinGW). But no matter where it is set, it must be defined after _any_ standard header is included, so in practice I think it's defined at the place where the patch tests for it. In any case, the only platform which really needs this is mingw.org's MinGW, where I actually tested this assumption. The other two, MinGW64 and Cygwin, don't support older platforms (they actually don't support XP anymore, only Vista and onward), so their default values are higher than 0x0501 anyway.
On 4/30/19 6:17 PM, Eli Zaretskii wrote: >> Cc: gdb-patches@sourceware.org >> From: Pedro Alves <palves@redhat.com> >> Date: Tue, 30 Apr 2019 18:03:57 +0100 >> >> The issue is where is that default set? > > On some internal header file (it differs between various flavors of > MinGW). Right, which is what I said. > But no matter where it is set, it must be defined after _any_ > standard header is included, so in practice I think it's defined at > the place where the patch tests for it. I think you mean "before". But I did not say that this was the wrong place (since I was the one that suggested the place). Only that pedantically the new code could/should be tweaked like this: - #ifdef __MINGW32__ + #if defined (__MINGW32__) || defined (__CYGWIN__) # ifdef _WIN32_WINNT # if _WIN32_WINNT < 0x0501 # undef _WIN32_WINNT # define _WIN32_WINNT 0x0501 # endif # else # define _WIN32_WINNT 0x0501 # endif #endif /* __MINGW32__ */ > > In any case, the only platform which really needs this is mingw.org's > MinGW, where I actually tested this assumption. The other two, > MinGW64 and Cygwin, don't support older platforms (they actually don't > support XP anymore, only Vista and onward), so their default values > are higher than 0x0501 anyway. > Right, like I said. Thanks, Pedro Alves
> Cc: lrn1986@gmail.com, gdb-patches@sourceware.org > From: Pedro Alves <palves@redhat.com> > Date: Tue, 30 Apr 2019 18:26:19 +0100 > > On 4/30/19 6:17 PM, Eli Zaretskii wrote: > >> Cc: gdb-patches@sourceware.org > >> From: Pedro Alves <palves@redhat.com> > >> Date: Tue, 30 Apr 2019 18:03:57 +0100 > >> > >> The issue is where is that default set? > > > > On some internal header file (it differs between various flavors of > > MinGW). > > Right, which is what I said. Yes. > > But no matter where it is set, it must be defined after _any_ > > standard header is included, so in practice I think it's defined at > > the place where the patch tests for it. > > I think you mean "before". No, I meant "after". The default value is set once you included at least on standard header. Hence you can at that place test for whether it is defined and what is its default value. Overriding that default is generally important only before including w32api headers, such as windows.h. > But I did not say that this was the wrong > place (since I was the one that suggested the place). Only that > pedantically the new code could/should be tweaked like this: > > - #ifdef __MINGW32__ > + #if defined (__MINGW32__) || defined (__CYGWIN__) If you want me to add __CYGWIN__, I'm okay with that. > > In any case, the only platform which really needs this is mingw.org's > > MinGW, where I actually tested this assumption. The other two, > > MinGW64 and Cygwin, don't support older platforms (they actually don't > > support XP anymore, only Vista and onward), so their default values > > are higher than 0x0501 anyway. > > > > Right, like I said. Sure, we agree. I didn't mean to contradict what you were saying, I wanted to back that up.
On 30.04.2019 18:25, Eli Zaretskii wrote: >> From: LRN >> Date: Tue, 30 Apr 2019 16:05:55 +0300 >> >>> - IIRC, Cygwin uses the same w32api headers as mingw.org. I don't know >>> whether Cygwin sets _WIN32_WINNT to a higher number already; we >>> haven't heard any complaints, so I guess it does. >>> >> >> Cygwin uses mingw-w64. > > Maybe I'm missing something, but I don't think this is relevant. > _WIN32_WINNT is not a MinGW invention, it's an official symbol in > Windows headers, so all Windows compilers need to support it in the > same way and with the same semantics. The only difference is the > default value. > > Am I missing something? > I replied to the "Cygwin uses the same w32api headers as mingw.org" part only.
On 4/30/19 6:40 PM, Eli Zaretskii wrote: >>> But no matter where it is set, it must be defined after _any_ >>> standard header is included, so in practice I think it's defined at >>> the place where the patch tests for it. >> I think you mean "before". > No, I meant "after". The default value is set once you included at > least on standard header. Hence you can at that place test for > whether it is defined and what is its default value. Sorry, but no, there's no need to wait until there's a default to override it. In fact, your patch is doing exactly that. common-defs.h is garanteed to be included before any started header. That's exactly why I suggested that spot. That's because the default is only defined if not already defined: /* Select Default WIN32_WINNT Value */ #if !defined(_WIN32_WINNT) && !defined(_CHICAGO_) #define _WIN32_WINNT _WIN32_WINNT_WS03 #endif This family of macros is usually defined on the command line / Makefile on Windows-only projects, even: $CC -D_WIN32_WINNT=xxx That's what MSVC/Visual Studio does. Or at least, used to last I used it, years ago. I did not suggest doing that because it's not as convenient on portable projects that don't just target Windows. > Overriding that > default is generally important only before including w32api headers, > such as windows.h. Agreed. But defining it really early just makes sure it's consistently set. Thanks, Pedro Alves
> Cc: lrn1986@gmail.com, gdb-patches@sourceware.org > From: Pedro Alves <palves@redhat.com> > Date: Tue, 30 Apr 2019 18:58:42 +0100 > > In fact, your patch is doing exactly that. common-defs.h is garanteed > to be included before any started header. That's exactly why I suggested > that spot. You were talking about the patch for master, whereas I was talking about the patch for the release branch.
On 4/30/19 7:34 PM, Eli Zaretskii wrote: >> Cc: lrn1986@gmail.com, gdb-patches@sourceware.org >> From: Pedro Alves <palves@redhat.com> >> Date: Tue, 30 Apr 2019 18:58:42 +0100 >> >> In fact, your patch is doing exactly that. common-defs.h is garanteed >> to be included before any started header. That's exactly why I suggested >> that spot. > > You were talking about the patch for master, whereas I was talking > about the patch for the release branch. Ah, indeed it seemed like we were kind of talking past each other. Still, my remarks apply likewise (we don't _have_ to define after the standard headers, before is fine and is the standard practice). But what I had in the 8.3 patch is fine as is, TBC. Thanks, Pedro Alves
> Cc: gdb-patches@sourceware.org > From: Pedro Alves <palves@redhat.com> > Date: Tue, 30 Apr 2019 17:31:22 +0100 > > On 4/30/19 4:23 PM, Eli Zaretskii wrote: > >> Cc: gdb-patches@sourceware.org > >> From: Pedro Alves <palves@redhat.com> > >> Date: Tue, 30 Apr 2019 14:10:53 +0100 > >> > >>> - The override in _WIN32_WINNT is no longer necessary. It could > >>> be removed at the same time. > >> > >> I meant the _WIN32_WINNT override in common/netstuff.c. > >> > >> I see now that there are more of those in other files too. > > > > I will review them for the changeset on master, but I don't think we > > should change them on the 8.3 branch. OK? > > Yes, my comments were for master, since changing removing the overrides > in the other files only makes sense with the _WIN32_WINNT definition > in common-defs.h. OK, done. I think I removed all the overrides that were there. Thanks.
> Cc: gdb-patches@sourceware.org > From: Pedro Alves <palves@redhat.com> > Date: Tue, 30 Apr 2019 13:56:52 +0100 > > - We could remove the dynamic loading of GetConsoleFontSize & friends > now, right? (for a separate patch/rainy day, not in this patch.) I will look at all the functions we load dynamically and see which could now be assumed available statically.
--- gdb/windows-nat.c~0 2019-03-27 00:52:05.000000000 +0200 +++ gdb/windows-nat.c 2019-04-28 11:00:38.415049400 +0300 @@ -34,6 +34,15 @@ #include <signal.h> #include <sys/types.h> #include <fcntl.h> +/* We need at least the level of XP for CONSOLE_FONT_INFO. */ +#ifdef _WIN32_WINNT +# if _WIN32_WINNT < 0x0501 +# undef _WIN32_WINNT +# define _WIN32_WINNT 0x0501 +# endif +#else +# define _WIN32_WINNT 0x0501 +#endif #include <windows.h> #include <imagehlp.h> #include <psapi.h> --- gdb/common/common-defs.h~0 2019-03-27 00:52:05.000000000 +0200 +++ gdb/common/common-defs.h 2019-04-28 11:26:11.785455800 +0300 @@ -72,6 +72,20 @@ #define _FORTIFY_SOURCE 2 #endif +/* We don't support Windows versions before XP, so we define + _WIN32_WINNT correspondingly to ensure the Windows API headers + expose the required symbols. */ +#ifdef __MINGW32__ +# ifdef _WIN32_WINNT +# if _WIN32_WINNT < 0x0501 +# undef _WIN32_WINNT +# define _WIN32_WINNT 0x0501 +# endif +# else +# define _WIN32_WINNT 0x0501 +# endif +#endif /* __MINGW32__ */ + #include <stdarg.h> #include <stdio.h> #include <stdlib.h>