diff mbox

[v2,2/3] frame: use get_prev_frame_always in skip_tailcall_frames

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

Commit Message

Metzger, Markus T Feb. 8, 2016, 8:14 a.m. UTC
> -----Original Message-----
> From: Joel Brobecker [mailto:brobecker@adacore.com]
> Sent: Sunday, February 7, 2016 2:01 PM
> To: Metzger, Markus T <markus.t.metzger@intel.com>
> Cc: palves@redhat.com; gdb-patches@sourceware.org
> Subject: Re: [PATCH v2 2/3] frame: use get_prev_frame_always in
> skip_tailcall_frames

Hi Joel,

Thanks for your review.


> > Following the practice in skip_artificial_frames, also use
> > get_prev_frame_always instead of get_prev_frame in
> > skip_tailcall_frames.
> >
> > 2016-02-05  Markus Metzger  <markus.t.metzger@intel.com>
> >
> > gdb/
> > 	* frame.c (skip_tailcall_frames): Call get_prev_frame_always.
> 
> This seems OK; but would you be able to create a testcase for this?

I modified an existing test case to cover the changes.  GDB dies with the modified
test and without the changes to skip_tailcall_frames.  This also showed another place
where we want to use get_prev_frame_always.

Here's the modified version of this patch:


commit d426b734a965ef21d03bf7ed30f20be7d7ed31fa
Author: Markus Metzger <markus.t.metzger@intel.com>
Date:   Fri Feb 5 09:39:05 2016 +0100

    frame: use get_prev_frame_always in skip_tailcall_frames and finish_command
    
    Following the practice in skip_artificial_frames, also use get_prev_frame_always
    instead of get_prev_frame in skip_tailcall_frames and in finish_command.
    
    2016-02-08  Markus Metzger  <markus.t.metzger@intel.com>
    
    gdb/
    	* frame.c (skip_tailcall_frames): Call get_prev_frame_always.
    	* infcmd.c (finish_command): Call get_prev_frame_always.
    
    testsuite/
    	* gdb.arch/amd64-tailcall-ret.exp: Set backtrace limit.

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. 9, 2016, 11:42 a.m. UTC | #1
On 02/08/2016 08:14 AM, Metzger, Markus T wrote:

> I modified an existing test case to cover the changes.  GDB dies with the modified
> test and without the changes to skip_tailcall_frames.  This also showed another place
> where we want to use get_prev_frame_always.
> 
> Here's the modified version of this patch:

Thanks.  Could you do the limiting test in e.g., gdb.base/finish.exp
and gdb.base/return.exp, so that it'd be covered on all archs?

Thanks,
Pedro Alves
Joel Brobecker Feb. 9, 2016, 11:58 a.m. UTC | #2
> > I modified an existing test case to cover the changes.  GDB dies with the modified
> > test and without the changes to skip_tailcall_frames.  This also showed another place
> > where we want to use get_prev_frame_always.
> > 
> > Here's the modified version of this patch:
> 
> Thanks.  Could you do the limiting test in e.g., gdb.base/finish.exp
> and gdb.base/return.exp, so that it'd be covered on all archs?

I was going to ask the very same :-). The fact that adding your test
showed we missed a spot raised the question as to how much of the
initial patch we were testing :).
Metzger, Markus T Feb. 9, 2016, 2:25 p.m. UTC | #3
> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Joel Brobecker
> Sent: Tuesday, February 9, 2016 12:58 PM
> To: Pedro Alves <palves@redhat.com>
> Cc: Metzger, Markus T <markus.t.metzger@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH v2 2/3] frame: use get_prev_frame_always in
> skip_tailcall_frames
> 
> > > I modified an existing test case to cover the changes.  GDB dies
> > > with the modified test and without the changes to
> > > skip_tailcall_frames.  This also showed another place where we want to
> use get_prev_frame_always.
> > >
> > > Here's the modified version of this patch:
> >
> > Thanks.  Could you do the limiting test in e.g., gdb.base/finish.exp
> > and gdb.base/return.exp, so that it'd be covered on all archs?
> 
> I was going to ask the very same :-). The fact that adding your test showed
> we missed a spot raised the question as to how much of the initial patch we
> were testing :).

I don't get your comment.

I'm beginning to wonder if not all-but-the-backtrace-command-related
get_prev_frame calls should really be calling get_prev_frame_always.

