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

["Metzger, Markus T" <markus.t.metzger@intel.com>]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Wednesday, February 17, 2016 4:32 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/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?

diff --git a/gdb/frame.c b/gdb/frame.c
index d621dd7..f436010 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -436,7 +436,7 @@ skip_artificial_frames (struct frame_info *frame)
   while (get_frame_type (frame) == INLINE_FRAME
         || get_frame_type (frame) == TAILCALL_FRAME)
     {
-      frame = get_prev_frame_always (frame);
+      frame = get_prev_frame (frame);
       if (frame == NULL)
        break;
     }

With this change, the regression tests for PR backtrace/15558 in
gdb.opt/inline-bt.exp and gdb.python/py-frame-inline.exp still pass.

I have not debugged it but with the recent changes skip_artificial_frames
should return NULL which should be handled by its callers and this seems
to do the right thing for those tests.

Or maybe they just rely on get_prev_frame_always in inline_frame_this_id.
Which makes you wonder why skip_artificial_frames was changed as part
of the bug-fix.

With skip_artificial_frames not unwinding past the backtrace limit, it
doesn't really make sense to call get_prev_frame_always in
frame_unwind_caller_id and frame_pop.

I changed those back to get_prev_frame but I left get_prev_frame_always
in inline_frame_this_id.  Curiously, tailcall_frame_this_id uses get_prev_frame
instead of get_prev_frame_always.  I somehow expected those to be aligned.

When I add a backtrace limit of 1 to the following tests...

#       modified:   gdb/testsuite/gdb.ada/out_of_line_in_inlined.exp
#       modified:   gdb/testsuite/gdb.arch/amd64-tailcall-cxx.exp
#       modified:   gdb/testsuite/gdb.arch/amd64-tailcall-noret.exp
#       modified:   gdb/testsuite/gdb.arch/amd64-tailcall-ret.exp
#       modified:   gdb/testsuite/gdb.arch/amd64-tailcall-self.exp
#       modified:   gdb/testsuite/gdb.base/finish.exp
#       modified:   gdb/testsuite/gdb.base/return.exp
#       modified:   gdb/testsuite/gdb.base/return2.exp
#       modified:   gdb/testsuite/gdb.dwarf2/dw2-inline-param.exp

(gdb.opt/inline-bt.exp already sets a backtrace limit)

...I get a few new fails in

gdb.base/finish.exp
gdb.base/return.exp
gdb.base/return2.exp
gdb.arch/amd64-tailcall-ret.exp

	Just needs handling the error messages we now get when the
	finish and return commands fail when they can no longer skip
	the tailcall frames.

gdb.arch/amd64-tailcall-cxx.exp

	This one is more interesting.  The backtrace output changes from:

	#0  g (x=x@entry=2) at gdb.arch/amd64-tailcall-cxx1.cc:23

	to

	#0  g (x=2) at gdb.arch/amd64-tailcall-cxx1.cc:23

	I have not looked into it.

The rest doesn't give any new fails.

Adding a backtrace limit of 1 to gdb.opt/inline-cmds.exp causes most of the
tests to fail.  It looks like stepping commands are affected by such an aggressive
backtrace limit.

Looking at infrun, GDB uses frame_unwind_caller_id and get_prev_frame (in
stepped_in_from) to detect calls.  Those should depend on the backtrace limit.


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

Stepping should break with such an aggressive backtrace limit.  So I looked into
tail-called and inlined functions.

For tailcalls, I was not able to "next" over a tail-called function.  I always ended
up in the function I called.  Even without a backtrace limit and with
skip_artificial_frames use get_prev_frame_always.  Is this the expected behavior?

For inlined functions, GDB crashes in an inline-frame-skipping loop that uses
get_prev_frame in dwarf2_frame_cfa.

With that fixed I would imagine that for every backtrace limit you could construct
a case where "next" over an inlined function breaks by stacking enough inlined
function calls.  I have not tried it.

For normal calls, a backtrace limit of 2 should suffice.  I have not tried it.

The question remains whether the backtrace limit should just cap the output of
the "backtrace" command or whether it should be a global limit.  If it were just the
former I would have expected the limit check to be in the "backtrace" command
function.

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