Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libc++ isnan(integer-type) broken on Windows with -std=c++20 after #69431 #70225

Open
petrhosek opened this issue Oct 25, 2023 · 3 comments
Open
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. platform:windows test-suite

Comments

@petrhosek
Copy link
Member

libcxx/fuzzing/random.pass.cpp, std/depr/depr.c.headers/math_h.pass.cpp and std/numerics/c.math/cmath.pass.cpp are all failing with tip-of-tree Clang after #69431 landed:

# .---command stderr------------
# | In file included from C:\b\s\w\ir\x\w\llvm-llvm-project\libcxx\test\libcxx\fuzzing\random.pass.cpp:12:
# | In file included from C:/b/s/w/ir/x/w/llvm_build/include/c++/v1\cmath:319:
# | In file included from C:/b/s/w/ir/x/w/llvm_build/include/c++/v1\math.h:301:
# | In file included from C:\b\s\w\ir\cache\windows_sdk\Windows Kits\10\Include\10.0.19041.0\ucrt\math.h:11:
# | C:\b\s\w\ir\cache\windows_sdk\Windows Kits\10\Include\10.0.19041.0\ucrt\corecrt_math.h:413:16: error: call to 'fpclassify' is ambiguous
# |   413 |         return fpclassify(_X) == FP_NAN;
# |       |                ^~~~~~~~~~
# | C:\b\s\w\ir\x\w\llvm-llvm-project\libcxx\test\libcxx\fuzzing\random.pass.cpp:162:12: note: in instantiation of function template specialization 'isnan<short>' requested here
# |   162 |   if (std::isnan(res)) {
# |       |            ^
# | C:\b\s\w\ir\x\w\llvm-llvm-project\libcxx\test\libcxx\fuzzing\random.pass.cpp:174:10: note: in instantiation of function template specialization 'helper<std::uniform_int_distribution<short>>' requested here
# |   174 |   return helper<std::uniform_int_distribution<std::int16_t>>(data, size)       ||
# |       |          ^
# | C:\b\s\w\ir\cache\windows_sdk\Windows Kits\10\Include\10.0.19041.0\ucrt\corecrt_math.h:288:31: note: candidate function
# |   288 |     _Check_return_ inline int fpclassify(_In_ float _X) throw()
# |       |                               ^
# | C:\b\s\w\ir\cache\windows_sdk\Windows Kits\10\Include\10.0.19041.0\ucrt\corecrt_math.h:293:31: note: candidate function
# |   293 |     _Check_return_ inline int fpclassify(_In_ double _X) throw()
# |       |                               ^
# | C:\b\s\w\ir\cache\windows_sdk\Windows Kits\10\Include\10.0.19041.0\ucrt\corecrt_math.h:298:31: note: candidate function
# |   298 |     _Check_return_ inline int fpclassify(_In_ long double _X) throw()
# |       |                               ^
# | C:\b\s\w\ir\cache\windows_sdk\Windows Kits\10\Include\10.0.19041.0\ucrt\corecrt_math.h:413:16: error: call to 'fpclassify' is ambiguous
# |   413 |         return fpclassify(_X) == FP_NAN;
# |       |                ^~~~~~~~~~
# | C:\b\s\w\ir\x\w\llvm-llvm-project\libcxx\test\libcxx\fuzzing\random.pass.cpp:162:12: note: in instantiation of function template specialization 'isnan<bool>' requested here
# |   162 |   if (std::isnan(res)) {
# |       |            ^
# | C:\b\s\w\ir\x\w\llvm-llvm-project\libcxx\test\libcxx\fuzzing\random.pass.cpp:176:10: note: in instantiation of function template specialization 'helper<std::bernoulli_distribution>' requested here
# |   176 |          helper<std::bernoulli_distribution>(data, size)                       ||
# |       |          ^
# | C:\b\s\w\ir\cache\windows_sdk\Windows Kits\10\Include\10.0.19041.0\ucrt\corecrt_math.h:288:31: note: candidate function
# |   288 |     _Check_return_ inline int fpclassify(_In_ float _X) throw()
# |       |                               ^
# | C:\b\s\w\ir\cache\windows_sdk\Windows Kits\10\Include\10.0.19041.0\ucrt\corecrt_math.h:293:31: note: candidate function
# |   293 |     _Check_return_ inline int fpclassify(_In_ double _X) throw()
# |       |                               ^
# | C:\b\s\w\ir\cache\windows_sdk\Windows Kits\10\Include\10.0.19041.0\ucrt\corecrt_math.h:298:31: note: candidate function
# |   298 |     _Check_return_ inline int fpclassify(_In_ long double _X) throw()
# |       |                               ^
# | In file included from C:\b\s\w\ir\x\w\llvm-llvm-project\libcxx\test\libcxx\fuzzing\random.pass.cpp:16:
# | In file included from C:/b/s/w/ir/x/w/llvm_build/include/c++/v1\random:1686:
# | C:/b/s/w/ir/x/w/llvm_build/include/c++/v1\__random/clamp_to_integral.h:35:87: error: shift count is negative [-Werror,-Wshift-count-negative]
# |    35 |   return _FloatBigger ? numeric_limits<_IntT>::max() :  (numeric_limits<_IntT>::max() >> _Bits << _Bits);
# |       |                                                                                       ^  ~~~~~
# | C:/b/s/w/ir/x/w/llvm_build/include/c++/v1\__random/clamp_to_integral.h:46:27: note: in instantiation of function template specialization 'std::__max_representable_int_for_float<short, double, true, -38>' requested here
# |    46 |   const _IntT __max_val = __max_representable_int_for_float<_IntT, _RealT>();
# |       |                           ^
# | C:/b/s/w/ir/x/w/llvm_build/include/c++/v1\__random/poisson_distribution.h:181:31: note: in instantiation of function template specialization 'std::__clamp_to_integral<short, double>' requested here
# |   181 |                 return _VSTD::__clamp_to_integral<result_type>(__tx);
# |       |                               ^
# | C:/b/s/w/ir/x/w/llvm_build/include/c++/v1\__random/poisson_distribution.h:95:17: note: in instantiation of function template specialization 'std::poisson_distribution<short>::operator()<std::mersenne_twister_engine<unsigned int, 32, 624, 397, 31, 2567483615, 11, 4294967295, 7, 2636928640, 15, 4022730752, 18, 1812433253>>' requested here
# |    95 |         {return (*this)(__g, __p_);}
# |       |                 ^
# | C:\b\s\w\ir\x\w\llvm-llvm-project\libcxx\test\libcxx\fuzzing\random.pass.cpp:161:23: note: in instantiation of function template specialization 'std::poisson_distribution<short>::operator()<std::mersenne_twister_engine<unsigned int, 32, 624, 397, 31, 2567483615, 11, 4294967295, 7, 2636928640, 15, 4022730752, 18, 1812433253>>' requested here
# |   161 |   volatile auto res = d(engine);
# |       |                       ^
# | C:\b\s\w\ir\x\w\llvm-llvm-project\libcxx\test\libcxx\fuzzing\random.pass.cpp:177:10: note: in instantiation of function template specialization 'helper<std::poisson_distribution<short>>' requested here
# |   177 |          helper<std::poisson_distribution<std::int16_t>>(data, size)           ||
# |       |          ^
# | In file included from C:\b\s\w\ir\x\w\llvm-llvm-project\libcxx\test\libcxx\fuzzing\random.pass.cpp:16:
# | In file included from C:/b/s/w/ir/x/w/llvm_build/include/c++/v1\random:1686:
# | C:/b/s/w/ir/x/w/llvm_build/include/c++/v1\__random/clamp_to_integral.h:35:96: error: shift count is negative [-Werror,-Wshift-count-negative]
# |    35 |   return _FloatBigger ? numeric_limits<_IntT>::max() :  (numeric_limits<_IntT>::max() >> _Bits << _Bits);
# |       |                                                                                                ^  ~~~~~
# | 4 errors generated.
# `-----------------------------
@ldionne
Copy link
Member

