On 12/1/21 04:46, Jakub Jelinek wrote:
On Tue, Nov 30, 2021 at 03:19:19PM -0500, Jason Merrill wrote:
On 11/30/21 07:17, Jakub Jelinek wrote:
On Mon, Nov 29, 2021 at 10:25:58PM -0500, Jason Merrill wrote:
It's a DR. Really, it was intended to be part of C++20; at the Cologne
meeting in 2019 CWG thought byteswap was going to make C++20, so this bugfix
could go in as part of that paper.
Ok, changed to be done unconditionally now.
Also, allowing indeterminate values that are never read was in C++20
(P1331).
Reading P1331R2 again, I'm still puzzled.
Our current behavior (both before and after this patch) is that if
some variable is scalar and has indeterminate value or if an aggregate
variable has some members (possibly nested) with indeterminate values,
in constexpr contexts we allow copying those into other vars of the
same type (e.g. the testcases in the patch below test mere copying
of the whole structures or unsigned char result of __builtin_bit_cast),
That seems to be a bug, since the copy involves an lvalue-to-rvalue
conversion.
Yeah. The questions are
1) where in constexpr.c to check for this and diagnose it
2) if we do it quite often, whether check for it won't be too expensive,
because we want to check recursively for CONSTRUCTORs with
CONSTRUCTOR_NO_CLEARING and possibly whether it has any members or
array elements that are omitted in the CONSTRUCTOR (or can we rely
on CONSTRUCTOR_NO_CLEARING being set only if it is incomplete? Then
we'd need to make sure we clear that flag if we store the last member
or array element into it)
E.g. for 1), I wonder where exactly the lvalue-to-rvalue conversions occur,
say for
struct S { int a, b, c; };
constexpr int
foo ()
{
struct S s;
s.a = 1;
s.c = 3;
return s.a; // Does lvalue-to-rvalue conversion happen here on whole s
// and then on s.a too, or just on s.a ?
Only on s.a. Class copy is modeled as memberwise copy, even though we
optimize it into block copy for trivially copyable classes.
}
E.g. when cxx_eval_component_reference or cxx_eval_array_reference
or cxx_eval_bit_field_ref we cxx_eval_constant_expression the base
first, so if the 1) diagnostics would be in cxx_eval_constant_expression
when it is called on an lvalue_p with !lval, we'd trigger it even when
we might not want to trigger it...
I think we can work around that so that we complain about the full
CONSTRUCTOR when we're doing a full copy, and otherwise only complain
about the member.
but we reject if we actually use them in some other way (e.g. try to
read a member from a variable that has that member indeterminate,
see e.g. bit-cast14.C (f5, f6, f7), even when reading it into an
unsigned char variable.
That's correct.
Then there is P1331R2 which makes the UB on
"an lvalue-to-rvalue conversion that is applied to an object with
indeterminate value ([basic.indet]);"
but isn't even the
unsigned char a = __builtin_bit_cast (unsigned char, u);
unsigned char b = a;
case non-constant then when __builtin_bit_cast returns indeterminate value?
Good point. So it would seem to follow that if the output is going to have
an indeterminate value, it's non-constant, we don't have to work hard in
constexpr evaluation, and f1-4 are all non-constant. And the new bit_cast
text is only interesting for non-constant evaluation.
I think it isn't that simple. One thing is that std::bit_cast is described
as a black box that takes a reference argument, so whether lvalue-to-rvalue
conversion happens there is just something that isn't guaranteed (though,
the way __builtin_bit_cast is implemented which is called with the value
rather than its address there is lvalue-to-rvalue conversion on it).
Eh, all functions in the library are described as black boxes. But your
next argument is more persuasive:
But another more important thing is that at least in the testcases in the
patch if one kills those extra copies of __builtin_bit_cast lhs to other
vars which are non-constant according to P1331R2, I don't see anything
that would imply they are non-constant. If there is the lvalue-to-rvalue
conversion on what the std::bit_cast argument refers to, that would make
non-constant e.g. class objects with some members indeterminate (e.g.
in the above foo s is like that), but in the testcases the
__builtin_bit_cast arguments have all named members initialized, all they
have is some padding bits in between those members and that can't be
non-constant in itself, otherwise most of constant evaluation would be
non-constant. And the std::bit_cast rules say what happens in that case.
Ah, good point.
__builtin_bit_cast returns rvalue, so no lvalue-to-rvalue conversion happens
in that case, so supposely
unsigned char a = __builtin_bit_cast (unsigned char, u);
is fine, but on
Eh, there's clearly an lvalue-rvalue conversion involved in reading from the
source value.
Yes, but if u doesn't contain any indeterminate (leaf) members, then
it shouldn't be non-constant.
So IMHO we need the patch I've posted (with the testcases slightly adjusted
not to do those extra copies afterwards for now),
and try to deal with the lvalue-to-rvalue conversion of indeterminate later,
and once done, perhaps in a copy of those testcases readd those extra copies
afterwards and verify it is rejected.
Makes sense, except:
+ gcc_assert (end == 1 || end == 2);
This seems to assert that the value representation of a bit-field can't
span more than two bytes, which is wrong for, say,
struct A
{
int : 1;
unsigned __int128 c : 128; // value representation spans 17 bytes
};
Jason
