Patchwork Conditionally include "<windows.h>" on common/pathstuff.c (and unbreak build on mingw*)

login
register
mail settings
Submitter Sergio Durigan Junior
Date March 1, 2018, 8:20 p.m.
Message ID <20180301202005.11563-1-sergiodj@redhat.com>
Download mbox | patch
Permalink /patch/26144/
State New
Headers show

Comments

Sergio Durigan Junior - March 1, 2018, 8:20 p.m.
commit b4987c956dfa44ca9fd8552f63e15f5fa094b2a4
Author: Sergio Durigan Junior <sergiodj@redhat.com>
Date:   Fri Feb 9 18:44:59 2018 -0500

    Create new common/pathstuff.[ch]

Introduced a regression when compiling for mingw*:

  /gdb/common/pathstuff.c: In function 'gdb::unique_xmalloc_ptr<char>
  gdb_realpath(const char*)':
  /gdb/common/pathstuff.c:56:14: error: 'MAX_PATH' was not declared in this scope
       char buf[MAX_PATH];
		^
  /gdb/common/pathstuff.c:57:5: error: 'DWORD' was not declared in this scope
       DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
       ^
  /gdb/common/pathstuff.c:57:11: error: expected ';' before 'len'
       DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
	     ^
  /gdb/common/pathstuff.c:63:9: error: 'len' was not declared in this scope
       if (len > 0 && len < MAX_PATH)
	   ^
  /gdb/common/pathstuff.c:64:54: error: 'buf' was not declared in this scope
	 return gdb::unique_xmalloc_ptr<char> (xstrdup (buf));
							^
  make[2]: *** [pathstuff.o] Error 1

The proper fix is to conditionally include "<windows.h>".  This commit
does that, without introducing any regressions as per tests made by
our BuildBot.

gdb/ChangeLog:
2018-03-01  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR gdb/22907
	* common/pathstuff.c: Conditionally include "<windows.h>".
---
 gdb/common/pathstuff.c | 4 ++++
 1 file changed, 4 insertions(+)
Simon Marchi - March 1, 2018, 8:46 p.m.
On 2018-03-01 15:20, Sergio Durigan Junior wrote:
> commit b4987c956dfa44ca9fd8552f63e15f5fa094b2a4
> Author: Sergio Durigan Junior <sergiodj@redhat.com>
> Date:   Fri Feb 9 18:44:59 2018 -0500
> 
>     Create new common/pathstuff.[ch]
> 
> Introduced a regression when compiling for mingw*:
> 
>   /gdb/common/pathstuff.c: In function 'gdb::unique_xmalloc_ptr<char>
>   gdb_realpath(const char*)':
>   /gdb/common/pathstuff.c:56:14: error: 'MAX_PATH' was not declared in
> this scope
>        char buf[MAX_PATH];
> 		^
>   /gdb/common/pathstuff.c:57:5: error: 'DWORD' was not declared in this 
> scope
>        DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
>        ^
>   /gdb/common/pathstuff.c:57:11: error: expected ';' before 'len'
>        DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
> 	     ^
>   /gdb/common/pathstuff.c:63:9: error: 'len' was not declared in this 
> scope
>        if (len > 0 && len < MAX_PATH)
> 	   ^
>   /gdb/common/pathstuff.c:64:54: error: 'buf' was not declared in this 
> scope
> 	 return gdb::unique_xmalloc_ptr<char> (xstrdup (buf));
> 							^
>   make[2]: *** [pathstuff.o] Error 1
> 
> The proper fix is to conditionally include "<windows.h>".  This commit
> does that, without introducing any regressions as per tests made by
> our BuildBot.
> 
> gdb/ChangeLog:
> 2018-03-01  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	PR gdb/22907
> 	* common/pathstuff.c: Conditionally include "<windows.h>".
> ---
>  gdb/common/pathstuff.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c
> index fc574dc32e..8c4093fc38 100644
> --- a/gdb/common/pathstuff.c
> +++ b/gdb/common/pathstuff.c
> @@ -23,6 +23,10 @@
>  #include "filenames.h"
>  #include "gdb_tilde_expand.h"
> 
> +#ifdef USE_WIN32API
> +#include <windows.h>
> +#endif
> +
>  /* See common/pathstuff.h.  */
> 
>  gdb::unique_xmalloc_ptr<char>

