libpq build problem with <io.h> on MS VC++ 7.0

Hi all

When building libpq using Visual Studio .NET 2002 (ie Visual C++ 7.0), I
encounter this error:

fe-lobj.c
C:\Program Files\Microsoft Visual Studio .NET\Vc7\include\io.h(205) : error
C2375: 'pgrename' : redefinition; different linkage
c:\libs\postgresql\src\include\port.h(148) : see declaration of
'pgrename'
C:\Program Files\Microsoft Visual Studio .NET\Vc7\include\io.h(275) : error
C2375: 'pgunlink' : redefinition; different linkage
c:\libs\postgresql\src\include\port.h(149) : see declaration of
'pgunlink'

As rename/unlink are #define'd to pgrename/pgunlink prior to <io.h>'s inclusion.

Simply reordering the headers fixes the problem (see attachment).

I believe this may be a problem on my compiler, but not necessarily others,
due to an additional compiler directive on the definition in io.h:

#define _CRTIMP __declspec(dllimport)
...
_CRTIMP int __cdecl unlink(const char *);

port.h's definition of pgrename() is obviously lacking a __declspec(dllimport).

Regards,

--
Andrew Francis
Lead Developer - Software
Family Health Network

OK, I have improved your comment and applied the patch. I mentioned the
problem is only on MS C, but we might as well include io.h there on all
Win32 platforms.

---------------------------------------------------------------------------

Andrew Francis wrote:
> Hi all
>
> When building libpq using Visual Studio .NET 2002 (ie Visual C++ 7.0), I
> encounter this error:
>
> fe-lobj.c
> C:\Program Files\Microsoft Visual Studio .NET\Vc7\include\io.h(205) : error
> C2375: 'pgrename' : redefinition; different linkage
> c:\libs\postgresql\src\include\port.h(148) : see declaration of
> 'pgrename'
> C:\Program Files\Microsoft Visual Studio .NET\Vc7\include\io.h(275) : error
> C2375: 'pgunlink' : redefinition; different linkage
> c:\libs\postgresql\src\include\port.h(149) : see declaration of
> 'pgunlink'
>
>
> As rename/unlink are #define'd to pgrename/pgunlink prior to <io.h>'s inclusion.
>
>
> Simply reordering the headers fixes the problem (see attachment).
>
>
> I believe this may be a problem on my compiler, but not necessarily others,
> due to an additional compiler directive on the definition in io.h:
>
> #define _CRTIMP __declspec(dllimport)
> ...
> _CRTIMP int __cdecl unlink(const char *);
>
> port.h's definition of pgrename() is obviously lacking a __declspec(dllimport).
>
>
> Regards,
>
> --
> Andrew Francis
> Lead Developer - Software
> Family Health Network
>
>

> *** fe-lobj-old.c Wed Aug 11 14:56:16 2004
> --- fe-lobj.c Wed Aug 11 14:55:55 2004
> ***************
> *** 13,33 ****
> *-------------------------------------------------------------------------
> */
> - #include "postgres_fe.h"
>
> ! #include <fcntl.h>
> ! #include <sys/stat.h>
> ! #include <errno.h>
>
> #ifdef WIN32
> #include "win32.h"
> - #include "io.h"
> #else
> #include <unistd.h>
> #endif
>
> #include "libpq-fe.h"
> #include "libpq-int.h"
> #include "libpq/libpq-fs.h" /* must come after sys/stat.h */
> -
>
> #define LO_BUFSIZE 8192
> --- 13,40 ----
> *-------------------------------------------------------------------------
> */
>
> ! #ifdef WIN32
> ! /*
> ! * As unlink/rename are #define'd in port.h (via postgres_fe.h), io.h
> ! * must be included first.
> ! */
> ! #include <io.h>
> ! #endif
> !
> ! #include "postgres_fe.h"
>
> #ifdef WIN32
> #include "win32.h"
> #else
> #include <unistd.h>
> #endif
>
> + #include <fcntl.h>
> + #include <sys/stat.h>
> + #include <errno.h>
> +
> #include "libpq-fe.h"
> #include "libpq-int.h"
> #include "libpq/libpq-fs.h" /* must come after sys/stat.h */
>
> #define LO_BUFSIZE 8192

>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faqs/FAQ.html

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> OK, I have improved your comment and applied the patch. I mentioned the
> problem is only on MS C, but we might as well include io.h there on all
> Win32 platforms.

