[PR,tdep/17379] Fix internal-error when stack pointer is invalid

  Hi,

this patch intends to fix PR tdep/17379:
  https://sourceware.org/bugzilla/show_bug.cgi?id=17379

The problem is that rs6000_frame_cache attempts to read the stack backchain via
read_memory_unsigned_integer, which throws an exception if the stack pointer is
invalid.  With this path, it calls safe_read_memory_integer instead, which
doesn't throw an exception and allows for safe handling of that situation.
Regression tested on ppc64{,le}.  Ok?

Thanks and regards,
--
Edjunior

gdb/
2014-09-11  Edjunior Barbosa Machado  <emachado@linux.vnet.ibm.com>
	    Ulrich Weigand  <uweigand@de.ibm.com>

	PR tdep/17379
	* rs6000-tdep.c (rs6000_frame_cache): Use safe_read_memory_integer
	instead of read_memory_unsigned_integer.

---
 gdb/rs6000-tdep.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
  

On Thursday, September 11 2014, Edjunior Barbosa Machado wrote:

> The problem is that rs6000_frame_cache attempts to read the stack backchain via
> read_memory_unsigned_integer, which throws an exception if the stack pointer is
> invalid.  With this path, it calls safe_read_memory_integer instead, which
> doesn't throw an exception and allows for safe handling of that situation.
> Regression tested on ppc64{,le}.  Ok?

Heya!

Thanks for the patch.  Not having reviewed the code deeply to understand
if it's the best approach, I would just like to point that a testcase
for this would be awesome.  As it turns out, you actually already have a
testcase almost written in the bug description :-).

Again, thanks for addressing those issues!

Cheers,
  

Hi guys,

See https://sourceware.org/bugzilla/show_bug.cgi?id=17384 .

When safe_read_memory_integer call fails, GDB prints a
surprising/confusing error message, more so in case the unwinder
is triggered for some reason other than the "bt" command, like
with "step"/"next".  I take you're now seeing the same errors
with this patch.

IMO, printing the error is not something a low-level helper function
like  safe_read_memory_integer should be doing, as GDB uses it when
probing with heuristics because it can't sure its guesses make sense
(whether there's a frame at all, etc.)  safe_frame_unwind_memory, which is
used in rs6000_in_function_epilogue_p doesn't print the error either.

Thoughts?

Thanks,
Pedro Alves

On 09/12/2014 12:03 AM, Edjunior Barbosa Machado wrote:
> Hi,
> 
> this patch intends to fix PR tdep/17379:
>   https://sourceware.org/bugzilla/show_bug.cgi?id=17379
> 
> The problem is that rs6000_frame_cache attempts to read the stack backchain via
> read_memory_unsigned_integer, which throws an exception if the stack pointer is
> invalid.  With this path, it calls safe_read_memory_integer instead, which
> doesn't throw an exception and allows for safe handling of that situation.
> Regression tested on ppc64{,le}.  Ok?
> 
> Thanks and regards,
> --
> Edjunior
> 
> gdb/
> 2014-09-11  Edjunior Barbosa Machado  <emachado@linux.vnet.ibm.com>
> 	    Ulrich Weigand  <uweigand@de.ibm.com>
> 
> 	PR tdep/17379
> 	* rs6000-tdep.c (rs6000_frame_cache): Use safe_read_memory_integer
> 	instead of read_memory_unsigned_integer.
> 
> ---
>  gdb/rs6000-tdep.c |   11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 730afe7..dabf448 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -3190,9 +3190,14 @@ rs6000_frame_cache (struct frame_info *this_frame, void **this_cache)
>      }
>  
>    if (!fdata.frameless)
> -    /* Frameless really means stackless.  */
> -    cache->base
> -      = read_memory_unsigned_integer (cache->base, wordsize, byte_order);
> +    {
> +      /* Frameless really means stackless.  */
> +      LONGEST backchain;
> +
> +      if (safe_read_memory_integer (cache->base, wordsize,
> +				    byte_order, &backchain))
> +        cache->base = (CORE_ADDR) backchain;
> +    }
>  
>    trad_frame_set_value (cache->saved_regs,
>  			gdbarch_sp_regnum (gdbarch), cache->base);