Обсуждение: windows consolidated cleanup

The attached patch is intended to clean up a bunch of compiler warnings
seen on Windows due to mismatches of signedness or constness, unused
variables, redefined macros and a missing prototype.

It doesn't clean up all the warnings by any means, but it fixes quite a few.

One thing I'm a bit confused about is this type of warning:

       src\backend\utils\misc\guc-file.c(977): warning C4003: not enough actual parameters for macro 'GUC_yywrap'


If someone can suggest a good fix That would be nice.

cheers

andrew

On 4/24/2011 1:29 AM, Andrew Dunstan wrote:
>
> The attached patch is intended to clean up a bunch of compiler warnings seen on
> Windows due to mismatches of signedness or constness, unused variables,
> redefined macros and a missing prototype.
>
> It doesn't clean up all the warnings by any means, but it fixes quite a few.
>
> One thing I'm a bit confused about is this type of warning:
>
> src\backend\utils\misc\guc-file.c(977): warning C4003: not enough actual
> parameters for macro 'GUC_yywrap'
>
>
> If someone can suggest a good fix That would be nice.
>

The macro is defined as taking one argument.

// guc-file.c line 354
#define GUC_yywrap(n) 1

The macro is overriding the prototype declared at line 627, which has a void 
argument list (assuming YY_SKIP_YYWRAP is !defined).  Since all code references 
to this do not provide an argument, I'd say the macro is incorrect.

-- 
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

On sön, 2011-04-24 at 01:29 -0400, Andrew Dunstan wrote:
> One thing I'm a bit confused about is this type of warning:
> 
>        src\backend\utils\misc\guc-file.c(977): warning C4003: not enough actual parameters for macro 'GUC_yywrap'
> 
> 
> If someone can suggest a good fix That would be nice.

Upstream claims that complaining about this is a bug in the compiler.

http://sourceforge.net/tracker/index.php?func=detail&aid=1783536&group_id=97492&atid=618177

The hunk below looks a bit evil.

At least a comment would be good to explain why this is necessary.

