RE: [PATCH v2 3/3] btrace, frame: fix crash in get_frame_type

["Metzger, Markus T" <markus dot t dot metzger at intel dot com>]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Pedro Alves
> Sent: Thursday, February 11, 2016 2:39 PM
> To: Metzger, Markus T <markus.t.metzger@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH v2 3/3] btrace, frame: fix crash in get_frame_type


> > There are other cases where frame_unwind_caller_xxx callers don't
> > check
> > frame_unwind_caller_id:
> >
> > 	gdb/mips-linux-tdep.c
> > 	gdb/glibc-tdep.c
> > 	gdb/obsd-tdep.c
> > 	gdb/tic6x-linux-tdep.c
> > 	gdb/sol2-tdep.c
> > 	gdb/nios2-linux-tdep.c
> >
> > They're used for skipping syscalls and ld.so.
> >
> > The latter should be called via gdbarch_skip_solib_resolver (gdbarch,
> > stop_pc) from infrun.c.
> >
> > Who is supposed to do the check in those cases?  Maybe they are already
> OK?
> 
> In the syscall cases, we're trying to determine the next PC where to place a
> breakpoint, in order to do a software single-step.  If we don't know where
> the caller is, we can't single-step, so we should probably error out.  OTOH, if
> the target_ops is record-like and we're single-stepping through the trace log,
> we shouldn't be trying to use software single-step at all.  So I think those are
> probably OK.
> 
> In the glibc_skip_solib_resolver case -- in theory, I guess it would be possible
> to construct a branch trace that records a tailcall to _dl_fixup, and that
> doesn't have any frame above that one?

_dl_runtime_resolve would need to tail-call _dl_fixup.  I don't think it can since
it needs to (tail-)call the resolved function after _dl_fixup returns.  This should
be safe, as well, then.


> If we don't know where the caller is, we can't skip the resolver in one go, so
> best to do is probably to return 0, and let infrun's stepping logic continue
> single-stepping.

You think we should add the check nevertheless in gdb/glibc-tdep.c?

The others I won't be able to test.  I could do the changes and rely on buildbot
to flag issues.  If we really want to change them.


I put the "info frame" changes into a separate patch and a test into the
tailcall-only.exp test.  Here are the stack.c changes:


commit 0dc54d83f375e8901061244807e078f281fca070
Author: Markus Metzger <markus.t.metzger@intel.com>
Date:   Thu Feb 11 11:07:09 2016 +0100

    stack: check frame_unwind_caller_id
    
    Callers of frame_unwind_caller_* functions are supposed to check
    frame_unwind_caller_id.
    
    Add such a check to frame_info and treat an invalid caller ID as if the caller
    PC were not available.
    
    2016-02-11  Markus Metzger  <markus.t.metzger@intel.com>
    
    gdb/
    	* stack.c (frame_info): Check frame_unwind_caller_id.

diff --git a/gdb/stack.c b/gdb/stack.c
index 89879f3..6e3acc7 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1509,27 +1509,32 @@ frame_info (char *addr_exp, int from_tty)
   wrap_here ("    ");
   printf_filtered ("saved %s = ", pc_regname);
 
-  TRY
-    {
-      caller_pc = frame_unwind_caller_pc (fi);
-      caller_pc_p = 1;
-    }
-  CATCH (ex, RETURN_MASK_ERROR)
+  if (!frame_id_p (frame_unwind_caller_id (fi)))
+    val_print_unavailable (gdb_stdout);
+  else
     {
-      switch (ex.error)
+      TRY
 	{
-	case NOT_AVAILABLE_ERROR:
-	  val_print_unavailable (gdb_stdout);
-	  break;
-	case OPTIMIZED_OUT_ERROR:
-	  val_print_not_saved (gdb_stdout);
-	  break;
-	default:
-	  fprintf_filtered (gdb_stdout, _("<error: %s>"), ex.message);
-	  break;
+	  caller_pc = frame_unwind_caller_pc (fi);
+	  caller_pc_p = 1;
 	}
+      CATCH (ex, RETURN_MASK_ERROR)
+	{
+	  switch (ex.error)
+	    {
+	    case NOT_AVAILABLE_ERROR:
+	      val_print_unavailable (gdb_stdout);
+	      break;
+	    case OPTIMIZED_OUT_ERROR:
+	      val_print_not_saved (gdb_stdout);
+	      break;
+	    default:
+	      fprintf_filtered (gdb_stdout, _("<error: %s>"), ex.message);
+	      break;
+	    }
+	}
+      END_CATCH
     }
-  END_CATCH
 
   if (caller_pc_p)
     fputs_filtered (paddress (gdbarch, caller_pc), gdb_stdout);


Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928