Re: [PATCH] c++: v2: Add __builtin_bit_cast to implement std::bit_cast [PR93121]

[Jason Merrill <jason@redhat.com>]

On 11/25/20 1:50 PM, Jakub Jelinek wrote:
> On Wed, Nov 25, 2020 at 12:26:17PM -0500, Jason Merrill wrote:
>>> +		      if (DECL_BIT_FIELD (fld)
>>> +			  && DECL_NAME (fld) == NULL_TREE)
>>> +			continue;
>>
>> I think you want to check DECL_PADDING_P here; the C and C++ front ends set
>> it on unnamed bit-fields, and that's what is_empty_type looks at.
> 
> Ok, changed in my copy.  I'll also post a patch for
> __builtin_clear_padding to use DECL_PADDING_P in there instead of
> DECL_BIT_FIELD/DECL_NAME==NULL.
> 
>>> +      if (TREE_CODE (TREE_TYPE (arg)) == ARRAY_TYPE)
>>> +	{
>>> +	  /* Don't perform array-to-pointer conversion.  */
>>> +	  arg = mark_rvalue_use (arg, loc, true);
>>> +	  if (!complete_type_or_maybe_complain (TREE_TYPE (arg), arg, complain))
>>> +	    return error_mark_node;
>>> +	}
>>> +      else
>>> +	arg = decay_conversion (arg, complain);
>>
>> bit_cast operates on an lvalue argument, so I don't think we want
>> decay_conversion at all here.
>>
>>> +      if (error_operand_p (arg))
>>> +	return error_mark_node;
>>> +
>>> +      arg = convert_from_reference (arg);
>>
>> This shouldn't be necessary; the argument should already be converted from
>> reference.  Generally we call convert_from_reference on the result of some
>> processing, not on an incoming argument.
> 
> Removing these two regresses some tests in the testsuite.
> It is true that std::bit_cast's argument must be a reference, and so when
> one uses std::bit_cast one won't run into these problems, but the builtin's
> argument itself is an rvalue and so we need to deal with people calling it
> directly.
> So, commenting out the decay_conversion and convert_from_reference results
> in:
> extern V v;
> ...
>    __builtin_bit_cast (int, v);
> no longer being reported as invalid use of incomplete type, but
> error: '__builtin_bit_cast' source size '' not equal to destination type size '4'
> (note nothing in between '' for the size because the size is NULL).
> Ditto for:
> extern V *p;
> ...
>    __builtin_bit_cast (int, *p);
> I guess I could add some hand written code to deal with incomplete types
> to cure these.  But e.g. decay_conversion also calls mark_rvalue_use which
> we also need e.g. for -Wunused-but-set*, but don't we also need it e.g. for
> lambdas? The builtin is after all using the argument as an rvalue
> (reads it).

OK, it isn't exactly use as an rvalue, but I guess it's close enough to 
work.

> Another change that commenting out those two parts causes is different
> diagnostics on bit-cast4.C,
> bit-cast4.C:7:30: error: '__builtin_bit_cast' is not a constant expression because 'const int* const' is a pointer type
> bit-cast4.C:7:30: error: '__builtin_bit_cast' is not a constant expression because 'int D::* const' is a pointer to member type
> bit-cast4.C:7:30: error: '__builtin_bit_cast' is not a constant expression because 'int (D::* const)() const' is a pointer to member type
> The tests expect 'const int*', 'int D::*' and 'int (D::*)() const', i.e. the
> toplevel qualifiers stripped from those.
> Commenting out just the arg = convert_from_reference (arg); doesn't regress
> anything though, it is the decay_conversion.

Let's just drop that part, then.

>>> +/* Attempt to interpret aggregate of TYPE from bytes encoded in target
>>> +   byte order at PTR + OFF with LEN bytes.  MASK contains bits set if the value
>>> +   is indeterminate.  */
>>> +
>>> +static tree
>>> +cxx_native_interpret_aggregate (tree type, const unsigned char *ptr, int off,
>>> +				int len, unsigned char *mask,
>>> +				const constexpr_ctx *ctx, bool *non_constant_p,
>>> +				location_t loc)
>>
>> Can this be, say, native_interpret_initializer in fold-const?  It doesn't
>> seem closely tied to the front end other than diagnostics that could move to
>> the caller, like you've already done for the non-aggregate case.
> 
> The middle-end doesn't need it ATM for anything, plus I think the
> ctx/non_constant_p/loc and diagnostics is really C++ FE specific.
> If you really want it in fold-const.c, the only way I can imagine it is
> that it would be
> tree
> native_interpret_aggregate (tree type, const unsigned char *ptr, int off,
> 			    int len, unsigned char *mask = NULL,
> 			    tree (*mask_callback) (void *, int) = NULL,
> 			    void *mask_data = NULL)
> where C++ would call it with the mask argument, as mask_callback a FE function
> that would emit the diagnostics and decide what to return when mask is set
> on something, and mask_data would be a pointer to struct containing
> const constexpr_ctx *ctx; bool *non_constant_p; location_t loc;
> for it.

Instead of a callback, the function could express mask-related failure 
in a way that the caller can detect and diagnose.  Perhaps by clearing 
mask elements that correspond to padding in the output, so you can again 
just scan through the mask to see if there are any elements set?

Jason