[4/8] gdb/s390: Fill gen_return_address hook.

  gdb/ChangeLog:

	* s390-linux-tdep.c (s390_gen_return_address): New function.
	(s390_gdbarch_init): Fill gen_return_address hook.
---
Added missing comment.

 gdb/ChangeLog         |  5 +++++
 gdb/s390-linux-tdep.c | 13 +++++++++++++
 2 files changed, 18 insertions(+)
  

Ping.

On 07/02/16 15:02, Marcin Kościelnicki wrote:
> gdb/ChangeLog:
>
> 	* s390-linux-tdep.c (s390_gen_return_address): New function.
> 	(s390_gdbarch_init): Fill gen_return_address hook.
> ---
> Added missing comment.
>
>   gdb/ChangeLog         |  5 +++++
>   gdb/s390-linux-tdep.c | 13 +++++++++++++
>   2 files changed, 18 insertions(+)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 6260040..0cf8bfc 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,5 +1,10 @@
>   2016-02-07  Marcin Kościelnicki  <koriakin@0x04.net>
>
> +	* s390-linux-tdep.c (s390_gen_return_address): New function.
> +	(s390_gdbarch_init): Fill gen_return_address hook.
> +
> +2016-02-07  Marcin Kościelnicki  <koriakin@0x04.net>
> +
>   	* s390-linux-tdep.c (s390_ax_pseudo_register_collect): New function.
>   	(s390_ax_pseudo_register_push_stack): New function.
>   	(s390_gdbarch_init): Fill ax_pseudo_register_collect and
> diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
> index 97bd564..0b91ed1 100644
> --- a/gdb/s390-linux-tdep.c
> +++ b/gdb/s390-linux-tdep.c
> @@ -627,6 +627,18 @@ s390_ax_pseudo_register_push_stack (struct gdbarch *gdbarch,
>     return 0;
>   }
>
> +/* The "gen_return_address" gdbarch method.  */
> +
> +static void
> +s390_gen_return_address (struct gdbarch *gdbarch,
> +			 struct agent_expr *ax, struct axs_value *value,
> +			 CORE_ADDR scope)
> +{
> +  value->type = register_type (gdbarch, S390_R14_REGNUM);
> +  value->kind = axs_lvalue_register;
> +  value->u.reg = S390_R14_REGNUM;
> +}
> +
>
>   /* A helper for s390_software_single_step, decides if an instruction
>      is a partial-execution instruction that needs to be executed until
> @@ -7970,6 +7982,7 @@ s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>   					  s390_ax_pseudo_register_collect);
>     set_gdbarch_ax_pseudo_register_push_stack
>         (gdbarch, s390_ax_pseudo_register_push_stack);
> +  set_gdbarch_gen_return_address (gdbarch, s390_gen_return_address);
>     tdesc_use_registers (gdbarch, tdesc, tdesc_data);
>     set_gdbarch_register_name (gdbarch, s390_register_name);
>
>
  

Ping.

On 25/02/16 20:23, Marcin Kościelnicki wrote:
> Ping.
>
> On 07/02/16 15:02, Marcin Kościelnicki wrote:
>> gdb/ChangeLog:
>>
>>     * s390-linux-tdep.c (s390_gen_return_address): New function.
>>     (s390_gdbarch_init): Fill gen_return_address hook.
>> ---
>> Added missing comment.
>>
>>   gdb/ChangeLog         |  5 +++++
>>   gdb/s390-linux-tdep.c | 13 +++++++++++++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 6260040..0cf8bfc 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,5 +1,10 @@
>>   2016-02-07  Marcin Kościelnicki  <koriakin@0x04.net>
>>
>> +    * s390-linux-tdep.c (s390_gen_return_address): New function.
>> +    (s390_gdbarch_init): Fill gen_return_address hook.
>> +
>> +2016-02-07  Marcin Kościelnicki  <koriakin@0x04.net>
>> +
>>       * s390-linux-tdep.c (s390_ax_pseudo_register_collect): New
>> function.
>>       (s390_ax_pseudo_register_push_stack): New function.
>>       (s390_gdbarch_init): Fill ax_pseudo_register_collect and
>> diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
>> index 97bd564..0b91ed1 100644
>> --- a/gdb/s390-linux-tdep.c
>> +++ b/gdb/s390-linux-tdep.c
>> @@ -627,6 +627,18 @@ s390_ax_pseudo_register_push_stack (struct
>> gdbarch *gdbarch,
>>     return 0;
>>   }
>>
>> +/* The "gen_return_address" gdbarch method.  */
>> +
>> +static void
>> +s390_gen_return_address (struct gdbarch *gdbarch,
>> +             struct agent_expr *ax, struct axs_value *value,
>> +             CORE_ADDR scope)
>> +{
>> +  value->type = register_type (gdbarch, S390_R14_REGNUM);
>> +  value->kind = axs_lvalue_register;
>> +  value->u.reg = S390_R14_REGNUM;
>> +}
>> +
>>
>>   /* A helper for s390_software_single_step, decides if an instruction
>>      is a partial-execution instruction that needs to be executed until
>> @@ -7970,6 +7982,7 @@ s390_gdbarch_init (struct gdbarch_info info,
>> struct gdbarch_list *arches)
>>                         s390_ax_pseudo_register_collect);
>>     set_gdbarch_ax_pseudo_register_push_stack
>>         (gdbarch, s390_ax_pseudo_register_push_stack);
>> +  set_gdbarch_gen_return_address (gdbarch, s390_gen_return_address);
>>     tdesc_use_registers (gdbarch, tdesc, tdesc_data);
>>     set_gdbarch_register_name (gdbarch, s390_register_name);
>>
>>
>
  

On Sun, Feb 07 2016, Marcin Kościelnicki wrote:

> diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
> index 97bd564..0b91ed1 100644
> --- a/gdb/s390-linux-tdep.c
> +++ b/gdb/s390-linux-tdep.c
> @@ -627,6 +627,18 @@ s390_ax_pseudo_register_push_stack (struct gdbarch *gdbarch,
>    return 0;
>  }
>  
> +/* The "gen_return_address" gdbarch method.  */
> +
> +static void
> +s390_gen_return_address (struct gdbarch *gdbarch,
> +			 struct agent_expr *ax, struct axs_value *value,
> +			 CORE_ADDR scope)
> +{
> +  value->type = register_type (gdbarch, S390_R14_REGNUM);
> +  value->kind = axs_lvalue_register;
> +  value->u.reg = S390_R14_REGNUM;
> +}

