[AST] Fix handling of long double and bool in __builtin_bit_cast

[erik.pilkington]

Summary

On x86, long double has 6 unused trailing bytes. This patch changes the constant evaluator to treat them as though they were padding bytes, so reading from them results in an indeterminate value, and nothing is written for them. Also, fix a similar bug with bool, but instead of treating the unused bits as padding, enforce that they're zero.

Fixes a bug Louis pointed out in: https://reviews.llvm.org/D75960

[ldionne]

LGTM at high level, but I'm not really familiar with the code.

clang/lib/AST/ExprConstant.cpp

6365

semanticsSizeInBits is the number of bits actually used in the type?

[jfb]

Maybe you should test nullptr as well, given that it's all padding?

clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp

407

Sanity-check: 0 and 1 work? IIRC we had a discussion about whether false was 0 and true was 1, and we concluded that false was indeed 0 but true wasn't specified to be 1.

[erik.pilkington]

Add tests for bit_cast<bool>((char)1);

clang/lib/AST/ExprConstant.cpp

6365

Yeah

clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp

226–250

In D76323#1933791, @jfb wrote:

Maybe you should test nullptr as well, given that it's all padding?

There are some existing nullptr padding tests here ^^^, which seem to cover this.

407

Yeah, they work, I added a test in the update. I don't think it really matters that true could technically be 2 in the standard here, since the representation on our implementation is always 1.

[rsmith]

clang/lib/AST/ExprConstant.cpp

6364–6371

This appears more complex than necessary. bitcastToAPInt already returned the correct number of bits (eg, 80 for x86 fp80) so you don't need to compute that yourself. How about leaving this function alone and changing visitInt to store the number of (value representation) bits in the APSInt rather than the full (object representation) width of the type? As is, visitInt would also be wrong in the same way if the integer type had any full bytes of padding.

6462

If you want this to be a general check for integer types with padding, you should zext or sext as appropriate for the signedness of the truncated value. (Eg, you could set the signedness of Truncated to match the integer type, and use extend rather than zext here.)

[erik.pilkington]

Address review comments.

clang/lib/AST/ExprConstant.cpp

6364–6371

Sure, this causes us to have to handle bool in visitInt, but I think we should have been doing that anyways.

[erik.pilkington]

Ping!

[erik.pilkington]

Ping!

[erik.pilkington]

Ping!

[erik.pilkington]

Ping, @rsmith did you want to take another look at this?

[ldionne]

Gentle ping. This is blocking the implementation of std::bit_cast in libc++.

[tambre]

I've been using this + D75960 in my employer's Clang build for almost two months. We have been using std::bit_cast in our codebase quite a bit and haven't encountered any issues with these two changes. It would be nice to see this merged.

[rsmith]

This looks OK to me.

I think we'll also need updates for _ExtInt now too -- we have more fundamental types with padding than only long double and bool now. (I continue to be displeased that we assume 8-bit char in __builtin_bit_cast, but that's not new in this patch.)

clang/include/clang/Basic/DiagnosticASTKinds.td

def note_constexpr_bit_cast_invalid_type : Note<
   "bit_cast %select{from|to}0 a %select{|type with a }1"
   "%select{union|pointer|member pointer|volatile|reference}2 "
   "%select{type|member}1 is not allowed in a constant expression">;
 def note_constexpr_bit_cast_invalid_subtype : Note<
   "invalid type %0 is a %select{member|base}1 of %2">;
 def note_constexpr_bit_cast_indet_dest : Note<
   "indeterminate value can only initialize an object of type 'unsigned char'"
   "%select{, 'char',|}1 or 'std::byte'; %0 is invalid">;
+def note_constexpr_bit_cast_unrepresentable_value : Note<
+  "value %1 cannot be represented in type %0">;
 def note_constexpr_pseudo_destructor : Note<
   "pseudo-destructor call is not permitted in constant expressions "
   "until C++20">;
 def note_constexpr_construct_complex_elem : Note<
   "construction of individual component of complex number is not yet supported "
   "in constant expressions">;
 def note_constexpr_destroy_complex_elem : Note<
   "destruction of individual component of complex number is not yet supported "

