MAINT: Avoid use of aligned_alloc in pocketfft on windows

[mborland]

Reference issue

#19726

What does this implement/fix?

Fixes detection of cpp standard on MSVC platforms since it does not correctly define the value of __cplusplus without command line intervention

Additional information

N/A

[h-vetinari]

Thanks for the PR!

As I said in the issue, it would be nice to just be able to rely on meson for this (which works with msvc, not yet with clang-cl; though I guess we also haven't tried C++ standards beyond 14 on windows much...), but I think this is fine as a general precaution.

[mborland]

Thanks for the PR!

As I said in the issue, it would be nice to just be able to rely on meson for this (which works with msvc, not yet with clang-cl; though I guess we also haven't tried C++ standards beyond 14 on windows much...), but I think this is fine as a general precaution.

MSVC has become substantially better recently, but unfortunately there still seems to be a good bit of development in 14 and older: https://www.jetbrains.com/lp/devecosystem-2023/cpp/.

I tested this in conda-forge/scipy-feedstock#262, and it actually doesn't work. While clang seems to follow MSVC w.r.t. __cplusplus, it does not seem to set _MSVC_LANG, so the end result is that we still don't pick up the definition for aligned_alloc.

[mborland]

The detection seems to be right, but you now have this: https://developercommunity.visualstudio.com/t/c17-stdaligned-alloc缺失/468021. We could try detection for __cpp_aligned_new: https://en.cppreference.com/w/cpp/feature_test, but I don't know how extensive the MSVC support is pre-C++20. It may not matter since the feature is not supported anyway.

[h-vetinari]

We could try detection for __cpp_aligned_new: https://en.cppreference.com/w/cpp/feature_test, but I don't know how extensive the MSVC support is pre-C++20. It may not matter since the feature is not supported anyway.

According to their docs (also here), feature test macros are supported in all standard modes.

[h-vetinari]

According to their docs (also here), feature test macros are supported in all standard modes.

Well, it can always get worse. They set the FTM to the C++17 value despite not having implemented it. 🤦

(or rather, one needs _aligned_malloc instead of aligned_alloc, which comes out about the same in terms of portability)

[h-vetinari]

I have picked the following commit into conda-forge/scipy-feedstock#262, and it works:

commit 640e25aefb687299d3bef06299f6c946c94aeb6a (HEAD -> conda)
Author: Matt Borland <matt@mattborland.com>
Date:   Tue Dec 26 06:50:21 2023 -0500

    Remove support for aligned_alloc in pocketfft for windows

    * MSVC is too non-conformant and broken...
    * Also harden detection of cpp standard on MSVC platforms

    Co-Authored-By: H. Vetinari <h.vetinari@gmx.com>

diff --git a/scipy/fft/_pocketfft/pocketfft_hdronly.h b/scipy/fft/_pocketfft/pocketfft_hdronly.h
index 8cdd16910..53e72ff53 100644
--- a/scipy/fft/_pocketfft/pocketfft_hdronly.h
+++ b/scipy/fft/_pocketfft/pocketfft_hdronly.h
@@ -149,7 +149,11 @@ template<> struct VLEN<double> { static constexpr size_t val=2; };
 #endif
 #endif

-#if __cplusplus >= 201703L
+// MSVC does not provide the standard aligned allocation functions,
+// yet _still_ sets the feature test macro (__cpp_aligned_new)...
+// While there are replacement functions (_aligned_malloc & _aligned_free),
+// those produce crashes when used here, see GH-19726
+#if __cplusplus >= 201703L && !defined(_MSC_VER)
 inline void *aligned_alloc(size_t align, size_t size)
   {
   void *ptr = ::aligned_alloc(align,size);
diff --git a/scipy/io/_fast_matrix_market/fast_matrix_market/include/fast_matrix_market/app/array.hpp b/scipy/io/_fast_matrix_market/fast_matrix_market/include/fast_matrix_market/app/array.hpp
index cb5045dd1..f9cc7f67d 100644
--- a/scipy/io/_fast_matrix_market/fast_matrix_market/include/fast_matrix_market/app/array.hpp
+++ b/scipy/io/_fast_matrix_market/fast_matrix_market/include/fast_matrix_market/app/array.hpp
@@ -8,7 +8,7 @@

 namespace fast_matrix_market {

-#if __cplusplus >= 202002L
+#if __cplusplus >= 202002L || _MSVC_LANG+0L >= 202002L
     // If available, use C++20 concepts for programmer clarity and better error messages.
     // If not using C++20 this shows what fast_matrix_market expects each template type to support.

diff --git a/scipy/io/_fast_matrix_market/fast_matrix_market/include/fast_matrix_market/app/doublet.hpp b/scipy/io/_fast_matrix_market/fast_matrix_market/include/fast_matrix_market/app/doublet.hpp
index 88c11aef0..ba16189a9 100644
--- a/scipy/io/_fast_matrix_market/fast_matrix_market/include/fast_matrix_market/app/doublet.hpp
+++ b/scipy/io/_fast_matrix_market/fast_matrix_market/include/fast_matrix_market/app/doublet.hpp
@@ -7,7 +7,7 @@
 #include "../fast_matrix_market.hpp"

 namespace fast_matrix_market {
-#if __cplusplus >= 202002L
+#if __cplusplus >= 202002L || _MSVC_LANG+0L >= 202002L
     // If available, use C++20 concepts for programmer clarity.
     // This shows what fast_matrix_market expects each template type to support.

diff --git a/scipy/io/_fast_matrix_market/fast_matrix_market/include/fast_matrix_market/app/triplet.hpp b/scipy/io/_fast_matrix_market/fast_matrix_market/include/fast_matrix_market/app/triplet.hpp
index 871305c3e..00b7abd9a 100644
--- a/scipy/io/_fast_matrix_market/fast_matrix_market/include/fast_matrix_market/app/triplet.hpp
+++ b/scipy/io/_fast_matrix_market/fast_matrix_market/include/fast_matrix_market/app/triplet.hpp
@@ -7,7 +7,7 @@
 #include "../fast_matrix_market.hpp"

 namespace fast_matrix_market {
-#if __cplusplus >= 202002L
+#if __cplusplus >= 202002L || _MSVC_LANG+0L >= 202002L
     // If available, use C++20 concepts for programmer clarity.
     // This shows what fast_matrix_market expects each template type to support.

That doesn't mean we have to take exactly that commit, I just wanted to test a variant along the lines of what I had in mind. If you want, I can also push it into this branch. Also happy to wait for other reviewers to chime in.

PS. I decided against the comments in _fast_matrix_market; in that case I think looking at the git history and finding this PR should be enough.

[mborland]

If it solves the issue I'm good with the patch being merged in. I did not expect this issue to require nearly this much iteration...

[rgommers]

This patch looks good, thanks to both of you for getting to the bottom of this.

We should ensure it's fixed upstream as well, to avoid problems in other packages or overwriting the fix in the future. Cc @mreineck for pocketfft and @alugowski for fast_matrix_market. Could you have a look at this and take over the fix in your packages if applicable there?

[rgommers]

LGTM now, thanks

[rgommers]

Looks like we're good to squash-merge this and backport to 1.12.x. I plan to do so soon unless there are more comments (or @h-vetinari beats me to it here).

[alugowski]

Good catch, I've upstreamed the change to stand-alone fast_matrix_market.

[mreineck]

Thanks for the heads-up!

Just for my understanding: the first change to pocketfft is purely cosmetic and does not change behavior at all, correct?
(It's still much cleaner, so I'm happy to port it over!)

The second change is already implemented upstream, as far as I can see; the line in question currently reads

#if (__cplusplus >= 201703L) && (!defined(__MINGW32__)) && (!defined(_MSC_VER))

In the successor package I've decided to only rely on my own aligned allocation function; the number of platforms doing this in weird ways or not at all is just too large.

[mborland]

Thanks for the heads-up!

Just for my understanding: the first change to pocketfft is purely cosmetic and does not change behavior at all, correct? (It's still much cleaner, so I'm happy to port it over!)

Yes; If you compile with -Wundef it'll fix that warning.

The second change is already implemented upstream, as far as I can see; the line in question currently reads

#if (__cplusplus >= 201703L) && (!defined(__MINGW32__)) && (!defined(_MSC_VER))

In the successor package I've decided to only rely on my own aligned allocation function; the number of platforms doing this in weird ways or not at all is just too large.

This stemmed from Clang-Cl which I don't think is covered by those macros so moving to your own implementation is likely better.