Fix SoundFonts failing to load with old MinGW versions#1756
Fix SoundFonts failing to load with old MinGW versions#1756derselbst merged 12 commits intorelease/2.5from
Conversation
|
|
|
||
| int fluid_file_seek(FILE *fd, fluid_long_long_t ofs, int whence) | ||
| { | ||
| #if (defined(__MINGW32__) || defined(__MINGW64__)) && defined(__GNUC__) && (__GNUC__ < 15) |
There was a problem hiding this comment.
gcc version has nothing to do with mingw version. That additional __MINGW64__ check is also redundant, because __MINGW32__ is always defined even for win64. I suggest that you guys simplify the condition to #ifdef __MINGW32__
There was a problem hiding this comment.
I don't quite understand your comment. As said in the PR description, I wrote a unit test and added a CI build that would have failed without this logic. After adding this macro #ifdef logic, the unit test passed for that particular build. If the gcc version was truly unrelated to the mingw version, the test should have continued to fail. I therefore conclude that MinGW does indeed define __GNUC__. This is backed by the fact that MinGW 8 reports as GCC: https://github.com/FluidSynth/fluidsynth/actions/runs/22018244095/job/63623147113#step:4:74
Solely using #ifdef __MINGW32__ is insufficient, as I want to apply this logic only for old MinGW. I chose version 15, because the other MinGW-based CI builds we have use version 15 and do not suffer from this problem.
I do not mind that __MINGW64__ might be redundant.
There was a problem hiding this comment.
Your workflow seems to use https://sourceforge.net/projects/mingw-w64/files/Toolchains%%20targetting%%20Win64/Personal%%20Builds/mingw-builds/8.1.0/threads-win32/sjlj/x86_64-8.1.0-release-win32-sjlj-rt_v6-rev0.7z which is a full toolchain prebuilt with a certain gcc/buinutils/mingw combination, and that toolchain happens to use a gcc older than 15. Are you truly assuming that all mingw-w64-v8.x toolchains are obligated to be accompanied by gcc < 15?
(Do as you wish, though, why would I care..)
There was a problem hiding this comment.
To further elaborate on the mistake here: Associating a gcc version with a mingw version is no different than associating a gcc version with a glibc version, which is meaningless. The issue is somewhere in mingw crt code, it may be present in v8.x, and I don't know what version it is fixed in -- and that's assuming that it is fixed. Therefore, always using the workaround for mingw is the safe and right thing to do here.
There was a problem hiding this comment.
The history of both files show that they were added in 2025.
The history goes older: Follow the link at the end on github ui.
By the way: what is the macro to know the MINGW version/runtime version?
https://github.com/mingw-w64/mingw-w64/blob/master/mingw-w64-headers/crt/_mingw_mac.h#L17-L42
We usually detect like this:
#ifdef __MINGW32__
#include <_mingw.h>
#ifdef __MINGW64_VERSION_MAJOR /* this is mingw-w64 */
/* do stuff.. */
#else /* this is old/ancient mingw */
/* do other stuff */
#endif
#endifThere was a problem hiding this comment.
Apologize Pedro, here's the correct SF link, I'll fix it in the code later (can't explain how to got wrong there): https://sourceforge.net/p/mingw-w64/bugs/864/
I understand that solely relying on the MinGW's GCC compiler version for this workaround is not sufficiently accurate. I can only tell from the two different types of MinGW based CI builds and therefore I assumed that the entire MinGW toolchain would be usually kept in some sort of version-sync.
However, I'm not that familiar with MinGW. If you think it's better to always apply for MinGW I'm open to change it.
There was a problem hiding this comment.
However, I'm not that familiar with MinGW. If you think it's better to always apply for MinGW I'm open to change it.
FWIW, that certainly is what I think.
There was a problem hiding this comment.
Ah, that seems to only affect 32 bit programs. I've not been able to reproduce the problem with the tools I have at hand, but then it seems to me that the workaround could be restricted to a combination MINGW and 32 bit. The only 32 bit toolchain I have installed here is the MSYS2 MINGW32 environment, which works fine and reports __MINGW64_VERSION_MAJOR = 14 (and GCC v15).
There was a problem hiding this comment.
Thanks to both of you for your inputs and @sezero for bringing this up.
I can confirm Pedro's observation that this problem only seems to occur with x86 targets: https://github.com/FluidSynth/fluidsynth/actions/runs/22111716715
Because of this, I would address this like so: #1759
3 participants
This PR fixes #1755. Particularly:
sizeofandalignofvarious datatypes and asserting some of them.READCHUNKinto two separate read calls to guard against any possibly introduced extra padding in theSFChunkstruct.