[RFC PATCH v2 2/2] MSVC: Fix an "incompatible pointer types" compiler warning

[RFC PATCH v2 2/2] MSVC: Fix an "incompatible pointer types" compiler warning

[Ramsay Jones]

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 add declarations for the
mingw_lstat() and mingw_fstat() functions and supporting macros to
msvc.h, suppressing the corresponding declarations in mingw.h, so
that we can use the appropriate structure type (and function) names
from the msvc headers.

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

Changes from v1:
    - moved the new declarations to msvc.h rather than clutter mingw.h
      with msvc related code.
    - don't even attempt to support older msvc compilers

The patch is still marked RFC because:
    - I'm still not sure if the flexibility to support both 32- and 64-bit
      time_t is required.
    - should -D_USE_32BIT_TIME_T be added to the Makefile?

ATB,
Ramsay Jones

 compat/mingw.h |    4 +++-
 compat/msvc.h  |   54 +++++++++++++++++++++++++++++-------------------------
 2 files changed, 32 insertions(+), 26 deletions(-)

diff --git a/compat/mingw.h b/compat/mingw.h
index 5b5258b..b5cec7f 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -175,13 +175,15 @@ int mingw_getpagesize(void);
  * mingw_fstat() instead of fstat() on Windows.
  */
 #define off_t off64_t
-#define stat _stati64
 #define lseek _lseeki64
+#ifndef ALREADY_DECLARED_STAT_FUNCS
+#define stat _stati64
 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
 #define _stati64(x,y) mingw_lstat(x,y)
+#endif
 
 int mingw_utime(const char *file_name, const struct utimbuf *times);
 #define utime mingw_utime
diff --git a/compat/msvc.h b/compat/msvc.h
index 9c753a5..f431007 100644
--- a/compat/msvc.h
+++ b/compat/msvc.h
@@ -21,30 +21,34 @@ static __inline int strcasecmp (const char *s1, const char *s2)
 }
 
 #undef ERROR
-#undef stat
-#undef _stati64
+
+/* Use mingw_lstat() instead of lstat()/stat() and mingw_fstat() instead
+ * of fstat(). We add the declaration of these functions here, suppressing
+ * the corresponding declarations in mingw.h, so that we can use the
+ * appropriate structure type (and function) names from the msvc headers.
+ */
+#if defined(_USE_32BIT_TIME_T)
+# define stat _stat32i64
+#else
+# define stat _stat64
+#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(_USE_32BIT_TIME_T)
+# define _stat32i64(x,y) mingw_lstat(x,y)
+#else
+# define _stat64(x,y) mingw_lstat(x,y)
+#endif
+
+#define ALREADY_DECLARED_STAT_FUNCS
+
 #include "compat/mingw.h"
-#undef stat
-#define stat _stati64
-#define _stat64(x,y) mingw_lstat(x,y)
-
-/*
-   Even though _stati64 is normally just defined at _stat64
-   on Windows, we specify it here as a proper struct to avoid
-   compiler warnings about macro redefinition due to magic in
-   mingw.h. Struct taken from ReactOS (GNU GPL license).
-*/
-struct _stati64 {
-	_dev_t  st_dev;
-	_ino_t  st_ino;
-	unsigned short st_mode;
-	short   st_nlink;
-	short   st_uid;
-	short   st_gid;
-	_dev_t  st_rdev;
-	__int64 st_size;
-	time_t  st_atime;
-	time_t  st_mtime;
-	time_t  st_ctime;
-};
+
+#undef ALREADY_DECLARED_STAT_FUNCS
+
 #endif
-- 
1.6.5

Re: [RFC PATCH v2 2/2] MSVC: Fix an "incompatible pointer types" compiler warning

[Johannes Sixt]

[Removed Junio from Cc because IIUC, he prefers to be left out of the loop in 
Windows related matters until they have settled.]

On Samstag, 5. Dezember 2009, Ramsay Jones wrote:
> The patch is still marked RFC because:
>     - I'm still not sure if the flexibility to support both 32- and 64-bit
>       time_t is required.
>     - should -D_USE_32BIT_TIME_T be added to the Makefile?

If *not* using -D_USE_32BIT_TIME_T  produces a build or code base that is in 
some way superior, why should we require it? For example, its absence could 
help a 64bit build.

-- Hannes

Re: [RFC PATCH v2 2/2] MSVC: Fix an "incompatible pointer types" compiler warning

[Johannes Schindelin]

Hi,

On Fri, 4 Dec 2009, Ramsay Jones wrote:

> Changes from v1:
>     - moved the new declarations to msvc.h rather than clutter mingw.h
>       with msvc related code.

Thanks,
Dscho

