tbeu/matio

LFS support on MinGW broken

Closed this issue · 4 comments

chris-se commented

LFS support on MinGW is currently somewhat broken (MinGW, x86_64, CMake build system). Since MinGW defines all of fseeko, ftello, fseeko64, ftello64, _fseeki64, and _ftelli64, and also the compile time constant __MINGW32__, the following sequence in mingw_private.h has some interesting results:

#if defined(__BORLANDC__) || defined(__MINGW32__) || defined(_MSC_VER)
#define mat_off_t __int64
#if defined(_MSC_VER) && defined(HAVE__FSEEKI64) && defined(HAVE__FTELLI64)
#define MATIO_LFS
#define fseeko _fseeki64
#define ftello _ftelli64
#elif defined(__BORLANDC__) && defined(HAVE__FSEEKI64) && defined(HAVE__FTELLI64)
#define MATIO_LFS
#define fseeko _fseeki64
#define ftello _ftelli64
#elif !defined(HAVE_FSEEKO) && !defined(HAVE_FTELLO) && defined(HAVE_FSEEKO64) && \
    defined(HAVE_FTELLO64)
#define MATIO_LFS
#define fseeko fseeko64
#define ftello ftello64
#endif
#elif defined(_FILE_OFFSET_BITS) && _FILE_OFFSET_BITS == 64 && defined(HAVE_FSEEKO) && \
    defined(HAVE_FTELLO)
#define MATIO_LFS
#define mat_off_t off_t
#endif

#if !defined(MATIO_LFS)
#define mat_off_t long
#define ftello ftell
#define fseeko fseek
#endif

Since __MINGW32__ is defined, the compiler will go into the first #if condition and execute #define mat_off_t __int64. But none of the nested #ifs apply to MinGW: the first (defined(_MSC_VER) && defined(HAVE__FSEEKI64) && defined(HAVE__FTELLI64)) because _MSC_VER is not defined, the second (defined(__BORLANDC__) && defined(HAVE__FSEEKI64) && defined(HAVE__FTELLI64)) because __BORLANDC__ is not defined, and the third (!defined(HAVE_FSEEKO) && !defined(HAVE_FTELLO) && defined(HAVE_FSEEKO64) && defined(HAVE_FTELLO64)) because HAVE_SEEKO and HAVE_FTELLO are defined (but also the 64bit counterparts).

But that means that MATIO_LFS is never defined, and the bottom #if !defined(MATIO_LFS) is also entered. That contains #define mat_off_t long. This has the consequence on MinGW that the compiler issues the following warnings:

.../src/matio_private.h:66: warning: "mat_off_t" redefined
   66 | #define mat_off_t long
      |
.../src/matio_private.h:44: note: this is the location of the previous definition
   44 | #define mat_off_t __int64
      |

Note though that long is 32bit on MinGW, even on 64bit platforms (since Win64 longs are 32bit)! This means that even on 64bit Windows, matio will use 32bit offsets!

I've tested the following patch and works on MinGW, as well as Linux and macOS:

--- a/src/matio_private.h       2023-11-12 13:25:19.000000000 +0100
+++ b/src/matio_private.h       2024-02-05 16:40:13.647955742 +0100
@@ -41,18 +41,19 @@
 #endif
 
 #if defined(__BORLANDC__) || defined(__MINGW32__) || defined(_MSC_VER)
-#define mat_off_t __int64
 #if defined(_MSC_VER) && defined(HAVE__FSEEKI64) && defined(HAVE__FTELLI64)
 #define MATIO_LFS
+#define mat_off_t __int64
 #define fseeko _fseeki64
 #define ftello _ftelli64
 #elif defined(__BORLANDC__) && defined(HAVE__FSEEKI64) && defined(HAVE__FTELLI64)
 #define MATIO_LFS
+#define mat_off_t __int64
 #define fseeko _fseeki64
 #define ftello _ftelli64
-#elif !defined(HAVE_FSEEKO) && !defined(HAVE_FTELLO) && defined(HAVE_FSEEKO64) && \
-    defined(HAVE_FTELLO64)
+#elif defined(HAVE_FSEEKO64) && defined(HAVE_FTELLO64)
 #define MATIO_LFS
+#define mat_off_t __int64
 #define fseeko fseeko64
 #define ftello ftello64
 #endif

Notes:

  • I've moved the #define mat_off_t __int64 into the inner conditions to ensure that the double-#define never happens again. (Even if no inner condition is matched.)
  • I've removed the !defined(HAVE_FSEEKO) && !defined(HAVE_FTELLO) from the #elif condition, because that doesn't make any sense in my eyes.

This is minimally invasive, and hopefully shouldn't cause breakage on other systems, but I'm sure the entire logic here could probably be refactored in a better manner.

Regardless, I'll be happy to provide a pull request fore the above patch if you agree with this solution.

tbeu commented

Regardless, I'll be happy to provide a pull request fore the above patch if you agree with this solution.

That would be appreciated. Thanks.

chris-se commented

Regardless, I'll be happy to provide a pull request fore the above patch if you agree with this solution.

That would be appreciated. Thanks.

Done, thanks: #235

tbeu commented

Resolved by #235. Thanks.