Thread: Re: Problem with ControlFileData structure being ABI depe ndent
> > 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 aswell? meaning we'd have it in a header file instead? /Magnus
"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!
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. /D
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
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. /D
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.
/D
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
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