Thread: popen and pclose redefinitions causing many warning in Windows build

Hi all,

Since commit a692ee5, code compilation on windows is full of warnings
caused by the re-definitions of popen and pclose:
In file included from ../../../src/include/c.h:1028:0,
                 from ../../../src/include/postgres.h:47,
                 from analyze.c:25:
../../../src/include/port.h:312:0: warning: "popen" redefined [enabled
by default]
In file included from ../../../src/include/c.h:81:0,
                 from ../../../src/include/postgres.h:47,
                 from analyze.c:25:
c:\mingw64\bin\../lib/gcc/x86_64-w64-mingw32/4.7.0/../../../../x86_64-w64-mingw32/include/stdio.h:48
9:0: note: this is the location of the previous definition
In file included from ../../../src/include/c.h:1028:0,
                 from ../../../src/include/postgres.h:47,
                 from analyze.c:25:
../../../src/include/port.h:313:0: warning: "pclose" redefined
[enabled by default]
In file included from ../../../src/include/c.h:81:0,
                 from ../../../src/include/postgres.h:47,
                 from analyze.c:25
The patch attached fixes that.
Regards,
--
Michael

On 05/08/2014 08:01 AM, Michael Paquier wrote:
> Hi all,
>
> Since commit a692ee5, code compilation on windows is full of warnings
> caused by the re-definitions of popen and pclose:
> In file included from ../../../src/include/c.h:1028:0,
>                   from ../../../src/include/postgres.h:47,
>                   from analyze.c:25:
> ../../../src/include/port.h:312:0: warning: "popen" redefined [enabled
> by default]

Hmm. Does the MinGW version of popen() and system() do the quoting for 
you? If we just #ifdef the defines, then we will not use the wrappers on 
MinGW, which would be wrong if the quoting is needed there. If it's not 
needed, then we shouldn't be compiling the wrapper functions in the 
first place.

- Heikki

Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> On 05/08/2014 08:01 AM, Michael Paquier wrote:
>> Since commit a692ee5, code compilation on windows is full of warnings
>> caused by the re-definitions of popen and pclose:

> Hmm. Does the MinGW version of popen() and system() do the quoting for 
> you? If we just #ifdef the defines, then we will not use the wrappers on 
> MinGW, which would be wrong if the quoting is needed there. If it's not 
> needed, then we shouldn't be compiling the wrapper functions in the 
> first place.

Another problem, if we do need the wrappers on mingw, is that the
"#undef" commands in system.c will presumably result in the wrong
things happening in the wrapper functions, since the platform needs
us to use their macros there.

The simplest workaround I can think of is to change the stanza in
port.h to be like
      #ifndef DONT_DEFINE_SYSTEM_POPEN      #undef system      #define system(a) pgwin32_system(a)      #undef popen
 #define popen(a,b) pgwin32_popen(a,b)      #undef pclose      #define pclose(a) _pclose(a)      #endif
 

and then have system.c do #define DONT_DEFINE_SYSTEM_POPEN before
including postgres.h.  This is pretty grotty, but on the other hand
it'd remove the need for the #undef's in system.c.
        regards, tom lane

On 05/08/2014 11:19 AM, Tom Lane wrote:
> Heikki Linnakangas <hlinnakangas@vmware.com> writes:
>> On 05/08/2014 08:01 AM, Michael Paquier wrote:
>>> Since commit a692ee5, code compilation on windows is full of warnings
>>> caused by the re-definitions of popen and pclose:
>> Hmm. Does the MinGW version of popen() and system() do the quoting for
>> you? If we just #ifdef the defines, then we will not use the wrappers on
>> MinGW, which would be wrong if the quoting is needed there. If it's not
>> needed, then we shouldn't be compiling the wrapper functions in the
>> first place.
> Another problem, if we do need the wrappers on mingw, is that the
> "#undef" commands in system.c will presumably result in the wrong
> things happening in the wrapper functions, since the platform needs
> us to use their macros there.



I'm pretty sure we need this on Mingw - this SYSTEMQUOTE stuff dates 
back well before 8.3, IIRC, which is when we first got full MSVC support.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> I'm pretty sure we need this on Mingw - this SYSTEMQUOTE stuff dates 
> back well before 8.3, IIRC, which is when we first got full MSVC support.