The _always extension isn't very intuitive, though, given that this should be
the standard function to use.  Should get_prev_frame maybe be renamed to
something like get_prev_frame_within_limit and get_prev_frame_always
to get_prev_frame?

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
Pedro Alves Feb. 9, 2016, 2:41 p.m. UTC | #4
On 02/09/2016 02:25 PM, Metzger, Markus T wrote:

> I'm beginning to wonder if not all-but-the-backtrace-command-related
> get_prev_frame calls should really be calling get_prev_frame_always.
> 
> The _always extension isn't very intuitive, though, given that this should be
> the standard function to use.  Should get_prev_frame maybe be renamed to
> something like get_prev_frame_within_limit and get_prev_frame_always
> to get_prev_frame?

Maybe a good idea.  See also:

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

I just noticed/remembered something -- the check for backtracing past
main and the entry point is in get_prev_frame, get_prev_frame_always
bypasses it.

This means that with your change, I think gdb now allows "finish" in
"main" or in "_start".

Maybe not a bad change, but I though it'd call it out explicitly.

Thanks,
Pedro Alves
Joel Brobecker Feb. 9, 2016, 2:43 p.m. UTC | #5
> > I was going to ask the very same :-). The fact that adding your test showed
> > we missed a spot raised the question as to how much of the initial patch we
> > were testing :).
> 
> I don't get your comment.

This is the logic behind it: Presumably, your initial patch did
fix something. It would be nice to have that tested, hence the
suggestion to add that. You then added a test, but I think it
only partially overlaps with the situation your initial patch
was trying to cover, because the test you added uncovered a spot
that you didn't need to change before. That's why I think there
is a strong chance that adding one more test would increase coverage
of your patch.

Or said differently, if we undid any hunk in your commit, would
a test immediately regress?

> I'm beginning to wonder if not all-but-the-backtrace-command-related
> get_prev_frame calls should really be calling get_prev_frame_always.
> 
> The _always extension isn't very intuitive, though, given that this should be
> the standard function to use.  Should get_prev_frame maybe be renamed to
> something like get_prev_frame_within_limit and get_prev_frame_always
> to get_prev_frame?

(need more time to answer that question)
Metzger, Markus T Feb. 9, 2016, 3:20 p.m. UTC | #6
> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Pedro Alves
> Sent: Tuesday, February 9, 2016 3:41 PM
> To: Metzger, Markus T <markus.t.metzger@intel.com>; Joel Brobecker
> <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH v2 2/3] frame: use get_prev_frame_always in
> skip_tailcall_frames
> 
> On 02/09/2016 02:25 PM, Metzger, Markus T wrote:
> 
> > I'm beginning to wonder if not all-but-the-backtrace-command-related
> > get_prev_frame calls should really be calling get_prev_frame_always.
> >
> > The _always extension isn't very intuitive, though, given that this
> > should be the standard function to use.  Should get_prev_frame maybe
> > be renamed to something like get_prev_frame_within_limit and
> > get_prev_frame_always to get_prev_frame?
> 
> Maybe a good idea.  See also:
> 
>  https://sourceware.org/bugzilla/show_bug.cgi?id=15558
> 
> I just noticed/remembered something -- the check for backtracing past main
> and the entry point is in get_prev_frame, get_prev_frame_always bypasses
> it.
> 
> This means that with your change, I think gdb now allows "finish" in "main" or
> in "_start".
> 
> Maybe not a bad change, but I though it'd call it out explicitly.

There are 46 calls to get_prev_frame ignoring this patch.  Each of them should
maybe be modified to call get_prev_frame_always, instead.  And none of this
is currently covered by the test suite.

The first step would probably be to look at all those calls and derive test cases
for each of them.  Then fix the failing tests one by one.

This is unrelated to this patch series.  Let me therefore drop this patch.

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
Metzger, Markus T Feb. 15, 2016, 9:50 a.m. UTC | #7
> -----Original Message-----
> From: Metzger, Markus T
> Sent: Tuesday, February 9, 2016 4:20 PM
> To: Pedro Alves <palves@redhat.com>; Joel Brobecker
> <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v2 2/3] frame: use get_prev_frame_always in
> skip_tailcall_frames


