Re: Problem with ControlFileData structure being ABI depe ndent

Started by Magnus Haganderabout 18 years ago7 messages

magnus@hagander.net

about 18 years ago

I guess my advice would be to see if we can define _USE_32BIT_TIME_T
in port/win32.h and make it go away that way. It'd definitely be nice
if MSVC and Mingw builds weren't binary-incompatible.

The attached patch defines it in the MSVC project files along with the
other API-config related macros. It fixes all the offsets so they match
mingw, but the CRC is still different for some as-yet unknown reason...

Is the project file really the proper place? Consider an add-on module - wouldn't we want the setting to go in that one as well? meaning we'd have it in a header file instead?

/Magnus

Gregory Stark

stark@enterprisedb.com

about 18 years ago

In reply to: Magnus Hagander (#1)

"Magnus Hagander" <magnus@hagander.net> writes:

I guess my advice would be to see if we can define _USE_32BIT_TIME_T
in port/win32.h and make it go away that way. It'd definitely be nice
if MSVC and Mingw builds weren't binary-incompatible.

The attached patch defines it in the MSVC project files along with the
other API-config related macros. It fixes all the offsets so they match
mingw, but the CRC is still different for some as-yet unknown reason...

Is the project file really the proper place? Consider an add-on module -
wouldn't we want the setting to go in that one as well? meaning we'd have it in
a header file instead?

For an add-on module which is actually using time_t to interface with Postgres
it had better define _USE_... itself or else it risks having some files use
them and some not.

An alternative is leaving it in the project file but putting something like
this in c.h:

#ifdef WIN32
#ifndef _USE_32BIT_TIME_T
#error "Postgres uses 32 bit time_t add #define _USE_32BIT_TIME_T on Windows
#endif
#endif

For modules which *do* use time_t this is safer. However for modules which
don't use time_t it'll be an unnecessary hassle.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's RemoteDBA services!

Dave Page

dpage@postgresql.org

about 18 years ago

In reply to: Gregory Stark (#2)

Gregory Stark wrote:

"Magnus Hagander" <magnus@hagander.net> writes:

I guess my advice would be to see if we can define _USE_32BIT_TIME_T
in port/win32.h and make it go away that way. It'd definitely be nice
if MSVC and Mingw builds weren't binary-incompatible.

The attached patch defines it in the MSVC project files along with the
other API-config related macros. It fixes all the offsets so they match
mingw, but the CRC is still different for some as-yet unknown reason...

Is the project file really the proper place? Consider an add-on module -
wouldn't we want the setting to go in that one as well? meaning we'd have it in
a header file instead?

For an add-on module which is actually using time_t to interface with Postgres
it had better define _USE_... itself or else it risks having some files use
them and some not.

Yeah, that was roughly my thinking having run into exactly that problem
whilst playing around earlier. We also have other 'api config' macros in
project files (though I appreciate those aren't changing ABI related
stuff) so it seemed a natural place.

My other thought was that many of the addons this is likely to affect
will be packaged as additional contrib modules, so they will
automatically pickup the macro if dropped in /contrib and built from there.

An alternative is leaving it in the project file but putting something like
this in c.h:

#ifdef WIN32
#ifndef _USE_32BIT_TIME_T
#error "Postgres uses 32 bit time_t add #define _USE_32BIT_TIME_T on Windows
#endif
#endif

For modules which *do* use time_t this is safer. However for modules which
don't use time_t it'll be an unnecessary hassle.

Yeah, that might be useful. We could narrow the scope of platforms
affected by using _MSC_VER instead of WIN32.

Tom Lane

tgl@sss.pgh.pa.us

about 18 years ago

In reply to: Dave Page (#3)

Dave Page <dpage@postgresql.org> writes:

Gregory Stark wrote:

An alternative is leaving it in the project file but putting something like
this in c.h:

Put it in win32.h, please. c.h shouldn't get cluttered with
platform-specific kluges when there's no need for it.

Is there a good reason not to just #define _USE_32BIT_TIME_T in win32.h?

regards, tom lane

Dave Page

dpage@postgresql.org

about 18 years ago

In reply to: Tom Lane (#4)

Tom Lane wrote:

Dave Page <dpage@postgresql.org> writes:

Gregory Stark wrote:

An alternative is leaving it in the project file but putting something like
this in c.h:

Put it in win32.h, please. c.h shouldn't get cluttered with
platform-specific kluges when there's no need for it.

Is there a good reason not to just #define _USE_32BIT_TIME_T in win32.h?

Yeah, the fact that addons may then end up partially compiled with and
partially without it being defined. It we just have it error as Greg
suggested, then it will force the authors to define it themselves, and
if they get that wrong it's their fault not ours.

Dave Page

dpage@postgresql.org

about 18 years ago

In reply to: Dave Page (#5)

1 attachment(s)

Re: [HACKERS] Problem with ControlFileData structure being ABI depe ndent

Dave Page wrote:

Tom Lane wrote:

Dave Page <dpage@postgresql.org> writes:

Gregory Stark wrote:

An alternative is leaving it in the project file but putting
something like
this in c.h:

Put it in win32.h, please. c.h shouldn't get cluttered with
platform-specific kluges when there's no need for it.

Is there a good reason not to just #define _USE_32BIT_TIME_T in win32.h?

Yeah, the fact that addons may then end up partially compiled with and
partially without it being defined. It we just have it error as Greg
suggested, then it will force the authors to define it themselves, and
if they get that wrong it's their fault not ours.

Patch attached.

Attachments:

32bit_time_t-2.difftext/x-patch; name=32bit_time_t-2.diffDownload

Index: src/include/port/win32.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/port/win32.h,v
retrieving revision 1.81
diff -c -r1.81 win32.h
*** src/include/port/win32.h	24 Nov 2007 01:55:26 -0000	1.81
--- src/include/port/win32.h	10 Dec 2007 09:42:44 -0000
***************
*** 45,50 ****
--- 45,61 ----

  #define USES_WINSOCK

+ /*
+  * Ensure that anyone building an extension is using a 32 bit time_t.
+  * On Mingw/Msys, that should always be the case, but MSVC++ defaults
+  * to 64 bits. We set that for our own build in the project files
+  */
+ #ifdef WIN32
+ #ifndef _USE_32BIT_TIME_T
+ #error "Postgres uses 32 bit time_t - add #define _USE_32BIT_TIME_T on Windows
+ #endif
+ #endif
+
  /* defines for dynamic linking on Win32 platform */
  #if defined(WIN32) || defined(__CYGWIN__)

Index: src/tools/msvc/Project.pm
===================================================================
RCS file: /projects/cvsroot/pgsql/src/tools/msvc/Project.pm,v
retrieving revision 1.14
diff -c -r1.14 Project.pm
*** src/tools/msvc/Project.pm	21 Aug 2007 15:10:41 -0000	1.14
--- src/tools/msvc/Project.pm	7 Dec 2007 11:14:29 -0000
***************
*** 489,495 ****
  	ConfigurationType="$cfgtype" UseOfMFC="0" ATLMinimizesCRunTimeLibraryUsage="FALSE" CharacterSet="2" WholeProgramOptimization="$p->{wholeopt}">
  	<Tool Name="VCCLCompilerTool" Optimization="$p->{opt}"
  		AdditionalIncludeDirectories="$self->{prefixincludes}src/include;src/include/port/win32;src/include/port/win32_msvc;$self->{includes}"
! 		PreprocessorDefinitions="WIN32;_WINDOWS;__WINDOWS__;__WIN32__;EXEC_BACKEND;WIN32_STACK_RLIMIT=4194304;_CRT_SECURE_NO_DEPRECATE;_CRT_NONSTDC_NO_DEPRECATE$self->{defines}$p->{defs}"
  		StringPooling="$p->{strpool}"
  		RuntimeLibrary="$p->{runtime}" DisableSpecificWarnings="$self->{disablewarnings}"
  EOF
--- 489,495 ----
  	ConfigurationType="$cfgtype" UseOfMFC="0" ATLMinimizesCRunTimeLibraryUsage="FALSE" CharacterSet="2" WholeProgramOptimization="$p->{wholeopt}">
  	<Tool Name="VCCLCompilerTool" Optimization="$p->{opt}"
  		AdditionalIncludeDirectories="$self->{prefixincludes}src/include;src/include/port/win32;src/include/port/win32_msvc;$self->{includes}"
! 		PreprocessorDefinitions="WIN32;_WINDOWS;__WINDOWS__;__WIN32__;EXEC_BACKEND;WIN32_STACK_RLIMIT=4194304;_CRT_SECURE_NO_DEPRECATE;_CRT_NONSTDC_NO_DEPRECATE;_USE_32BIT_TIME_T$self->{defines}$p->{defs}"
  		StringPooling="$p->{strpool}"
  		RuntimeLibrary="$p->{runtime}" DisableSpecificWarnings="$self->{disablewarnings}"
  EOF

Magnus Hagander

magnus@hagander.net

about 18 years ago

In reply to: Dave Page (#6)

Re: [HACKERS] Problem with ControlFileData structure being ABI depe ndent

On Mon, Dec 10, 2007 at 09:56:39AM +0000, Dave Page wrote:

Dave Page wrote:

Tom Lane wrote:

Dave Page <dpage@postgresql.org> writes:

Gregory Stark wrote:

An alternative is leaving it in the project file but putting
something like
this in c.h:

Put it in win32.h, please. c.h shouldn't get cluttered with
platform-specific kluges when there's no need for it.

Is there a good reason not to just #define _USE_32BIT_TIME_T in win32.h?

Yeah, the fact that addons may then end up partially compiled with and
partially without it being defined. It we just have it error as Greg
suggested, then it will force the authors to define it themselves, and
if they get that wrong it's their fault not ours.

Patch attached.

Applide with two tiny modifications - the missing quote taht you mentioned,
and changed the #ifdef to only affect MSVC and not mingw.

//Magnus