ldionne commented Oct 31, 2023

Do we still need this issue? I thought this was on Clang to fix long-term or is it not?

@H-G-Hristov
Copy link
Contributor

The failing tests were XFAILED in #79619
This is still an issue IMO.

@jyknight jyknight changed the title libc++ tests are failing on Windows after #69431 libc++ isnan(integer-type) broken on Windows with -std=c++20 after #69431 Mar 12, 2024
@jyknight
Copy link
Member

Just ran into this issue when migrating to Clang c++20 for some windows code. I've retitled the bug to indicate what it's actually about now.

In the MSVC UCRT headers, their math.h is actually intended to implement the full C++ spec requirements for the header. They don't expect the C++ stdlib to provide its own wrapper math.h for C++-specific functionality, and their STL does not do so -- unlike Unix implementations, where libc provides a math.h which only implement the C standard functions, and the C++ stdlib is expected to wrap it.

Unfortunately, as the very-old microsoft/STL#519 issue records, their header is non-compliant with C++11, by not providing integer overloads for any of the floating-point functions like isnan/isinf/etc. Apparently the C++ standard didn't clearly-enough require the integer overloads in C++11 (though it now does), and so they've been dragging their feet.

Thus, libc++ currently defines fpclassify and signbit overloads for MSVC, to add this support.

Unfortunately, the libc++ workaround to the MSVC header bug relies upon -fdelayed-template-parsing to function.

Because the MSVC math.h defines std::isnan in terms of fpclassify, libc++'s addition of more overloads for std::fpclassify after the MSVC header was included should not be visible to the MSVC math.h's isnan function template -- but they were previously, only because of the nonstandard -fdelayed-template-parsing.

Now that we aren't using that nonstandard mode by default in -std=c++20, that means that std::isnan(integer-type) no longer works in C++20.

In my opinion, the best thing for libc++ to do here would actually be to stop trying to workaround math.h bugs in MSVC -- and therefore become buggy by missing the integer overloads for isnan/etc unconditionally in MSVC mode, until MSVC fixes its math.h header.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. platform:windows test-suite
Projects
None yet
Development

No branches or pull requests

5 participants