I am actually fairly uncomfortable with this solution. We learned the
hard way awhile back that we want to include postgres.h or siblings
*before* any system header file, because the system header files may
default to things we do not want otherwise. (Typical examples have to
do with 32-bit vs 64-bit file offsets.) Perhaps it will be okay to
violate that rule on this one specific file on this one specific
platform, but I would not bet on it.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > OK, I have improved your comment and applied the patch. I mentioned the
> > problem is only on MS C, but we might as well include io.h there on all
> > Win32 platforms.
>
> I am actually fairly uncomfortable with this solution. We learned the
> hard way awhile back that we want to include postgres.h or siblings
> *before* any system header file, because the system header files may
> default to things we do not want otherwise. (Typical examples have to
> do with 32-bit vs 64-bit file offsets.) Perhaps it will be okay to
> violate that rule on this one specific file on this one specific
> platform, but I would not bet on it.

The only other option I can think of is to #undef those to defines,
include io.h, then re-include port.h? Is that better?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> The only other option I can think of is to #undef those to defines,
> include io.h, then re-include port.h? Is that better?

How about not #define'ing rename() etc in port.h in the first place?

We could put
#ifdef WIN32
#define rename(x) pgrename(x)
#endif
into those very few .c files that need it. (I'm assuming that
forgetting to include this in a file that calls rename() will yield an
obvious build failure on Windows --- if not then the idea is no good.)

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > The only other option I can think of is to #undef those to defines,
> > include io.h, then re-include port.h? Is that better?
>
> How about not #define'ing rename() etc in port.h in the first place?
>
> We could put
> #ifdef WIN32
> #define rename(x) pgrename(x)
> #endif
> into those very few .c files that need it. (I'm assuming that
> forgetting to include this in a file that calls rename() will yield an
> obvious build failure on Windows --- if not then the idea is no good.)

I think they have rename --- it just isn't reliable like ours is.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
>
>>The only other option I can think of is to #undef those to defines,
>>include io.h, then re-include port.h? Is that better?
>
> How about not #define'ing rename() etc in port.h in the first place?
>
> We could put
> #ifdef WIN32
> #define rename(x) pgrename(x)
> #endif
> into those very few .c files that need it.

How about avoiding #define altogether, and:

- Always use pgrename/pgunlink instead of rename/unlink

- Provide stubs for non-Win32 systems

#ifndef WIN32
int pgrename(const char *from, const char *to) {
return rename(from,to);
}
#endif

#define is evil, afterall.

--
Andrew Francis
Lead Developer - Software
Family Health Network

Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > > The only other option I can think of is to #undef those to defines,
> > > include io.h, then re-include port.h? Is that better?
> >
> > How about not #define'ing rename() etc in port.h in the first place?
> >
> > We could put
> > #ifdef WIN32
> > #define rename(x) pgrename(x)
> > #endif
> > into those very few .c files that need it. (I'm assuming that
> > forgetting to include this in a file that calls rename() will yield an
> > obvious build failure on Windows --- if not then the idea is no good.)
>
> I think they have rename --- it just isn't reliable like ours is.

One other thing we could do is to do the out-of-order includes only for
MS VC. That is the only platform that needs it, and let the other
Win32's do the regular include.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Andrew Francis wrote:
> Tom Lane wrote:
> > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> >
> >>The only other option I can think of is to #undef those to defines,
> >>include io.h, then re-include port.h? Is that better?
> >
> > How about not #define'ing rename() etc in port.h in the first place?
> >
> > We could put
> > #ifdef WIN32
> > #define rename(x) pgrename(x)
> > #endif
> > into those very few .c files that need it.
>
> How about avoiding #define altogether, and:
>
> - Always use pgrename/pgunlink instead of rename/unlink
>
> - Provide stubs for non-Win32 systems
>
> #ifndef WIN32
> int pgrename(const char *from, const char *to) {
> return rename(from,to);
> }
> #endif
>

We could do it but we have avoided that for cases where Unix would just
be a pass-through.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:
> Bruce Momjian wrote:
> > Tom Lane wrote:
> > > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > > > The only other option I can think of is to #undef those to defines,
> > > > include io.h, then re-include port.h? Is that better?
> > >
> > > How about not #define'ing rename() etc in port.h in the first place?
> > >
> > > We could put
> > > #ifdef WIN32
> > > #define rename(x) pgrename(x)
> > > #endif
> > > into those very few .c files that need it. (I'm assuming that
> > > forgetting to include this in a file that calls rename() will yield an
> > > obvious build failure on Windows --- if not then the idea is no good.)
> >
> > I think they have rename --- it just isn't reliable like ours is.
>
> One other thing we could do is to do the out-of-order includes only for
> MS VC. That is the only platform that needs it, and let the other
> Win32's do the regular include.

