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

Alan Hayward
2018-10-10T11:54:24Z

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3345 bytes --]

> 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

\x16º&Öéj×!zÊÞ¶êç×yÛÙb²Ö«r\x18\x1dn–­r\x17¬