Christopher, does that fix the issue for you?  If so, the patch LGTM.

Simon
Yao Qi - March 2, 2018, 11:11 a.m.
Sergio Durigan Junior <sergiodj@redhat.com> writes:

> Introduced a regression when compiling for mingw*:
>
>   /gdb/common/pathstuff.c: In function 'gdb::unique_xmalloc_ptr<char>
>   gdb_realpath(const char*)':
>   /gdb/common/pathstuff.c:56:14: error: 'MAX_PATH' was not declared in this scope
>        char buf[MAX_PATH];

Fedora-x86_64-w64-mingw32 catches this build failure
https://gdb-build.sergiodj.net/builders/Fedora-x86_64-w64-mingw32/builds/1113/steps/compile%20gdb/logs/stdio
but it doesn't send notification on build failure.  Could you change the
buildbot config to send notification on build failure?  The same as
other builders do.
Christophe Lyon - March 2, 2018, 11:46 a.m.
On 1 March 2018 at 21:46, Simon Marchi <simon.marchi@polymtl.ca> wrote:
> On 2018-03-01 15:20, Sergio Durigan Junior wrote:
>>
>> commit b4987c956dfa44ca9fd8552f63e15f5fa094b2a4
>> Author: Sergio Durigan Junior <sergiodj@redhat.com>
>> Date:   Fri Feb 9 18:44:59 2018 -0500
>>
>>     Create new common/pathstuff.[ch]
>>
>> Introduced a regression when compiling for mingw*:
>>
>>   /gdb/common/pathstuff.c: In function 'gdb::unique_xmalloc_ptr<char>
>>   gdb_realpath(const char*)':
>>   /gdb/common/pathstuff.c:56:14: error: 'MAX_PATH' was not declared in
>> this scope
>>        char buf[MAX_PATH];
>>                 ^
>>   /gdb/common/pathstuff.c:57:5: error: 'DWORD' was not declared in this
>> scope
>>        DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
>>        ^
>>   /gdb/common/pathstuff.c:57:11: error: expected ';' before 'len'
>>        DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
>>              ^
>>   /gdb/common/pathstuff.c:63:9: error: 'len' was not declared in this
>> scope
>>        if (len > 0 && len < MAX_PATH)
>>            ^
>>   /gdb/common/pathstuff.c:64:54: error: 'buf' was not declared in this
>> scope
>>          return gdb::unique_xmalloc_ptr<char> (xstrdup (buf));
>>                                                         ^
>>   make[2]: *** [pathstuff.o] Error 1
>>
>> The proper fix is to conditionally include "<windows.h>".  This commit
>> does that, without introducing any regressions as per tests made by
>> our BuildBot.
>>
>> gdb/ChangeLog:
>> 2018-03-01  Sergio Durigan Junior  <sergiodj@redhat.com>
>>
>>         PR gdb/22907
>>         * common/pathstuff.c: Conditionally include "<windows.h>".
>> ---
>>  gdb/common/pathstuff.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c
>> index fc574dc32e..8c4093fc38 100644
>> --- a/gdb/common/pathstuff.c
>> +++ b/gdb/common/pathstuff.c
>> @@ -23,6 +23,10 @@
>>  #include "filenames.h"
>>  #include "gdb_tilde_expand.h"
>>
>> +#ifdef USE_WIN32API
>> +#include <windows.h>
>> +#endif
>> +
>>  /* See common/pathstuff.h.  */
>>
>>  gdb::unique_xmalloc_ptr<char>
>
>
> Christopher, does that fix the issue for you?  If so, the patch LGTM.
>

