This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2 1/2] Add lang_struct_return to _push_dummy_call
> 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