Re: [PATCH v2] infcall: Add support for integer literals as reference function paramters

[Andrew Burgess <aburgess@redhat.com>]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
>
> Keith> This patch attempts to mitigate the shortcomings of passing literals
> Keith> to inferior function calls requiring references.  The specific use case here
> Keith> is std::map's operator[]:
>
> Thanks for doing this.
>
> Keith>  struct value *
> Keith> -value_coerce_to_target (struct value *val)
> Keith> +value_coerce_to_target (struct value *val, struct type *param_type)
> Keith>  {
>
> I wasn't totally sold on doing the work here and not in value_arg_coerce
> but I guess it makes sense.
>
> Keith> +  if (param_type != nullptr && param_type->language () == language_cplus
> Keith> +      && TYPE_IS_REFERENCE (param_type))
> Keith> +    {
>
> Can we get here with a value that has an address, and so we don't need
> to make a copy?  Like if there's a local variable 'x', and you do 'print
> map[x]'?

I took a look at this patch too, and came here to say the same thing.
In fact, this is exactly why the 'param_type->language () ==
language_cplus' is needed.  The Fortran breakage that Keith mentions
does this:

  (gdb) p return_string(returned_string_debugger, 40)
  (gdb) p returned_string_debugger

The first line calls 'return_string' passing in the inferior variable
'returned_string_debugger' (length 40), the function is then going to
fill that variable in for us, which we print on the next line.

Without the 'param_type->language() == language_cplus' bit, we end up
creating a copy of 'returned_string_debugger' in the inferior, which the
'return_string' function fills in.  Then back in GDB we try to print the
original variable, which _hasn't_ been filled in.

This is all just a long way around of saying that Tom's right.

> If the argument is already in memory, then just casting its address to a
> reference would be a lot better, especially because memcpy()ing the data
> into the inferior is only really correct for trivially-copyable types.

I agree with this.

This function `value_coerce_to_target` does seem like a sensible place
to update to fix this, but given that the forcing seems to be entangled
with the parameter type, I wonder if this should be fixed back in
`value_arg_coerce` (infcall.c)?  Specifically, in this block:

    case TYPE_CODE_REF:
    case TYPE_CODE_RVALUE_REF:
      {
	struct value *new_value;

	if (TYPE_IS_REFERENCE (arg_type))
	  return value_cast_pointers (type, arg, 0);

	/* Cast the value to the reference's target type, and then
	   convert it back to a reference.  This will issue an error
	   if the value was not previously in memory - in some cases
	   we should clearly be allowing this, but how?  */
	new_value = value_cast (type->target_type (), arg);
	new_value = value_ref (new_value, type->code ());
	return new_value;
      }

The comment seems (to me) to hint at the exact problem that you are
trying to solve here.  Maybe the solution is to split
`value_coerce_to_target` and `value_must_coerce_to_target` into some
smaller pieces and then reuse those within `value_arg_coerce`?

> I don't know if that property (trivially-copyable) is all that easy to
> detect in gdb, but if it is, I guess it would make sense to detect it
> and throw an exception.  Since in that case we'd be looking at something
> like a callee that wants memory but where the object has been SROA'd or
> something along those lines.

We have `language_pass_by_reference`, the return value of which will, I
think, tell you if a type is trivially-copyable or not.  I don't think
this is going to be the common path though; don't objects that are not
trivially-copyable have to live in memory?  I'm sure there might be some
edge case here, but for me, I'd be happy with a patch that ignores this
for now, and we can come back to this later if/when we find a
problematic case.

Thanks,
Andrew