[libc] first_trailing_one(0) should be 0.

[c8ef]

Fix this minor bug. See more context at #129892.

[llvmbot]

@llvm/pr-subscribers-libc

Author: Connector Switch (c8ef)

Changes

Fix this minor bug. See more context at #129892.

---

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

3 Files Affected:

• (modified) libc/src/__support/big_int.h (+3-2)
• (modified) libc/src/__support/math_extras.h (+3-2)
• (modified) libc/test/src/__support/math_extras_test.cpp (+1)

diff --git a/libc/src/__support/big_int.h b/libc/src/__support/big_int.h
index f44624a7eafce..b8d0a0487eeb8 100644
--- a/libc/src/__support/big_int.h
+++ b/libc/src/__support/big_int.h
@@ -1383,8 +1383,9 @@ first_trailing_zero(T value) {
 template <typename T>
 [[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<is_big_int_v<T>, int>
 first_trailing_one(T value) {
-  return value == cpp::numeric_limits<T>::max() ? 0
-                                                : cpp::countr_zero(value) + 1;
+  return (value == 0 || value == cpp::numeric_limits<T>::max())
+             ? 0
+             : cpp::countr_zero(value) + 1;
 }
 
 } // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/__support/math_extras.h b/libc/src/__support/math_extras.h
index 6f4a006aad270..c40fdb9cc97a1 100644
--- a/libc/src/__support/math_extras.h
+++ b/libc/src/__support/math_extras.h
@@ -146,8 +146,9 @@ first_trailing_zero(T value) {
 template <typename T>
 [[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, int>
 first_trailing_one(T value) {
-  return value == cpp::numeric_limits<T>::max() ? 0
-                                                : cpp::countr_zero(value) + 1;
+  return (value == 0 || value == cpp::numeric_limits<T>::max())
+             ? 0
+             : cpp::countr_zero(value) + 1;
 }
 
 template <typename T>
diff --git a/libc/test/src/__support/math_extras_test.cpp b/libc/test/src/__support/math_extras_test.cpp
index 01b216081d034..d77bc5d2f80de 100644
--- a/libc/test/src/__support/math_extras_test.cpp
+++ b/libc/test/src/__support/math_extras_test.cpp
@@ -95,6 +95,7 @@ TYPED_TEST(LlvmLibcBitTest, FirstTrailingZero, UnsignedTypesNoBigInt) {
 }
 
 TYPED_TEST(LlvmLibcBitTest, FirstTrailingOne, UnsignedTypesNoBigInt) {
+  EXPECT_EQ(first_trailing_one<T>(static_cast<T>(0)), 0);
   EXPECT_EQ(first_trailing_one<T>(cpp::numeric_limits<T>::max()), 0);
   for (int i = 0U; i != cpp::numeric_limits<T>::digits; ++i) {
     auto lhs = T(T(1) << size_t(i));

[lntue]

it should be just value == 0. When value is not 0, cpp::countr_zero(value) + 1 should be correct.

[c8ef]

It will fail with inputs like USHRT_MAX because we need it to return 0, but cpp::countr_zero(USHRT_MAX) + 1 = 1.

[lntue]

it should be just value == 0, the other case is cpp::countr_zero(value) + 1

[c8ef]

It will fail with inputs like USHRT_MAX because we need it to return 0, but cpp::countr_zero(USHRT_MAX) + 1 = 1.

[lntue]

Why should it return 0 with USHRT_MAX? The least significant index of the first bit 1 of USHRT_MAX is 0, so according to the spec, stdc_first_trailing_one_us(USHRT_MAX) should be 1. Hence cpp::countr_zero(USHRT_MAX) + 1 is the correct. If our tests do not reflect that, then the tests also need to be updated.

[c8ef]

Got it finally. Sorry for being careless. I'll update this tonight.

[c8ef]

@lntue Please take another look, thanks!

[lntue]

Thanks for finding and fixing the issue!