> > > I'm beginning to wonder if not all-but-the-backtrace-command-related
> > > get_prev_frame calls should really be calling get_prev_frame_always.
> > >
> > > The _always extension isn't very intuitive, though, given that this
> > > should be the standard function to use.  Should get_prev_frame maybe
> > > be renamed to something like get_prev_frame_within_limit and
> > > get_prev_frame_always to get_prev_frame?
> >
> > Maybe a good idea.  See also:
> >
> >  https://sourceware.org/bugzilla/show_bug.cgi?id=15558
> >
> > I just noticed/remembered something -- the check for backtracing past
> > main and the entry point is in get_prev_frame, get_prev_frame_always
> > bypasses it.
> >
> > This means that with your change, I think gdb now allows "finish" in
> > "main" or in "_start".
> >
> > Maybe not a bad change, but I though it'd call it out explicitly.
> 
> There are 46 calls to get_prev_frame ignoring this patch.  Each of them
> should maybe be modified to call get_prev_frame_always, instead.  And
> none of this is currently covered by the test suite.

I'm wondering in which cases GDB should ignore the user-defined backtrace
limit.  And if GDB should ignore it at all.

If the limit is set, some aspects of GDB may not function any longer.  But that's
to be expected, isn't it?

GDB shouldn't crash, of course.  But I'm not sure if it should ignore user settings
in too many cases.  Maybe we should even switch back to get_prev_frame in
skip_artificial_frames and rely on handling the NULL return if we exceed the
backtrace limit?

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
Pedro Alves Feb. 17, 2016, 3:32 p.m. UTC | #8
On 02/15/2016 09:50 AM, Metzger, Markus T wrote:

> I'm wondering in which cases GDB should ignore the user-defined backtrace
> limit.  And if GDB should ignore it at all.
> 
> If the limit is set, some aspects of GDB may not function any longer.  But that's
> to be expected, isn't it?
> 
> GDB shouldn't crash, of course.  But I'm not sure if it should ignore user settings
> in too many cases.  

I'm starting to think the same way.  Want to give it a try and see what breaks?

> Maybe we should even switch back to get_prev_frame in
> skip_artificial_frames and rely on handling the NULL return if we exceed the
> backtrace limit?

If all places will end up throwing an error, it may be the better to
make skip_artificial_frames itself throw.  We'd have to take a deeper look at each
case though.

We need to also keep in mind that there may be cases where skip_artificial_frames might
be used in internal-facing code, where it might still be necessary get past inline
frames to reach the real stack frame.  I guess sticking a "set backtrace limit 1" in
some of the inline tests would expose this.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/frame.c b/gdb/frame.c
index b7832c7..6ab8834 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -443,8 +443,12 @@  skip_artificial_frames (struct frame_info *frame)
 struct frame_info *
 skip_tailcall_frames (struct frame_info *frame)
 {
+  /* Note we use get_prev_frame_always, and not get_prev_frame.  The
+     latter will truncate the frame chain, leading to this function
+     unintentionally returning a null_frame_id (e.g., when the user
+     sets a backtrace limit).  */
   while (get_frame_type (frame) == TAILCALL_FRAME)
-    frame = get_prev_frame (frame);
+    frame = get_prev_frame_always (frame);
 
   return frame;
 }
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 930dc61..e98f6f5 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1958,7 +1958,10 @@  finish_command (char *arg, int from_tty)
   /* Done with ARGS.  */
   do_cleanups (args_chain);
 
-  frame = get_prev_frame (get_selected_frame (_("No selected frame.")));
+  /* Note we use get_prev_frame_always, and not get_prev_frame.  The latter
+     will truncate the frame chain, leading to this function unintentionally
+     throwing an error (e.g., when the user sets a backtrace limit).  */
+  frame = get_prev_frame_always (get_selected_frame (_("No selected frame.")));
   if (frame == 0)
     error (_("\"finish\" not meaningful in the outermost frame."));
 
diff --git a/gdb/testsuite/gdb.arch/amd64-tailcall-ret.exp b/gdb/testsuite/gdb.arch/amd64-tailcall-ret.exp
index 2642cdd..0334b54 100644
--- a/gdb/testsuite/gdb.arch/amd64-tailcall-ret.exp
+++ b/gdb/testsuite/gdb.arch/amd64-tailcall-ret.exp
@@ -36,6 +36,11 @@  if ![runto_main] {
 gdb_breakpoint "g"
 gdb_continue_to_breakpoint "g" ".* v = 2;"
 
+# Severely limit the back trace.  The limit should be ignored for "return"
+# and "finish" commands.
+#
+gdb_test "set backtrace limit 1"
+
 gdb_test "return" { f \(\); /\* second \*/} "return" \
          {Make g return now\? \(y or n\) } "y"