diff mbox

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

Message ID A78C989F6D9628469189715575E55B2333260743@IRSMSX104.ger.corp.intel.com
State New
Headers show

Commit Message

Metzger, Markus T Feb. 11, 2016, 3:42 p.m. UTC
> -----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.



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

Comments

Pedro Alves Feb. 11, 2016, 3:58 p.m. UTC | #1
On 02/11/2016 03:42 PM, Metzger, Markus T wrote:

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

Let's leave it be.  If may be that the use case that trips on it would
be best fixed differently.

> 
> 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 don't think we should change the others without evidence that they'd need it.
If we do trip on problems on those, I think the fix should be elsewhere.

I'd still early in the release cycle, let's wait and see.

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

OK.

Thanks,
Pedro Alves
Metzger, Markus T Feb. 11, 2016, 4:06 p.m. UTC | #2
> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Thursday, February 11, 2016 4:58 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


> >> 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?
> 
> Let's leave it be.  If may be that the use case that trips on it would be best
> fixed differently.
> 
> >
> > 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 don't think we should change the others without evidence that they'd need
> it.
> If we do trip on problems on those, I think the fix should be elsewhere.
> 
> I'd still early in the release cycle, let's wait and see.

OK.  I'll send the complete series again.

Pathes 1 and 2 are unchanged and you've seen almost everything of patch 3,
already.  Actually, patch 2 has been replaced by a completely unrelated patch,
but you've OK'ed it already in your last reply.

Thanks a lot for your review and for pointing out issues!

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
diff mbox

Patch

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