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

[Yao Qi <qiyaoltc at gmail dot com>]

Pedro Alves <palves@redhat.com> writes:

Hi Pedro,

After trying two different approaches of ultimate-fallback unwinder, and
think again about 'wrapping methods of struct frame_unwind', I think
'wrapping struct frame_unwind methods' still works, maybe.

> 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 can be done by wrapping fi->unwind->this_id with TRY/CATCH, call
get_frame_pc_if_available, if PC is available, call
frame_id_build_unavailable_stack otherwise, fall back to
outer_frame_id.  I did that in my patch below,

>   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.

unwinder->sniffer is wrapped by TRY/CATCH nowadays, so we don't have to
change anything.

>
> 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.)

In my patch, this_frame->unwind->stop_reason is wrapped by TRY/CATCH,
and the stop_reason is set to UNWIND_UNAVAILABLE if error is
NOT_AVAILABLE_ERROR.  I don't set stop_reason to UNWIND_MEMORY_ERROR as
you suggested, because I think it can be a follow-up improvement.  Let
us focus on unavailable things first.

> 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 my patch can do this.  The patch below is an RFC.  Run
gdb.trace/*.exp tests on both x86_64-linux and aarch64-linux.  What do
you think?  This patch
https://sourceware.org/ml/gdb-patches/2016-04/msg00429.html is applied
locally to make sure x86 prologue unwinders are selected by gdb.

-- 
Yao (éå)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 6f2e38e..52d89b7f 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -559,8 +559,7 @@ aarch64_make_prologue_cache (struct frame_info *this_frame, void **this_cache)
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
-      if (ex.error != NOT_AVAILABLE_ERROR)
-	throw_exception (ex);
+      throw_exception (ex);
     }
   END_CATCH
 
@@ -687,8 +686,7 @@ aarch64_make_stub_cache (struct frame_info *this_frame, void **this_cache)
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
-      if (ex.error != NOT_AVAILABLE_ERROR)
-	throw_exception (ex);
+      throw_exception (ex);
     }
   END_CATCH
 
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 0065523..192d27b 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2510,8 +2510,7 @@ amd64_frame_cache (struct frame_info *this_frame, void **this_cache)
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
-      if (ex.error != NOT_AVAILABLE_ERROR)
-	throw_exception (ex);
+      throw_exception (ex);
     }
   END_CATCH
 
@@ -2638,8 +2637,7 @@ amd64_sigtramp_frame_cache (struct frame_info *this_frame, void **this_cache)
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
-      if (ex.error != NOT_AVAILABLE_ERROR)
-	throw_exception (ex);
+      throw_exception (ex);
     }
   END_CATCH
 
@@ -2819,8 +2817,7 @@ amd64_epilogue_frame_cache (struct frame_info *this_frame, void **this_cache)
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
-      if (ex.error != NOT_AVAILABLE_ERROR)
-	throw_exception (ex);
+      throw_exception (ex);
     }
   END_CATCH
 
diff --git a/gdb/frame.c b/gdb/frame.c
index d621dd7..60deca3 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -478,7 +478,26 @@ compute_frame_id (struct frame_info *fi)
   /* Find THIS frame's ID.  */
   /* Default to outermost if no ID is found.  */
   fi->this_id.value = outer_frame_id;
-  fi->unwind->this_id (fi, &fi->prologue_cache, &fi->this_id.value);
+
+  TRY
+    {
+      fi->unwind->this_id (fi, &fi->prologue_cache, &fi->this_id.value);
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      if (ex.error == NOT_AVAILABLE_ERROR)
+	{
+	  CORE_ADDR pc;
+
+	  /* Fall back to outer_frame_id if PC isn't available.  */
+	  if (get_frame_pc_if_available (fi, &pc))
+	    fi->this_id.value = frame_id_build_unavailable_stack (pc);
+	}
+      else
+	throw_exception (ex);
+    }
+  END_CATCH
+
   gdb_assert (frame_id_p (fi->this_id.value));
   fi->this_id.p = 1;
   if (frame_debug)
@@ -1882,9 +1901,20 @@ get_prev_frame_always_1 (struct frame_info *this_frame)
 
   /* Check that this frame is unwindable.  If it isn't, don't try to
      unwind to the prev frame.  */
-  this_frame->stop_reason
-    = this_frame->unwind->stop_reason (this_frame,
-				       &this_frame->prologue_cache);
+  TRY
+    {
+      this_frame->stop_reason
+	= this_frame->unwind->stop_reason (this_frame,
+					   &this_frame->prologue_cache);
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      if (ex.error == NOT_AVAILABLE_ERROR)
+	this_frame->stop_reason = UNWIND_UNAVAILABLE;
+      else
+	throw_exception (ex);
+    }
+  END_CATCH
 
   if (this_frame->stop_reason != UNWIND_NO_REASON)
     {
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 83a4881..f52eb0f 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -2074,8 +2074,7 @@ i386_frame_cache (struct frame_info *this_frame, void **this_cache)
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
-      if (ex.error != NOT_AVAILABLE_ERROR)
-	throw_exception (ex);
+      throw_exception (ex);
     }
   END_CATCH
 
@@ -2254,8 +2253,7 @@ i386_epilogue_frame_cache (struct frame_info *this_frame, void **this_cache)
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
-      if (ex.error != NOT_AVAILABLE_ERROR)
-	throw_exception (ex);
+      throw_exception (ex);
     }
   END_CATCH
 
@@ -2450,8 +2448,7 @@ i386_sigtramp_frame_cache (struct frame_info *this_frame, void **this_cache)
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
-      if (ex.error != NOT_AVAILABLE_ERROR)
-	throw_exception (ex);
+      throw_exception (ex);
     }
   END_CATCH