tbeu / matio

MATLAB MAT File I/O Library
https://matio.sourceforge.io
BSD 2-Clause "Simplified" License
338 stars 97 forks source link

LFS support on MinGW broken #234

Closed chris-se closed 8 months ago

chris-se commented 8 months ago

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:

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 8 months ago

Yes, I also noticed recently that it is broken, see my similar change here: https://github.com/modelica/ModelicaStandardLibrary/pull/4285/files#diff-fd202881aff26f5dd968dfcd19796cb461c19bf5d3f35ebc30a40b3e1d531a65R276

tbeu commented 8 months ago

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 8 months ago

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: https://github.com/tbeu/matio/pull/235

tbeu commented 8 months ago

Resolved by #235. Thanks.