I tried googling for some info on this, and got a number of hits
suggesting that mingw didn't emulate popen at all till pretty recently.
For instance this:
https://lists.fedoraproject.org/pipermail/fedora-mingw/2009-September/002087.html
Jones is an ex-coworker of mine, and I'm pretty sure that if he said
it wasn't there then it wasn't there.

So I'm confused about how this worked at all in mingw back-when.
        regards, tom lane

On 05/08/2014 12:14 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> I'm pretty sure we need this on Mingw - this SYSTEMQUOTE stuff dates
>> back well before 8.3, IIRC, which is when we first got full MSVC support.
> I tried googling for some info on this, and got a number of hits
> suggesting that mingw didn't emulate popen at all till pretty recently.
> For instance this:
> https://lists.fedoraproject.org/pipermail/fedora-mingw/2009-September/002087.html
> Jones is an ex-coworker of mine, and I'm pretty sure that if he said
> it wasn't there then it wasn't there.
>
> So I'm confused about how this worked at all in mingw back-when.


Mingw didn't, possibly, but we didn't rely on that. The MS C runtime 
library (msvcrt.dll_ has had _popen() forever, and we had this for 
Windows in PG 8.0 and have it still in all the back branches AFAICT:
   #define popen(a,b) _popen(a,b)   #define pclose(a) _pclose(a)


cheers

andrew

On Thu, May 08, 2014 at 12:14:44PM -0400, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
> > I'm pretty sure we need this on Mingw - this SYSTEMQUOTE stuff dates 
> > back well before 8.3, IIRC, which is when we first got full MSVC support.
> 
> I tried googling for some info on this, and got a number of hits
> suggesting that mingw didn't emulate popen at all till pretty recently.
> For instance this:
> https://lists.fedoraproject.org/pipermail/fedora-mingw/2009-September/002087.html
> Jones is an ex-coworker of mine, and I'm pretty sure that if he said
> it wasn't there then it wasn't there.

I doubt MinGW has overridden popen() at runtime; that would be contrary to its
design criteria.  The headers, however, are MinGW territory.  MinGW declares
both _popen() and popen() as functions.  MinGW-w64, a project more distinct
from MinGW than it sounds, uses "#define popen _popen":

MinGW: http://sourceforge.net/p/mingw/mingw-org-wsl/ci/master/tree/include/stdio.h#l467
MinGW-w64: http://sourceforge.net/p/mingw-w64/code/HEAD/tree/trunk/mingw-w64-headers/crt/stdio.h#l496

Building with any recent MinGW-w64, 32-bit or 64-bit, gets the reported
warnings; building with MinGW proper does not.

-- 
Noah Misch
EnterpriseDB                                 http://www.enterprisedb.com

On 05/09/2014 02:56 AM, Noah Misch wrote:
> On Thu, May 08, 2014 at 12:14:44PM -0400, Tom Lane wrote:
>> Andrew Dunstan <andrew@dunslane.net> writes:
>>> I'm pretty sure we need this on Mingw - this SYSTEMQUOTE stuff dates
>>> back well before 8.3, IIRC, which is when we first got full MSVC support.
>>
>> I tried googling for some info on this, and got a number of hits
>> suggesting that mingw didn't emulate popen at all till pretty recently.
>> For instance this:
>> https://lists.fedoraproject.org/pipermail/fedora-mingw/2009-September/002087.html
>> Jones is an ex-coworker of mine, and I'm pretty sure that if he said
>> it wasn't there then it wasn't there.
>
> I doubt MinGW has overridden popen() at runtime; that would be contrary to its
> design criteria.  The headers, however, are MinGW territory.  MinGW declares
> both _popen() and popen() as functions.  MinGW-w64, a project more distinct
> from MinGW than it sounds, uses "#define popen _popen":
>
> MinGW: http://sourceforge.net/p/mingw/mingw-org-wsl/ci/master/tree/include/stdio.h#l467
> MinGW-w64: http://sourceforge.net/p/mingw-w64/code/HEAD/tree/trunk/mingw-w64-headers/crt/stdio.h#l496
>
> Building with any recent MinGW-w64, 32-bit or 64-bit, gets the reported
> warnings; building with MinGW proper does not.

Hmm. The MinGW-w64 header does this:

> #if !defined(NO_OLDNAMES) && !defined(popen)
> #define popen _popen
> #define pclose _pclose
> #endif

So if we defined popen() before including stdio.h, that would get rid of 
the warning. But we don't usually do things in that order.

Could we define NO_OLDNAMES? I couldn't find any documentation on it, 
but it seems to a bunch of lot of wrapper functions and defines. If we 
can get away without them, that seems like a good thing...

- Heikki

On 05/14/2014 08:15 AM, Heikki Linnakangas wrote:
> On 05/09/2014 02:56 AM, Noah Misch wrote:
>> On Thu, May 08, 2014 at 12:14:44PM -0400, Tom Lane wrote:
>>> Andrew Dunstan <andrew@dunslane.net> writes:
>>>> I'm pretty sure we need this on Mingw - this SYSTEMQUOTE stuff dates
>>>> back well before 8.3, IIRC, which is when we first got full MSVC 
>>>> support.
>>>
>>> I tried googling for some info on this, and got a number of hits
>>> suggesting that mingw didn't emulate popen at all till pretty recently.
>>> For instance this:
>>> https://lists.fedoraproject.org/pipermail/fedora-mingw/2009-September/002087.html 
>>>
>>> Jones is an ex-coworker of mine, and I'm pretty sure that if he said
>>> it wasn't there then it wasn't there.
>>
>> I doubt MinGW has overridden popen() at runtime; that would be 
>> contrary to its
>> design criteria.  The headers, however, are MinGW territory. MinGW 
>> declares
>> both _popen() and popen() as functions.  MinGW-w64, a project more 
>> distinct
>> from MinGW than it sounds, uses "#define popen _popen":
>>
>> MinGW: 
>> http://sourceforge.net/p/mingw/mingw-org-wsl/ci/master/tree/include/stdio.h#l467
>> MinGW-w64: 
>> http://sourceforge.net/p/mingw-w64/code/HEAD/tree/trunk/mingw-w64-headers/crt/stdio.h#l496
>>
>> Building with any recent MinGW-w64, 32-bit or 64-bit, gets the reported
>> warnings; building with MinGW proper does not.
>
> Hmm. The MinGW-w64 header does this:
>
>> #if !defined(NO_OLDNAMES) && !defined(popen)
>> #define popen _popen
>> #define pclose _pclose
>> #endif
>
> So if we defined popen() before including stdio.h, that would get rid 
> of the warning. But we don't usually do things in that order.
>
> Could we define NO_OLDNAMES? I couldn't find any documentation on it, 
> but it seems to a bunch of lot of wrapper functions and defines. If we 
> can get away without them, that seems like a good thing...
>
>


Why don't we simply undefine it if it's defined before we redefine it? 
We don't need or want their definition, as our implementation calls 
_popen directly.

cheers

andrew

On Wed, May 14, 2014 at 03:15:38PM +0300, Heikki Linnakangas wrote:
> On 05/09/2014 02:56 AM, Noah Misch wrote:
> >MinGW: http://sourceforge.net/p/mingw/mingw-org-wsl/ci/master/tree/include/stdio.h#l467
> >MinGW-w64: http://sourceforge.net/p/mingw-w64/code/HEAD/tree/trunk/mingw-w64-headers/crt/stdio.h#l496
> >
> >Building with any recent MinGW-w64, 32-bit or 64-bit, gets the reported
> >warnings; building with MinGW proper does not.
> 
> Hmm. The MinGW-w64 header does this:
> 
> >#if !defined(NO_OLDNAMES) && !defined(popen)
> >#define popen _popen
> >#define pclose _pclose
> >#endif
> 
> So if we defined popen() before including stdio.h, that would get
> rid of the warning. But we don't usually do things in that order.

True.  I have no strong preference between that and use of #undef.

> Could we define NO_OLDNAMES? I couldn't find any documentation on
> it, but it seems to a bunch of lot of wrapper functions and defines.
> If we can get away without them, that seems like a good thing...

That's a bit like compiling with "gcc -std=c89" on Unix.  It would lead us to
add "#define strdup(x) _strdup(x)" and similar.  I wouldn't do that.

-- 
Noah Misch
EnterpriseDB                                 http://www.enterprisedb.com

On 05/14/2014 05:37 PM, Noah Misch wrote:
> On Wed, May 14, 2014 at 03:15:38PM +0300, Heikki Linnakangas wrote:
>> On 05/09/2014 02:56 AM, Noah Misch wrote:
>>> MinGW: http://sourceforge.net/p/mingw/mingw-org-wsl/ci/master/tree/include/stdio.h#l467
>>> MinGW-w64: http://sourceforge.net/p/mingw-w64/code/HEAD/tree/trunk/mingw-w64-headers/crt/stdio.h#l496
>>>
>>> Building with any recent MinGW-w64, 32-bit or 64-bit, gets the reported
>>> warnings; building with MinGW proper does not.
>>
>> Hmm. The MinGW-w64 header does this:
>>
>>> #if !defined(NO_OLDNAMES) && !defined(popen)
>>> #define popen _popen
>>> #define pclose _pclose
>>> #endif
>>
>> So if we defined popen() before including stdio.h, that would get
>> rid of the warning. But we don't usually do things in that order.
>
> True.  I have no strong preference between that and use of #undef.

I think I would prefer #undef. The risk with that is if some platform 
has #defined popen() to something else entirely, for some good reason, 
we would be bypassing that hypothetical wrapper. But I guess we'll cross 
that bridge if we get there.

>> Could we define NO_OLDNAMES? I couldn't find any documentation on
>> it, but it seems to a bunch of lot of wrapper functions and defines.
>> If we can get away without them, that seems like a good thing...
>
> That's a bit like compiling with "gcc -std=c89" on Unix.  It would lead us to
> add "#define strdup(x) _strdup(x)" and similar.  I wouldn't do that.

Ugh. I can't believe they marked strdup(x) as deprecated.

- Heikki

On Wed, May 14, 2014 at 05:51:24PM +0300, Heikki Linnakangas wrote:
> On 05/14/2014 05:37 PM, Noah Misch wrote:
> >On Wed, May 14, 2014 at 03:15:38PM +0300, Heikki Linnakangas wrote:
> >>On 05/09/2014 02:56 AM, Noah Misch wrote:
> >>>MinGW: http://sourceforge.net/p/mingw/mingw-org-wsl/ci/master/tree/include/stdio.h#l467
> >>>MinGW-w64: http://sourceforge.net/p/mingw-w64/code/HEAD/tree/trunk/mingw-w64-headers/crt/stdio.h#l496
> >>>
> >>>Building with any recent MinGW-w64, 32-bit or 64-bit, gets the reported
> >>>warnings; building with MinGW proper does not.
> >>
> >>Hmm. The MinGW-w64 header does this:
> >>
> >>>#if !defined(NO_OLDNAMES) && !defined(popen)
> >>>#define popen _popen
> >>>#define pclose _pclose
> >>>#endif
> >>
> >>So if we defined popen() before including stdio.h, that would get
> >>rid of the warning. But we don't usually do things in that order.
> >
> >True.  I have no strong preference between that and use of #undef.
> 
> I think I would prefer #undef. The risk with that is if some
> platform has #defined popen() to something else entirely, for some
> good reason, we would be bypassing that hypothetical wrapper. But I
> guess we'll cross that bridge if we get there.

Works for me.  Since "(popen)(x, y)" shall behave the same as "popen(x, y)",
such a hypothetical system header would be buggy, anyway.

> >>Could we define NO_OLDNAMES? I couldn't find any documentation on
> >>it, but it seems to a bunch of lot of wrapper functions and defines.
> >>If we can get away without them, that seems like a good thing...
> >
> >That's a bit like compiling with "gcc -std=c89" on Unix.  It would lead us to
> >add "#define strdup(x) _strdup(x)" and similar.  I wouldn't do that.
> 
> Ugh. I can't believe they marked strdup(x) as deprecated.

That reflected the function's absence from ISO C, not a value judgement.

-- 
Noah Misch
EnterpriseDB                                 http://www.enterprisedb.com

On 05/14/2014 06:06 PM, Noah Misch wrote:
> On Wed, May 14, 2014 at 05:51:24PM +0300, Heikki Linnakangas wrote:
>> On 05/14/2014 05:37 PM, Noah Misch wrote:
>>> On Wed, May 14, 2014 at 03:15:38PM +0300, Heikki Linnakangas wrote:
>>>> On 05/09/2014 02:56 AM, Noah Misch wrote:
>>>>> MinGW: http://sourceforge.net/p/mingw/mingw-org-wsl/ci/master/tree/include/stdio.h#l467
>>>>> MinGW-w64: http://sourceforge.net/p/mingw-w64/code/HEAD/tree/trunk/mingw-w64-headers/crt/stdio.h#l496
>>>>>
>>>>> Building with any recent MinGW-w64, 32-bit or 64-bit, gets the reported
>>>>> warnings; building with MinGW proper does not.
>>>>
>>>> Hmm. The MinGW-w64 header does this:
>>>>
>>>>> #if !defined(NO_OLDNAMES) && !defined(popen)
>>>>> #define popen _popen
>>>>> #define pclose _pclose
>>>>> #endif
>>>>
>>>> So if we defined popen() before including stdio.h, that would get
>>>> rid of the warning. But we don't usually do things in that order.
>>>
>>> True.  I have no strong preference between that and use of #undef.
>>
>> I think I would prefer #undef. The risk with that is if some
>> platform has #defined popen() to something else entirely, for some
>> good reason, we would be bypassing that hypothetical wrapper. But I
>> guess we'll cross that bridge if we get there.
>
> Works for me.  Since "(popen)(x, y)" shall behave the same as "popen(x, y)",
> such a hypothetical system header would be buggy, anyway.

Ok, I committed #undefs. I don't have a Mingw(-w64) environment to test 
with, so let's see if the buildfarm likes it.

- Heikki

On Thu, May 15, 2014 at 6:20 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:
> Ok, I committed #undefs. I don't have a Mingw(-w64) environment to test
> with, so let's see if the buildfarm likes it.
There does not seem to be a buildfarm machine using MinGW-w64... Btw,
I tested latest master on a Windows box and MinGW-w64 is happier now.
Thanks!
-- 
Michael

On 05/15/2014 04:15 PM, Michael Paquier wrote:
> On Thu, May 15, 2014 at 6:20 PM, Heikki Linnakangas
> <hlinnakangas@vmware.com> wrote:
>> Ok, I committed #undefs. I don't have a Mingw(-w64) environment to test
>> with, so let's see if the buildfarm likes it.
> There does not seem to be a buildfarm machine using MinGW-w64...

Jacana. It has "gcc 4.8.1" listed as the compiler, but if you look at 
the config in detail, it's mingw-w64. The popen/pclose warnings are 
there. It hasn't performed a build after I committed the fix yet.

> Btw, I tested latest master on a Windows box and MinGW-w64 is happier now.

Thanks!

- Heikki

Heikki Linnakangas wrote:
> On 05/15/2014 04:15 PM, Michael Paquier wrote:
> >On Thu, May 15, 2014 at 6:20 PM, Heikki Linnakangas
> ><hlinnakangas@vmware.com> wrote:
> >>Ok, I committed #undefs. I don't have a Mingw(-w64) environment to test
> >>with, so let's see if the buildfarm likes it.
> >There does not seem to be a buildfarm machine using MinGW-w64...
> 
> Jacana. It has "gcc 4.8.1" listed as the compiler, but if you look
> at the config in detail, it's mingw-w64. The popen/pclose warnings
> are there. It hasn't performed a build after I committed the fix
> yet.

There are no warnings about popen in Jacana currently.  These are the
warnings that remain:

x86_64-w64-mingw32-gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g
-I../../../../src/include-I/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/include
-I../pgsql/src/include/port/win32-DEXEC_BACKEND -I/c/prog/3p64/include/libxml2  -I/c/prog/3p64/include
-I/c/prog/3p64/openssl/include"-I/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/include/port/win32" -DBUILDING_DLL
-c -o mingwcompat.o /home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/backend/port/win32/mingwcompat.c
 
c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/backend/port/win32/mingwcompat.c:60:1: warning:
'RegisterWaitForSingleObject'redeclared without dllimport attribute: previous dllimport ignored
[-Wattributes]RegisterWaitForSingleObject(PHANDLEphNewWaitObject,^
 

x86_64-w64-mingw32-gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g
-I/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/interfaces/libpq-I../../../src/include
-I/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/include-I../pgsql/src/include/port/win32 -DEXEC_BACKEND
-I/c/prog/3p64/include/libxml2 -I/c/prog/3p64/include -I/c/prog/3p64/openssl/include
"-I/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/include/port/win32" -c -o parallel.o
/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/bin/pg_dump/parallel.c
c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/bin/pg_dump/parallel.c: In function 'pgpipe':
c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/bin/pg_dump/parallel.c:1332:2: warning: overflow
inimplicit constant conversion [-Woverflow] handles[0] = handles[1] = INVALID_SOCKET; ^
 
c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/bin/pg_dump/parallel.c:1386:3: warning: overflow
inimplicit constant conversion [-Woverflow]  handles[1] = INVALID_SOCKET;  ^
 

x86_64-w64-mingw32-gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g  -I.
-I/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/contrib/pg_stat_statements-I../../src/include
-I/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/include-I../pgsql/src/include/port/win32 -DEXEC_BACKEND
-I/c/prog/3p64/include/libxml2 -I/c/prog/3p64/include -I/c/prog/3p64/openssl/include
"-I/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/include/port/win32" -c -o pg_stat_statements.o
/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/contrib/pg_stat_statements/pg_stat_statements.c
c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/contrib/pg_stat_statements/pg_stat_statements.c: In
function'pgss_ProcessUtility':
 
c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/contrib/pg_stat_statements/pg_stat_statements.c:998:4:
warning:unknown conversion type character 'l' in format [-Wformat=]   sscanf(completionTag, "COPY " UINT64_FORMAT,
&rows)!= 1)   ^
 
c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/contrib/pg_stat_statements/pg_stat_statements.c:998:4:
warning:too many arguments for format [-Wformat-extra-args]
 



-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, May 23, 2014 at 10:43 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> x86_64-w64-mingw32-gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g
-I../../../../src/include-I/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/include
-I../pgsql/src/include/port/win32-DEXEC_BACKEND -I/c/prog/3p64/include/libxml2  -I/c/prog/3p64/include
-I/c/prog/3p64/openssl/include"-I/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/include/port/win32" -DBUILDING_DLL
-c -o mingwcompat.o /home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/backend/port/win32/mingwcompat.c 
> c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/backend/port/win32/mingwcompat.c:60:1: warning:
'RegisterWaitForSingleObject'redeclared without dllimport attribute: previous dllimport ignored [-Wattributes] 
>  RegisterWaitForSingleObject(PHANDLE phNewWaitObject,
>  ^
This one is also an old warning, looking at the buildfarm it is
present as well in REL9_2_STABLE... In mingw-w64
RegisterWaitForSingleObject is already defined in winbase.h here:
http://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-headers/include/winbase.h
What do you think about adding a #ifndef _WINBASE_ block that includes
LoadKernel32 and RegisterWaitForSingleObject in mingwcompat.c?

> x86_64-w64-mingw32-gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g
-I/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/interfaces/libpq-I../../../src/include
-I/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/include-I../pgsql/src/include/port/win32 -DEXEC_BACKEND
-I/c/prog/3p64/include/libxml2 -I/c/prog/3p64/include -I/c/prog/3p64/openssl/include
"-I/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/include/port/win32" -c -o parallel.o
/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/bin/pg_dump/parallel.c
> c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/bin/pg_dump/parallel.c: In function 'pgpipe':
> c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/bin/pg_dump/parallel.c:1332:2: warning: overflow
inimplicit constant conversion [-Woverflow] 
>   handles[0] = handles[1] = INVALID_SOCKET;
>   ^
> c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/bin/pg_dump/parallel.c:1386:3: warning: overflow
inimplicit constant conversion [-Woverflow] 
>    handles[1] = INVALID_SOCKET;
>    ^
In mingw-w64, SOCKET_INVALID is defined as ~0:
http://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-headers/include/psdk_inc/_socket_types.h
Is this overflow caused because SOCKET_INVALID corresponds to a 64b
value in mingw-w64?
Looking at the code this exists since 9.3.

> x86_64-w64-mingw32-gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g  -I.
-I/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/contrib/pg_stat_statements-I../../src/include
-I/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/include-I../pgsql/src/include/port/win32 -DEXEC_BACKEND
-I/c/prog/3p64/include/libxml2 -I/c/prog/3p64/include -I/c/prog/3p64/openssl/include
"-I/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/include/port/win32" -c -o pg_stat_statements.o
/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/contrib/pg_stat_statements/pg_stat_statements.c
> c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/contrib/pg_stat_statements/pg_stat_statements.c: In
function'pgss_ProcessUtility': 
>
c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/contrib/pg_stat_statements/pg_stat_statements.c:998:4:
warning:unknown conversion type character 'l' in format [-Wformat=] 
>     sscanf(completionTag, "COPY " UINT64_FORMAT, &rows) != 1)
>     ^
>
c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/contrib/pg_stat_statements/pg_stat_statements.c:998:4:
warning:too many arguments for format [-Wformat-extra-args] 
Hm... After a little bit of googling, I found that:
http://sourceforge.net/p/mingw/bugs/1315/
This seems to be part of the standard of Microsoft, making sscanf not
accept ISO-C99 format specifiers. Looking more into the code, this is
a pretty old warning introduced by a5495cd of 2009. This code is btw
correct as the number of rows returned by a COPY is uint64. Any idea
what would be doable here?

Regards,
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Fri, May 23, 2014 at 10:43 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/contrib/pg_stat_statements/pg_stat_statements.c: In
function'pgss_ProcessUtility':
 
>>
c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/contrib/pg_stat_statements/pg_stat_statements.c:998:4:
warning:unknown conversion type character 'l' in format [-Wformat=]
 
>> sscanf(completionTag, "COPY " UINT64_FORMAT, &rows) != 1)
>> ^
>>
c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/contrib/pg_stat_statements/pg_stat_statements.c:998:4:
warning:too many arguments for format [-Wformat-extra-args]
 

