Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully

[Pedro Alves <palves@redhat.com>]

On 02/12/2016 02:46 PM, Yao Qi wrote:

> we can wrap methods of 'struct frame_unwind' with try/catch, and handle
> NOT_AVAILABLE_ERROR properly.  In this way, each unwinder doesn't have
> to worry about unavailable memory at all.
> 
> Pedro, what do you think?  Did you try this approach in the rest of 9
> different ways :) (you said you "implemented this differently in about
> 10 different ways" in your email) ?

I no longer recall exactly what I tried.  :-)

I think it may be a good idea.

There are a few constraints that we need to keep in mind:

- Frames that only have the PC available should have distinct frame ids,
  and it should be distinct from outer_frame_id.  (See frame_id_build_unavailable_stack calls).

  This makes e.g., the frame_id_eq check in tfind_1 work as intended, see:
   https://sourceware.org/ml/gdb-patches/2013-12/msg00535.html

- When an unwind sniffer throws, it'll destroy its
  struct frame_unwind_cache.  So if we don't catch the error, the
  frame's this_id method can't return something more detailed than
  outer_frame_id.

I don't see this done by wrapping methods of 'struct frame_unwind'.

I think it'd work to have an ultimate-fallback unwinder that
frame_unwind_find_by_frame returns instead of the internal error at
the end.  This would return UNWIND_UNAVAILABLE or UNWIND_MEMORY_ERROR
in the unwinder->stop_reason method, depending on the error the last registered
unwinder thrown.  (That last unwinder will always be the arch's heuristic unwinder.)
And it would return frame_id_build_unavailable_stack(PC) in the unwinder->this_id
method if the last error was UNWIND_UNAVAILABLE, outer_frame_id otherwise
(or we add a new frame_id_build_stackless function, to go along with
frame_id_build_unavailable_stack).

I think that would fix the cases where we end up internal erroring,
like in today's Andreas' patch:

 https://sourceware.org/ml/gdb-patches/2016-02/msg00773.html

And then the heuristic unwinders probably no longer need to care to
use the safe_read_memory_xxx functions.

And it'd fix the bogus cases where the sentinel frame level (-1)
shows through, due to:

 struct frame_info *
 get_current_frame (void)
 {
 ...
  if (current_frame == NULL)
    {
      struct frame_info *sentinel_frame =
	create_sentinel_frame (current_program_space, get_current_regcache ());
      if (catch_exceptions (current_uiout, unwind_to_current_frame,
			    sentinel_frame, RETURN_MASK_ERROR) != 0)
	{
	  /* Oops! Fake a current frame?  Is this useful?  It has a PC
             of zero, for instance.  */
	  current_frame = sentinel_frame;
	}

See recent example here:
 https://sourceware.org/ml/gdb-patches/2016-01/msg00222.html

Thanks,
Pedro Alves