Yes, I applied it to my gdb-8.1-branch , and the build now succeeds.
Thanks

> Simon
Sergio Durigan Junior - March 2, 2018, 12:28 p.m.
On Friday, March 02 2018, Yao Qi wrote:

> Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
>> Introduced a regression when compiling for mingw*:
>>
>>   /gdb/common/pathstuff.c: In function 'gdb::unique_xmalloc_ptr<char>
>>   gdb_realpath(const char*)':
>>   /gdb/common/pathstuff.c:56:14: error: 'MAX_PATH' was not declared in this scope
>>        char buf[MAX_PATH];
>
> Fedora-x86_64-w64-mingw32 catches this build failure
> https://gdb-build.sergiodj.net/builders/Fedora-x86_64-w64-mingw32/builds/1113/steps/compile%20gdb/logs/stdio
> but it doesn't send notification on build failure.  Could you change the
> buildbot config to send notification on build failure?  The same as
> other builders do.

Yeah, it's on my TODO list.  I've also noticed this after Christophe
reported the failure.  Thanks, Yao.
Sergio Durigan Junior - March 2, 2018, 12:34 p.m.
On Friday, March 02 2018, Christophe Lyon wrote:

> On 1 March 2018 at 21:46, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>> Christopher, does that fix the issue for you?  If so, the patch LGTM.
>>
>
> Yes, I applied it to my gdb-8.1-branch , and the build now succeeds.
> Thanks

Thanks, Christophe and Simon.

Pushed to master:

ab818ade016bcd794980438775e15c7a74f054f9

And to the 8.1 branch:

a74e30b243dec8b38c8c769da04635210f9ef986
Sergio Durigan Junior - March 2, 2018, 12:37 p.m.
On Friday, March 02 2018, I wrote:

> On Friday, March 02 2018, Yao Qi wrote:
>
>> Sergio Durigan Junior <sergiodj@redhat.com> writes:
>>
>>> Introduced a regression when compiling for mingw*:
>>>
>>>   /gdb/common/pathstuff.c: In function 'gdb::unique_xmalloc_ptr<char>
>>>   gdb_realpath(const char*)':
>>>   /gdb/common/pathstuff.c:56:14: error: 'MAX_PATH' was not declared in this scope
>>>        char buf[MAX_PATH];
>>
>> Fedora-x86_64-w64-mingw32 catches this build failure
>> https://gdb-build.sergiodj.net/builders/Fedora-x86_64-w64-mingw32/builds/1113/steps/compile%20gdb/logs/stdio
>> but it doesn't send notification on build failure.  Could you change the
>> buildbot config to send notification on build failure?  The same as
>> other builders do.
>
> Yeah, it's on my TODO list.  I've also noticed this after Christophe
> reported the failure.  Thanks, Yao.

E-mail notifications enabled for this builder.

Thanks,
Eli Zaretskii - March 2, 2018, 1:32 p.m.
> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Simon Marchi <simon.marchi@ericsson.com>,	Pedro Alves <palves@redhat.com>,	Joel Brobecker <brobecker@adacore.com>,	Christophe Lyon <christophe.lyon@linaro.org>,	Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Thu,  1 Mar 2018 15:20:05 -0500
> 
> +#ifdef USE_WIN32API
> +#include <windows.h>
> +#endif

Isn't USE_WIN32API specific to gdbserver?  If so, perhaps it's better
to use __MINGW32__ or _WIN32 instead?
Simon Marchi - March 2, 2018, 3:06 p.m.
On 2018-03-02 08:32, Eli Zaretskii wrote:
>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Cc: Simon Marchi <simon.marchi@ericsson.com>,	Pedro Alves 
>> <palves@redhat.com>,	Joel Brobecker 
>> <brobecker@adacore.com>,	Christophe Lyon 
>> <christophe.lyon@linaro.org>,	Sergio Durigan Junior 
>> <sergiodj@redhat.com>
>> Date: Thu,  1 Mar 2018 15:20:05 -0500
>> 
>> +#ifdef USE_WIN32API
>> +#include <windows.h>
>> +#endif
> 
> Isn't USE_WIN32API specific to gdbserver?  If so, perhaps it's better
> to use __MINGW32__ or _WIN32 instead?

