Re: [PATCH v2 1/2] Add lang_struct_return to _push_dummy_call

[Alan Hayward <Alan.Hayward@arm.com>]

> On 9 Oct 2018, at 17:14, Pedro Alves <palves@redhat.com> wrote:
> 
> On 10/01/2018 04:52 PM, Alan Hayward wrote:
>> Make call_function_by_hand_dummy pass this down.
>> 
>> 2018-10-01  Alan Hayward  <alan.hayward@arm.com>
>> 
> 
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index 023e8eb453..504b040c2e 100644
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -1512,8 +1512,8 @@ pass_in_v_vfp_candidate (struct gdbarch *gdbarch, struct regcache *regcache,
>> static CORE_ADDR
>> aarch64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>> 			 struct regcache *regcache, CORE_ADDR bp_addr,
>> -			 int nargs,
>> -			 struct value **args, CORE_ADDR sp, int struct_return,
>> +			 int nargs, struct value **args, CORE_ADDR sp,
>> +			 int struct_return, int lang_struct_return_unused,
> 
> Here this is called "lang_struct_return_unused", but all the other
> cases were just "lang_struct_return".
> 
> I suppose it'd be clearer if "struct_return" were renamed to
> "abi_struct_return", but I empathize with not having
> changed it in this patch…
> 

This was because lang_struct_return is already a local parameter within aarch64_push_dummy_call.
The next patch renames the function arg back to lang_struct_return.

(However, if I do the enum below it should remove that issue).

>> 			 CORE_ADDR struct_addr)
>> {
>>   int argnum;
> 
> 
>> --- a/gdb/gdbarch.sh
>> +++ b/gdb/gdbarch.sh
>> @@ -485,7 +485,7 @@ M;struct frame_id;dummy_id;struct frame_info *this_frame;this_frame
>> # deprecated_fp_regnum.
>> v;int;deprecated_fp_regnum;;;-1;-1;;0
>> 
>> -M;CORE_ADDR;push_dummy_call;struct value *function, struct regcache *regcache, CORE_ADDR bp_addr, int nargs, struct value **args, CORE_ADDR sp, int struct_return, CORE_ADDR struct_addr;function, regcache, bp_addr, nargs, args, sp, struct_return, struct_addr
>> +M;CORE_ADDR;push_dummy_call;struct value *function, struct regcache *regcache, CORE_ADDR bp_addr, int nargs, struct value **args, CORE_ADDR sp, int struct_return, int lang_struct_return, CORE_ADDR struct_addr;function, regcache, bp_addr, nargs, args, sp, struct_return, lang_struct_return, struct_addr
>> v;int;call_dummy_location;;;;AT_ENTRY_POINT;;0
>> M;CORE_ADDR;push_dummy_code;CORE_ADDR sp, CORE_ADDR funaddr, struct value **args, int nargs, struct type *value_type, CORE_ADDR *real_pc, CORE_ADDR *bp_addr, struct regcache *regcache;sp, funaddr, args, nargs, value_type, real_pc, bp_addr, regcache
>> 
> 
> No documentation, no goddie.  What does the new parameter
> mean?  
> 
> I'd think that a bool instead of an int would be better.
> Or clearer still, an enum, like:
> 
> enum class return_how { STRUCT, NORMAL };
> 
> If you want to do a preparatory patch adding the enum and
> renaming "struct_return", it'd be awesome, I think.

I considered doing this but didn’t want to add a global enum to gdbarch.h
for just a single use. Agreed it is cleaner. I’ll fix it up for v3.


> 
> But I suppose that given the existence of the "int struct_return"
> parameter, I can't really complain.  We could always add such
> an enum later on and change both parameters at the same time
> throughout.  
> 
> So feel free to leave it be as is.
> 
> Thanks,
> Pedro Alves