[libcxx-commits] [libcxx] [libc++] Fix incorrect overflow checking in std::lcm (PR #96310)

[Louis Dionne via libcxx-commits]

https://github.com/ldionne updated https://github.com/llvm/llvm-project/pull/96310

>From 04cf65e2996c09cc94c34d6167c6c8d7597b0af2 Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Fri, 21 Jun 2024 10:03:33 -0400
Subject: [PATCH 1/4] [libc++] Fix incorrect overflow checking in std::lcm

We should have been using __builtin_mul_overflow from the start instead
of adding a manual (and error-prone) check for overflow.

Fixes #96196
---
 libcxx/include/__numeric/gcd_lcm.h            |  7 +++--
 .../numeric.ops/numeric.ops.lcm/lcm.pass.cpp  | 26 ++++++++++++++++++-
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/libcxx/include/__numeric/gcd_lcm.h b/libcxx/include/__numeric/gcd_lcm.h
index 87ebbae0157f5..3916db2fae0e6 100644
--- a/libcxx/include/__numeric/gcd_lcm.h
+++ b/libcxx/include/__numeric/gcd_lcm.h
@@ -117,8 +117,11 @@ _LIBCPP_CONSTEXPR _LIBCPP_HIDE_FROM_ABI common_type_t<_Tp, _Up> lcm(_Tp __m, _Up
   using _Rp  = common_type_t<_Tp, _Up>;
   _Rp __val1 = __ct_abs<_Rp, _Tp>()(__m) / std::gcd(__m, __n);
   _Rp __val2 = __ct_abs<_Rp, _Up>()(__n);
-  _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN((numeric_limits<_Rp>::max() / __val1 > __val2), "Overflow in lcm");
-  return __val1 * __val2;
+  _Rp __res;
+  bool __overflow = __builtin_mul_overflow(__val1, __val2, &__res);
+  (void)__overflow;
+  _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(!__overflow, "Overflow in lcm");
+  return __res;
 }
 
 #endif // _LIBCPP_STD_VER
diff --git a/libcxx/test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp b/libcxx/test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp
index 16bfbbef41c45..992ecf64a8567 100644
--- a/libcxx/test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp
+++ b/libcxx/test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp
@@ -89,6 +89,13 @@ constexpr bool do_test(int = 0)
     return accumulate;
 }
 
+template <class T>
+constexpr bool test_limits() {
+    assert(std::lcm(std::numeric_limits<T>::max() - 1, std::numeric_limits<T>::max() - 1) == std::numeric_limits<T>::max() - 1);
+    assert(std::lcm(std::numeric_limits<T>::max(), std::numeric_limits<T>::max()) == std::numeric_limits<T>::max());
+    return true;
+}
+
 int main(int argc, char**)
 {
     int non_cce = argc; // a value that can't possibly be constexpr
@@ -141,5 +148,22 @@ int main(int argc, char**)
     assert(res1 == 1324997410816LL);
     }
 
-  return 0;
+    // https://github.com/llvm/llvm-project/issues/96196
+    {
+        assert(test_limits<unsigned int>());
+        assert(test_limits<std::uint32_t>());
+        assert(test_limits<std::uint64_t>());
+        assert(test_limits<int>());
+        assert(test_limits<std::int32_t>());
+        assert(test_limits<std::int64_t>());
+
+        static_assert(test_limits<unsigned int>(), "");
+        static_assert(test_limits<std::uint32_t>(), "");
+        static_assert(test_limits<std::uint64_t>(), "");
+        static_assert(test_limits<int>(), "");
+        static_assert(test_limits<std::int32_t>(), "");
+        static_assert(test_limits<std::int64_t>(), "");
+    }
+
+    return 0;
 }

>From f0d63a0cc8fc09c8a353788e75ee67b834db5752 Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Fri, 21 Jun 2024 10:28:38 -0400
Subject: [PATCH 2/4] Fix modules issue

---
 .../test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp   | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libcxx/test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp b/libcxx/test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp
index 992ecf64a8567..edd6d9fef0d71 100644
--- a/libcxx/test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp
+++ b/libcxx/test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp
@@ -16,6 +16,7 @@
 #include <cassert>
 #include <climits>
 #include <cstdint>
+#include <limits>
 #include <type_traits>
 #include "test_macros.h"
 

>From 0e5972b4c67fc8ea8f31731da85f0f2b5cfaf6e3 Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Mon, 24 Jun 2024 23:35:59 -0500
Subject: [PATCH 3/4] Use maybe_unused attribute

---
 libcxx/include/__numeric/gcd_lcm.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/libcxx/include/__numeric/gcd_lcm.h b/libcxx/include/__numeric/gcd_lcm.h
index 3916db2fae0e6..90c7253f6f124 100644
--- a/libcxx/include/__numeric/gcd_lcm.h
+++ b/libcxx/include/__numeric/gcd_lcm.h
@@ -118,8 +118,7 @@ _LIBCPP_CONSTEXPR _LIBCPP_HIDE_FROM_ABI common_type_t<_Tp, _Up> lcm(_Tp __m, _Up
   _Rp __val1 = __ct_abs<_Rp, _Tp>()(__m) / std::gcd(__m, __n);
   _Rp __val2 = __ct_abs<_Rp, _Up>()(__n);
   _Rp __res;
-  bool __overflow = __builtin_mul_overflow(__val1, __val2, &__res);
-  (void)__overflow;
+  [[maybe_unused]] bool __overflow = __builtin_mul_overflow(__val1, __val2, &__res);
   _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(!__overflow, "Overflow in lcm");
   return __res;
 }

>From 95bb475468ee9d474df47bcaf69c6f49d99991d6 Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Mon, 24 Jun 2024 23:36:42 -0500
Subject: [PATCH 4/4] NFC fix #endif comment:

---
 libcxx/include/__numeric/gcd_lcm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libcxx/include/__numeric/gcd_lcm.h b/libcxx/include/__numeric/gcd_lcm.h
index 90c7253f6f124..2a63ccfcccdb6 100644
--- a/libcxx/include/__numeric/gcd_lcm.h
+++ b/libcxx/include/__numeric/gcd_lcm.h
@@ -123,7 +123,7 @@ _LIBCPP_CONSTEXPR _LIBCPP_HIDE_FROM_ABI common_type_t<_Tp, _Up> lcm(_Tp __m, _Up
   return __res;
 }
 
-#endif // _LIBCPP_STD_VER
+#endif // _LIBCPP_STD_VER >= 17
 
 _LIBCPP_END_NAMESPACE_STD