[libc] -Wshift-count-overflow in libc/src/__support/UInt.h

[nickdesaulniers]

[692/693] Building CXX object projects/libc/src/stdio/prin...s/libc.src.stdio.printf_core.converter.dir/converter.cpp.o
In file included from /android0/llvm-project/libc/src/__support/UInt128.h:12,
                 from /android0/llvm-project/libc/src/__support/FPUtil/FloatProperties.h:12,
                 from /android0/llvm-project/libc/src/__support/FPUtil/FPBits.h:17,
                 from /android0/llvm-project/libc/src/stdio/printf_core/core_structs.h:13,
                 from /android0/llvm-project/libc/src/stdio/printf_core/converter.h:12,
                 from /android0/llvm-project/libc/src/stdio/printf_core/converter.cpp:9:
/android0/llvm-project/libc/src/__support/UInt.h: In instantiation of ‘constexpr __llvm_libc_18_0_0_git::cpp::BigInt<Bits, Signed>::operator T() const [with T = unsigned int; <template-parameter-2-2> = void; long unsigned int Bits = 320; bool Signed = false]’:
/android0/llvm-project/libc/src/__support/float_to_string.h:381:57:   required from here
/android0/llvm-project/libc/src/__support/UInt.h:125:53: warning: left shift count >= width of type [-Wshift-count-overflow]
  125 |       return static_cast<T>((static_cast<T>(val[1]) << 64) + lo);
      |                             ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~

I'm trying to get -Werror re-enabled in #74506; building with GCC flags the above warning.

[llvmbot]

@llvm/issue-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

``` [692/693] Building CXX object projects/libc/src/stdio/prin...s/libc.src.stdio.printf_core.converter.dir/converter.cpp.o In file included from /android0/llvm-project/libc/src/__support/UInt128.h:12, from /android0/llvm-project/libc/src/__support/FPUtil/FloatProperties.h:12, from /android0/llvm-project/libc/src/__support/FPUtil/FPBits.h:17, from /android0/llvm-project/libc/src/stdio/printf_core/core_structs.h:13, from /android0/llvm-project/libc/src/stdio/printf_core/converter.h:12, from /android0/llvm-project/libc/src/stdio/printf_core/converter.cpp:9: /android0/llvm-project/libc/src/__support/UInt.h: In instantiation of ‘constexpr __llvm_libc_18_0_0_git::cpp::BigInt<Bits, Signed>::operator T() const [with T = unsigned int; <template-parameter-2-2> = void; long unsigned int Bits = 320; bool Signed = false]’: /android0/llvm-project/libc/src/__support/float_to_string.h:381:57: required from here /android0/llvm-project/libc/src/__support/UInt.h:125:53: warning: left shift count >= width of type [-Wshift-count-overflow] 125 | return static_cast<T>((static_cast<T>(val[1]) << 64) + lo); | ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~ ``` I'm trying to get -Werror re-enabled in https://github.com//pull/74506; building with GCC flags the above warning.

[nickdesaulniers]

@lntue is there any way we can use SFINAE to split BigInt::operator T() in two?

I'd imagine we could have the implementation when sizeof(T) == 8 be:

return static_cast<T>(val[0]);

then the existing implementation (when sizeof(T) > 8) be:

    // T is 128-bit.
    T lo = static_cast<T>(val[0]);

    if constexpr (Bits == 64) {
      if constexpr (Signed) {
        // Extend sign for negative numbers.
        return (val[0] >> 63) ? ((T(-1) << 64) + lo) : lo;
      } else {
        return lo;
      }
    } else {
      return static_cast<T>((static_cast<T>(val[1]) << 64) + lo);
    }

my SFINAE skills aren't great; so not sure if that's possible...

[lntue]

You can split the template into 2 with SFINAE as follow:

template <typename T, typename = cpp::enable_if_t<cpp::is_integral_v<T> &&
                                                     sizeof(T) <= 8 &&
                                                     !cpp::is_same_v<T, bool>>>
LIBC_INLINE constexpr explicit operator T() const {
  return static_cast<T>(val[0]);
}

template <typename T, typename = cpp::enable_if_t<cpp::is_integral_v<T> &&
                                                     sizeof(T) == 16>
LIBC_INLINE constexpr explicit operator T() const {
  // T is 128-bit.
  ...
}

[nickdesaulniers]

That's what I would have thought. I have:

diff --git a/libc/src/__support/UInt.h b/libc/src/__support/UInt.h
index 3bec2e3a4713..5d9049186b82 100644
--- a/libc/src/__support/UInt.h
+++ b/libc/src/__support/UInt.h
@@ -104,12 +104,15 @@ template <size_t Bits, bool Signed> struct BigInt {
   }
 
   template <typename T, typename = cpp::enable_if_t<cpp::is_integral_v<T> &&