I see it defined in both gdb and gdbserver:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/configure.ac;h=698fc7b83456f8c5a63ae0050dc8ec65069290f7;hb=HEAD#l1908
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/gdbserver/configure.ac;h=99801681ff47ee8dcd9ad2e5ae282dcd113c83e4;hb=HEAD#l281

Simon
Sergio Durigan Junior - March 2, 2018, 6:20 p.m.
On Friday, March 02 2018, Simon Marchi wrote:

> On 2018-03-02 08:32, Eli Zaretskii wrote:
>>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>>> Cc: Simon Marchi <simon.marchi@ericsson.com>,	Pedro Alves
>>> <palves@redhat.com>,	Joel Brobecker <brobecker@adacore.com>,
>>> Christophe Lyon <christophe.lyon@linaro.org>,	Sergio Durigan
>>> Junior <sergiodj@redhat.com>
>>> Date: Thu,  1 Mar 2018 15:20:05 -0500
>>>
>>> +#ifdef USE_WIN32API
>>> +#include <windows.h>
>>> +#endif
>>
>> Isn't USE_WIN32API specific to gdbserver?  If so, perhaps it's better
>> to use __MINGW32__ or _WIN32 instead?
>
> I see it defined in both gdb and gdbserver:
>
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/configure.ac;h=698fc7b83456f8c5a63ae0050dc8ec65069290f7;hb=HEAD#l1908
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/gdbserver/configure.ac;h=99801681ff47ee8dcd9ad2e5ae282dcd113c83e4;hb=HEAD#l281

It's also present on common/filestuff.c, which is shared.  That's
actually where I took the idea from.
Eli Zaretskii - March 3, 2018, 7:36 a.m.
> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  gdb-patches@sourceware.org,  simon.marchi@ericsson.com,  palves@redhat.com,  brobecker@adacore.com,  christophe.lyon@linaro.org
> Date: Fri, 02 Mar 2018 13:20:38 -0500
> 
> >>> +#ifdef USE_WIN32API
> >>> +#include <windows.h>
> >>> +#endif
> >>
> >> Isn't USE_WIN32API specific to gdbserver?  If so, perhaps it's better
> >> to use __MINGW32__ or _WIN32 instead?
> >
> > I see it defined in both gdb and gdbserver:
> >
> > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/configure.ac;h=698fc7b83456f8c5a63ae0050dc8ec65069290f7;hb=HEAD#l1908
> > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/gdbserver/configure.ac;h=99801681ff47ee8dcd9ad2e5ae282dcd113c83e4;hb=HEAD#l281
> 
> It's also present on common/filestuff.c, which is shared.  That's
> actually where I took the idea from.

Those could be mistakes due to moving stuff into common/.  So I think
we should decide what that symbol means and how to use it, and then
see if the practice somehow deviates from those rules.

I asked my question because AFAIR this symbol used to be only in
gdbserver sources.  If nowadays we can use it anywhere, then I'd
suggest to replace it with something common with GDB, because the
meaning of USE_WIN32API in the context of GDB is unclear to me.
Yao Qi - March 5, 2018, 12:07 p.m.
Sergio Durigan Junior <sergiodj@redhat.com> writes:

> E-mail notifications enabled for this builder.

Thank you, Sergio.

Patch

diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c
index fc574dc32e..8c4093fc38 100644
--- a/gdb/common/pathstuff.c
+++ b/gdb/common/pathstuff.c
@@ -23,6 +23,10 @@ 
 #include "filenames.h"
 #include "gdb_tilde_expand.h"
 
+#ifdef USE_WIN32API
+#include <windows.h>
+#endif
+
 /* See common/pathstuff.h.  */
 
 gdb::unique_xmalloc_ptr<char>