Re: [patch v4 24/24] record-btrace: skip tail calls in back trace

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Wed, 03 Jul 2013 11:14:34 +0200, Markus Metzger wrote:
> The branch trace represents the caller/callee relationship of tail calls.  The
> caller of a tail call is shown in the back trace and in the function-call
> history.
> 
> This is not consistent with GDB's normal behavior, where the tail caller is not
> shown in the back trace.

This depends on the compiler and its options.  With recent GCCs and -O2 -g
compilation tail calls are shown.  They are even tested for (full) reverse
execution:
Running ./gdb.reverse/amd64-tailcall-reverse.exp ...
Running ./gdb.arch/amd64-tailcall-ret.exp ...
Running ./gdb.arch/amd64-tailcall-cxx.exp ...
Running ./gdb.arch/amd64-tailcall-noret.exp ...

In the -O0 -g mode they are not shown just because of the lack of debug info.
AFAIK it is too expensive for GCC to produce it while -O0 -g compilation
should be fast.

Surprisingly this gives in some cases -O2 -g compilation better debugging
experience than -O0 -g compilation.

Still the missing tailcalls in -O0 -g mode is a defect of the compiler and GDB
should not try to mimick it when it can provide better debugging output.

If you find the reverse execution should be really equal to the forward
execution you could suppress the tail calls only if symtab->call_site_htab is
NULL (and therefore the compiler did not provide tail calls info).


Still when I revert this GDB code patch then gdb.btrace/rn-dl-bind.exp does not
reverse-next properly - what is the reason?

reverse-next^M
__GI_____strtoul_l_internal (nptr=<unavailable>, endptr=<unavailable>, base=<optimized out>, group=<optimized out>, loc=<optimized out>) at ../stdlib/strtol_l.c:531^M
531     }^M
(gdb) FAIL: gdb.btrace/rn-dl-bind.exp: rn-dl-bind, 2.3
bt^M
#0  __GI_____strtoul_l_internal (nptr=<unavailable>, endptr=<unavailable>, base=<optimized out>, group=<optimized out>, loc=<optimized out>) at ../stdlib/strtol_l.c:531^M
#1  0x00007ffff7228f8d in __GI_strtoul (nptr=<error reading variable: Registers are not available in btrace record history>, endptr=<error reading variable: Registers are not available in btrace record history>, base=<error reading variable: Registers are not available in btrace record history>) at ../stdlib/strtol.c:108^M
#2  _dl_runtime_resolve () at ../sysdeps/x86_64/dl-trampoline.S:56^M
#3  0x00000000004004c6 in ?? ()^M
#4  0x00000000004004fb in strtoul@plt ()^M
#5  0x000000000040060c in test () at ./gdb.btrace/rn-dl-bind.c:26^M
#6  0x0000000000400621 in main () at ./gdb.btrace/rn-dl-bind.c:35^M
Backtrace stopped: not enough registers or memory available to unwind further^M