-                                                    sizeof(T) <= 16 &&
+                                                    sizeof(T) <= 8 &&
                                                     !cpp::is_same_v<T, bool>>>
   LIBC_INLINE constexpr explicit operator T() const {
-    if constexpr (sizeof(T) <= 8)
-      return static_cast<T>(val[0]);
+    return static_cast<T>(val[0]);
+  }
 
+  template <typename T, typename = cpp::enable_if_t<cpp::is_integral_v<T> &&
+                                                    sizeof(T) == 16>>
+  LIBC_INLINE constexpr explicit operator T() const {
     // T is 128-bit.
     T lo = static_cast<T>(val[0]);
 
@@ -121,7 +124,6 @@ template <size_t Bits, bool Signed> struct BigInt {
         return lo;
       }
     } else {
-      // TODO: silence shift warning
       return static_cast<T>((static_cast<T>(val[1]) << 64) + lo);
     }
   }

but I observe the error with GCC:

/android0/llvm-project/libc/src/__support/UInt.h:115:34: error: ‘template<long unsigned int Bits, bool Signed> template<class T, class> constexpr __llvm_libc_18_0_0_git::cpp::BigInt<Bits, Signed>::operator T() const’ cannot be overloaded with ‘template<long unsigned int Bits, bool Signed> template<class T, class> constexpr __llvm_libc_18_0_0_git::cpp::BigInt<Bits, Signed>::operator T() const’
  115 |   LIBC_INLINE constexpr explicit operator T() const {
      |                                  ^~~~~~~~
/android0/llvm-project/libc/src/__support/UInt.h:109:34: note: previous declaration ‘template<long unsigned int Bits, bool Signed> template<class T, class> constexpr __llvm_libc_18_0_0_git::cpp::BigInt<Bits, Signed>::operator T() const’
  109 |   LIBC_INLINE constexpr explicit operator T() const {
      |                                  ^~~~~~~~

I thought the usage of cpp::enable_if_t would make these implementations mutually exclusive. How is that considered a redeclaration?

[lntue]

How about this one:

  template <typename T>
  LIBC_INLINE constexpr explicit operator
  typename cpp::enable_if_t<cpp::is_integral_v<T> && sizeof(T) <= 8 &&
                            !cpp::is_same_v<T, bool>, T>() const {
    return static_cast<T>(val[0]);
  }

  template <typename T>
  LIBC_INLINE constexpr explicit operator
  typename cpp::enable_if_t<cpp::is_integral_v<T> && sizeof(T) == 16, T>() const {
    // T is 128-bit.
    T lo = static_cast<T>(val[0]);

    if constexpr (Bits == 64) {
      if constexpr (Signed) {
        // Extend sign for negative numbers.
        return (val[0] >> 63) ? ((T(-1) << 64) + lo) : lo;
      } else {
        return lo;
      }
    } else {
      return static_cast<T>((static_cast<T>(val[1]) << 64) + lo);
    }
  }

[nickdesaulniers]

gah, you beat me to it! I have this working for either g++ or clang++:

diff --git a/libc/src/__support/UInt.h b/libc/src/__support/UInt.h
index 3bec2e3a4713..16a1fcc7d807 100644
--- a/libc/src/__support/UInt.h
+++ b/libc/src/__support/UInt.h
@@ -103,13 +103,21 @@ template <size_t Bits, bool Signed> struct BigInt {
       val[i] = words[i];
   }
 
-  template <typename T, typename = cpp::enable_if_t<cpp::is_integral_v<T> &&
-                                                    sizeof(T) <= 16 &&
-                                                    !cpp::is_same_v<T, bool>>>
+
+
+  template<typename T>
   LIBC_INLINE constexpr explicit operator T() const {
-    if constexpr (sizeof(T) <= 8)
-      return static_cast<T>(val[0]);
+    return to<T>();
+  }
 
+  template <typename T>
+  LIBC_INLINE constexpr
+  cpp::enable_if_t<cpp::is_integral_v<T> && sizeof(T) <= 8 && !cpp::is_same_v<T, bool>, T> to() const {
+    return static_cast<T>(val[0]);
+  }
+  template <typename T>
+  LIBC_INLINE constexpr
+  cpp::enable_if_t<cpp::is_integral_v<T> && sizeof(T) == 16, T> to() const {
     // T is 128-bit.
     T lo = static_cast<T>(val[0]);
 
@@ -121,7 +129,6 @@ template <size_t Bits, bool Signed> struct BigInt {
         return lo;
       }
     } else {
-      // TODO: silence shift warning
       return static_cast<T>((static_cast<T>(val[1]) << 64) + lo);
     }
   }

I guess what I was missing was the use of the typename keyword preceding the use of cpp::enable_if_t. Looking at what you have, I can guess what it's doing, but I don't have the understanding of why typename was necessary.