Ah, one thing we have done is to reference everything as pg* and define
it to be the libc function on unix and give a compatibility function on
Win32. We do that with pgpipe. That might be our best solution.

Personally I never liked defining _away_ from libc function names
(rename -> pgrename), but I think defining _to_ libc names is fine.

Let me fix this tomorrow. I always felt we had too many tricks in
adding these functions and was never sure we had it consistent.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Andrew Francis wrote:
>> How about avoiding #define altogether, and:
>> - Always use pgrename/pgunlink instead of rename/unlink

> We could do it but we have avoided that for cases where Unix would just
> be a pass-through.

To put that in a more positive light: we like to think that our code is
Posix-compliant and runs in a Posix-compliant environment. We're not
thrilled about introducing non-Posix-isms for the convenience of one
platform ... especially if there's no easy way to enforce that the
nonstandard coding convention be used.

Back on track: if rename() does exist under Windows then my idea is
unreliable. Any other thoughts? How about #including <io.h> in port.h
(for Windows only of course) before we #define these things?

regards, tom lane

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Ah, one thing we have done is to reference everything as pg* and define
> it to be the libc function on unix and give a compatibility function on
> Win32. We do that with pgpipe. That might be our best solution.

We should do that sort of thing only as a very last resort. It's
particularly bad when we cannot easily enforce that all references use
the pgxxx function. pgpipe is manageable because there are very few
places that need to use it, but the same cannot be said of rename.

Personally I'd rather get rid of pgpipe as well ...

regards, tom lane

Bruce,

I posted the attached patch 4 days ago, with the comment
"The attached patch will redefine unlink and rename only if FRONTEND is
not defined.".

I still believe this a good way to fix it.

Tom Lane wrote:
>
>
> To put that in a more positive light: we like to think that our code is
> Posix-compliant and runs in a Posix-compliant environment. We're not
> thrilled about introducing non-Posix-isms for the convenience of one
> platform ... especially if there's no easy way to enforce that the
> nonstandard coding convention be used.
>
> Back on track: if rename() does exist under Windows then my idea is
> unreliable. Any other thoughts? How about #including <io.h> in port.h
> (for Windows only of course) before we #define these things?

Probably won't work, because pgrename and rename do not have the same
definition/linkage.

Regards,
Andreas

Andreas Pflug <pgadmin(at)pse-consulting(dot)de> writes:
>> Back on track: if rename() does exist under Windows then my idea is
>> unreliable. Any other thoughts? How about #including <io.h> in port.h
>> (for Windows only of course) before we #define these things?

> Probably won't work, because pgrename and rename do not have the same
> definition/linkage.

So? The compiler would see something like

extern linkage_spec rename(...);

extern int pgrename(...);

#define rename pgrename

so the conflict of linkage spec shouldn't bother anything.

> I posted the attached patch 4 days ago, with the comment
> "The attached patch will redefine unlink and rename only if FRONTEND is
> not defined.".

> I still believe this a good way to fix it.

The conflict would still exist. AFAICS it's pure chance that it's
not affecting any backend modules at the moment.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Ah, one thing we have done is to reference everything as pg* and define
> > it to be the libc function on unix and give a compatibility function on
> > Win32. We do that with pgpipe. That might be our best solution.
>
> We should do that sort of thing only as a very last resort. It's
> particularly bad when we cannot easily enforce that all references use
> the pgxxx function. pgpipe is manageable because there are very few
> places that need to use it, but the same cannot be said of rename.
>
> Personally I'd rather get rid of pgpipe as well ...

Yes, that is true that we can't know we hit all the places that need to
use pg*.

I added a comment in port.h:

* There is some inconsistency here because sometimes we require pg*, like
* pgpipe, but in other cases we define rename to pgrename just on Win32.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Andrew Francis wrote:
> >> How about avoiding #define altogether, and:
> >> - Always use pgrename/pgunlink instead of rename/unlink
>
> > We could do it but we have avoided that for cases where Unix would just
> > be a pass-through.
>
> To put that in a more positive light: we like to think that our code is
> Posix-compliant and runs in a Posix-compliant environment. We're not
> thrilled about introducing non-Posix-isms for the convenience of one
> platform ... especially if there's no easy way to enforce that the
> nonstandard coding convention be used.
>
> Back on track: if rename() does exist under Windows then my idea is
> unreliable. Any other thoughts? How about #including <io.h> in port.h
> (for Windows only of course) before we #define these things?

Sure, should we do that? I see 12 mentions of io.h in the code, and we
already include some win32 includes in port.h.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073