Under which circumstances is this supposed to work?  And how reliable
does it need to be?  The ABI only guarantees that r14 holds the return
address at function entry.  Anywhere else it likely doesn't.

--
Andreas
  

On 11/03/16 12:20, Andreas Arnez wrote:
> On Sun, Feb 07 2016, Marcin Kościelnicki wrote:
>
>> diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
>> index 97bd564..0b91ed1 100644
>> --- a/gdb/s390-linux-tdep.c
>> +++ b/gdb/s390-linux-tdep.c
>> @@ -627,6 +627,18 @@ s390_ax_pseudo_register_push_stack (struct gdbarch *gdbarch,
>>     return 0;
>>   }
>>
>> +/* The "gen_return_address" gdbarch method.  */
>> +
>> +static void
>> +s390_gen_return_address (struct gdbarch *gdbarch,
>> +			 struct agent_expr *ax, struct axs_value *value,
>> +			 CORE_ADDR scope)
>> +{
>> +  value->type = register_type (gdbarch, S390_R14_REGNUM);
>> +  value->kind = axs_lvalue_register;
>> +  value->u.reg = S390_R14_REGNUM;
>> +}
>
> Under which circumstances is this supposed to work?  And how reliable
> does it need to be?  The ABI only guarantees that r14 holds the return
> address at function entry.  Anywhere else it likely doesn't.
>
> --
> Andreas
>

Quoting gdbarch.sh:

# Generate bytecodes to collect the return address in a frame.
# Since the bytecodes run on the target, possibly with GDB not even
# connected, the full unwinding machinery is not available, and
# typically this function will issue bytecodes for one or more likely
# places that the return address may be found.
m:void:gen_return_address:struct agent_expr *ax, struct axs_value 
*value, CORE_ADDR scope:ax, value, scope::default_gen_return_address::0

ie. it's supposed to collect some value that will likely help the 
unwinder - if it collects the wrong thing, the unwinder (knowing the 
right place for sure) will simply consider the previous frame PC to be 
unavailable.

We could also try to collect 14*<wordsize>(%r11), hoping that's the save 
slot for %r14, but the interface unfortunately doesn't support 
collecting multiple values (no matter what the comment above says).