On sön, 2011-04-24 at 01:29 -0400, Andrew Dunstan wrote:
> *** a/src/backend/port/win32/socket.c
> --- b/src/backend/port/win32/socket.c
> ***************
> *** 370,376 **** pgwin32_recv(SOCKET s, char *buf, int len, int f)
>   }
>   
>   int
> ! pgwin32_send(SOCKET s, char *buf, int len, int flags)
>   {
>         WSABUF          wbuf;
>         int                     r;
> --- 370,376 ----
>   }
>   
>   int
> ! pgwin32_send(SOCKET s, const char *buf, int len, int flags)
>   {
>         WSABUF          wbuf;
>         int                     r;
> ***************
> *** 380,386 **** pgwin32_send(SOCKET s, char *buf, int len, int flags)
>                 return -1;
>   
>         wbuf.len = len;
> !       wbuf.buf = buf;
>   
>         /*
>          * Readiness of socket to send data to UDP socket may be not true: socket
> --- 380,386 ----
>                 return -1;
>   
>         wbuf.len = len;
> !       wbuf.buf = (char *) buf;
>   
>         /*
>          * Readiness of socket to send data to UDP socket may be not true: socket
> 
> 

On 04/24/2011 09:11 AM, Andrew Chernow wrote:
> On 4/24/2011 1:29 AM, Andrew Dunstan wrote:
>>
>> The attached patch is intended to clean up a bunch of compiler 
>> warnings seen on
>> Windows due to mismatches of signedness or constness, unused variables,
>> redefined macros and a missing prototype.
>>
>> It doesn't clean up all the warnings by any means, but it fixes quite 
>> a few.
>>
>> One thing I'm a bit confused about is this type of warning:
>>
>> src\backend\utils\misc\guc-file.c(977): warning C4003: not enough actual
>> parameters for macro 'GUC_yywrap'
>>
>>
>> If someone can suggest a good fix That would be nice.
>>
>
> The macro is defined as taking one argument.
>
> // guc-file.c line 354
> #define GUC_yywrap(n) 1
>
> The macro is overriding the prototype declared at line 627, which has 
> a void argument list (assuming YY_SKIP_YYWRAP is !defined).  Since all 
> code references to this do not provide an argument, I'd say the macro 
> is incorrect.

Thanks for looking.

All our scanners are in fact defined with "%option noyywrap", so 
YY_SKIP_WRAP is defined.

But the macro is incorrect unless you're generating a reentrant scanner 
as we are for the core scanner (which is why we don't see this error for 
the core scanner, only all the others).

I have a mildly ugly fix for it in pg_flex.bat (only MSVC compilers 
complain about it AFAIK).

cheers

andrew

On 04/24/2011 10:53 AM, Peter Eisentraut wrote:
> On sön, 2011-04-24 at 01:29 -0400, Andrew Dunstan wrote:
>> One thing I'm a bit confused about is this type of warning:
>>
>>         src\backend\utils\misc\guc-file.c(977): warning C4003: not enough actual parameters for macro 'GUC_yywrap'
>>
>>
>> If someone can suggest a good fix That would be nice.
> Upstream claims that complaining about this is a bug in the compiler.
>
> http://sourceforge.net/tracker/index.php?func=detail&aid=1783536&group_id=97492&atid=618177
>

One person said that. But the bug is still open, and has been for four 
years (so you can see how much attention they pay to this tracker at least.)

cheers

andrew

Peter Eisentraut <peter_e@gmx.net> writes:
> The hunk below looks a bit evil.
> At least a comment would be good to explain why this is necessary.

Yeah, having to cast away const seems uglier than the original problem.
Can't we avoid that?

BTW, all of my machines as well as the Single Unix Spec are agreed that
the second argument to send() is "const void *", not "const char *".
If we're going to tweak this I think we should make it match exactly.
        regards, tom lane

Andrew Dunstan <andrew@dunslane.net> writes:
> The attached patch is intended to clean up a bunch of compiler warnings 
> seen on Windows due to mismatches of signedness or constness, unused 
> variables, redefined macros and a missing prototype.

BTW, this hunk:

> *** a/src/pl/plpython/plpython.c
> --- b/src/pl/plpython/plpython.c
> ***************
> *** 84,89 **** typedef int Py_ssize_t;
> --- 84,101 ----
>           PyObject_HEAD_INIT(type) size,
>   #endif 
> + /*
> +  * Some Python headers define these two symbols (e.g. on Windows) which is
> +  * possibly a bit unfriendly. Use the Postgres definitions (or lack thereof).
> +  */ 
> + #ifdef HAVE_STRERROR
> + #undef HAVE_STRERROR
> + #endif
> + 
> + #ifdef HAVE_TZNAME
> + #undef HAVE_TZNAME
> + #endif
> + 
>   #include "postgres.h" 
>   /* system stuff */

is indicative of far worse problems than the one it claims to solve.
This file is in fundamental violation of the first commandment of
Postgres #includes, which is "thou shalt have no other gods before c.h".
We need to put postgres.h *before* the Python.h include.  I don't know
what issues led to the current arrangement but it is fraught with
portability gotchas.  In particular it's just about guaranteed to fail
on platforms where <stdio.h> reacts to _FILE_OFFSET_BITS --- plpython.c
is going to get compiled expecting a different stdio library than the
rest of the backend.
        regards, tom lane

On 04/24/2011 12:25 PM, Tom Lane wrote:
> Andrew Dunstan<andrew@dunslane.net>  writes:
>> The attached patch is intended to clean up a bunch of compiler warnings
>> seen on Windows due to mismatches of signedness or constness, unused
>> variables, redefined macros and a missing prototype.
> BTW, this hunk:
>
>

[snip]

> is indicative of far worse problems than the one it claims to solve.
> This file is in fundamental violation of the first commandment of
> Postgres #includes, which is "thou shalt have no other gods before c.h".
> We need to put postgres.h *before* the Python.h include.  I don't know
> what issues led to the current arrangement but it is fraught with
> portability gotchas.  In particular it's just about guaranteed to fail
> on platforms where<stdio.h>  reacts to _FILE_OFFSET_BITS --- plpython.c
> is going to get compiled expecting a different stdio library than the
> rest of the backend.
>
>             


Well, I certainly noticed that postgres.h wasn't first, but assumed it 
had been sanctioned long ago. It's been that way for a long time.

I'll leave that bit out of this cleanup, but we should look at this 
whole mess separately ASAP.

cheers

andrew

On 04/24/2011 12:14 PM, Tom Lane wrote:
> Peter Eisentraut<peter_e@gmx.net>  writes:
>> The hunk below looks a bit evil.
>> At least a comment would be good to explain why this is necessary.
> Yeah, having to cast away const seems uglier than the original problem.
> Can't we avoid that?


I'm not sure how, since the second argument to send() is declared const, 
and the buf member of a WSABUF isn't. Why is  this worse? The compiler 
warning is effectively telling us that the compiler will be discarding 
constness anyway, isn't it?

> BTW, all of my machines as well as the Single Unix Spec are agreed that
> the second argument to send() is "const void *", not "const char *".
> If we're going to tweak this I think we should make it match exactly.
>


I'm OK with that - not sure if it will generate *more* casts or 
warnings, though.

cheers

andrew

On 04/24/2011 09:11 AM, Andrew Chernow wrote:
> On 4/24/2011 1:29 AM, Andrew Dunstan wrote:
>>
>> The attached patch is intended to clean up a bunch of compiler 
>> warnings seen on
>> Windows due to mismatches of signedness or constness, unused variables,
>> redefined macros and a missing prototype.
>>
>> It doesn't clean up all the warnings by any means, but it fixes quite 
>> a few.
>>
>> One thing I'm a bit confused about is this type of warning:
>>
>> src\backend\utils\misc\guc-file.c(977): warning C4003: not enough actual
>> parameters for macro 'GUC_yywrap'
>>
>>
>> If someone can suggest a good fix That would be nice.
>>
>
> The macro is defined as taking one argument.
>
> // guc-file.c line 354
> #define GUC_yywrap(n) 1
>
> The macro is overriding the prototype declared at line 627, which has 
> a void argument list (assuming YY_SKIP_YYWRAP is !defined).  Since all 
> code references to this do not provide an argument, I'd say the macro 
> is incorrect.


Here's the fix that worked for me:
   *** a/src/tools/msvc/pgflex.bat   --- b/src/tools/msvc/pgflex.bat   ***************   *** 25,32 **** if "%1" ==
"contrib\seg\segscan.l"call :generate %1 contrib\seg\segscan.c   --- 25,38 ----      echo Unknown flex input: %1
exit1 
 
   + REM for non-reentrant scanners we need to fix up yywrap definition   + REM to keep the MS compiler happy   + REM
forreentrant scanners (like the core scanner) we do not   + REM need to (and must not) change the yywrap definition
:generate      flex %3 -o%2 %1   + if errorlevel 1 exit %errorlevel%   + if not "%1" == "src\backend\parser\scan.l"
perl-pi.bak -e "s/yywrap\(n\)/yywrap()/" %2      exit %errorlevel%       :noflex
 


cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> On 04/24/2011 09:11 AM, Andrew Chernow wrote:
>> The macro is overriding the prototype declared at line 627, which has 
>> a void argument list (assuming YY_SKIP_YYWRAP is !defined).  Since all 
>> code references to this do not provide an argument, I'd say the macro 
>> is incorrect.

> Here's the fix that worked for me:

Could we please make this conditional on seeing "%option reentrant" in
the .l file, instead of hard-wiring an assumption about which scanners
are re-entrant?
        regards, tom lane

On sön, 2011-04-24 at 12:25 -0400, Tom Lane wrote:
> This file is in fundamental violation of the first commandment of
> Postgres #includes, which is "thou shalt have no other gods before c.h".
> We need to put postgres.h *before* the Python.h include.  I don't know
> what issues led to the current arrangement but it is fraught with
> portability gotchas.  In particular it's just about guaranteed to fail
> on platforms where <stdio.h> reacts to _FILE_OFFSET_BITS --- plpython.c
> is going to get compiled expecting a different stdio library than the
> rest of the backend.

Here is where this happened:

commit ab6ee1f9fc7039b1e8d8ebf939da3fd55e73efad
Author: Joe Conway <mail@joeconway.com>
Date:   Thu Aug 5 03:10:29 2004 +0000
   Move include for Python.h above postgres.h to eliminate compiler warning.

diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
index 76ea031..07eed86 100644
--- a/src/pl/plpython/plpython.c
+++ b/src/pl/plpython/plpython.c
@@ -29,11 +29,12 @@ * MAINTENANCE, SUPPORT, UPDATES, ENHANCEMENTS, OR MODIFICATIONS. * * IDENTIFICATION
- * $PostgreSQL: pgsql/src/pl/plpython/plpython.c,v 1.52 2004/08/04 21:34:29 tgl Exp $
+ * $PostgreSQL: pgsql/src/pl/plpython/plpython.c,v 1.53 2004/08/05 03:10:29 joe Exp $ *
**********************************************************************/
 
+#include <Python.h>#include "postgres.h"/* system stuff */
@@ -54,7 +55,6 @@#include "utils/syscache.h"#include "utils/typcache.h"
-#include <Python.h>#include <compile.h>#include <eval.h>

If you switch it back around, you indeed get a bunch of annoying
warnings.  This will need some playing around it get right.

On 04/24/2011 07:31 PM, Peter Eisentraut wrote:
> On sön, 2011-04-24 at 12:25 -0400, Tom Lane wrote:
>> This file is in fundamental violation of the first commandment of
>> Postgres #includes, which is "thou shalt have no other gods before c.h".
>> We need to put postgres.h *before* the Python.h include.  I don't know
>> what issues led to the current arrangement but it is fraught with
>> portability gotchas.  In particular it's just about guaranteed to fail
>> on platforms where<stdio.h>  reacts to _FILE_OFFSET_BITS --- plpython.c
>> is going to get compiled expecting a different stdio library than the
>> rest of the backend.
> Here is where this happened:
>
> commit ab6ee1f9fc7039b1e8d8ebf939da3fd55e73efad
> Author: Joe Conway<mail@joeconway.com>
> Date:   Thu Aug 5 03:10:29 2004 +0000
>
>      Move include for Python.h above postgres.h to eliminate compiler warning.
>
> diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
> index 76ea031..07eed86 100644
> --- a/src/pl/plpython/plpython.c
> +++ b/src/pl/plpython/plpython.c
> @@ -29,11 +29,12 @@
>    * MAINTENANCE, SUPPORT, UPDATES, ENHANCEMENTS, OR MODIFICATIONS.
>    *
>    * IDENTIFICATION
> - * $PostgreSQL: pgsql/src/pl/plpython/plpython.c,v 1.52 2004/08/04 21:34:29 tgl Exp $
> + * $PostgreSQL: pgsql/src/pl/plpython/plpython.c,v 1.53 2004/08/05 03:10:29 joe Exp $
>    *
>    *********************************************************************
>    */
>
> +#include<Python.h>
>   #include "postgres.h"
>
>   /* system stuff */
> @@ -54,7 +55,6 @@
>   #include "utils/syscache.h"
>   #include "utils/typcache.h"
>
> -#include<Python.h>
>   #include<compile.h>
>   #include<eval.h>
>
>
> If you switch it back around, you indeed get a bunch of annoying
> warnings.  This will need some playing around it get right.
>
>

On my Linux system the attached compiles without warnings. If this seems
like the way to go I'll investigate more on Windows.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> On my Linux system the attached compiles without warnings. If this seems 
> like the way to go I'll investigate more on Windows.

Hmm ...

> +/*
> + * Save settings the Python headers might override 
> + */
> +#ifdef _POSIX_C_SOURCE
> +#define _PGSAVE_POSIX_C_SOURCE _POSIX_C_SOURCE
> +#undef _POSIX_C_SOURCE
> +#endif
> ...
> +/*
> + * Restore settings the Python headers might have overridden.
> + */
> +#ifdef _PGSAVE_POSIX_C_SOURCE
> +#undef _POSIX_C_SOURCE
> +#define _POSIX_C_SOURCE _PGSAVE_POSIX_C_SOURCE
> +#undef _PGSAVE_POSIX_C_SOURCE
> +#endif

I don't believe that this sequence will restore the contents of the 
_POSIX_C_SOURCE macro to what it was before.  For that matter, it's
not even quite right about ensuring that the macro's defined-ness
status is restored (what if the python headers set _POSIX_C_SOURCE
when it wasn't set before?).  We might not need more than defined-ness
to be right, though.

What in the world are the python headers doing fooling with these
macros, anyway??
        regards, tom lane

On 07/24/2011 11:46 PM, Tom Lane wrote:

[python headers set _POSIX_C_SOURCE and _XOPEN_SOURCE]
> What in the world are the python headers doing fooling with these
> macros, anyway??
>
Good question.    It seems unfriendly. It looks like you're just about guaranteed to get a warning if you include any
systemheader before you include Python.h.
 

So either we have to dance around that or we have to give up the idea that postgres.h must come first. It wouldn't be
thefirst time we've had to do that sort of dance.
 

The reason we get warnings about these and not about many other things it defines (such as the HAVE_foo macros) is that
theseare set to values different from those encountered in the previously included headers.
 

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> On 07/24/2011 11:46 PM, Tom Lane wrote:
>>> [python headers set _POSIX_C_SOURCE and _XOPEN_SOURCE]

>> What in the world are the python headers doing fooling with these
>> macros, anyway??

> The reason we get warnings about these and not about many other things it defines (such as the HAVE_foo macros) is
thatthese are set to values different from those encountered in the previously included headers.
 

That's pretty scary in itself, since it suggests that the Python guys
know or think that changing those values will do something magic.

I'm worried that they are trying to do the same kind of thing that
we are trying to do with our put-postgres.h-first rule, namely ensure
that all loadable modules match the core's idea of libc properties.
If that's what's going on here, and their idea of those properties
is different from our standard build, then we may have worse problems
than a compiler warning.
        regards, tom lane

Andrew Dunstan <andrew@dunslane.net> writes:
> [python headers set _POSIX_C_SOURCE and _XOPEN_SOURCE]

BTW ... so far as I can find, there is no attempt anywhere in the
Postgres sources to set either of these macros.  And my understanding of
their purpose is that *system* headers should not be setting them at
all, rather the application sets them to indicate which POSIX feature
level it would like.  So perhaps the real question here is where the
heck are your conflicting values coming from ...
        regards, tom lane

On 07/25/2011 10:36 AM, Tom Lane wrote:
> Andrew Dunstan<andrew@dunslane.net>  writes:
>> [python headers set _POSIX_C_SOURCE and _XOPEN_SOURCE]
> BTW ... so far as I can find, there is no attempt anywhere in the
> Postgres sources to set either of these macros.  And my understanding of
> their purpose is that *system* headers should not be setting them at
> all, rather the application sets them to indicate which POSIX feature
> level it would like.  So perhaps the real question here is where the
> heck are your conflicting values coming from ...
>
>             


_POSIX_C_SOURCE at least is defined in features.h, which is included by 
huge numbers of system headers, many of which are included by c.h.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> On 07/25/2011 10:36 AM, Tom Lane wrote:
>> Andrew Dunstan<andrew@dunslane.net>  writes:
>>> [python headers set _POSIX_C_SOURCE and _XOPEN_SOURCE]

>> BTW ... so far as I can find, there is no attempt anywhere in the
>> Postgres sources to set either of these macros.  And my understanding of
>> their purpose is that *system* headers should not be setting them at
>> all, rather the application sets them to indicate which POSIX feature
>> level it would like.  So perhaps the real question here is where the
>> heck are your conflicting values coming from ...

> _POSIX_C_SOURCE at least is defined in features.h, which is included by 
> huge numbers of system headers, many of which are included by c.h.

What is features.h, and have its authors read the POSIX standard?
AFAICS they have no business defining this symbol.
        regards, tom lane

On 07/25/2011 10:52 AM, Tom Lane wrote:
> Andrew Dunstan<andrew@dunslane.net>  writes:
>> On 07/25/2011 10:36 AM, Tom Lane wrote:
>>> Andrew Dunstan<andrew@dunslane.net>   writes:
>>>> [python headers set _POSIX_C_SOURCE and _XOPEN_SOURCE]
>>> BTW ... so far as I can find, there is no attempt anywhere in the
>>> Postgres sources to set either of these macros.  And my understanding of
>>> their purpose is that *system* headers should not be setting them at
>>> all, rather the application sets them to indicate which POSIX feature
>>> level it would like.  So perhaps the real question here is where the
>>> heck are your conflicting values coming from ...
>> _POSIX_C_SOURCE at least is defined in features.h, which is included by
>> huge numbers of system headers, many of which are included by c.h.
> What is features.h, and have its authors read the POSIX standard?
> AFAICS they have no business defining this symbol.
>
>             
   [andrew@emma ~]$ rpm -q -f /usr/include/features.h   glibc-headers-2.13-1.x86_64

   [andrew@emma ~]$ cat foo.c   #include <stdio.h>   #include <Python.h>
   main() {}
   [andrew@emma ~]$ gcc -I/usr/include/python2.7/ -c foo.c   In file included from
/usr/include/python2.7/pyconfig.h:6:0,                    from /usr/include/python2.7/Python.h:8,
fromfoo.c:2:   /usr/include/python2.7/pyconfig-64.h:1158:0: warning:   "_POSIX_C_SOURCE" redefined
/usr/include/features.h:214:0:note: this is the location of the   previous definition
 



See now?

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> On 07/25/2011 10:52 AM, Tom Lane wrote:
>> What is features.h, and have its authors read the POSIX standard?
>> AFAICS they have no business defining this symbol.

>     [andrew@emma ~]$ rpm -q -f /usr/include/features.h
>     glibc-headers-2.13-1.x86_64

Oh, for some reason I was thinking this was mingw-specific.

[ pokes around ... ]  I still think it's a bad idea for the header
files to be defining this, but they'll probably point at the part
of the POSIX spec that says the results are undefined if the macro
is changed after the first system header is #included.

I can't immediately think of any way to actually do what you were
trying to do (ie, save and restore the definition of the macro).
I wonder whether it would be good enough to do this:
#include postgres.h
#include everything else we want except python headers
#undef _POSIX_C_SOURCE#undef _XOPEN_SOURCE
#include python headers
... rest of .c file ...

This should only fail if (a) some macro imported from system headers
attempts to test the value of a feature macro, and (b) the results
vary between the system default setting and the setting the python
headers selected.  Neither of these things seem very probable.
        regards, tom lane

On 07/25/2011 12:03 PM, Tom Lane wrote:
> Andrew Dunstan<andrew@dunslane.net>  writes:
>> On 07/25/2011 10:52 AM, Tom Lane wrote:
>>> What is features.h, and have its authors read the POSIX standard?
>>> AFAICS they have no business defining this symbol.
>>      [andrew@emma ~]$ rpm -q -f /usr/include/features.h
>>      glibc-headers-2.13-1.x86_64
> Oh, for some reason I was thinking this was mingw-specific.
>
> [ pokes around ... ]  I still think it's a bad idea for the header
> files to be defining this, but they'll probably point at the part
> of the POSIX spec that says the results are undefined if the macro
> is changed after the first system header is #included.
>
> I can't immediately think of any way to actually do what you were
> trying to do (ie, save and restore the definition of the macro).
> I wonder whether it would be good enough to do this:
>
>     #include postgres.h
>
>     #include everything else we want except python headers
>
>     #undef _POSIX_C_SOURCE
>     #undef _XOPEN_SOURCE
>
>     #include python headers
>
>     ... rest of .c file ...
>
> This should only fail if (a) some macro imported from system headers
> attempts to test the value of a feature macro, and (b) the results
> vary between the system default setting and the setting the python
> headers selected.  Neither of these things seem very probable.


OK, attached gives a clean build and passes regression on my Windows box
that builds with Python. I had to undefine a few more things and save
and restore our *snprintf settings (with code borrowed from plperl.h,
where we did this sort of cleanup a while ago).

cheers

andrew