Re: [RFC PATCH v2 2/2] MSVC: Fix an "incompatible pointer types" compiler warning

[Ramsay Jones]

Johannes Sixt wrote:
> On Samstag, 5. Dezember 2009, Ramsay Jones wrote:
>> The patch is still marked RFC because:
>>     - I'm still not sure if the flexibility to support both 32- and 64-bit
>>       time_t is required.
>>     - should -D_USE_32BIT_TIME_T be added to the Makefile?
> 
> If *not* using -D_USE_32BIT_TIME_T  produces a build or code base that is in 
> some way superior, why should we require it? For example, its absence could 
> help a 64bit build.

Indeed.

I only added the second bullet because you seemed to be advocating it, in your
last email, so I'm waiting for your answer on that. :-D

Unfortunately the patch can not be a simple as the first version (Dscho was
quite right to complain about adding too much clutter to mingw.h with msvc
related code), but the moral equivalent would have msvc.h look like:

    25	/* Use mingw_lstat() instead of lstat()/stat() and mingw_fstat() instead
    26	 * of fstat(). We add the declaration of these functions here, suppressing
    27	 * the corresponding declarations in mingw.h, so that we can use the
    28	 * appropriate structure type (and function) names from the msvc headers.
    29	 */
    30	#define stat _stat64
    31	int mingw_lstat(const char *file_name, struct stat *buf);
    32	int mingw_fstat(int fd, struct stat *buf);
    33	#define fstat mingw_fstat
    34	#define lstat mingw_lstat
    35	#define _stat64(x,y) mingw_lstat(x,y)
    36	#define ALREADY_DECLARED_STAT_FUNCS
    37	
    38	#include "compat/mingw.h"
    39	
    40	#undef ALREADY_DECLARED_STAT_FUNCS

This works fine, *provided* you do not need to compile with -D_USE_32BIT_TIME_T,
which would produce this warning:

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

This would actually be *worse* than the original code, since the struct _stat64
would not have the same "shape" as the struct _stat32i64; it would not write
outside of the allocated memory, at least, but the time fields would obviously
be written to the wrong offsets etc,. In the original code, the struct _stati64
would have the correct "shape", since the time fields are declared with time_t.

At first I thought there would be no need to set _USE_32BIT_TIME_T.  After some
thought, however, I could not be confident that it would *never* be necessary.
(my only concern was to revert to 32-bit time_t, perhaps only temporarily, while
fixing a breakage; I did not consider wanting to keep "compatibility" with the
MinGW code). This lead to the more complicated/flexible RFC patch.

I was hoping for someone to say: "Hey, we will *never* need to compile with
-D_USE_32BIT_TIME_T, so just get rid of those #if conditionals and simplify
the code ...". :-P

Since nobody has said any such thing, I have to conclude that the extra
flexibility is required. That being the case, I have to be happy with the
patch as-is and propose to remove the RFC.

Before I do that, do you have any further comments or concerns about the
v2 patch that you want me to address?

ATB,
Ramsay Jones

Re: [RFC PATCH v2 2/2] MSVC: Fix an "incompatible pointer types" compiler warning

[Johannes Sixt]

On Dienstag, 8. Dezember 2009, Ramsay Jones wrote:
>     30	#define stat _stat64
>     31	int mingw_lstat(const char *file_name, struct stat *buf);
>     32	int mingw_fstat(int fd, struct stat *buf);
>     33	#define fstat mingw_fstat
>     34	#define lstat mingw_lstat
>     35	#define _stat64(x,y) mingw_lstat(x,y)
>     36	#define ALREADY_DECLARED_STAT_FUNCS
>     37
>     38	#include "compat/mingw.h"
>     39
>     40	#undef ALREADY_DECLARED_STAT_FUNCS
>
> This works fine, *provided* you do not need to compile with
> -D_USE_32BIT_TIME_T, which would produce this warning:
>
>     ...mingw.c(223) : warning C4133: 'function' : incompatible types - \
>     from '_stat64 *' to '_stat32i64 *'
>
> This would actually be *worse* than the original code, since the struct
> _stat64 would not have the same "shape" as the struct _stat32i64; ...

To cut this short: According to your explanations, using -D_USE_32BIT_TIME_T 
with MSVC is bad. Please reroll without references to _USE_32BIT_TIME_T.

-- Hannes

Re: [RFC PATCH v2 2/2] MSVC: Fix an "incompatible pointer types" compiler warning

[Ramsay Jones]

Johannes Sixt wrote:
> To cut this short: According to your explanations, using -D_USE_32BIT_TIME_T 
> with MSVC is bad. Please reroll without references to _USE_32BIT_TIME_T.

OK, will do...

ATB,
Ramsay Jones