Message ID | 835zrbe36c.fsf@gnu.org |
---|---|
State | New |
Headers | show |
On Thu, 18 Apr 2019 18:36:11 +0300 Eli Zaretskii <eliz@gnu.org> wrote: > --- gdb/windows-nat.c~0 2019-03-27 00:52:05.000000000 +0200 > +++ gdb/windows-nat.c 2019-04-18 11:37:02.061945600 +0300 > @@ -81,6 +81,16 @@ > #define GetConsoleFontSize dyn_GetConsoleFontSize > #define GetCurrentConsoleFont dyn_GetCurrentConsoleFont > > +#if _WIN32_WINNT < 0x0501 > + > +typedef > +struct _CONSOLE_FONT_INFO > +{ DWORD nFont; > + COORD dwFontSize; > +} CONSOLE_FONT_INFO, *PCONSOLE_FONT_INFO; > + > +#endif > + > typedef BOOL WINAPI (AdjustTokenPrivileges_ftype) (HANDLE, BOOL, > PTOKEN_PRIVILEGES, > DWORD, PTOKEN_PRIVILEGES, I had expected to see a typedef for struct CONSOLE_FONT_INFO, but instead see one for _CONSOLE_FONT_INFO (where the difference is the leading underscore.) I'm guessing that there's some magic in Windows specific header files which makes this work, but I just want to check to make sure that this wasn't a typo on your part... Kevin
On 4/18/19 4:36 PM, Eli Zaretskii wrote: > MinGW64 no longer supports versions of Windows older than XP, but > mingw.org's MinGW does. Since we load GetCurrentConsoleFont > dynamically at run time, and have code for when the load fails, it > makes sense to not assume XP at compile time. The patch below does > that. OK to push (with the pertinent ChangeLog entry)? Didn't we drop support for < XP some time ago? Thanks, Pedro Alves
On 4/18/19 6:20 PM, Pedro Alves wrote: > On 4/18/19 4:36 PM, Eli Zaretskii wrote: > >> MinGW64 no longer supports versions of Windows older than XP, but >> mingw.org's MinGW does. Since we load GetCurrentConsoleFont >> dynamically at run time, and have code for when the load fails, it >> makes sense to not assume XP at compile time. The patch below does >> that. OK to push (with the pertinent ChangeLog entry)? > > Didn't we drop support for < XP some time ago? > From gdb/NEWS: ~~~~~~~~~~~~ * Removed targets GDB no longer supports native debugging on versions of MS-Windows before Windows XP. ~~~~~~~~~~~~ So shouldn't we instead be setting _WIN32_WINNT to some appropriate number? Thanks, Pedro Alves
> Date: Thu, 18 Apr 2019 10:00:58 -0700 > From: Kevin Buettner <kevinb@redhat.com> > Cc: gdb-patches@sourceware.org > > > +#if _WIN32_WINNT < 0x0501 > > + > > +typedef > > +struct _CONSOLE_FONT_INFO > > +{ DWORD nFont; > > + COORD dwFontSize; > > +} CONSOLE_FONT_INFO, *PCONSOLE_FONT_INFO; > > + > > +#endif > > + > > typedef BOOL WINAPI (AdjustTokenPrivileges_ftype) (HANDLE, BOOL, > > PTOKEN_PRIVILEGES, > > DWORD, PTOKEN_PRIVILEGES, > > I had expected to see a typedef for struct CONSOLE_FONT_INFO, but > instead see one for _CONSOLE_FONT_INFO (where the difference is > the leading underscore.) The struct is with the leading underscore, the typedef isn't. > I'm guessing that there's some magic in Windows specific header files > which makes this work, but I just want to check to make sure that this > wasn't a typo on your part... It wasn't a typo, and there is no magic ;-)
> From: Pedro Alves <palves@redhat.com> > Date: Thu, 18 Apr 2019 18:22:14 +0100 > > * Removed targets > > GDB no longer supports native debugging on versions of MS-Windows > before Windows XP. I was talking about compile time, not run time. In any case, if we don't support systems older than XP, then why do we load those functions dynamically at run time and call them via a function pointer? > So shouldn't we instead be setting _WIN32_WINNT to some > appropriate number? I don't mind, but where? And also: should we make such changes on the 8.3 branch at this time?
On Thu, 18 Apr 2019 21:54:05 +0300 Eli Zaretskii <eliz@gnu.org> wrote: > > Date: Thu, 18 Apr 2019 10:00:58 -0700 > > From: Kevin Buettner <kevinb@redhat.com> > > Cc: gdb-patches@sourceware.org > > > > > +#if _WIN32_WINNT < 0x0501 > > > + > > > +typedef > > > +struct _CONSOLE_FONT_INFO > > > +{ DWORD nFont; > > > + COORD dwFontSize; > > > +} CONSOLE_FONT_INFO, *PCONSOLE_FONT_INFO; > > > + > > > +#endif > > > + > > > typedef BOOL WINAPI (AdjustTokenPrivileges_ftype) (HANDLE, BOOL, > > > PTOKEN_PRIVILEGES, > > > DWORD, PTOKEN_PRIVILEGES, > > > > I had expected to see a typedef for struct CONSOLE_FONT_INFO, but > > instead see one for _CONSOLE_FONT_INFO (where the difference is > > the leading underscore.) > > The struct is with the leading underscore, the typedef isn't. > > > I'm guessing that there's some magic in Windows specific header files > > which makes this work, but I just want to check to make sure that this > > wasn't a typo on your part... > > It wasn't a typo, and there is no magic ;-) Apologies - I clearly wasn't awake enough when I looked at your patch. So... no objections from me regarding this patch. Kevin
> Date: Thu, 18 Apr 2019 13:38:46 -0700 > From: Kevin Buettner <kevinb@redhat.com> > Cc: gdb-patches@sourceware.org > > > It wasn't a typo, and there is no magic ;-) > > Apologies - I clearly wasn't awake enough when I looked at your patch. No sweat. Thanks for reviewing the patch.
On 4/18/19 7:56 PM, Eli Zaretskii wrote: >> From: Pedro Alves <palves@redhat.com> >> Date: Thu, 18 Apr 2019 18:22:14 +0100 >> >> * Removed targets >> >> GDB no longer supports native debugging on versions of MS-Windows >> before Windows XP. > > I was talking about compile time, not run time. Not sure it makes any practical difference. From the text above, you could claim that we wanted to support hosting GDB on older versions of Windows, but not support native debugging, say, so you could still remote debug from such older Windows boxes. But was that the intention? I'd say keep it simple -- if we don't support native debugging, then we don't support building at all either. The subject for the commit that added that NEWS entry sound like that was the intention: commit 742a7df5f4a149f0818aaccfc432c4c0c9a6e26d Author: Eli Zaretskii <eliz@gnu.org> AuthorDate: Sat Mar 2 15:18:32 2019 +0200 GDB no longer supports Windows before XP. But surprisingly, I can't find the discussion behind this commit in the archives to see the context and what was decided. > > In any case, if we don't support systems older than XP, then why do we > load those functions dynamically at run time and call them via a > function pointer? Because that code is older than the decision to stop supporting older Windows versions, surely? : ChangeLog-2010: * windows-nat.c (GetConsoleFontSize, GetCurrentConsoleFont): ChangeLog-2010: (bad_GetCurrentConsoleFont, bad_GetConsoleFontSize): New functions. ChangeLog-2010: GetCurrentConsoleFont. ChangeLog-2015: (GetCurrentConsoleFont_ftype, GetModuleInformation_ftype) > >> So shouldn't we instead be setting _WIN32_WINNT to some >> appropriate number? > > I don't mind, but where? I'd do it in common/common-defs.h, before any #include, where we define other macros that must be defined before any include, like __STDC_LIMIT_MACROS, _FORTIFY_SOURCE, etc. > And also: should we make such changes on the > 8.3 branch at this time? Not sure. Off hand I'd think it's pretty safe, but maybe for 8.3 your patch is safer. Thanks, Pedro Alves
> Cc: gdb-patches@sourceware.org > From: Pedro Alves <palves@redhat.com> > Date: Fri, 19 Apr 2019 11:51:46 +0100 > > building at all either. The subject for the commit that added that NEWS entry > sound like that was the intention: > > commit 742a7df5f4a149f0818aaccfc432c4c0c9a6e26d > Author: Eli Zaretskii <eliz@gnu.org> > AuthorDate: Sat Mar 2 15:18:32 2019 +0200 > > GDB no longer supports Windows before XP. > > But surprisingly, I can't find the discussion behind this commit > in the archives to see the context and what was decided. The discussion which led to that change is here: https://www.sourceware.org/ml/gdb-patches/2019-02/msg00574.html > >> So shouldn't we instead be setting _WIN32_WINNT to some > >> appropriate number? > > > > I don't mind, but where? > > I'd do it in common/common-defs.h, before any #include, where we define > other macros that must be defined before any include, like > __STDC_LIMIT_MACROS, _FORTIFY_SOURCE, etc. > > > And also: should we make such changes on the > > 8.3 branch at this time? > > Not sure. Off hand I'd think it's pretty safe, but maybe for 8.3 > your patch is safer. 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?
On 4/19/19 12:23 PM, Eli Zaretskii wrote: >> Cc: gdb-patches@sourceware.org >> From: Pedro Alves <palves@redhat.com> >> Date: Fri, 19 Apr 2019 11:51:46 +0100 >> >> building at all either. The subject for the commit that added that NEWS entry >> sound like that was the intention: >> >> commit 742a7df5f4a149f0818aaccfc432c4c0c9a6e26d >> Author: Eli Zaretskii <eliz@gnu.org> >> AuthorDate: Sat Mar 2 15:18:32 2019 +0200 >> >> GDB no longer supports Windows before XP. >> >> But surprisingly, I can't find the discussion behind this commit >> in the archives to see the context and what was decided. > > The discussion which led to that change is here: > > https://www.sourceware.org/ml/gdb-patches/2019-02/msg00574.html > So we're already assuming XP at build time in common code, in common/netstuff.c. >>>> So shouldn't we instead be setting _WIN32_WINNT to some >>>> appropriate number? >>> >>> I don't mind, but where? >> >> I'd do it in common/common-defs.h, before any #include, where we define >> other macros that must be defined before any include, like >> __STDC_LIMIT_MACROS, _FORTIFY_SOURCE, etc. >> >>> And also: should we make such changes on the >>> 8.3 branch at this time? >> >> Not sure. Off hand I'd think it's pretty safe, but maybe for 8.3 >> your patch is safer. > > 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. Thanks, Pedro Alves
--- gdb/windows-nat.c~0 2019-03-27 00:52:05.000000000 +0200 +++ gdb/windows-nat.c 2019-04-18 11:37:02.061945600 +0300 @@ -81,6 +81,16 @@ #define GetConsoleFontSize dyn_GetConsoleFontSize #define GetCurrentConsoleFont dyn_GetCurrentConsoleFont +#if _WIN32_WINNT < 0x0501 + +typedef +struct _CONSOLE_FONT_INFO +{ DWORD nFont; + COORD dwFontSize; +} CONSOLE_FONT_INFO, *PCONSOLE_FONT_INFO; + +#endif + typedef BOOL WINAPI (AdjustTokenPrivileges_ftype) (HANDLE, BOOL, PTOKEN_PRIVILEGES, DWORD, PTOKEN_PRIVILEGES,