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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