clang/lib/AST/ExprConstant.cpp

 static const FunctionDecl *getVirtualOperatorDelete(QualType T) {
   if (CXXRecordDecl *RD = T->getAsCXXRecordDecl())
     if (CXXDestructorDecl *DD = RD->getDestructor())
       return DD->isVirtual() ? DD->getOperatorDelete() : nullptr;
   return nullptr;
 }
 
 /// Check that the given object is a suitable pointer to a heap allocation that
 /// still exists and is of the right kind for the purpose of a deletion.
 ///
 /// On success, returns the heap allocation to deallocate. On failure, produces
 /// a diagnostic and returns None.
 static Optional<DynAlloc *> CheckDeleteKind(EvalInfo &Info, const Expr *E,
                                             const LValue &Pointer,
                                             DynAlloc::Kind DeallocKind) {
   auto PointerAsString = [&] {
     return Pointer.toString(Info.Ctx, Info.Ctx.VoidPtrTy);
   };
 
   DynamicAllocLValue DA = Pointer.Base.dyn_cast<DynamicAllocLValue>();
   if (!DA) {
     Info.FFDiag(E, diag::note_constexpr_delete_not_heap_alloc)
         << PointerAsString();
     if (Pointer.Base)
class BitCastBuffer {
   // bit-fields.
   // FIXME: Its possible under the C++ standard for 'char' to not be 8 bits, but
   // we don't support a host or target where that is the case. Still, we should
   // use a more generic type in case we ever do.
   SmallVector<Optional<unsigned char>, 32> Bytes;
 
   static_assert(std::numeric_limits<unsigned char>::digits >= 8,
                 "Need at least 8 bit unsigned char");
 
   bool TargetIsLittleEndian;
 
 public:
   BitCastBuffer(CharUnits Width, bool TargetIsLittleEndian)
       : Bytes(Width.getQuantity()),
         TargetIsLittleEndian(TargetIsLittleEndian) {}
 
   LLVM_NODISCARD
if (Val.hasArrayFiller()) {
           return false;
       }
     }
 
     return true;
   }
 
   bool visitInt(const APSInt &Val, QualType Ty, CharUnits Offset) {
-    CharUnits Width = Info.Ctx.getTypeSizeInChars(Ty);
-    SmallVector<unsigned char, 8> Bytes(Width.getQuantity());
-    llvm::StoreIntToMemory(Val, &*Bytes.begin(), Width.getQuantity());
+    APSInt AdjustedVal = Val;
+    unsigned Width = AdjustedVal.getBitWidth();
+    if (Ty->isBooleanType()) {
+      Width = Info.Ctx.getTypeSize(Ty);
+      AdjustedVal = AdjustedVal.extend(Width);
+    }
+
+    SmallVector<unsigned char, 8> Bytes(Width / 8);
+    llvm::StoreIntToMemory(AdjustedVal, &*Bytes.begin(), Width / 8);
     Buffer.writeObject(Offset, Bytes);
     return true;
   }
 
   bool visitFloat(const APFloat &Val, QualType Ty, CharUnits Offset) {
     APSInt AsInt(Val.bitcastToAPInt());
     return visitInt(AsInt, Ty, Offset);
   }
class BufferToAPValueConverter {
   // Ideally this will be unreachable.
   llvm::NoneType unsupportedType(QualType Ty) {
     Info.FFDiag(BCE->getBeginLoc(),
                 diag::note_constexpr_bit_cast_unsupported_type)
         << Ty;
     return None;
   }
 
+  llvm::NoneType unrepresentableValue(QualType Ty, const APSInt &Val) {
+    Info.FFDiag(BCE->getBeginLoc(),
+                diag::note_constexpr_bit_cast_unrepresentable_value)
+        << Ty << Val.toString(/*Radix=*/10);
+    return None;
+  }
+
   Optional<APValue> visit(const BuiltinType *T, CharUnits Offset,
                           const EnumType *EnumSugar = nullptr) {
     if (T->isNullPtrType()) {
       uint64_t NullValue = Info.Ctx.getTargetNullPointerValue(QualType(T, 0));
       return APValue((Expr *)nullptr,
                      /*Offset=*/CharUnits::fromQuantity(NullValue),
                      APValue::NoLValuePath{}, /*IsNullPtr=*/true);
     }
 
     CharUnits SizeOf = Info.Ctx.getTypeSizeInChars(T);
+
+    // Work around floating point types that contain unused padding bytes. This
+    // is really just `long double` on x86, which is the only fundamental type
+    // with padding bytes.
+    if (T->isRealFloatingType()) {
+      const llvm::fltSemantics &Semantics =
+          Info.Ctx.getFloatTypeSemantics(QualType(T, 0));
+      unsigned NumBits = llvm::APFloatBase::getSizeInBits(Semantics);
+      assert(NumBits % 8 == 0);
+      CharUnits NumBytes = CharUnits::fromQuantity(NumBits / 8);
+      if (NumBytes != SizeOf)
+        SizeOf = NumBytes;
+    }
+
     SmallVector<uint8_t, 8> Bytes;
     if (!Buffer.readObject(Offset, SizeOf, Bytes)) {
       // If this is std::byte or unsigned char, then its okay to store an
       // indeterminate value.
       bool IsStdByte = EnumSugar && EnumSugar->isStdByteType();
       bool IsUChar =
           !EnumSugar && (T->isSpecificBuiltinType(BuiltinType::UChar) ||
                          T->isSpecificBuiltinType(BuiltinType::Char_U));
       if (!IsStdByte && !IsUChar) {
         QualType DisplayType(EnumSugar ? (const Type *)EnumSugar : T, 0);
         Info.FFDiag(BCE->getExprLoc(),
                     diag::note_constexpr_bit_cast_indet_dest)
             << DisplayType << Info.Ctx.getLangOpts().CharIsSigned;
         return None;
       }
 
       return APValue::IndeterminateValue();
     }
 
     APSInt Val(SizeOf.getQuantity() * Info.Ctx.getCharWidth(), true);
     llvm::LoadIntFromMemory(Val, &*Bytes.begin(), Bytes.size());
 
     if (T->isIntegralOrEnumerationType()) {
       Val.setIsSigned(T->isSignedIntegerOrEnumerationType());
+
+      unsigned IntWidth = Info.Ctx.getIntWidth(QualType(T, 0));
+      if (IntWidth != Val.getBitWidth()) {
+        APSInt Truncated = Val.trunc(IntWidth);
+        if (Truncated.extend(Val.getBitWidth()) != Val)
+          return unrepresentableValue(QualType(T, 0), Val);
+        Val = Truncated;
+      }
+
       return APValue(Val);
     }
 
     if (T->isRealFloatingType()) {
       const llvm::fltSemantics &Semantics =
           Info.Ctx.getFloatTypeSemantics(QualType(T, 0));
       return APValue(APFloat(Semantics, Val));
     }

clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp

 };
 
 static_assert(sizeof(int) == 4);
 static_assert(sizeof(long long) == 8);
 
 template <class To, class From>
 constexpr To bit_cast(const From &from) {
   static_assert(sizeof(To) == sizeof(From));
+  // expected-note@+9 {{cannot be represented in type 'bool'}}
+#ifdef __x86_64
+  // expected-note@+7 {{or 'std::byte'; '__int128' is invalid}}
+#endif
 #ifdef __CHAR_UNSIGNED__
   // expected-note@+4 2 {{indeterminate value can only initialize an object of type 'unsigned char', 'char', or 'std::byte'; 'signed char' is invalid}}
 #else
   // expected-note@+2 2 {{indeterminate value can only initialize an object of type 'unsigned char' or 'std::byte'; 'signed char' is invalid}}
 #endif
   return __builtin_bit_cast(To, from);
 }
 

 }
 
 void test_array_fill() {
   constexpr unsigned char a[4] = {1, 2};
   constexpr unsigned int i = bit_cast<unsigned int>(a);
   static_assert(i == (LITTLE_END ? 0x00000201 : 0x01020000));
 }
 
 typedef decltype(nullptr) nullptr_t;
 
 #ifdef __CHAR_UNSIGNED__
 // expected-note@+5 {{indeterminate value can only initialize an object of type 'unsigned char', 'char', or 'std::byte'; 'unsigned long' is invalid}}
 #else
 // expected-note@+3 {{indeterminate value can only initialize an object of type 'unsigned char' or 'std::byte'; 'unsigned long' is invalid}}
 #endif
 // expected-error@+1 {{constexpr variable 'test_from_nullptr' must be initialized by a constant expression}}
 constexpr unsigned long test_from_nullptr = __builtin_bit_cast(unsigned long, nullptr);
 
 constexpr int test_from_nullptr_pass = (__builtin_bit_cast(unsigned char[8], nullptr), 0);
 
 constexpr int test_to_nullptr() {
   nullptr_t npt = __builtin_bit_cast(nullptr_t, 0ul);
 
   struct indet_mem {
     unsigned char data[sizeof(void *)];
   };
   indet_mem im = __builtin_bit_cast(indet_mem, nullptr);
   nullptr_t npt2 = __builtin_bit_cast(nullptr_t, im);
 
   return 0;
 }
 
 constexpr int ttn = test_to_nullptr();
 
 // expected-warning@+2 {{returning reference to local temporary object}}
 // expected-note@+1 {{temporary created here}}
 constexpr const long &returns_local() { return 0L; }
 
 // expected-error@+2 {{constexpr variable 'test_nullptr_bad' must be initialized by a constant expression}}
 // expected-note@+1 {{read of temporary whose lifetime has ended}}
 constexpr nullptr_t test_nullptr_bad = __builtin_bit_cast(nullptr_t, returns_local());

 constexpr IdentityInStruct identity2a = {42};
 constexpr unsigned char identity2b = __builtin_bit_cast(unsigned char, identity2a.n);
 
 union IdentityInUnion {
   unsigned char n;
 };
 constexpr IdentityInUnion identity3a = {42};
 constexpr unsigned char identity3b = __builtin_bit_cast(unsigned char, identity3a.n);
+
+namespace test_bool {
+
+constexpr bool test_bad_bool = bit_cast<bool>('A'); // expected-error {{must be initialized by a constant expression}} expected-note{{in call}}
+
+static_assert(round_trip<signed char>(true), "");
+static_assert(round_trip<unsigned char>(false), "");
+static_assert(round_trip<bool>(false), "");
+
+static_assert(round_trip<bool>((char)0), "");
+static_assert(round_trip<bool>((char)1), "");
+}
+
+namespace test_long_double {
+#ifdef __x86_64
+constexpr __int128_t test_cast_to_int128 = bit_cast<__int128_t>((long double)0); // expected-error{{must be initialized by a constant expression}} expected-note{{in call}}
+
+constexpr long double ld = 3.1425926539;
+
+struct bytes {
+  unsigned char d[16];
+};
+
+static_assert(round_trip<bytes>(ld), "");
+
+static_assert(round_trip<long double>(10.0L));
+
+constexpr bool f(bool read_uninit) {
+  bytes b = bit_cast<bytes>(ld);
+  unsigned char ld_bytes[10] = {
+    0x0,  0x48, 0x9f, 0x49, 0xf0,
+    0x3c, 0x20, 0xc9, 0x0,  0x40,
+  };
+
+  for (int i = 0; i != 10; ++i)
+    if (ld_bytes[i] != b.d[i])
+      return false;
+
+  if (read_uninit && b.d[10]) // expected-note{{read of uninitialized object is not allowed in a constant expression}}
+    return false;
+
+  return true;
+}
+
+static_assert(f(/*read_uninit=*/false), "");
+static_assert(f(/*read_uninit=*/true), ""); // expected-error{{static_assert expression is not an integral constant expression}} expected-note{{in call to 'f(true)'}}
+
+constexpr bytes ld539 = {
+  0x0, 0x0,  0x0,  0x0,
+  0x0, 0x0,  0xc0, 0x86,
+  0x8, 0x40, 0x0,  0x0,
+  0x0, 0x0,  0x0,  0x0,
+};
+
+constexpr long double fivehundredandthirtynine = 539.0;
+
+static_assert(bit_cast<long double>(ld539) == fivehundredandthirtynine, "");
+
+#else
+static_assert(round_trip<__int128_t>(34.0L));
+#endif
+}