Fix compiler warnings on 64-bit Windows

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

Fix compiler warnings on 64-bit Windows

Peter Eisentraut-6
GCC reports various instances of

     warning: cast to pointer from integer of different size
[-Wint-to-pointer-cast]
     warning: cast from pointer to integer of different size
[-Wpointer-to-int-cast]

and MSVC equivalently

     warning C4312: 'type cast': conversion from 'int' to 'void *' of
greater size
     warning C4311: 'type cast': pointer truncation from 'void *' to 'long'

in ECPG test files.  This is because void* and long are cast back and
forth, but on 64-bit Windows, these have different sizes.  Fix by
using intptr_t instead.

The code actually worked fine because the integer values in use are
all small.  So this is just to get the test code to compile warning-free.

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

0001-Fix-compiler-warnings-on-64-bit-Windows.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Fix compiler warnings on 64-bit Windows

Tom Lane-2
Peter Eisentraut <[hidden email]> writes:
> GCC reports various instances of
>      warning: cast to pointer from integer of different size
> [-Wint-to-pointer-cast]
>      warning: cast from pointer to integer of different size
> [-Wpointer-to-int-cast]
> in ECPG test files.  This is because void* and long are cast back and
> forth, but on 64-bit Windows, these have different sizes.  Fix by
> using intptr_t instead.

Hm.  Silencing the warnings is a laudable goal, but I'm very dubious
of allowing these test files to depend on pg_config.h.  That doesn't
correspond to real-world ECPG usage, so it seems likely that it could
come back to bite us some day.

According to C99 and POSIX, intptr_t should be provided by <stdint.h> ...
now that we're requiring C99, can we get away with just #include'ing
that directly in these test files?

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Fix compiler warnings on 64-bit Windows

Peter Eisentraut-6
On 2020-02-13 16:19, Tom Lane wrote:
> According to C99 and POSIX, intptr_t should be provided by <stdint.h> ...
> now that we're requiring C99, can we get away with just #include'ing
> that directly in these test files?

I think in the past we were worried about the C library not being fully
C99.  But the build farm indicates that even the trailing edge OS X and
HP-UX members have it, so I'm content to require it.  Then we should
probably remove the Autoconf tests altogether.

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


Reply | Threaded
Open this post in threaded view
|

Re: Fix compiler warnings on 64-bit Windows

Tom Lane-2
Peter Eisentraut <[hidden email]> writes:
> On 2020-02-13 16:19, Tom Lane wrote:
>> According to C99 and POSIX, intptr_t should be provided by <stdint.h> ...
>> now that we're requiring C99, can we get away with just #include'ing
>> that directly in these test files?

> I think in the past we were worried about the C library not being fully
> C99.  But the build farm indicates that even the trailing edge OS X and
> HP-UX members have it, so I'm content to require it.  Then we should
> probably remove the Autoconf tests altogether.

Yeah, I think that the C99 requirement has obsoleted a number of configure
tests and related hackery in c.h.  We just haven't got round to cleaning
that up yet.

BTW: I'm still concerned about the possibility of the C library being
less than C99.  The model that was popular back then, and which still
exists on e.g. gaur, was that you could install a C99 *compiler* on
a pre-C99 system, and the compiler would bring its own standard header
files as necessary.  While I don't have the machine booted up to check,
I'm pretty sure that gaur's <stdint.h> is being supplied by the gcc
installation not directly from /usr/include.  On the other hand, that
compiler installation is still dependent on the vendor-supplied libc.

So the sorts of tests I think we can get away with removing have to do
with the presence of C99-required headers, macros, typedefs, etc.
Anything that is checking the presence or behavior of code in libc,
we probably need to be more careful about.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Fix compiler warnings on 64-bit Windows

Peter Eisentraut-6
On 2020-02-14 15:52, Tom Lane wrote:

> Yeah, I think that the C99 requirement has obsoleted a number of configure
> tests and related hackery in c.h.  We just haven't got round to cleaning
> that up yet.
>
> BTW: I'm still concerned about the possibility of the C library being
> less than C99.  The model that was popular back then, and which still
> exists on e.g. gaur, was that you could install a C99 *compiler* on
> a pre-C99 system, and the compiler would bring its own standard header
> files as necessary.  While I don't have the machine booted up to check,
> I'm pretty sure that gaur's <stdint.h> is being supplied by the gcc
> installation not directly from /usr/include.  On the other hand, that
> compiler installation is still dependent on the vendor-supplied libc.
Yeah, stdint.h belongs to the compiler, whereas intttypes.h belongs to
the C library.  So if we require a C99 compiler we can get rid of all
tests and workarounds for stdint.h missing.  Patch attached.

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

0001-Require-stdint.h.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Fix compiler warnings on 64-bit Windows

Tom Lane-2
Peter Eisentraut <[hidden email]> writes:
> On 2020-02-14 15:52, Tom Lane wrote:
>> BTW: I'm still concerned about the possibility of the C library being
>> less than C99.  The model that was popular back then, and which still
>> exists on e.g. gaur, was that you could install a C99 *compiler* on
>> a pre-C99 system, and the compiler would bring its own standard header
>> files as necessary.  While I don't have the machine booted up to check,
>> I'm pretty sure that gaur's <stdint.h> is being supplied by the gcc
>> installation not directly from /usr/include.  On the other hand, that
>> compiler installation is still dependent on the vendor-supplied libc.

> Yeah, stdint.h belongs to the compiler, whereas intttypes.h belongs to
> the C library.  So if we require a C99 compiler we can get rid of all
> tests and workarounds for stdint.h missing.  Patch attached.

I tried this on gaur's host, and got:

$ make -s
In file included from ../../src/include/postgres_fe.h:25,
                 from base64.c:18:
../../src/include/c.h:67:20: stdint.h: No such file or directory
make[2]: *** [base64.o] Error 1
make[1]: *** [all-common-recurse] Error 2
make: *** [all-src-recurse] Error 2

Ooops.  Poking around, it looks like this version of gcc has brought its
own stdbool.h, but not stdint.h:

$ ls /usr/include/std*
/usr/include/std_space.h   /usr/include/stdio.h
/usr/include/stdarg.h      /usr/include/stdlib.h
/usr/include/stddef.h
$ find /opt/gcc-3.4.6 -name 'std*.h'
/opt/gcc-3.4.6/lib/gcc/hppa2.0-hp-hpux10.20/3.4.6/include/stdarg.h
/opt/gcc-3.4.6/lib/gcc/hppa2.0-hp-hpux10.20/3.4.6/include/stdbool.h
/opt/gcc-3.4.6/lib/gcc/hppa2.0-hp-hpux10.20/3.4.6/include/stddef.h
/opt/gcc-3.4.6/lib/gcc/hppa2.0-hp-hpux10.20/3.4.6/include/stdio.h
/opt/gcc-3.4.6/lib/gcc/hppa2.0-hp-hpux10.20/3.4.6/include/stdlib.h
/opt/gcc-3.4.6/lib/gcc/hppa2.0-hp-hpux10.20/3.4.6/install-tools/include/stdarg.h
/opt/gcc-3.4.6/lib/gcc/hppa2.0-hp-hpux10.20/3.4.6/install-tools/include/stdbool.h
/opt/gcc-3.4.6/lib/gcc/hppa2.0-hp-hpux10.20/3.4.6/install-tools/include/stddef.h

Kind of annoying.  Perhaps more recent gcc versions fixed that?
Anyway, this seems like a bit of a blocker for this idea, at least
unless I update or retire this buildfarm critter.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Fix compiler warnings on 64-bit Windows

Tom Lane-2
I wrote:
> Ooops.  Poking around, it looks like this version of gcc has brought its
> own stdbool.h, but not stdint.h:
> ...
> Kind of annoying.  Perhaps more recent gcc versions fixed that?

Here we go, in the gcc 4.5.x release notes:

    GCC now ensures that a C99-conforming <stdint.h> is present on most
    targets, and uses information about the types in this header to
    implement the Fortran bindings to those types. GCC does not ensure the
    presence of such a header, and does not implement the Fortran
    bindings, on the following targets: NetBSD, VxWorks, VMS, SymbianOS,
    WinCE, LynxOS, Netware, QNX, Interix, TPF.

4.5 seems annoyingly recent for this purpose (barely 10 years old).
Also, I'd previously tried and failed to use 4.2.4 and 4.0.4 on that
platform --- they didn't seem to be able to cope with the old header
files.  (Now I wonder if the lack of stdint.h had something to do
with it... although those versions did build, they just were buggy.)

Anyway, I'll have a go at updating gaur to use 4.5.x.  There is a
sane-looking stdint.h on my second-oldest dinosaur, prairiedog.
Don't know about the situation on Windows, though.  We might want
to take a close look at NetBSD, too, based on the GCC notes.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Fix compiler warnings on 64-bit Windows

Juan José Santamaría Flecha


On Mon, Feb 17, 2020 at 4:52 PM Tom Lane <[hidden email]> wrote:

Anyway, I'll have a go at updating gaur to use 4.5.x.  There is a
sane-looking stdint.h on my second-oldest dinosaur, prairiedog.
Don't know about the situation on Windows, though.  We might want
to take a close look at NetBSD, too, based on the GCC notes.


As for Windows, stdint.h was included in VS2010, and currently Postgres supports VS2013 to 2019.

Regards
Reply | Threaded
Open this post in threaded view
|

Re: Fix compiler warnings on 64-bit Windows

Tom Lane-2
=?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <[hidden email]> writes:
> On Mon, Feb 17, 2020 at 4:52 PM Tom Lane <[hidden email]> wrote:
>> Anyway, I'll have a go at updating gaur to use 4.5.x.  There is a
>> sane-looking stdint.h on my second-oldest dinosaur, prairiedog.
>> Don't know about the situation on Windows, though.  We might want
>> to take a close look at NetBSD, too, based on the GCC notes.

> As for Windows, stdint.h was included in VS2010, and currently Postgres
> supports VS2013 to 2019.

I've now updated gaur to gcc 4.5.4 (took a little more hair-pulling
than I would have wished).  I confirm that 0001-Require-stdint.h.patch
works in that environment, so I think you can go ahead and push it.

I think there is room for more extensive trimming of no-longer-useful
configure checks, but I'll start a separate thread about that.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Fix compiler warnings on 64-bit Windows

Peter Eisentraut-6
On 2020-02-20 17:24, Tom Lane wrote:

> =?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <[hidden email]> writes:
>> On Mon, Feb 17, 2020 at 4:52 PM Tom Lane <[hidden email]> wrote:
>>> Anyway, I'll have a go at updating gaur to use 4.5.x.  There is a
>>> sane-looking stdint.h on my second-oldest dinosaur, prairiedog.
>>> Don't know about the situation on Windows, though.  We might want
>>> to take a close look at NetBSD, too, based on the GCC notes.
>
>> As for Windows, stdint.h was included in VS2010, and currently Postgres
>> supports VS2013 to 2019.
>
> I've now updated gaur to gcc 4.5.4 (took a little more hair-pulling
> than I would have wished).  I confirm that 0001-Require-stdint.h.patch
> works in that environment, so I think you can go ahead and push it.

Done, and also the appropriately reworked Windows warnings patch from
the beginning of the thread.

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