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

[Andrew Burgess <aburgess@redhat.com>]

Keith Seitz <keiths@redhat.com> writes:

> This patch attempts to mitigate the shortcomings of passing literals
> to inferior function calls requiring references.  The specific use case here
> is std::map's operator[]:
>
> std::map int_map<int, int>;
> int_map[1] = 10;
> (gdb) print int_map[1]
> Attempt to take address of value not located in memory.
>
> This is occurring because while value_coerce_to_target understands
> that some values need to be allocated and copied to the inferior's
> memory, it only considers the actual parsed type of the argument value,
> ignoring the actual type of the function parameter. That is,
> in this specific case, the value's parsed type is TYPE_CODE_INT, but
> the function requires TYPE_CODE_REF. We need to account for the
> reference.
>
> In value_arg_coerce, we have special handling for references, but it
> has not specifically dealt with this case. It now checks if the
> reference is in memory, and if it isn't, it copies it, if the type
> is trivially copyable.
>
> As a result of this patch, the last remaining failure in c++/15372 is now
> fixed, and that bug can be closed.
>
> With this patch, we can now print map entries with integer keys:
>
> (gdb) print int_map[1]
> $1 = (std::map<int, int, std::less<int>, std::allocator<std::pair<int const, int> > >::mapped_type &) @0x41f2d4: 10
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=15372
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=25957
>
> Changes in v4
> - Allocate to stack instead of heap

I'm interested by this change.  Within value_arg_coerce, a few lines
before your change, there's this comment:

  /* Force the value to the target if we will need its address.  At
     this point, we could allocate arguments on the stack instead of
     calling malloc if we knew that their addresses would not be
     saved by the called function.  */
  arg = value_coerce_to_target (arg);

I don't believe there's anything stopping the inferior function taking
the address of the reference argument and storing it.  Could you explain
this change a little more?

> - Use value::force_lval to simply copying to inferior memory
> - Add some addition tests
>
> Changes in v3
> - Move logic to value_arg_coerce
> - Add some attempt to limit copying to trivially copyable types
> ---
>  gdb/infcall.c                                |  26 +++-
>  gdb/testsuite/gdb.cp/ref-params.cc           |  95 +++++++++++-
>  gdb/testsuite/gdb.cp/ref-params.exp          | 146 +++++++++++++++++++
>  gdb/testsuite/gdb.cp/rvalue-ref-overload.exp |   1 -
>  4 files changed, 261 insertions(+), 7 deletions(-)
>
> diff --git a/gdb/infcall.c b/gdb/infcall.c
> index dcbae679d07..c4e9605a665 100644
> --- a/gdb/infcall.c
> +++ b/gdb/infcall.c
> @@ -63,6 +63,8 @@ static bool debug_infcall = false;
>  #define INFCALL_SCOPED_DEBUG_START_END(fmt, ...) \
>    scoped_debug_start_end (debug_infrun, "infcall", fmt, ##__VA_ARGS__)
>  
> +static CORE_ADDR reserve_stack_space (const type *values_type, CORE_ADDR &sp);
> +
>  /* Implement 'show debug infcall'.  */
>  
>  static void
> @@ -246,7 +248,8 @@ show_unwind_on_timeout_p (struct ui_file *file, int from_tty,
>  
>  static struct value *
>  value_arg_coerce (struct gdbarch *gdbarch, struct value *arg,
> -		  struct type *param_type, int is_prototyped)
> +		  struct type *param_type, int is_prototyped,
> +		  CORE_ADDR *sp, struct thread_info *call_thread)
>  {
>    const struct builtin_type *builtin = builtin_type (gdbarch);
>    struct type *arg_type = check_typedef (arg->type ());
> @@ -276,10 +279,23 @@ value_arg_coerce (struct gdbarch *gdbarch, struct value *arg,
>  	  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?  */
> +	   convert it back to a reference.  If the value is not already
> +	   in memory (e.g., a literal), we need to allocate space in the
> +	   inferior and copy it there.  */
>  	new_value = value_cast (type->target_type (), arg);
> +	if (new_value->lval () != lval_memory
> +	    && language_pass_by_reference (new_value->type ())
> +	    .trivially_copyable)
> +	  {
> +	    CORE_ADDR addr;
> +
> +	    gdb_assert (sp != nullptr);

Given reserve_stack_space expects a reference to a CORE_ADDR, I think
I'd just make the argument to this function 'CORE_ADDR &sp', then this
assert can be dropped.

Thanks,
Andrew