Support for pthreads under mingw-w64.

[Lord-Kamina]

This PR does two things:

1. It checks for and uses pthreads if available, when compiling for mingw-w64.
2. It adds logic that checks for the presence of the generated headers (toFloat.h, eLut.h, b44ExpLogTable.h and dwaLookups.h) before trying to build them. Additionally, it streamlines the workflow somewhat by allowing the user to point to a previous (native) build when cross-compiling; so if CMake detects that it's cross-compiling, it can use previously built native executables to generate those headers.

P.S. I'm really sorry about the whitespace diffs, I swear I really tried  😅

[Lord-Kamina]

And, now I also fixed symlinks in windows. Since dlls get installed to BINDIR; you were creating broken symlinks in LIBDIR.

[meshula]

This PR has strong bones. Apologies if I've missed some logic guards in trying to read the diff. I think that a few more _MSC_VER HAVE_PTHREADS guards might be necessary? Some comments on consistency of preprocessor macro logic.

[meshula]

We should prevent this from triggering during a Visual Studio build, presumably with a && defined(HAVE_PTHREAD)

[Lord-Kamina]

We should test it on MSVC for sure (I don't have ab MSVC environment to do it) but the reason I put so many checks everywhere is for example for a while I was getting the correct defines in some translating units but not others.

[meshula]

I believe the meaning here has now changed to "if windows and msvc", so the test should be to add && defined(_MSC_VER), since it's unrelated to the existence of pthreads really.

[Lord-Kamina]

Not necessarily; it's possible to have mingw-w64 without winpthreads (not the default anymore but it. could happen)

[meshula]

Since this test is covered in the other header, it's sufficient to just have it there without the pthread guard, isn't it?

[Lord-Kamina]

I put it in a header of its own like this because of what I said above; an issue I faced while doing this was some translation units getting the defines and some not getting them;n I thought it better to drop it into a header rather than copy-paste it elsewhere. Perhaps you could replace the other versions with including this header instead.

[meshula]

should include a HAVE_PTHREAD guard here?

[Lord-Kamina]

I don't think so, if pthread is not present, the mingw header will just include windows.h and be done.

[meshula]

redundant? It'd be nicer to have this windows define business in one place rather than multiple.

[meshula]

tbh I didn't know #if defined FOO works without parens. we should do it one way consistently.

[Lord-Kamina]

Yeah, I wanted to unify style but missed a few.

[meshula]

ditto in all similar

[Lord-Kamina]

See my above explanations for why I dropped them everywhere.

[meshula]

HAVE_PTHREAD guard?

[Lord-Kamina]

As I said earlier, if pthread is not actually available, this should just include windows.h.

[Ericson2314]

+1 on all review comments.

[kdt3rd]

Could we split this into two PRs? The pthread bits and separately the checks for the headers? (I think both are valid, but I'd like to get to a point where we have pre-generated forms of those headers checked in, and that seems like a separate work component)

[Lord-Kamina]

This check could probably be improved somewhat.

[Lord-Kamina]

Could we split this into two PRs? The pthread bits and separately the checks for the headers? (I think both are valid, but I'd like to get to a point where we have pre-generated forms of those headers checked in, and that seems like a separate work component)

Yeah I lumped it all together because it started innocently updating some stuff for MXE snd I kinda just didn't stop 😂

I won't be able to do it until tomorrow at least though.

[Lord-Kamina]

There, it should be much cleaner now.

1. I've fixed the CMake and Autoconf tests for pthreads and semaphores.
2. Because of the above, I've removed the MinGWThread.h header I had created entirely.
3. I've also removed odd tabs mixed-in with spaces here and there (I put that in its own commit though so you can take it or leave it)
4. Finally, I've homogenized usage of #ifdef, #ifndef and #if defined in the following way: the first two will be used when checking a single definition and the latter will be preferred when checking more than one macro at a time.

Mind you, the spaces and macro syntax thing I only did in IlmThread; I thought of going ahead and doing the same elsewhere but it would have just polluted this PR with crap so I scrapped the idea. From what I can tell, though... random tabs were used throughout many source-files in an attempt to make things easier to read. However, at least IMO they seldom actually help or are well done anyway, except for a few files dealing with bitwise operations.

[kdt3rd]

Great, thanks for splitting things apart!
Question: Is there a reason not to just use the std::thread layer? We have already made c++11 a requirement for 2.4, so I think we were going to start cleaning these legacy implementations out in future releases....

[Lord-Kamina]

Apparently on some platforms, std threads rely on pthreads anyway.
For me personally, this started from a massive PR I sent to MXE, fixing many libraries which were hard-coded to use win-threads.

[meshula]

This reply is about cross-checking assumptions. It is a follow up to the discussion around this PR, and trying to understand how to move forward. Please bear with me.

I understand that the PR checks for the existence of pthreads, and uses pthreads if possible on mingw.

According to the docs I've been able to dredge up, it seems that pthreads itself is only available on mingw if one selects the "pthread" model from the mingw installer. Is this true?

Furthermore, I am surprised that in 2019, mingw doesn't support c++11 yet, but that's what stackoverflow and reddit seem to think. Is this information up to date? https://github.com/meganz/mingw-std-threads Using std::thread on mingw seems therefore problematic if that information is accurate.

Looking at this implementation: https://github.com/meganz/mingw-std-threads/blob/master/mingw.thread.h I note that they implement std::threads in terms of windows native threading. Given that winpthreads, if optionally installed by a user into mingw, is implemented in terms of windows native threading, is there an advantage, or specific requirement to use pthreads on Windows?

Would it make sense to massage the existing IlmThread code to be compilable with mingw? That approach would yield a smaller amount of code to maintain, and avoid having to care about whether a user has installed winpthreads or not, therefore yielding less complexity in the cmake script.

[Lord-Kamina]

This reply is about cross-checking assumptions. It is a follow up to the discussion around this PR, and trying to understand how to move forward. Please bear with me.

I understand that the PR checks for the existence of pthreads, and uses pthreads if possible on mingw.

This is correct.

According to the docs I've been able to dredge up, it seems that pthreads itself is only available on mingw if one selects the "pthread" model from the mingw installer. Is this true?

For several years now, MinGW-w64 (which is itself a fork of the original MinGW) has had the option of incorporating pthread support by using the winpthreads library; and this became the default about a year or two ago if I'm not mistaken.

Furthermore, I am surprised that in 2019, mingw doesn't support c++11 yet, but that's what stackoverflow and reddit seem to think. Is this information up to date?

This is definitely incorrect. I've been using c++11 and c++14 for over a year while compiling with MXE. MXE by default uses gcc 5.5.0 but has 6.x, 7.x, 8.x and 9.x available as plug-ins. Other toolchains that rely on MinGW-w64 use newer versions of gcc by default.

https://github.com/meganz/mingw-std-threads Using std::thread on mingw seems therefore problematic if that information is accurate.

I think std::thread might not be supported when using native-win32 threads and might be one of the reasons why the pthread implementation was made the default. That github repo you linked says:

This code has been tested to work with MinGW-w64 5.3.0, but should work with any other MinGW version that has the std threading classes missing, has C++11 support for lambda functions, variadic templates, and has working mutex helper classes in <mutex>.

MinGW-w64 is currently at v6.0.0 (which was released a while ago) and on its homepage claims to support c++11 threading via winpthreads (I think in posix platforms, std threads also end-up using pthreads, which is what I was referring to earlier)

Looking at this implementation: https://github.com/meganz/mingw-std-threads/blob/master/mingw.thread.h I note that they implement std::threads in terms of windows native threading. Given that winpthreads, if optionally installed by a user into mingw, is implemented in terms of windows native threading, is there an advantage, or specific requirement to use pthreads on Windows?

winpthreads is technically extra, but AFAIK it's pretty widely used. What I use (MXE; a package-manager for cross-compiling to windows) has supported it for years, and they became the default at least a year ago. I believe MSYS2 also uses winpthreads.

is there an advantage, or specific requirement to use pthreads on Windows?

Requirement; I don't know but I don't think so. Advantage, probably easier to maintain code.

Would it make sense to massage the existing IlmThread code to be compilable with mingw? That approach would yield a smaller amount of code to maintain, and avoid having to care about whether a user has installed winpthreads or not, therefore yielding less complexity in the cmake script.

Some people might still prefer to use win32-native threads on a mingw environment; and then there's also the MSVC crowd (and they're not exactly few). While winpthreads can also be used with MSVC, I think that is much rarer. One thing to note is that my PR relies on checking a header that is always installed on mingw-w64, but if winpthread is not installed, it's just a stub file with a comment. In those cases, both CMake and Autoconf should realize pthreads are not available and use the code that had been used up-to now. For most libraries, supporting winpthread is not really too hard; in the case of IlmThread, I had to work a bit chiefly because many checks assumed pthreads COULD NOT be present on windows and so some of the appropriate checks were downright not-reachable on non-unix systems.

[meshula]

Ok, thanks, that helps a lot. I assume testing is a matter of installing mingw and building the usual way in a shell, so I’ll give it a go. Unfortunately we’re about to be subject to a power outage here in California, so that’s potentially going to cause a delay.

[kleisauke]

Thanks! I'm able to successfully build this PR in a MXE environment (with -DILMBASE_FORCE_CXX03=ON). It only needs the extra patches I provided in the comments.

fwiw, to build with -DILMBASE_FORCE_CXX03=ON it may be necessary to have this patch integrated.

[kleisauke]

__MINGW64_VERSION_MAJOR is always undefined here, since windows.h isn't included. This should fix that:

diff --git a/IlmBase/IlmThread/IlmThreadWin32.cpp b/IlmBase/IlmThread/IlmThreadWin32.cpp
index 1111111..2222222 100644
--- a/IlmBase/IlmThread/IlmThreadWin32.cpp
+++ b/IlmBase/IlmThread/IlmThreadWin32.cpp
@@ -40,10 +40,11 @@
 
 #include "IlmBaseConfig.h"
 
+#include "IlmThread.h"   // We need to have windows.h before checking for __MINGW64_VERSION_MAJOR
+
 #ifdef ILMBASE_FORCE_CXX03
 #if ((defined _WIN32 || defined _WIN64) && !defined(__MINGW64_VERSION_MAJOR))
 
-#include "IlmThread.h"
 #include "Iex.h"
 #include <iostream>
 #include <assert.h>

[kleisauke]

As a side note, we could also simply use the __MINGW32__ marcro. See:
https://sourceforge.net/p/predef/wiki/Compilers/#mingw-and-mingw-w64

It does not need to have the <windows.h> included.

[Lord-Kamina]

__MINGW32__ is not appropriate because it's also present in the original MinGW and my PR is for Mingw-w64.

I hadn't tested it with forcing of CXX03 mode so I guess that's something I have to fix. Why are you purposefully disabling c++11 though?

[kleisauke]

__MINGW32__ is not appropriate because it's also present in the original MinGW and my PR is for Mingw-w64.

Ah, I see. Thanks for the clarification.

Why are you purposefully disabling c++11 though?

I'm building without POSIX threads (MXE target: x86_64-w64-mingw32.shared.win32) because our library is somehow slower with POSIX threads functionality than with the native win32 implementation (see libvips/build-win64#27 for the benchmarks).

Building without POSIX threads has the disadvantage that the C++11 <thread>, <mutex>, and <future> implementations aren't available (see for example this log). So, unfortunately, I have to enable the CXX03 mode within OpenEXR.

[Lord-Kamina]

I see. I'll probably integrate your fixes whenever I get back to my computer.

[Ericson2314]

BTW I recently learned of https://github.com/mstorsjo/llvm-mingw, which use mingw windows headers and libs, but not the POSIX emulation. libc++ evidentially has direct windows support, and it might be faster too.

There's also clang + MSVC directly. I am working with the llvm-mingw author a bit to make a fully automated version cross toolchain for that in NixOS/nixpkgs#72366. (Fully automated in that it downloads VS, and builds all the packages.)

[kleisauke]

@Ericson2314 Thanks for the suggestion regarding https://github.com/mstorsjo/llvm-mingw! I've integrated it into our MXE-based build environment (libvips/build-win64-mxe@f8c17ee) and I'm considering making a PR at MXE to integrate it there too.

[kleisauke]

__MINGW64_VERSION_MAJOR is always undefined here, since windows.h isn't included. This should fix that:

diff --git a/IlmBase/IlmThread/IlmThreadMutexWin32.cpp b/IlmBase/IlmThread/IlmThreadMutexWin32.cpp
index 1111111..2222222 100644
--- a/IlmBase/IlmThread/IlmThreadMutexWin32.cpp
+++ b/IlmBase/IlmThread/IlmThreadMutexWin32.cpp
@@ -40,9 +40,10 @@
 
 #include "IlmBaseConfig.h"
 
+#include "IlmThreadMutex.h"  // We need to have windows.h before checking for __MINGW64_VERSION_MAJOR
+
 #ifdef ILMBASE_FORCE_CXX03
 #    if ((defined _WIN32 || defined _WIN64) && !defined(__MINGW64_VERSION_MAJOR))
-#        include "IlmThreadMutex.h"
 #        include "Iex.h"
 
 ILMTHREAD_INTERNAL_NAMESPACE_SOURCE_ENTER

[Lord-Kamina]

Whoops

[kleisauke]

_mutex has now been defined twice. This should fix that:

diff --git a/IlmBase/IlmThread/IlmThreadMutex.h b/IlmBase/IlmThread/IlmThreadMutex.h
index 1111111..2222222 100644
--- a/IlmBase/IlmThread/IlmThreadMutex.h
+++ b/IlmBase/IlmThread/IlmThreadMutex.h
@@ -117,9 +117,9 @@ class ILMTHREAD_EXPORT Mutex
     void lock () const;
     void unlock () const;
 
-    #if (defined(_WIN32) || defined(_WIN64))
-    #elif HAVE_PTHREAD
+    #if (defined(_WIN32) || defined(_WIN64)) && !defined(HAVE_PTHREAD)
     mutable CRITICAL_SECTION _mutex;
+    #elif defined(HAVE_PTHREAD)
     mutable pthread_mutex_t _mutex;
     #endif
 

[Lord-Kamina]

Thanks for catching that also.

[kleisauke]

The addTask and removeTask functions are now conditionally excluded when compiling with ILMBASE_FORCE_CXX03, which causes build failures. This should fix that:

diff --git a/IlmBase/IlmThread/IlmThreadPool.cpp b/IlmBase/IlmThread/IlmThreadPool.cpp
index 1111111..2222222 100644
--- a/IlmBase/IlmThread/IlmThreadPool.cpp
+++ b/IlmBase/IlmThread/IlmThreadPool.cpp
@@ -67,9 +67,9 @@ struct TaskGroup::Data
      Data ();
     ~Data ();
     
-#ifndef ILMBASE_FORCE_CXX03
     void addTask () ;
     void removeTask ();
+#ifndef ILMBASE_FORCE_CXX03
     std::atomic<int> numPending;
 #else
     int numPending;     // number of pending tasks to still execute

(sorry for not catching this earlier, I excluded commit f1180f8 when I tested this PR)

[cary-ilm]

Apologies for the delay in followup, but the TSC just discussed this PR. From my reading of this, there are parts of if that seem straightforward that we'd be willing to merge if it's cleaned up.

• The entire automated header-generation process has been retired (Remove all build-time header generation #606), so those changes have been addressed elsewhere, so please remove anything related to that.

• We're actively removing compatibility code that supports compilation by pre-C++11 compilers; our policy is to prioritize simplicity and maintainability of the code over support for older compilers. As a part of the ASWF, we're guided by the VFX reference platform, which already stipulates C++14. It's also our intentionto retire the custom thread management in IlmThread and fully migrate to C++11 threading, so we'd like to avoid making substantial changes to the current threading support, since it should be retired in an upcoming release anyway. But it looks like your changes are limited to cleaning up #ifdef's and defines, right?

• I'm not following the changes to LibraryDefine, what's going on there?

• We'd prefer large-scale formatting changes as a separate PR, and with the whitespace changes mixed in, it's difficult to sort out what's changed. Can you diff the whitespace changes out, even better, submit a new PR that more clearly delineates the proposed changes.

[Lord-Kamina]

Apologies for the delay in followup, but the TSC just discussed this PR. From my reading of this, there are parts of if that seem straightforward that we'd be willing to merge if it's cleaned up.

• The entire automated header-generation process has been retired (Remove all build-time header generation #606), so those changes have been addressed elsewhere, so please remove anything related to that.

You mean from the discussion? I'll check it again over the weekend but I think I removed all that from the code already.

• We're actively removing compatibility code that supports compilation by pre-C++11 compilers; our policy is to prioritize simplicity and maintainability of the code over support for older compilers. As a part of the ASWF, we're guided by the VFX reference platform, which already stipulates C++14. It's also our intentionto retire the custom thread management in IlmThread and fully migrate to C++11 threading, so we'd like to avoid making substantial changes to the current threading support, since it should be retired in an upcoming release anyway. But it looks like your changes are limited to cleaning up #ifdef's and defines, right?

For the most part, yes it's just cleaning-up ifdefs. I also did fix some tests in CMake and autoconf so they would automatically pick the pthread support up if building in mingw-w64 with pthread support.

Dropping old c++ makes sense to me; there are legitimate use cases for it, though. If I might make a middle-ground solution, perhaps you could make a minor release with the threading changes for pre-c++11 compilers before you drop support for them?

• I'm not following the changes to LibraryDefine, what's going on there?

This, essentially: And, now I also fixed symlinks in windows. Since dlls get installed to BINDIR; you were creating broken symlinks in LIBDIR.
In windows, dll files are installed not to /lib but to /bin; your Library define was creating broken symlinks on some platforms.

• We'd prefer large-scale formatting changes as a separate PR, and with the whitespace changes mixed in, it's difficult to sort out what's changed. Can you diff the whitespace changes out, even better, submit a new PR that more clearly delineates the proposed changes.

That's why I left those in a separate commit, it should be trivial to rebase it out.

[Lord-Kamina]

@cary-ilm I'd entirely forgotten about how exactly I'd found the issues with LibraryDefine but then I got notified about this and remembered. Here's a practical demonstration of why it was an issue: openscenegraph/OpenSceneGraph#849

[meshula]

Hi there @Lord-Kamina, I know you said that it should be trivial to rebase out all the unrelated whitespace and formatting changes, but it wasn't, for me... So I went ahead and created a clean pull which I think has got all your code edits.

#673

I don't have a way to test the cmake/mingw or make/autoconf changes, so I left those out. I'm hoping that you can add them to 673, and we can pick up this merge from there?

[Lord-Kamina]

It's probably better if I clean this PR up? I'll take a look during the weekend.

[meshula]

That would be great, thanks.

[cary-ilm]

Just checking, any update on this?

[Lord-Kamina]

Ooph crap, sorry. Been a bit busy at work and it completely slipped my mind.

[cary-ilm]

@Lord-Kamina, we'll be cutting a 2.5 release later this week, if you can clean it up in the next couple days we will try to review it include it in the release.

[Lord-Kamina]

@Lord-Kamina, we'll be cutting a 2.5 release later this week, if you can clean it up in the next couple days we will try to review it include it in the release.

Thanks for the heads-up. Last time I realized @meshula was right and it wasn't so easy. Then COVID-19 happened and I work in an ER so my mind's been elsewhere. Will try to get it done Friday, hopefully before you guys make your release.

[cary-ilm]

Before you do any more work here, check out #707 and see how many issues that fixes. @meshula did the real work, I think to accomplish what you were after here. Note that all of the compile-time header generation is gone, too. There may be nothing left to do, but please confirm.

[Lord-Kamina]

Before you do any more work here, check out #707 and see how many issues that fixes. @meshula did the real work, I think to accomplish what you were after here. Note that all of the compile-time header generation is gone, too. There may be nothing left to do, but please confirm.

I haven't tried that PR yet but looking at it, It's missing some includes and the bit that fixes the output install targets as well as the fixes to the CMake and autoconf scripts that actually configure everything properly.

[cary-ilm]

Resolved by #708, thanks!