diff mbox

Fix compilation using mingw.org's MinGW

Message ID 835zrbe36c.fsf@gnu.org
State New, archived
Headers

Commit Message

Eli Zaretskii April 18, 2019, 3:36 p.m. UTC 
  windows-nat.c uses CONSOLE_FONT_INFO unconditionally, but the
corresponding declaration in the Windows API headers is conditional,
because the APIs using CONSOLE_FONT_INFO are only available since
Windows XP.  When that condition doesn't hold, windows-nat.c won't
compile:

     windows-nat.c:114:10: error: 'CONSOLE_FONT_INFO' has not been declared
	       CONSOLE_FONT_INFO *);
	       ^~~~~~~~~~~~~~~~~
     windows-nat.c: In function 'void windows_set_console_info(STARTUPINFOA*, DWORD*)':
     windows-nat.c:2174:7: error: 'CONSOLE_FONT_INFO' was not declared in this scope
	    CONSOLE_FONT_INFO cfi;
	    ^~~~~~~~~~~~~~~~~
     windows-nat.c:2174:7: note: suggested alternative: 'CONSOLE_CURSOR_INFO'
	    CONSOLE_FONT_INFO cfi;
	    ^~~~~~~~~~~~~~~~~
	    CONSOLE_CURSOR_INFO
     windows-nat.c:2176:48: error: 'cfi' was not declared in this scope
	    GetCurrentConsoleFont (hconsole, FALSE, &cfi);
						     ^~~
     windows-nat.c: At global scope:
     windows-nat.c:3302:55: error: 'CONSOLE_FONT_INFO' has not been declared
      bad_GetCurrentConsoleFont (HANDLE w, BOOL bMaxWindow, CONSOLE_FONT_INFO *f)
							    ^~~~~~~~~~~~~~~~~
     windows-nat.c: In function 'BOOL bad_GetCurrentConsoleFont(HANDLE, BOOL, int*)':

     windows-nat.c:3304:6: error: request for member 'nFont' in '* f', which is of non-class type 'int'
	f->nFont = 0;
	   ^~~~~

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

Comments

Kevin Buettner April 18, 2019, 5 p.m. UTC | #1 
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
Pedro Alves April 18, 2019, 5:20 p.m. UTC | #2 
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
Pedro Alves April 18, 2019, 5:22 p.m. UTC | #3 
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
Eli Zaretskii April 18, 2019, 6:54 p.m. UTC | #4 
> 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 ;-)
Eli Zaretskii April 18, 2019, 6:56 p.m. UTC | #5 
> 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?
Kevin Buettner April 18, 2019, 8:38 p.m. UTC | #6 
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
Eli Zaretskii April 19, 2019, 6:18 a.m. UTC | #7 
> 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.
Pedro Alves April 19, 2019, 10:51 a.m. UTC | #8 
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
Eli Zaretskii April 19, 2019, 11:23 a.m. UTC | #9 
> 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?
Pedro Alves April 19, 2019, 11:33 a.m. UTC | #10 
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
diff mbox

Patch

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