[libc] Use __builtin_bit_cast only when src and dest types are trivially copyable

[mikhail.ramalho]

Summary

This patch fixes the following compilation error:

/home/mgadelha/tools/llvm-project/libc/src/__support/CPP/bit.h:29:10: error: __builtin_bit_cast source type must be trivially copyable
  return __builtin_bit_cast(To, from);
         ^
/home/mgadelha/tools/llvm-project/libc/src/__support/FPUtil/FPBits.h:119:47: note: in instantiation of function template specialization '__llvm_libc::cpp::bit_cast<long double, __llvm_libc::cpp::UInt<128>>' requested here
  LIBC_INLINE T get_val() const { return cpp::bit_cast<T>(bits); }

This happen when building for a 32 bit target (in my case it was
riscv32) without int128 support, so we use UInt<128> instead, which
is not trivially copyable.

[sivachandra]

libc/src/__support/CPP/bit.h

15

We cannot use the C++ standard library in the runtime components of the libc. Would marking the copy constructor with = default solve your problem: https://github.com/llvm/llvm-project/blob/main/libc/src/__support/UInt.h#L40

[mikhail.ramalho]

libc/src/__support/CPP/bit.h

15

Not really... We get the following error:

/home/root/llvm-project/libc/src/__support/CPP/bit.h:29:10: error: __builtin_bit_cast destination type must be trivially copyable
  return __builtin_bit_cast(To, from);
         ^
/home/root/llvm-project/libc/src/__support/FPUtil/FPBits.h:111:50: note: in instantiation of function template specialization '__llvm_libc::cpp::bit_cast<__llvm_libc::cpp::UInt<128>, long double>' requested here
  constexpr explicit FPBits(XType x) : bits(cpp::bit_cast<UIntType>(x)) {}
                                                 ^
/home/root/llvm-project/libc/src/__support/FPUtil/NearestIntegerOperations.h:56:13: note: in instantiation of function template specialization '__llvm_libc::fputil::FPBits<long double>::FPBits<long double, 0>' requested here
  FPBits<T> bits(x);
            ^
/home/root/llvm-project/libc/src/math/generic/ceill.cpp:16:18: note: in instantiation of function template specialization '__llvm_libc::fputil::ceil<long double, 0>' requested here
  return fputil::ceil(x);

Maybe can use the compiler builtin __is_trivially_copyable (like https://github.com/llvm/llvm-project/blob/main/libcxx/include/__type_traits/is_trivially_copyable.h) and add it to libc/src/__support/CPP/type_traits.h? WDYT?

[mikhail.ramalho]

Remove C++ standard library in the runtime components of the libc

[asb]

In case it's helpful, I'll note you can force enable int128 support for rv32 clang with -fforce-enable-int128. This is a bit of a hacky solution to allow building compiler-rt for RV32 (which relies heavily in int128_t for its long double routines - I believe this wasn't an issue for other 32-bit architectures as RV32 is fairly unique in having sizeof(long double) == 16 rather than 8. GCC doesn't have such an option though, so avoiding that compiler option is probably preferable.

[sivachandra]

While this change is OK, we do want the UInt class to be trivially copyable. Do you know why it is not trivially copyable even when defaulted?

[mikhail.ramalho]

In D148739#4282973, @sivachandra wrote:

While this change is OK, we do want the UInt class to be trivially copyable. Do you know why it is not trivially copyable even when defaulted?

We also needed to default operator=, I'll update the patch with these changes.

[mikhail.ramalho]

@sivachandra while we don't need this anymore, I think it's a good addition to the codebase.

[sivachandra]

libc/src/__support/CPP/bit.h

32

I am on the fence about this. Should we be bit-casting instances of classes which are not trivially copyable? So, may be add something like this instead of trying to bit-cast:

static_assert(cpp::is_trivially_copyable<To>::value && cpp::is_trivially_copyable<From>::value,
                      "Cannot bit-cast instances of non-trivially copyable classes.");

[mikhail.ramalho]

libc/src/__support/CPP/bit.h

32

alright

BTW, in the possible implementation section of https://en.cppreference.com/w/cpp/numeric/bit_cast it also requires the dest type to be trivially constructible when using memcpy, should I add it too?

[mikhail.ramalho]

Added static assertions

[sivachandra]

libc/src/__support/CPP/bit.h

38

Shouldn't this be outside of the builtin check? Ideally, before the declaration of to on line 35?

[sivachandra]

libc/src/__support/CPP/type_traits.h

31

This is a libc internal file so we don't need the _ prefixes. Just use T here and below, and also just Args.

[mikhail.ramalho]

Addressed comments

libc/src/__support/CPP/bit.h

 //===-- Freestanding version of bit_cast  -----------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIBC_SUPPORT_CPP_BIT_H
 #define LLVM_LIBC_SUPPORT_CPP_BIT_H
 
+#include "src/__support/CPP/type_traits.h"
 #include "src/__support/macros/config.h" // LIBC_HAS_BUILTIN
 
 namespace __llvm_libc::cpp {
 
 #if LIBC_HAS_BUILTIN(__builtin_bit_cast)
 #define LLVM_LIBC_HAS_BUILTIN_BIT_CAST
 #endif
 
 #if LIBC_HAS_BUILTIN(__builtin_memcpy_inline)
 #define LLVM_LIBC_HAS_BUILTIN_MEMCPY_INLINE
 #endif
 
 // This function guarantees the bitcast to be optimized away by the compiler for
 // GCC >= 8 and Clang >= 6.
 template <class To, class From> constexpr To bit_cast(const From &from) {
   static_assert(sizeof(To) == sizeof(From), "To and From must be of same size");
 #if defined(LLVM_LIBC_HAS_BUILTIN_BIT_CAST)
+  if constexpr (cpp::is_trivially_copyable<To>::value &&
+                cpp::is_trivially_copyable<From>::value)
     return __builtin_bit_cast(To, from);
-#else
+#endif // defined(LLVM_LIBC_HAS_BUILTIN_BIT_CAST)
   To to;
   char *dst = reinterpret_cast<char *>(&to);
   const char *src = reinterpret_cast<const char *>(&from);
 #if defined(LLVM_LIBC_HAS_BUILTIN_MEMCPY_INLINE)
   __builtin_memcpy_inline(dst, src, sizeof(To));
 #else
   for (unsigned i = 0; i < sizeof(To); ++i)
     dst[i] = src[i];
 #endif // defined(LLVM_LIBC_HAS_BUILTIN_MEMCPY_INLINE)
   return to;
-#endif // defined(LLVM_LIBC_HAS_BUILTIN_BIT_CAST)
 }
 
 } // namespace __llvm_libc::cpp
 
 #endif // LLVM_LIBC_SUPPORT_CPP_BIT_H

libc/src/__support/CPP/type_traits.h

 
 template <typename T, T v> struct integral_constant {
   using value_type = T;
   static constexpr T value = v;
 };
 using true_type = cpp::integral_constant<bool, true>;
 using false_type = cpp::integral_constant<bool, false>;
 
+template <class _Tp>
+struct is_trivially_copyable
+    : public integral_constant<bool, __is_trivially_copyable(_Tp)> {};
+
 template <typename T, typename U> struct is_same : cpp::false_type {};
 template <typename T> struct is_same<T, T> : cpp::true_type {};
 template <typename T, typename U>
 inline constexpr bool is_same_v = is_same<T, U>::value;
 
 template <class T> struct is_const : cpp::false_type {};
 template <class T> struct is_const<const T> : cpp::true_type {};
 template <class T> inline constexpr bool is_const_v = is_const<T>::value;