Compiler warnings with MinGW

Started by Michael Paquierover 6 years ago9 messages

michael@paquier.xyz

over 6 years ago

1 attachment(s)

Hi all,

Just browsing through the logs of the buildfarm, I have noticed that
some buildfarm animals complain with warnings (jacana uses MinGW):
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=jacana&dt=2019-07-19%2001%3A45%3A28&stg=make

There are two of them:
c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.build/../pgsql/src/backend/port/win32/mingwcompat.c:60:1:
warning: 'RegisterWaitForSingleObject' redeclared without dllimport
attribute: previous dllimport ignored [-Wattributes]

c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.build/../pgsql/src/bin/pg_basebackup/pg_basebackup.c:1448:8:
warning: variable 'filemode' set but not used
[-Wunused-but-set-variable]
Jul 18 21:59:49 int filemode;

The first one has been discussed already some time ago and is a cause
of 811be893, but nothing got actually fixed (protagonists in CC):
/messages/by-id/CABUevEyeZfUvaYMuNop3NyRvvRh2Up2tStK8SXVAPDERf8p9eg@mail.gmail.com

The second one is rather obvious to fix, because we don't care about
the file mode on Windows, so the attached should do the work. I am
actually surprised that the Visual Studio compilers don't complain
about that, but let's fix it.

Thoughts?
--
Michael

Attachments:

mingw-warning.patchtext/x-diff; charset=us-asciiDownload

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 15f43f9432..77a7c148ba 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1445,7 +1445,9 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum)
 
 		if (file == NULL)
 		{
+#ifndef WIN32
 			int			filemode;
+#endif
 
 			/*
 			 * No current file, so this must be the header for a new file
@@ -1459,8 +1461,10 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum)
 
 			current_len_left = read_tar_number(&copybuf[124], 12);
 
+#ifndef WIN32
 			/* Set permissions on the file */
 			filemode = read_tar_number(&copybuf[100], 8);
+#endif
 
 			/*
 			 * All files are padded up to 512 bytes

Andrew Dunstan

andrew.dunstan@2ndquadrant.com

over 6 years ago

In reply to: Michael Paquier (#1)

Re: Compiler warnings with MinGW

On 7/19/19 1:08 AM, Michael Paquier wrote:

Hi all,

Just browsing through the logs of the buildfarm, I have noticed that
some buildfarm animals complain with warnings (jacana uses MinGW):
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=jacana&dt=2019-07-19%2001%3A45%3A28&stg=make

There are two of them:
c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.build/../pgsql/src/backend/port/win32/mingwcompat.c:60:1:
warning: 'RegisterWaitForSingleObject' redeclared without dllimport
attribute: previous dllimport ignored [-Wattributes]

c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.build/../pgsql/src/bin/pg_basebackup/pg_basebackup.c:1448:8:
warning: variable 'filemode' set but not used
[-Wunused-but-set-variable]
Jul 18 21:59:49 int filemode;

The first one has been discussed already some time ago and is a cause
of 811be893, but nothing got actually fixed (protagonists in CC):
/messages/by-id/CABUevEyeZfUvaYMuNop3NyRvvRh2Up2tStK8SXVAPDERf8p9eg@mail.gmail.com

To answer Magnus' question in that thread, the Mingw headers on jacana
declare the function with WINBASEAPI which in turn is defined as
DECLSPEC_IMPORT, as long as _KERNEL32_ isn't defined, and we don't do
that (and I don't think anything else does either).

So the fix Peter proposed looks like it should be correct.

The second one is rather obvious to fix, because we don't care about
the file mode on Windows, so the attached should do the work. I am
actually surprised that the Visual Studio compilers don't complain
about that, but let's fix it.

Thoughts?

+1.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Michael Paquier

michael@paquier.xyz

over 6 years ago

In reply to: Andrew Dunstan (#2)

Re: Compiler warnings with MinGW

On Fri, Jul 19, 2019 at 08:41:28AM -0400, Andrew Dunstan wrote:

On 7/19/19 1:08 AM, Michael Paquier wrote:

The second one is rather obvious to fix, because we don't care about
the file mode on Windows, so the attached should do the work. I am
actually surprised that the Visual Studio compilers don't complain
about that, but let's fix it.

Thoughts?

+1.

Just wanted to double-check something. We usually don't bother
back-patching warning fixes like this one, right?
--
Michael

Michael Paquier

michael@paquier.xyz

over 6 years ago

In reply to: Michael Paquier (#3)

Re: Compiler warnings with MinGW

On Sat, Jul 20, 2019 at 06:19:34PM +0900, Michael Paquier wrote:

Just wanted to double-check something. We usually don't bother
back-patching warning fixes like this one, right?

I have double-checked the thing, and applied it only on HEAD as we
have that for some time (since 9.1 actually and 00cdd83 has improved
the original situation here).
--
Michael

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

over 6 years ago

In reply to: Andrew Dunstan (#2)

Re: Compiler warnings with MinGW

On 2019-07-19 14:41, Andrew Dunstan wrote:

On 7/19/19 1:08 AM, Michael Paquier wrote:

Just browsing through the logs of the buildfarm, I have noticed that
some buildfarm animals complain with warnings (jacana uses MinGW):
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=jacana&dt=2019-07-19%2001%3A45%3A28&stg=make

There are two of them:
c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.build/../pgsql/src/backend/port/win32/mingwcompat.c:60:1:
warning: 'RegisterWaitForSingleObject' redeclared without dllimport
attribute: previous dllimport ignored [-Wattributes]

The first one has been discussed already some time ago and is a cause
of 811be893, but nothing got actually fixed (protagonists in CC):
/messages/by-id/CABUevEyeZfUvaYMuNop3NyRvvRh2Up2tStK8SXVAPDERf8p9eg@mail.gmail.com

To answer Magnus' question in that thread, the Mingw headers on jacana
declare the function with WINBASEAPI which in turn is defined as
DECLSPEC_IMPORT, as long as _KERNEL32_ isn't defined, and we don't do
that (and I don't think anything else does either).

So the fix Peter proposed looks like it should be correct.

I'm not sure exactly what the upstream of mingw is these days, but I
think the original issue that led to 811be893 has long been fixed [0]https://sourceforge.net/p/mingw-w64/mingw-w64/ci/9d937a7f4f766f903c9433044f77bfa97a0bc1d8/,
and the other stuff in mingwcompat.c is also no longer relevant [1]https://sourceforge.net/p/mingw-w64/mingw-w64/ci/88ab6fbdd0a185702a1fce4db935e303030e082f/. I
think mingwcompat.c could be removed altogether. I'm not sure to what
extent we need to support 5+ year old mingw versions.

[0]: https://sourceforge.net/p/mingw-w64/mingw-w64/ci/9d937a7f4f766f903c9433044f77bfa97a0bc1d8/
https://sourceforge.net/p/mingw-w64/mingw-w64/ci/9d937a7f4f766f903c9433044f77bfa97a0bc1d8/
[1]: https://sourceforge.net/p/mingw-w64/mingw-w64/ci/88ab6fbdd0a185702a1fce4db935e303030e082f/
https://sourceforge.net/p/mingw-w64/mingw-w64/ci/88ab6fbdd0a185702a1fce4db935e303030e082f/

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Michael Paquier

michael@paquier.xyz

over 6 years ago

In reply to: Peter Eisentraut (#5)

Re: Compiler warnings with MinGW

On Sat, Sep 07, 2019 at 12:11:25AM +0200, Peter Eisentraut wrote:

I'm not sure exactly what the upstream of mingw is these days, but I
think the original issue that led to 811be893 has long been fixed [0],
and the other stuff in mingwcompat.c is also no longer relevant [1]. I
think mingwcompat.c could be removed altogether. I'm not sure to what
extent we need to support 5+ year old mingw versions.

On HEAD I would not be against removing that as this leads to a
cleanup of our code. For MSVC, we only support VS 2013~ on HEAD, so
saying that we don't support MinGW older than what was proposed 5
years ago sounds sensible.
--
Michael

Magnus Hagander

magnus@hagander.net

over 6 years ago

In reply to: Michael Paquier (#6)

Re: Compiler warnings with MinGW

On Sat, Sep 7, 2019 at 4:58 AM Michael Paquier <michael@paquier.xyz> wrote:

On Sat, Sep 07, 2019 at 12:11:25AM +0200, Peter Eisentraut wrote:

I'm not sure exactly what the upstream of mingw is these days, but I
think the original issue that led to 811be893 has long been fixed [0],
and the other stuff in mingwcompat.c is also no longer relevant [1]. I
think mingwcompat.c could be removed altogether. I'm not sure to what
extent we need to support 5+ year old mingw versions.

On HEAD I would not be against removing that as this leads to a
cleanup of our code. For MSVC, we only support VS 2013~ on HEAD, so
saying that we don't support MinGW older than what was proposed 5
years ago sounds sensible.

+1, definitely.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

over 6 years ago

In reply to: Magnus Hagander (#7)

Re: Compiler warnings with MinGW

On 2019-09-09 14:24, Magnus Hagander wrote:

On Sat, Sep 7, 2019 at 4:58 AM Michael Paquier <michael@paquier.xyz
<mailto:michael@paquier.xyz>> wrote:

On Sat, Sep 07, 2019 at 12:11:25AM +0200, Peter Eisentraut wrote:

I'm not sure exactly what the upstream of mingw is these days, but I
think the original issue that led to 811be893 has long been fixed [0],
and the other stuff in mingwcompat.c is also no longer relevant

[1]. I

think mingwcompat.c could be removed altogether. I'm not sure to what
extent we need to support 5+ year old mingw versions.

On HEAD I would not be against removing that as this leads to a
cleanup of our code. For MSVC, we only support VS 2013~ on HEAD, so
saying that we don't support MinGW older than what was proposed 5
years ago sounds sensible.

+1, definitely.

committed

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Michael Paquier

michael@paquier.xyz

over 6 years ago

In reply to: Peter Eisentraut (#8)

Re: Compiler warnings with MinGW

On Tue, Sep 17, 2019 at 12:00:39PM +0200, Peter Eisentraut wrote:

committed

Thanks, Peter.
--
Michael