Unfortunately, this interface is just not very well-designed - both x86 
and aarch64 just take a shot in the dark like this patch.  A better way 
would be to reuse the existing unwinders and remove this hook 
altogether, or (for while-stepping, where we can't predict the PC) 
actually allow multiple values and aim at a few likely locations.  But 
IMO that's not in scope for this patchset.
  

On Fri, Mar 11 2016, Marcin Kościelnicki wrote:

> We could also try to collect 14*<wordsize>(%r11), hoping that's the
> save slot for %r14, but the interface unfortunately doesn't support
> collecting multiple values (no matter what the comment above says).

Nah, that doesn't help either, since most functions don't use r11 as a
frame pointer.  There is just no way to locate the return address unless
we have call frame information or perform code analysis.

> Unfortunately, this interface is just not very well-designed - both
> x86 and aarch64 just take a shot in the dark like this patch.  A
> better way would be to reuse the existing unwinders and remove this
> hook altogether, or (for while-stepping, where we can't predict the
> PC) actually allow multiple values and aim at a few likely locations.
> But IMO that's not in scope for this patchset.

The point I was trying to make is that r14 is fairly *unlikely* to
contain the return address, unless we're near function entry.  If we
just called a function, then r14 contains an address within our own
function.  Otherwise r14 can also contain something else entirely.

Is there a way to admit that we don't know the return address?  What if
we always return garbage?  E.g., maybe it's better to always return 0?
  

On 11/03/16 13:18, Andreas Arnez wrote:
> On Fri, Mar 11 2016, Marcin Kościelnicki wrote:
>
>> We could also try to collect 14*<wordsize>(%r11), hoping that's the
>> save slot for %r14, but the interface unfortunately doesn't support
>> collecting multiple values (no matter what the comment above says).
>
> Nah, that doesn't help either, since most functions don't use r11 as a
> frame pointer.  There is just no way to locate the return address unless
> we have call frame information or perform code analysis.
>
>> Unfortunately, this interface is just not very well-designed - both
>> x86 and aarch64 just take a shot in the dark like this patch.  A
>> better way would be to reuse the existing unwinders and remove this
>> hook altogether, or (for while-stepping, where we can't predict the
>> PC) actually allow multiple values and aim at a few likely locations.
>> But IMO that's not in scope for this patchset.
>
> The point I was trying to make is that r14 is fairly *unlikely* to
> contain the return address, unless we're near function entry.  If we
> just called a function, then r14 contains an address within our own
> function.  Otherwise r14 can also contain something else entirely.

Well, it works for leaf functions... not much, but not totally useless 
either.
>
> Is there a way to admit that we don't know the return address?  What if
> we always return garbage?  E.g., maybe it's better to always return 0?
>
We can always error() in there (and KFAIL the testcase in gdb.trace that 
exercises it).  However, returning garbage here doesn't result in 
garbage backtrace - this only collects data, if the unwinder actually 
doing the work later determines it should look for the return address on 
the stack, it'll just ignore our collected $r14 and consider the return 
address unavailable (unless another collect rule happened to match it).
  

On Fri, Mar 11 2016, Marcin Kościelnicki wrote:

> We can always error() in there (and KFAIL the testcase in gdb.trace
> that exercises it).  However, returning garbage here doesn't result in
> garbage backtrace - this only collects data, if the unwinder actually
> doing the work later determines it should look for the return address
> on the stack, it'll just ignore our collected $r14 and consider the
> return address unavailable (unless another collect rule happened to
> match it).

Well, from that test case it appears that `$_ret' is generally not
expected to work very reliably.  Since r14 usually does work near
function entry, this may be sufficient for now.

So I'm OK with the patch.  Please add a small comment stating that this
is a best-can-do approach that usually works near function entry and may
yield wrong results otherwise.
  

On 03/11/2016 03:31 PM, Andreas Arnez wrote:
> So I'm OK with the patch.  Please add a small comment stating that this
> is a best-can-do approach that usually works near function entry and may
> yield wrong results otherwise.

I think that should be put in the manual, even.  Users will also trip on
this, not just our tests.

Thanks,
Pedro Alves
  

On 11/03/16 16:31, Andreas Arnez wrote:
> On Fri, Mar 11 2016, Marcin Kościelnicki wrote:
>
>> We can always error() in there (and KFAIL the testcase in gdb.trace
>> that exercises it).  However, returning garbage here doesn't result in
>> garbage backtrace - this only collects data, if the unwinder actually
>> doing the work later determines it should look for the return address
>> on the stack, it'll just ignore our collected $r14 and consider the
>> return address unavailable (unless another collect rule happened to
>> match it).
>
> Well, from that test case it appears that `$_ret' is generally not
> expected to work very reliably.  Since r14 usually does work near
> function entry, this may be sufficient for now.
>
> So I'm OK with the patch.  Please add a small comment stating that this
> is a best-can-do approach that usually works near function entry and may
> yield wrong results otherwise.
>

Thanks, pushed with this comment:

/* The "gen_return_address" gdbarch method.  Since this is supposed to be
    just a best-effort method, and we don't really have the means to run
    the full unwinder here, just collect the link register.  */
  

On Sun, Mar 13 2016, Marcin Kościelnicki wrote:

> On 11/03/16 16:31, Andreas Arnez wrote:
>> On Fri, Mar 11 2016, Marcin Kościelnicki wrote:
>>
>>> We can always error() in there (and KFAIL the testcase in gdb.trace
>>> that exercises it).  However, returning garbage here doesn't result in
>>> garbage backtrace - this only collects data, if the unwinder actually
>>> doing the work later determines it should look for the return address
>>> on the stack, it'll just ignore our collected $r14 and consider the
>>> return address unavailable (unless another collect rule happened to
>>> match it).
>>
>> Well, from that test case it appears that `$_ret' is generally not
>> expected to work very reliably.  Since r14 usually does work near
>> function entry, this may be sufficient for now.
>>
>> So I'm OK with the patch.  Please add a small comment stating that this
>> is a best-can-do approach that usually works near function entry and may
>> yield wrong results otherwise.
>>
>
> Thanks, pushed with this comment:
>
> /* The "gen_return_address" gdbarch method.  Since this is supposed to be
>    just a best-effort method, and we don't really have the means to run
>    the full unwinder here, just collect the link register.  */

Very good, thanks.