RFC patch, 2 partsMSVC: Fix an "incompatible pointer types" compiler warning

In particular, the following warning is issued while compiling compat/msvc.c:

    ...mingw.c(223) : warning C4133: 'function' : incompatible \
types - from '_stati64 *' to '_stat64 *'

which relates to a call of _fstati64() in the mingw_fstat() function definition.

This is caused by various layers of macro magic and attempts to avoid macro redefinition compiler warnings. For example, the call to _fstati64() mentioned above is actually a call to _fstat64(), since macro _USE_32BIT_TIME_T is not defined, and expects a pointer to a struct _stat64 rather than the struct _stati64 which is passed to mingw_fstat().

The definition of struct _stati64 given in compat/msvc.h had the same "shape" as the definition of struct _stat64, so the call to _fstat64() does not actually cause any runtime errors, but the structure types are indeed incompatible. Also, the "shape" of struct _stati64 changes, depending on the _USE_32BIT_TIME_T macro, since the time_t type is defined as either __time64_t or __time32_t.

When _USE_32BIT_TIME_T is defined, the call to _fstati64() is actually a call to _fstat32i64() and expects a struct _stat32i64 pointer parameter. (struct _stati64 would again have the same "shape" as struct _stat32i64).

The _USE_32BIT_TIME_T macro, along with all of the additional structure type definitions, function definitions, and overloading macro magic was introduced in msvc 2005.

In order to avoid the compiler warning, we use the appropriate structure type names (and function names) from the msvc headers. This allows us to compile with -D_USE_32BIT_TIME_T if necessary. Note that the original mingw code should work with an msvc/sdk prior to 2005. We attempt to detect this by checking for _stati64 being defined as a macro and, if not defined, conditionally compiling the original code.

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---

The first version of this patch was much simpler; the diffstat showed a net decrease of 15 lines of code! The extra fat comes from additions to the compat/mingw.h file. The original change looked like this (along with the identical change to compat/msvc.h):

    @@ -175,13 +175,21 @@ int mingw_getpagesize(void);
      * mingw_fstat() instead of fstat() on Windows.
      */
     #define off_t off64_t
    -#define stat _stati64
     #define lseek _lseeki64
    +#if defined(_MSC_VER)
    +#define stat _stat64
    +#else
    +#define stat _stati64
    +#endif
     int mingw_lstat(const char *file_name, struct stat *buf);
     int mingw_fstat(int fd, struct stat *buf);
     #define fstat mingw_fstat
     #define lstat mingw_lstat
    +#if defined(_MSC_VER)
    +#define _stat64(x,y) mingw_lstat(x,y)
    +#else
     #define _stati64(x,y) mingw_lstat(x,y)
    +#endif
 
     int mingw_utime(const char *file_name, const struct utimbuf *times);
     #define utime mingw_utime

This works with my version of msvc/sdk, provided we have no need to compile with -D_USE_32BIT_TIME_T. (I was a little concerned when I noticed that the time_t type was 64-bits; I checked a few of the obvious places to see if this causes any breakage, but didn't find any).

Also, I added the "&& defined(_stati64)" in the hope that it would work with older msvc/sdk versions.

The reason for the RFC is:

    - maybe we don't need the flexibility of compiling with/without the 32-bit
      time_t definition (which *works* BTW) and can revert to the original patch?
    - I *think* this will work with older msvc, but I can't test it!
    - I've tried to be careful not to break the MinGW build, but again I can't
      test it. (I will be shocked if I have ;-)

ATB, Ramsay Jones

 compat/mingw.h |   27 ++++++++++++++++++++++++++-
 compat/msvc.h  |   25 +------------------------
 2 files changed, 27 insertions(+), 25 deletions(-)

Ramsay Jones schrieb:

"if necessary"? Who defines when -D_USE_32BIT_TIME_T is necessary?

I think that this is an unnecessary complication and I did wonder why you added this extra check. Anybody doing some serious development with MS's tools is using VS2005 at least. But isn't the .vcproj file made for VS2008 anyway?

Indeed I'm wondering why we should cater for 64 bit time_t. It is an unnessary complication as long as MinGW gcc supports only 32 bit time_t and the old MSVCRT.DLL.

This should not be a concern, IMHO.

It compiles without warnings and doesn't break t/t[01]* ;)

-- Hannes

Johannes Sixt wrote:

If I'm reading the msdn documentation correctly, from msvc 2005 onwards, the time_t type is 64-bits by default. In order to support "legacy apps" which break with a 64-bit time_t, the _USE_32BIT_TIME_T macro may be set in the makefile/project-file to enable the old 32-bit time_t type. (The headers contain a lot of appropriately defined "static __inline" functions which call 32- or 64-bit versions of the main time related functions; eg. mktime() will call either _mktime64() or _mktime32()).

Note that 64-bit windows does not support a 32-bit time_t.

So, the "if necessary" means: if git is broken with a 64-bit time_t and it may take some time to fix it (or we can defer fixing it for a long time). And the "Who" is: er... well "us" ;-)

Great! So, I will drop that part of the patch.

I don't know - I don't use it. (Marius?)

Ah, so you want to add -D_USE_32BIT_TIME_T to the Makefile? Do we care about 64-bit Windows?

Thanks! v2 of patch comming soon.

ATB, Ramsay Jones

Hi,

On Thu, 3 Dec 2009, Ramsay Jones wrote:

I'd prefer to have the MSVC-specific definitions in msvc.h, along with a definition of, say, ALREADY_DEFINED_STATI64 or some such (which tells mingw.h not to do anything about those types). There is no need to clutter mingw.h with stuff for MSVC.

Ciao, Dscho

Johannes Schindelin wrote:

Ah, yeah, I did think about this. As I said, the original patch was much simpler and I thought the change to mingw.h was just this side of being objectionable ;-) However, as the patch continued to gain weight I should have re-evaluated that... my bad.

New version of the patch coming soon.

ATB, Ramsay Jones