[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

[Richard Smith - zygoloid via Phabricator via llvm-commits]

rsmith added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:5360
+  // 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.
----------------
A `static_assert(std::numeric_limits<unsigned char>::digits >= 8);` would be nice.


================
Comment at: clang/lib/AST/ExprConstant.cpp:5461-5469
+    case APValue::LValue: {
+      LValue LVal;
+      LVal.setFrom(Info.Ctx, Val);
+      APValue RVal;
+      if (!handleLValueToRValueConversion(Info, BCE, Ty.withConst(),
+                                          LVal, RVal))
+        return false;
----------------
This looks wrong: bitcasting a pointer should not dereference the pointer and store its pointee! (Likewise for a reference member.) But I think this should actually simply be unreachable: we already checked for pointers and reference members in `checkBitCastConstexprEligibilityType`.

(However, I think it currently *is* reached, because there's also some cases where the operand of the bitcast is an lvalue, and the resulting lvalue gets misinterpreted as a value of the underlying type, landing us here. See below.)


================
Comment at: clang/lib/AST/ExprConstant.cpp:5761-5764
+    for (FieldDecl *FD : Record->fields()) {
+      if (!checkBitCastConstexprEligibilityType(Loc, FD->getType(), Info, Ctx))
+        return note(0, FD->getType(), FD->getBeginLoc());
+    }
----------------
The spec for `bit_cast` also asks us to check for members of reference type. That can't happen because a type with reference members can never be trivially-copyable, but please include an assert here to demonstrate to a reader that we've thought about and handled that case.


================
Comment at: clang/lib/AST/ExprConstant.cpp:7098-7099
   case CK_BitCast:
+    // CK_BitCast originating from a __builtin_bit_cast have different constexpr
+    // semantics. This is only reachable when bit casting to nullptr_t.
+    if (isa<BuiltinBitCastExpr>(E))
----------------
I think this is reversed from the way I'd think about it: casts to `void*` are modeled as bit-casts, and so need special handling here. (Theoretically it'd be preferable to call the base class function for all other kinds of bitcast we encounter, but it turns out to not matter because only bitcasts to `nullptr_t` are ever evaluatable currently. In future we might want to support bit-casts between integers and pointers (for constant folding only), and then it might matter.)


================
Comment at: clang/lib/Sema/SemaCast.cpp:2801-2802
+void CastOperation::CheckBitCast() {
+  if (isPlaceholder())
+    SrcExpr = Self.CheckPlaceholderExpr(SrcExpr.get());
+
----------------
erik.pilkington wrote:
> rsmith wrote:
> > Should we be performing the default lvalue conversions here too? Or maybe materializing a temporary? (Are we expecting a glvalue or prvalue as the operand of `bit_cast`? It seems unnecessarily complex for the AST representation to allow either.)
> Okay, new patch filters the expression through DefaultLvalueConversion (which deals with placeholders too).
Hmm, sorry, on reflection I think I've led you down the wrong path here.

I think we really do want the operand of the bitcast here to be a glvalue. In the case where the operand is of class type, it will be an lvalue even after `DefaultLvalueConversion`, because there's no such thing as a `CK_LValueToRValue` conversion on class type in C++. (For example, this is why you currently need the code in `BitCastReader` that deals with `APValue::LValue` -- when bitcasting a class type, you get an lvalue denoting the object rather than the object itself.)

Also, forming an lvalue-to-rvalue conversion here is at least theoretically wrong, because that conversion (notionally) discards the values of padding bits in the "from" object, whereas `std::bit_cast` requires those values to be preserved into the destination if they are defined in the source object.

Perhaps we should also have different cast kinds for an lvalue-to-rvalue bitcast (this new thing) versus an rvalue-to-rvalue bitcast (`CK_BitCast`).


================
Comment at: clang/test/CodeGenCXX/builtin-bit-cast.cpp:100-112
+void test_array(int (&ary)[2]) {
+  // CHECK-LABEL: define void @_Z10test_arrayRA2_i
+  __builtin_bit_cast(unsigned long, ary);
+
+  // CHECK: [[ARY_PTR:%.*]] = alloca [2 x i32]*, align 8
+  // CHECK-NEXT: [[TEMP]] = alloca [2 x i32], align 8
+  // store ary into ARY_PTR
----------------
If we remove the `DefaultLvalueConversion` from building the bitcast expression, we should be able to avoid the unnecessary extra alloca here, and instead `memcpy` directly from `ary` to the `unsigned long`. (This is the most important testcase in this file, since it's the only one that gives `__builtin_bit_cast` an lvalue operand, which is what `std::bit_cast` will do.)


================
Comment at: clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp:1
+// RUN: %clang_cc1 -verify -std=c++2a -fsyntax-only -triple x86_64-apple-macosx10.14.0 %s
+// RUN: %clang_cc1 -verify -std=c++2a -fsyntax-only -triple aarch64_be-linux-gnu %s
----------------
Please add a test that structs with reference members are rejected, along with the other cases from [bit.cast]/2.4-2.8.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62825/new/

https://reviews.llvm.org/D62825