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 ?
}
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...
> > 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).
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.
> > __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.
Jakub