> Hm... After a little bit of googling, I found that:
> http://sourceforge.net/p/mingw/bugs/1315/
> This seems to be part of the standard of Microsoft, making sscanf not
> accept ISO-C99 format specifiers. Looking more into the code, this is
> a pretty old warning introduced by a5495cd of 2009. This code is btw
> correct as the number of rows returned by a COPY is uint64. Any idea
> what would be doable here?

Yeah, this is bad: sscanf is likely to make the wrong conclusion about the
width of "rows", resulting in returning garbage, on Windows.  It seems
pretty brain-dead to me that mingw wouldn't make sure that printf and
scanf accept comparable format specifiers, but on the other hand,
brain-dead behavior seems to be the norm on that platform.

The mingw bug cited above recommends using the format macros defined
by <inttypes.h>; but that approach doesn't excite me, because those
macros are a relatively recent invention (I don't see them in SUS v2)
so relying on them could easily fail on other platforms.

The best alternative I can think of is to use strncmp() to check for
whether the head of the string matches "COPY ", and then perform the
integer conversion using strtoull() #ifdef HAVE_STRTOULL and strtoul()
otherwise.
        regards, tom lane

On Tue, May 27, 2014 at 1:39 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The best alternative I can think of is to use strncmp() to check for
> whether the head of the string matches "COPY ", and then perform the
> integer conversion using strtoull() #ifdef HAVE_STRTOULL and strtoul()
> otherwise.
What about the attached?
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Tue, May 27, 2014 at 1:39 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The best alternative I can think of is to use strncmp() to check for
>> whether the head of the string matches "COPY ", and then perform the
>> integer conversion using strtoull() #ifdef HAVE_STRTOULL and strtoul()
>> otherwise.

> What about the attached?

Looks good to me --- pushed.
        regards, tom lane

On Mon, May 26, 2014 at 09:50:42PM +0900, Michael Paquier wrote:
> > x86_64-w64-mingw32-gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g
-I/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/interfaces/libpq-I../../../src/include
-I/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/include-I../pgsql/src/include/port/win32 -DEXEC_BACKEND
-I/c/prog/3p64/include/libxml2 -I/c/prog/3p64/include -I/c/prog/3p64/openssl/include
"-I/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/include/port/win32" -c -o parallel.o
/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/bin/pg_dump/parallel.c
> > c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/bin/pg_dump/parallel.c: In function 'pgpipe':
> > c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/bin/pg_dump/parallel.c:1332:2: warning:
overflowin implicit constant conversion [-Woverflow] 
> >   handles[0] = handles[1] = INVALID_SOCKET;
> >   ^
> > c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.5100/../pgsql/src/bin/pg_dump/parallel.c:1386:3: warning:
overflowin implicit constant conversion [-Woverflow] 
> >    handles[1] = INVALID_SOCKET;
> >    ^
> In mingw-w64, SOCKET_INVALID is defined as ~0:
> http://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-headers/include/psdk_inc/_socket_types.h
> Is this overflow caused because SOCKET_INVALID corresponds to a 64b
> value in mingw-w64?
> Looking at the code this exists since 9.3.

I think this is caused because the variable is not defined as SOCKET.
The attached patch fixes this.  This should prevent the warning.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

Bruce Momjian <bruce@momjian.us> writes:
> I think this is caused because the variable is not defined as SOCKET. 
> The attached patch fixes this.  This should prevent the warning.

Surely that's just going to move the errors somewhere else.  The call
site still expects the argument to be int[].
        regards, tom lane

On Wed, May 28, 2014 at 12:29:28PM -0400, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > I think this is caused because the variable is not defined as SOCKET.
> > The attached patch fixes this.  This should prevent the warning.
>
> Surely that's just going to move the errors somewhere else.  The call
> site still expects the argument to be int[].

Ah, yes, you are right.  This is a similar problem I had with libpq
where PQsocket() had to return an int.

Attached is an updated patch which follows my previous coding of
checking for PGINVALID_SOCKET, and if not equal, assigns the value to an
integer handle.  I would also like to rename variable 's' to
'listen_sock', but that is not in the patch, for clarity reasons.

Should this be held for 9.5?  I think it is only warning removal.  On
the other hand, portability is what we do during beta testing.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

Bruce Momjian wrote:
> On Wed, May 28, 2014 at 12:29:28PM -0400, Tom Lane wrote:
> > Bruce Momjian <bruce@momjian.us> writes:
> > > I think this is caused because the variable is not defined as SOCKET. 
> > > The attached patch fixes this.  This should prevent the warning.
> > 
> > Surely that's just going to move the errors somewhere else.  The call
> > site still expects the argument to be int[].
> 
> Ah, yes, you are right.  This is a similar problem I had with libpq
> where PQsocket() had to return an int.
> 
> Attached is an updated patch which follows my previous coding of
> checking for PGINVALID_SOCKET, and if not equal, assigns the value to an
> integer handle.  I would also like to rename variable 's' to
> 'listen_sock', but that is not in the patch, for clarity reasons.
> 
> Should this be held for 9.5?  I think it is only warning removal.  On
> the other hand, portability is what we do during beta testing.

I think this should go in 9.4, but as you say it's only warning removal
so there is probably little point in patching further back (this code
dates back to 9.3.)

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Jun  6, 2014 at 01:18:24PM -0400, Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > On Wed, May 28, 2014 at 12:29:28PM -0400, Tom Lane wrote:
> > > Bruce Momjian <bruce@momjian.us> writes:
> > > > I think this is caused because the variable is not defined as SOCKET. 
> > > > The attached patch fixes this.  This should prevent the warning.
> > > 
> > > Surely that's just going to move the errors somewhere else.  The call
> > > site still expects the argument to be int[].
> > 
> > Ah, yes, you are right.  This is a similar problem I had with libpq
> > where PQsocket() had to return an int.
> > 
> > Attached is an updated patch which follows my previous coding of
> > checking for PGINVALID_SOCKET, and if not equal, assigns the value to an
> > integer handle.  I would also like to rename variable 's' to
> > 'listen_sock', but that is not in the patch, for clarity reasons.
> > 
> > Should this be held for 9.5?  I think it is only warning removal.  On
> > the other hand, portability is what we do during beta testing.
> 
> I think this should go in 9.4, but as you say it's only warning removal
> so there is probably little point in patching further back (this code
> dates back to 9.3.)

Agreed and applied.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +