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++][math] Fix undue overflowing of std::hypot(x,y,z) #93350

Merged
merged 22 commits into from
Jul 23, 2024

Conversation

PaulXiCao
Copy link
Contributor

Closes #92782.

The 3-dimentionsional std::hypot(x,y,z) was sub-optimally implemented. This lead to possible over-/underflows in (intermediate) results which can be circumvented by this proposed change.

The idea is to to scale the arguments (see linked issue for full discussion).

Tests have been added for problematic over- and underflows.

@PaulXiCao PaulXiCao requested a review from a team as a code owner May 24, 2024 21:27
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label May 24, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented May 24, 2024

@llvm/pr-subscribers-libcxx

Author: None (PaulXiCao)

Changes

Closes #92782.

The 3-dimentionsional std::hypot(x,y,z) was sub-optimally implemented. This lead to possible over-/underflows in (intermediate) results which can be circumvented by this proposed change.

The idea is to to scale the arguments (see linked issue for full discussion).

Tests have been added for problematic over- and underflows.


Full diff: https://github.com/llvm/llvm-project/pull/93350.diff

3 Files Affected:

  • (modified) libcxx/include/__math/hypot.h (+78)
  • (modified) libcxx/include/cmath (+1-24)
  • (modified) libcxx/test/std/numerics/c.math/cmath.pass.cpp (+62-15)
diff --git a/libcxx/include/__math/hypot.h b/libcxx/include/__math/hypot.h
index 8e2c0f5b6b6d8..6b68e7bfed341 100644
--- a/libcxx/include/__math/hypot.h
+++ b/libcxx/include/__math/hypot.h
@@ -9,11 +9,16 @@
 #ifndef _LIBCPP___MATH_HYPOT_H
 #define _LIBCPP___MATH_HYPOT_H
 
+#include <__algorithm/max.h>
 #include <__config>
+#include <__math/abs.h>
+#include <__math/roots.h>
 #include <__type_traits/enable_if.h>
 #include <__type_traits/is_arithmetic.h>
 #include <__type_traits/is_same.h>
 #include <__type_traits/promote.h>
+#include <array>
+#include <limits>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
@@ -41,6 +46,79 @@ inline _LIBCPP_HIDE_FROM_ABI typename __promote<_A1, _A2>::type hypot(_A1 __x, _
   return __math::hypot((__result_type)__x, (__result_type)__y);
 }
 
+#if _LIBCPP_STD_VER >= 17
+template <class _Real>
+struct __hypot_factors {
+  _Real __threshold;
+  _Real __scale_xyz;
+  _Real __scale_M;
+};
+
+// returns [underflow_factors, overflow_factors]
+template <class _Real>
+std::array<__hypot_factors<_Real>, 2> __create_factors() {
+  static_assert(std::numeric_limits<_Real>::is_iec559);
+
+  __hypot_factors<_Real> __underflow, __overflow;
+  if constexpr (std::is_same_v<_Real, float>) {
+    static_assert(-125 == std::numeric_limits<_Real>::min_exponent);
+    static_assert(+128 == std::numeric_limits<_Real>::max_exponent);
+    __underflow = __hypot_factors<_Real>{0x1.0p-62f, 0x1.0p70f, 0x1.0p-70f};
+    __overflow  = __hypot_factors<_Real>{0x1.0p62f, 0x1.0p-70f, 0x1.0p+70f};
+  } else if constexpr (std::is_same_v<_Real, double>) {
+    static_assert(-1021 == std::numeric_limits<_Real>::min_exponent);
+    static_assert(+1024 == std::numeric_limits<_Real>::max_exponent);
+    __underflow = __hypot_factors<_Real>{0x1.0p-510, 0x1.0p600, 0x1.0p-600};
+    __overflow  = __hypot_factors<_Real>{0x1.0p510, 0x1.0p-600, 0x1.0p+600};
+  } else { // long double
+    static_assert(std::is_same_v<_Real, long double>);
+    static_assert(-16'381 == std::numeric_limits<_Real>::min_exponent);
+    static_assert(+16'384 == std::numeric_limits<_Real>::max_exponent);
+    __underflow = __hypot_factors<_Real>{0x1.0p-8'190l, 0x1.0p9'000l, 0x1.0p-9'000l};
+    __overflow  = __hypot_factors<_Real>{0x1.0p8'190l, 0x1.0p-9'000l, 0x1.0p+9'000l};
+  }
+  return {__underflow, __overflow};
+}
+
+template <class _Real>
+_Real __hypot(_Real __x, _Real __y, _Real __z) {
+  const auto [__underflow, __overflow] = __create_factors<_Real>();
+  _Real __M                            = std::max({__math::fabs(__x), __math::fabs(__y), __math::fabs(__z)});
+  if (__M > __overflow.__threshold) { // x*x + y*y + z*z might overflow
+    __x *= __overflow.__scale_xyz;
+    __y *= __overflow.__scale_xyz;
+    __z *= __overflow.__scale_xyz;
+    __M = __overflow.__scale_M;
+  } else if (__M < __underflow.__threshold) { // x*x + y*y + z*z might underflow
+    __x *= __underflow.__scale_xyz;
+    __y *= __underflow.__scale_xyz;
+    __z *= __underflow.__scale_xyz;
+    __M = __underflow.__scale_M;
+  } else
+    __M = 1;
+  return __M * __math::sqrt(__x * __x + __y * __y + __z * __z);
+}
+
+inline _LIBCPP_HIDE_FROM_ABI float hypot(float __x, float __y, float __z) { return __hypot(__x, __y, __z); }
+
+inline _LIBCPP_HIDE_FROM_ABI double hypot(double __x, double __y, double __z) { return __hypot(__x, __y, __z); }
+
+inline _LIBCPP_HIDE_FROM_ABI long double hypot(long double __x, long double __y, long double __z) {
+  return __hypot(__x, __y, __z);
+}
+
+template <class _A1,
+          class _A2,
+          class _A3,
+          std::enable_if_t< is_arithmetic_v<_A1> && is_arithmetic_v<_A2> && is_arithmetic_v<_A3>, int> = 0 >
+_LIBCPP_HIDE_FROM_ABI typename __promote<_A1, _A2, _A3>::type hypot(_A1 __x, _A2 __y, _A3 __z) _NOEXCEPT {
+  using __result_type = typename __promote<_A1, _A2, _A3>::type;
+  static_assert(!(
+      std::is_same_v<_A1, __result_type> && std::is_same_v<_A2, __result_type> && std::is_same_v<_A3, __result_type>));
+  return __hypot(static_cast<__result_type>(__x), static_cast<__result_type>(__y), static_cast<__result_type>(__z));
+}
+#endif
+
 } // namespace __math
 
 _LIBCPP_END_NAMESPACE_STD
diff --git a/libcxx/include/cmath b/libcxx/include/cmath
index dd194bbb55896..49f35caa7b74e 100644
--- a/libcxx/include/cmath
+++ b/libcxx/include/cmath
@@ -305,6 +305,7 @@ constexpr long double lerp(long double a, long double b, long double t) noexcept
 */
 
 #include <__config>
+#include <__math/hypot.h>
 #include <__type_traits/enable_if.h>
 #include <__type_traits/is_arithmetic.h>
 #include <__type_traits/is_constant_evaluated.h>
@@ -544,30 +545,6 @@ using ::scalbnl _LIBCPP_USING_IF_EXISTS;
 using ::tgammal _LIBCPP_USING_IF_EXISTS;
 using ::truncl _LIBCPP_USING_IF_EXISTS;
 
-#if _LIBCPP_STD_VER >= 17
-inline _LIBCPP_HIDE_FROM_ABI float hypot(float __x, float __y, float __z) {
-  return sqrt(__x * __x + __y * __y + __z * __z);
-}
-inline _LIBCPP_HIDE_FROM_ABI double hypot(double __x, double __y, double __z) {
-  return sqrt(__x * __x + __y * __y + __z * __z);
-}
-inline _LIBCPP_HIDE_FROM_ABI long double hypot(long double __x, long double __y, long double __z) {
-  return sqrt(__x * __x + __y * __y + __z * __z);
-}
-
-template <class _A1, class _A2, class _A3>
-inline _LIBCPP_HIDE_FROM_ABI
-    typename enable_if_t< is_arithmetic<_A1>::value && is_arithmetic<_A2>::value && is_arithmetic<_A3>::value,
-                          __promote<_A1, _A2, _A3> >::type
-    hypot(_A1 __lcpp_x, _A2 __lcpp_y, _A3 __lcpp_z) _NOEXCEPT {
-  typedef typename __promote<_A1, _A2, _A3>::type __result_type;
-  static_assert((!(is_same<_A1, __result_type>::value && is_same<_A2, __result_type>::value &&
-                   is_same<_A3, __result_type>::value)),
-                "");
-  return std::hypot((__result_type)__lcpp_x, (__result_type)__lcpp_y, (__result_type)__lcpp_z);
-}
-#endif
-
 template <class _A1, __enable_if_t<is_floating_point<_A1>::value, int> = 0>
 _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR bool __constexpr_isnan(_A1 __lcpp_x) _NOEXCEPT {
 #if __has_builtin(__builtin_isnan)
diff --git a/libcxx/test/std/numerics/c.math/cmath.pass.cpp b/libcxx/test/std/numerics/c.math/cmath.pass.cpp
index 9379084499792..544c197ceb1af 100644
--- a/libcxx/test/std/numerics/c.math/cmath.pass.cpp
+++ b/libcxx/test/std/numerics/c.math/cmath.pass.cpp
@@ -12,14 +12,17 @@
 
 // <cmath>
 
+#include <array>
 #include <cmath>
 #include <limits>
 #include <type_traits>
 #include <cassert>
 
+#include "fp_compare.h"
 #include "test_macros.h"
 #include "hexfloat.h"
 #include "truncate_fp.h"
+#include "type_algorithms.h"
 
 // convertible to int/float/double/etc
 template <class T, int N=0>
@@ -1113,6 +1116,44 @@ void test_fmin()
     assert(std::fmin(1,0) == 0);
 }
 
+struct TestHypot3 {
+    template <class Real>
+    static void operator()() {
+        const auto check = [](Real elem, Real abs_tol) {
+            assert(std::isfinite(std::hypot(elem, Real(0), Real(0))));
+            assert(fptest_close(std::hypot(elem, Real(0), Real(0)), elem, abs_tol));
+            assert(std::isfinite(std::hypot(elem, elem, Real(0))));
+            assert(fptest_close(std::hypot(elem, elem, Real(0)), std::sqrt(Real(2)) * elem, abs_tol));
+            assert(std::isfinite(std::hypot(elem, elem, elem)));
+            assert(fptest_close(std::hypot(elem, elem, elem), std::sqrt(Real(3)) * elem, abs_tol));
+        };
+
+        { // check for overflow
+            const auto [elem, abs_tol] = []() -> std::array<Real, 2> {
+                if constexpr (std::is_same_v<Real, float>)
+                    return {1e20f, 1e16f};
+                else if constexpr (std::is_same_v<Real, double>)
+                    return {1e300, 1e287};
+                else // long double
+                    return {1e4000l, 1e3985l};
+            }();
+            check(elem, abs_tol);
+        }
+
+        { // check for underflow
+            const auto [elem, abs_tol] = []() -> std::array<Real, 2> {
+                if constexpr (std::is_same_v<Real, float>)
+                    return {1e-20f, 1e-24f};
+                else if constexpr (std::is_same_v<Real, double>)
+                    return {1e-287, 1e-300};
+                else // long double
+                    return {1e-3985l, 1e-4000l};
+            }();
+            check(elem, abs_tol);
+        }
+    }
+};
+
 void test_hypot()
 {
     static_assert((std::is_same<decltype(std::hypot((float)0, (float)0)), float>::value), "");
@@ -1136,24 +1177,30 @@ void test_hypot()
     assert(std::hypot(3,4) == 5);
 
 #if TEST_STD_VER > 14
-    static_assert((std::is_same<decltype(std::hypot((float)0, (float)0, (float)0)), float>::value), "");
-    static_assert((std::is_same<decltype(std::hypot((float)0, (bool)0, (float)0)), double>::value), "");
-    static_assert((std::is_same<decltype(std::hypot((float)0, (unsigned short)0, (double)0)), double>::value), "");
-    static_assert((std::is_same<decltype(std::hypot((float)0, (int)0, (long double)0)), long double>::value), "");
-    static_assert((std::is_same<decltype(std::hypot((float)0, (double)0, (long)0)), double>::value), "");
-    static_assert((std::is_same<decltype(std::hypot((float)0, (long double)0, (unsigned long)0)), long double>::value), "");
-    static_assert((std::is_same<decltype(std::hypot((float)0, (int)0, (long long)0)), double>::value), "");
-    static_assert((std::is_same<decltype(std::hypot((float)0, (int)0, (unsigned long long)0)), double>::value), "");
-    static_assert((std::is_same<decltype(std::hypot((float)0, (double)0, (double)0)), double>::value), "");
-    static_assert((std::is_same<decltype(std::hypot((float)0, (long double)0, (long double)0)), long double>::value), "");
-    static_assert((std::is_same<decltype(std::hypot((float)0, (float)0, (double)0)), double>::value), "");
-    static_assert((std::is_same<decltype(std::hypot((float)0, (float)0, (long double)0)), long double>::value), "");
-    static_assert((std::is_same<decltype(std::hypot((float)0, (double)0, (long double)0)), long double>::value), "");
-    static_assert((std::is_same<decltype(std::hypot((int)0, (int)0, (int)0)), double>::value), "");
-    static_assert((std::is_same<decltype(hypot(Ambiguous(), Ambiguous(), Ambiguous())), Ambiguous>::value), "");
+    // clang-format off
+    static_assert((std::is_same_v<decltype(std::hypot((float)0, (float)0,          (float)0)),              float>));
+    static_assert((std::is_same_v<decltype(std::hypot((float)0, (bool)0,           (float)0)),              double>));
+    static_assert((std::is_same_v<decltype(std::hypot((float)0, (unsigned short)0, (double)0)),             double>));
+    static_assert((std::is_same_v<decltype(std::hypot((float)0, (int)0,            (long double)0)),        long double>));
+    static_assert((std::is_same_v<decltype(std::hypot((float)0, (double)0,         (long)0)),               double>));
+    static_assert((std::is_same_v<decltype(std::hypot((float)0, (long double)0,    (unsigned long)0)),      long double>));
+    static_assert((std::is_same_v<decltype(std::hypot((float)0, (int)0,            (long long)0)),          double>));
+    static_assert((std::is_same_v<decltype(std::hypot((float)0, (int)0,            (unsigned long long)0)), double>));
+    static_assert((std::is_same_v<decltype(std::hypot((float)0, (double)0,         (double)0)),             double>));
+    static_assert((std::is_same_v<decltype(std::hypot((float)0, (long double)0,    (long double)0)),        long double>));
+    static_assert((std::is_same_v<decltype(std::hypot((float)0, (float)0,          (double)0)),             double>));
+    static_assert((std::is_same_v<decltype(std::hypot((float)0, (float)0,          (long double)0)),        long double>));
+    static_assert((std::is_same_v<decltype(std::hypot((float)0, (double)0,         (long double)0)),        long double>));
+    static_assert((std::is_same_v<decltype(std::hypot((int)0,   (int)0,            (int)0)),                double>));
+    static_assert((std::is_same_v<decltype(hypot(Ambiguous(), Ambiguous(), Ambiguous())), Ambiguous>));
+    // clang-format on
 
     assert(std::hypot(2,3,6) == 7);
     assert(std::hypot(1,4,8) == 9);
+
+    // Check for undue over-/underflows of intermediate results.
+    // See discussion at https://github.com/llvm/llvm-project/issues/92782.
+    types::for_each(types::floating_point_types(), TestHypot3());
 #endif
 }
 

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! I have some comments. TBH, I have not really looked deeply into the implementation changes, I'm waiting for you to address some of my comments about explaining what's going on. But I'm sure I'll be OK with the changes once I understand them :).

libcxx/include/__math/hypot.h Outdated Show resolved Hide resolved
libcxx/include/__math/hypot.h Outdated Show resolved Hide resolved
libcxx/include/__math/hypot.h Outdated Show resolved Hide resolved
libcxx/include/__math/hypot.h Outdated Show resolved Hide resolved
libcxx/include/__math/hypot.h Outdated Show resolved Hide resolved
libcxx/include/__math/hypot.h Outdated Show resolved Hide resolved
libcxx/include/__math/hypot.h Show resolved Hide resolved
static_assert((std::is_same<decltype(std::hypot((int)0, (int)0, (int)0)), double>::value), "");
static_assert((std::is_same<decltype(hypot(Ambiguous(), Ambiguous(), Ambiguous())), Ambiguous>::value), "");
// clang-format off
static_assert((std::is_same_v<decltype(std::hypot((float)0, (float)0, (float)0)), float>));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static_assert((std::is_same_v<decltype(std::hypot((float)0, (float)0, (float)0)), float>));
static_assert(std::is_same_v<decltype(std::hypot((float)0, (float)0, (float)0)), float>);

While we're at it, you don't need double parens. Also applies to the surrounding lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that as well, but it is like this all throughout this rather large file. I tried to stay consistent to it...

What do you think if I create another mr which changes only this style issue all throughout the file later on? (And keeping it as is for this mr?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds totally reasonable!

libcxx/test/std/numerics/c.math/cmath.pass.cpp Outdated Show resolved Hide resolved
@ldionne ldionne self-assigned this Jul 4, 2024
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically LGTM with still a bit of feedback.

libcxx/include/__math/hypot.h Outdated Show resolved Hide resolved
libcxx/include/__math/hypot.h Outdated Show resolved Hide resolved
@PaulXiCao PaulXiCao force-pushed the hypot branch 2 times, most recently from cf378ed to 268ca5b Compare July 7, 2024 17:40
@ldionne ldionne added this to the LLVM 19.X Release milestone Jul 9, 2024
@ldionne
Copy link
Member

ldionne commented Jul 18, 2024

@PaulXiCao This seems to be failing with many error messages that seem legitimate:

__math/hypot.h:74:15: error: magnitude of floating-point constant too large for type 'long double'; maximum is 1.7976931348623157E+308 [-Werror,-Wliteral-range]
      return {0x1.0p+8'190l, 0x1.0p-9'000l};
              ^

Copy link

github-actions bot commented Jul 18, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@PaulXiCao
Copy link
Contributor Author

@PaulXiCao This seems to be failing with many error messages that seem legitimate:

__math/hypot.h:74:15: error: magnitude of floating-point constant too large for type 'long double'; maximum is 1.7976931348623157E+308 [-Werror,-Wliteral-range]
      return {0x1.0p+8'190l, 0x1.0p-9'000l};
              ^

@ldionne Should be fixed now.

@ldionne ldionne merged commit 9628777 into llvm:main Jul 23, 2024
57 checks passed
Copy link

@PaulXiCao Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

@ldionne
Copy link
Member

ldionne commented Jul 23, 2024

/cherry-pick 9628777

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Jul 23, 2024
The 3-dimentionsional `std::hypot(x,y,z)` was sub-optimally implemented.
This lead to possible over-/underflows in (intermediate) results which
can be circumvented by this proposed change.

The idea is to to scale the arguments (see linked issue for full
discussion).

Tests have been added for problematic over- and underflows.

Closes llvm#92782

(cherry picked from commit 9628777)
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 23, 2024

/pull-request #100141

sgundapa pushed a commit to sgundapa/upstream_effort that referenced this pull request Jul 23, 2024
The 3-dimentionsional `std::hypot(x,y,z)` was sub-optimally implemented.
This lead to possible over-/underflows in (intermediate) results which
can be circumvented by this proposed change.

The idea is to to scale the arguments (see linked issue for full
discussion).

Tests have been added for problematic over- and underflows.

Closes llvm#92782
@PaulXiCao PaulXiCao deleted the hypot branch July 23, 2024 17:51
@kamaub
Copy link
Contributor

kamaub commented Jul 23, 2024

It looks like this commit breaks the following buildbots: sanitizer-ppc64le-linux build no. 1522 and sanitizer-ppc64be-linux build no. 1249. I will attempt to isolate and provide more information about the failure but could you take a look and provide a fix as soon as possible or revert if the fix will take too long.

tru pushed a commit to llvmbot/llvm-project that referenced this pull request Jul 24, 2024
The 3-dimentionsional `std::hypot(x,y,z)` was sub-optimally implemented.
This lead to possible over-/underflows in (intermediate) results which
can be circumvented by this proposed change.

The idea is to to scale the arguments (see linked issue for full
discussion).

Tests have been added for problematic over- and underflows.

Closes llvm#92782

(cherry picked from commit 9628777)
@hctim
Copy link
Collaborator

hctim commented Jul 24, 2024

And, for your reference, this only breaks when building libcxx on PowerPC (as far as we're aware). The sanitizer buildbots are okay on other architectures (x86, arm64):

[1683/1855] Building CXX object libcxx/src/CMakeFiles/cxx_static.dir/valarray.cpp.o
FAILED: libcxx/src/CMakeFiles/cxx_static.dir/valarray.cpp.o 
/home/buildbots/llvm-external-buildbots/workers/ppc64le-sanitizer/sanitizer-ppc64le/build/build_default/./bin/clang++ --target=powerpc64le-unknown-linux-gnu -DLIBCXX_BUILDING_LIBCXXABI -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_LIBCPP_BUILDING_LIBRARY -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_LINK_PTHREAD_LIB -D_LIBCPP_LINK_RT_LIB -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/libcxx/src -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-sanitizer/sanitizer-ppc64le/build/build_default/include/powerpc64le-unknown-linux-gnu/c++/v1 -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-sanitizer/sanitizer-ppc64le/build/build_default/include/c++/v1 -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/libcxxabi/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG -std=c++23 -UNDEBUG -faligned-allocation -nostdinc++ -fvisibility-inlines-hidden -fvisibility=hidden -fsized-deallocation -Wall -Wextra -Wnewline-eof -Wshadow -Wwrite-strings -Wno-unused-parameter -Wno-long-long -Werror=return-type -Wextra-semi -Wundef -Wunused-template -Wformat-nonliteral -Wno-user-defined-literals -Wno-covered-switch-default -Wno-suggest-override -Wno-error -MD -MT libcxx/src/CMakeFiles/cxx_static.dir/valarray.cpp.o -MF libcxx/src/CMakeFiles/cxx_static.dir/valarray.cpp.o.d -o libcxx/src/CMakeFiles/cxx_static.dir/valarray.cpp.o -c /home/buildbots/llvm-external-buildbots/workers/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/libcxx/src/valarray.cpp
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm-project/libcxx/src/valarray.cpp:9:
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-sanitizer/sanitizer-ppc64le/build/build_default/include/c++/v1/valarray:363:
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-sanitizer/sanitizer-ppc64le/build/build_default/include/c++/v1/cmath:316:
/home/buildbots/llvm-external-buildbots/workers/ppc64le-sanitizer/sanitizer-ppc64le/build/build_default/include/c++/v1/__math/hypot.h:60:17: error: static assertion failed due to requirement 'std::numeric_limits<long double>::is_iec559'
   60 |   static_assert(std::numeric_limits<_Real>::is_iec559);
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/buildbots/llvm-external-buildbots/workers/ppc64le-sanitizer/sanitizer-ppc64le/build/build_default/include/c++/v1/__math/hypot.h:94:65: note: in instantiation of function template specialization 'std::__math::__hypot_factors<long double>' requested here
   94 |   const auto [__overflow_threshold, __overflow_scale] = __math::__hypot_factors<_Real>();
      |                                                                 ^
/home/buildbots/llvm-external-buildbots/workers/ppc64le-sanitizer/sanitizer-ppc64le/build/build_default/include/c++/v1/__math/hypot.h:116:18: note: in instantiation of function template specialization 'std::__math::__hypot<long double>' requested here
  116 |   return __math::__hypot(__x, __y, __z);
      |                  ^
/home/buildbots/llvm-external-buildbots/workers/ppc64le-sanitizer/sanitizer-ppc64le/build/build_default/include/c++/v1/__math/hypot.h:76:19: error: static assertion failed due to requirement 'sizeof(long double) == sizeof(double)'
   76 |     static_assert(sizeof(_Real) == sizeof(double));
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/buildbots/llvm-external-buildbots/workers/ppc64le-sanitizer/sanitizer-ppc64le/build/build_default/include/c++/v1/__math/hypot.h:76:33: note: expression evaluates to '16 == 8'
   76 |     static_assert(sizeof(_Real) == sizeof(double));
      |                   ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
2 errors generated.

hctim added a commit that referenced this pull request Jul 24, 2024
…93350)"

This reverts commit 9628777.

More details in #93350, but
this broke the PowerPC sanitizer bots.
@PaulXiCao
Copy link
Contributor Author

I will probably not have time to work on this before the weekend. The fix is probably another special case guarded by preprocessor #ifdef.

yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Summary:
The 3-dimentionsional `std::hypot(x,y,z)` was sub-optimally implemented.
This lead to possible over-/underflows in (intermediate) results which
can be circumvented by this proposed change.

The idea is to to scale the arguments (see linked issue for full
discussion).

Tests have been added for problematic over- and underflows.

Closes #92782

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251315
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
…93350)"

Summary:
This reverts commit 9628777.

More details in #93350, but
this broke the PowerPC sanitizer bots.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60250681
PaulXiCao added a commit to PaulXiCao/llvm-project that referenced this pull request Jul 26, 2024
Harini0924 pushed a commit to Harini0924/llvm-project that referenced this pull request Aug 1, 2024
…lvm#93350)"

This reverts commit 9628777.

More details in llvm#93350, but
this broke the PowerPC sanitizer bots.
ldionne pushed a commit that referenced this pull request Aug 5, 2024
This is in relation to mr #93350. It was merged to main, but reverted
because of failing sanitizer builds on PowerPC.

The fix includes replacing the hard-coded threshold constants (e.g.
`__overflow_threshold`) for different floating-point sizes by a general
computation using `std::ldexp`. Thus, it should now work for all architectures.
This has the drawback of not being `constexpr` anymore as `std::ldexp`
is not implemented as `constexpr` (even though the standard mandates it
for C++23).

Closes #92782
ldionne pushed a commit to ldionne/llvm-project that referenced this pull request Aug 5, 2024
…lvm#93350)"

This reverts commit 9628777.

More details in llvm#93350, but
this broke the PowerPC sanitizer bots.

(cherry picked from commit 1031335)
ldionne pushed a commit to ldionne/llvm-project that referenced this pull request Aug 5, 2024
)

This is in relation to mr llvm#93350. It was merged to main, but reverted
because of failing sanitizer builds on PowerPC.

The fix includes replacing the hard-coded threshold constants (e.g.
`__overflow_threshold`) for different floating-point sizes by a general
computation using `std::ldexp`. Thus, it should now work for all architectures.
This has the drawback of not being `constexpr` anymore as `std::ldexp`
is not implemented as `constexpr` (even though the standard mandates it
for C++23).

Closes llvm#92782

(cherry picked from commit 72825fd)
banach-space pushed a commit to banach-space/llvm-project that referenced this pull request Aug 7, 2024
)

This is in relation to mr llvm#93350. It was merged to main, but reverted
because of failing sanitizer builds on PowerPC.

The fix includes replacing the hard-coded threshold constants (e.g.
`__overflow_threshold`) for different floating-point sizes by a general
computation using `std::ldexp`. Thus, it should now work for all architectures.
This has the drawback of not being `constexpr` anymore as `std::ldexp`
is not implemented as `constexpr` (even though the standard mandates it
for C++23).

Closes llvm#92782
tru pushed a commit to ldionne/llvm-project that referenced this pull request Aug 13, 2024
…lvm#93350)"

This reverts commit 9628777.

More details in llvm#93350, but
this broke the PowerPC sanitizer bots.

(cherry picked from commit 1031335)
tru pushed a commit to ldionne/llvm-project that referenced this pull request Aug 13, 2024
)

This is in relation to mr llvm#93350. It was merged to main, but reverted
because of failing sanitizer builds on PowerPC.

The fix includes replacing the hard-coded threshold constants (e.g.
`__overflow_threshold`) for different floating-point sizes by a general
computation using `std::ldexp`. Thus, it should now work for all architectures.
This has the drawback of not being `constexpr` anymore as `std::ldexp`
is not implemented as `constexpr` (even though the standard mandates it
for C++23).

Closes llvm#92782

(cherry picked from commit 72825fd)
kstoimenov pushed a commit to kstoimenov/llvm-project that referenced this pull request Aug 15, 2024
)

This is in relation to mr llvm#93350. It was merged to main, but reverted
because of failing sanitizer builds on PowerPC.

The fix includes replacing the hard-coded threshold constants (e.g.
`__overflow_threshold`) for different floating-point sizes by a general
computation using `std::ldexp`. Thus, it should now work for all architectures.
This has the drawback of not being `constexpr` anymore as `std::ldexp`
is not implemented as `constexpr` (even though the standard mandates it
for C++23).

Closes llvm#92782
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect overflow in std::hypot
6 participants