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

[Keith Seitz <keiths@redhat.com>]

Hi,

Thank you for looking at my patch.

On 1/28/26 5:54 AM, Andrew Burgess wrote:
> Keith Seitz <keiths@redhat.com> writes:
> 
>> 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?

Hmm... I see where you're headed with this, and I had not thought
of this possibility. This would certainly provide a counter-
argument to a previous review suggesting stack allocation.

However, if we malloc memory to hold this integer during the
function call, is this also not free'd after the call returns?
That pretty much leaves us in the same dilemma.

[Pardon my ignorance; I am just learning this code for the (nearly)
first time.]

>> - 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.

I will make that change.

Thank you again for taking a look at this,
Keith