> It further causes the finish command to fail for tail calls.
> 
> This patch skips tail calls when computing the back trace during replay.  The
> finish command now works also for tail calls.
> 
> The tail caller is still shown in the function-call history.
> 
> I'm not sure which is the better behavior.  I liked seeing the tail caller in
> the call stack and I'm not using the finish command very often.  On the other
> hand, reverse/replay should be as close to live debugging as possible.
> 
> 2013-07-03  Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* record-btrace.c (record_btrace_frame_sniffer): Skip tail calls.
> 
> testsuite/
> 	* gdb.btrace/tailcall.exp: Update.  Add stepping tests.
> 	* gdb.btrace/rn-dl-bind.c: New.
> 	* gdb.btrace/rn-dl-bind.exp: New.
> 
> 
> ---
>  gdb/record-btrace.c                     |   15 ++++++----
>  gdb/testsuite/gdb.btrace/rn-dl-bind.c   |   37 +++++++++++++++++++++++
>  gdb/testsuite/gdb.btrace/rn-dl-bind.exp |   48 +++++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.btrace/tailcall.exp   |   25 +++++++++++++--
>  4 files changed, 115 insertions(+), 10 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.btrace/rn-dl-bind.c
>  create mode 100644 gdb/testsuite/gdb.btrace/rn-dl-bind.exp
> 
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index b45a5fb..9feda30 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -1026,7 +1026,7 @@ record_btrace_frame_this_id (struct frame_info *this_frame, void **this_cache,
>    cache = *this_cache;
>  
>    stack = 0;
> -  code = get_frame_func (this_frame);
> +  code = cache->pc;
>    special = (CORE_ADDR) cache->bfun;
>  
>    *this_id = frame_id_build_special (stack, code, special);
> @@ -1120,6 +1120,13 @@ record_btrace_frame_sniffer (const struct frame_unwind *self,
>    caller = bfun->up;
>    pc = 0;
>  
> +  /* Skip tail calls.  */
> +  while (caller != NULL && (bfun->flags & BFUN_UP_LINKS_TO_TAILCALL) != 0)
> +    {
> +      bfun = caller;
> +      caller = bfun->up;
> +    }
> +
>    /* Determine where to find the PC in the upper function segment.  */
>    if (caller != NULL)
>      {
> @@ -1133,11 +1140,7 @@ record_btrace_frame_sniffer (const struct frame_unwind *self,
>  	  insn = VEC_last (btrace_insn_s, caller->insn);
>  	  pc = insn->pc;
>  
> -	  /* We link directly to the jump instruction in the case of a tail
> -	     call, since the next instruction will likely be outside of the
> -	     caller function.  */
> -	  if ((bfun->flags & BFUN_UP_LINKS_TO_TAILCALL) == 0)
> -	    pc += gdb_insn_length (get_frame_arch (this_frame), pc);
> +	  pc += gdb_insn_length (get_frame_arch (this_frame), pc);
>  	}
>  
>        DEBUG ("[frame] sniffed frame for %s on level %d",
> diff --git a/gdb/testsuite/gdb.btrace/rn-dl-bind.c b/gdb/testsuite/gdb.btrace/rn-dl-bind.c
> new file mode 100644
> index 0000000..4930297
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/rn-dl-bind.c
> @@ -0,0 +1,37 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2013 Free Software Foundation, Inc.
> +
> +   Contributed by Intel Corp. <markus.t.metzger@intel.com>
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdlib.h>
> +
> +int test (void)
> +{
> +  int ret;
> +
> +  ret = strtoul ("42", NULL, 10);	/* test.1 */
> +  return ret;				/* test.2 */
> +}					/* test.3 */
> +
> +int
> +main (void)
> +{
> +  int ret;
> +
> +  ret = test ();			/* main.1 */
> +  return ret;				/* main.2 */
> +}					/* main.3 */
> diff --git a/gdb/testsuite/gdb.btrace/rn-dl-bind.exp b/gdb/testsuite/gdb.btrace/rn-dl-bind.exp
> new file mode 100644
> index 0000000..4d803f9
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/rn-dl-bind.exp
> @@ -0,0 +1,48 @@
> +# This testcase is part of GDB, the GNU debugger.
> +#
> +# Copyright 2013 Free Software Foundation, Inc.
> +#
> +# Contributed by Intel Corp. <markus.t.metzger@intel.com>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# check for btrace support
> +if { [skip_btrace_tests] } { return -1 }
> +
> +# start inferior
> +standard_testfile
> +if [prepare_for_testing $testfile.exp $testfile $srcfile {c++ debug}] {
> +    return -1
> +}
> +if ![runto_main] {
> +    return -1
> +}
> +
> +# trace the code for the call to test
> +gdb_test_no_output "record btrace" "rn-dl-bind, 0.1"
> +gdb_test "next" ".*main\.2.*" "rn-dl-bind, 0.2"
> +
> +# just dump the function-call-history to help debugging
> +gdb_test_no_output "set record function-call-history-size 0" "rn-dl-bind, 0.3"
> +gdb_test "record function-call-history /cli 1" ".*" "rn-dl-bind, 0.4"
> +
> +# check that we can reverse-next and next
> +gdb_test "reverse-next" ".*main\.1.*" "rn-dl-bind, 1.1"
> +gdb_test "next" ".*main\.2.*" "rn-dl-bind, 1.2"
> +
> +# now go into test and try to reverse-next and next over the library call
> +gdb_test "reverse-step" ".*test\.3.*" "rn-dl-bind, 2.1"
> +gdb_test "reverse-step" ".*test\.2.*" "rn-dl-bind, 2.2"
> +gdb_test "reverse-next" ".*test\.1.*" "rn-dl-bind, 2.3"
> +gdb_test "next" ".*test\.2.*" "rn-dl-bind, 2.4"
> diff --git a/gdb/testsuite/gdb.btrace/tailcall.exp b/gdb/testsuite/gdb.btrace/tailcall.exp
> index 5cadee0..df8d66a 100644
> --- a/gdb/testsuite/gdb.btrace/tailcall.exp
> +++ b/gdb/testsuite/gdb.btrace/tailcall.exp
> @@ -57,12 +57,29 @@ gdb_test "record goto 4" "
>  # check the backtrace
>  gdb_test "backtrace" "
>  #0.*bar.*at .*x86-tailcall.c:24.*\r
> -#1.*foo.*at .*x86-tailcall.c:29.*\r
> -#2.*main.*at .*x86-tailcall.c:37.*\r
> +#1.*main.*at .*x86-tailcall.c:37.*\r
>  Backtrace stopped: not enough registers or memory available to unwind further" "backtrace in bar"

You should use \[^\r\n\]* instead of .* in all your testcases but it is
everywhere.  Normally it is not so serious so I did not require a change but
for example here it produces false positive as it will still incorrectly match:
	backtrace
	#0  0x00000000004005b5 in bar () at gdb/testsuite/gdb.btrace/x86-tailcall.c:24
	#1  foo () at gdb/testsuite/gdb.btrace/x86-tailcall.c:29
	#2  0x00000000004005d5 in main () at gdb/testsuite/gdb.btrace/x86-tailcall.c:37
	Backtrace stopped: not enough registers or memory available to unwind further
	(gdb) PASS: gdb.btrace/tailcall.exp: backtrace in bar

In fact it would be better to fix it wherever you can.


>  
>  # walk the backtrace
>  gdb_test "up" "
> -.*foo \\(\\) at .*x86-tailcall.c:29.*" "up to foo"
> -gdb_test "up" "
>  .*main \\(\\) at .*x86-tailcall.c:37.*" "up to main"
> +gdb_test "down" "
> +#0.*bar.*at .*x86-tailcall.c:24.*" "down to bar"
> +
> +# test stepping into and out of tailcalls.
> +gdb_test "finish" "
> +.*main.*at .*x86-tailcall.c:37.*" "step, 1.1"
> +gdb_test "reverse-step" "
> +.*bar.*at .*x86-tailcall.c:24.*" "step, 1.2"
> +gdb_test "reverse-finish" "
> +.*foo \\(\\) at .*x86-tailcall.c:29.*" "step, 1.3"
> +gdb_test "reverse-step" "
> +.*main.*at .*x86-tailcall.c:37.*" "step, 1.4"
> +gdb_test "next" "
> +.*main.*at .*x86-tailcall.c:39.*" "step, 1.5"
> +gdb_test "reverse-next" "
> +.*main.*at .*x86-tailcall.c:37.*" "step, 1.6"
> +gdb_test "step" "
> +.*foo \\(\\) at .*x86-tailcall.c:29.*" "step, 1.7"
> +gdb_test "finish" "
> +.*main.*at .*x86-tailcall.c:37.*" "step, 1.8"
> -- 
> 1.7.1