[PATCH] btrace: preserve call stack on function switch

[PATCH] btrace: preserve call stack on function switch

[Markus Metzger]

On 64-bit FC25, the _dl_runtime_resolve function uses a conditional branch to
'call' a particular variant optimized for that system:

    (gdb) disas _dl_runtime_resolve_avx_opt
    Dump of assembler code for function _dl_runtime_resolve_avx_opt:
       0x00007ffff7deeb60 <+0>: push   %rax
       0x00007ffff7deeb61 <+1>: push   %rcx
       0x00007ffff7deeb62 <+2>: push   %rdx
       0x00007ffff7deeb63 <+3>: mov    $0x1,%ecx
       0x00007ffff7deeb68 <+8>: xgetbv
       0x00007ffff7deeb6b <+11>: mov    %eax,%r11d
       0x00007ffff7deeb6e <+14>: pop    %rdx
       0x00007ffff7deeb6f <+15>: pop    %rcx
       0x00007ffff7deeb70 <+16>: pop    %rax
       0x00007ffff7deeb71 <+17>: and    $0x4,%r11d
       0x00007ffff7deeb75 <+21>: bnd je 0x7ffff7def4a0 <_dl_runtime_resolve_sse_vex>
    End of assembler dump.

When computing the function-level trace, btrace treats this as a switch from
_dl_runtime_resolve_avx_opt to _dl_runtime_resolve_sse_vex.  We know that we
switched functions but we can't really say in which caller/callee relationship
those two functions are.

In addition to preserving the indentaion level, also preserve the caller
information.  This is a heuristic since we don't really know.  But at least in
this case, this seems to be the right thing to do.

This fixes a fail in gdb.btrace/rn-dl-bind.exp on 64-bit FC25.

2017-02-01  Markus Metzger  <markus.t.metzger@intel.com>

	* btrace.c (ftrace_new_switch): Preserve up link and flags.
---
 gdb/btrace.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 6d621e4..ddf6692 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -448,9 +448,11 @@ ftrace_new_switch (struct btrace_function *prev,
 {
   struct btrace_function *bfun;
 
-  /* This is an unexplained function switch.  The call stack will likely
-     be wrong at this point.  */
+  /* This is an unexplained function switch.  We can't really be sure about the
+     call stack, yet the best I can think of right now is to preserve it.  */
   bfun = ftrace_new_function (prev, mfun, fun);
+  bfun->up = prev->up;
+  bfun->flags = prev->flags;
 
   ftrace_debug (bfun, "new switch");
 
-- 
1.8.3.1

Re: [PATCH] btrace: preserve call stack on function switch

[Luis Machado]

On 02/01/2017 03:12 AM, Markus Metzger wrote:
> On 64-bit FC25, the _dl_runtime_resolve function uses a conditional branch to
> 'call' a particular variant optimized for that system:
>
>     (gdb) disas _dl_runtime_resolve_avx_opt
>     Dump of assembler code for function _dl_runtime_resolve_avx_opt:
>        0x00007ffff7deeb60 <+0>: push   %rax
>        0x00007ffff7deeb61 <+1>: push   %rcx
>        0x00007ffff7deeb62 <+2>: push   %rdx
>        0x00007ffff7deeb63 <+3>: mov    $0x1,%ecx
>        0x00007ffff7deeb68 <+8>: xgetbv
>        0x00007ffff7deeb6b <+11>: mov    %eax,%r11d
>        0x00007ffff7deeb6e <+14>: pop    %rdx
>        0x00007ffff7deeb6f <+15>: pop    %rcx
>        0x00007ffff7deeb70 <+16>: pop    %rax
>        0x00007ffff7deeb71 <+17>: and    $0x4,%r11d
>        0x00007ffff7deeb75 <+21>: bnd je 0x7ffff7def4a0 <_dl_runtime_resolve_sse_vex>
>     End of assembler dump.
>
> When computing the function-level trace, btrace treats this as a switch from
> _dl_runtime_resolve_avx_opt to _dl_runtime_resolve_sse_vex.  We know that we
> switched functions but we can't really say in which caller/callee relationship
> those two functions are.
>
> In addition to preserving the indentaion level, also preserve the caller
> information.  This is a heuristic since we don't really know.  But at least in
> this case, this seems to be the right thing to do.
>
> This fixes a fail in gdb.btrace/rn-dl-bind.exp on 64-bit FC25.
>
> 2017-02-01  Markus Metzger  <markus.t.metzger@intel.com>
>
> 	* btrace.c (ftrace_new_switch): Preserve up link and flags.
> ---
>  gdb/btrace.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/btrace.c b/gdb/btrace.c
> index 6d621e4..ddf6692 100644
> --- a/gdb/btrace.c
> +++ b/gdb/btrace.c
> @@ -448,9 +448,11 @@ ftrace_new_switch (struct btrace_function *prev,
>  {
>    struct btrace_function *bfun;
>
> -  /* This is an unexplained function switch.  The call stack will likely
> -     be wrong at this point.  */
> +  /* This is an unexplained function switch.  We can't really be sure about the
> +     call stack, yet the best I can think of right now is to preserve it.  */
>    bfun = ftrace_new_function (prev, mfun, fun);
> +  bfun->up = prev->up;
> +  bfun->flags = prev->flags;
>
>    ftrace_debug (bfun, "new switch");
>
>

I don't know much about btrace, but the patch looks reasonable given the 
explanation.

 From what i understood, this adds an heuristic where previously there 
was none? We just declared defeat before the patch?

RE: [PATCH] btrace: preserve call stack on function switch

[Metzger, Markus T]

Hello Luis,

Thanks for your review.

>  From what i understood, this adds an heuristic where previously there
> was none? We just declared defeat before the patch?

We previously preserved the stack-level but not the caller information.  This
left the parts of the trace before and after the switch disconnected at higher
levels.

Regards,
Markus.

> -----Original Message-----
> From: Luis Machado [mailto:lgustavo@codesourcery.com]
> Sent: Tuesday, February 7, 2017 12:56 AM
> To: Metzger, Markus T <markus.t.metzger@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH] btrace: preserve call stack on function switch
> 
> On 02/01/2017 03:12 AM, Markus Metzger wrote:
> > On 64-bit FC25, the _dl_runtime_resolve function uses a conditional branch to
> > 'call' a particular variant optimized for that system:
> >
> >     (gdb) disas _dl_runtime_resolve_avx_opt
> >     Dump of assembler code for function _dl_runtime_resolve_avx_opt:
> >        0x00007ffff7deeb60 <+0>: push   %rax
> >        0x00007ffff7deeb61 <+1>: push   %rcx
> >        0x00007ffff7deeb62 <+2>: push   %rdx
> >        0x00007ffff7deeb63 <+3>: mov    $0x1,%ecx
> >        0x00007ffff7deeb68 <+8>: xgetbv
> >        0x00007ffff7deeb6b <+11>: mov    %eax,%r11d
> >        0x00007ffff7deeb6e <+14>: pop    %rdx
> >        0x00007ffff7deeb6f <+15>: pop    %rcx
> >        0x00007ffff7deeb70 <+16>: pop    %rax
> >        0x00007ffff7deeb71 <+17>: and    $0x4,%r11d
> >        0x00007ffff7deeb75 <+21>: bnd je 0x7ffff7def4a0
> <_dl_runtime_resolve_sse_vex>
> >     End of assembler dump.
> >
> > When computing the function-level trace, btrace treats this as a switch from
> > _dl_runtime_resolve_avx_opt to _dl_runtime_resolve_sse_vex.  We know
> that we
> > switched functions but we can't really say in which caller/callee relationship
> > those two functions are.
> >
> > In addition to preserving the indentaion level, also preserve the caller
> > information.  This is a heuristic since we don't really know.  But at least in
> > this case, this seems to be the right thing to do.
> >
> > This fixes a fail in gdb.btrace/rn-dl-bind.exp on 64-bit FC25.
> >
> > 2017-02-01  Markus Metzger  <markus.t.metzger@intel.com>
> >
> > 	* btrace.c (ftrace_new_switch): Preserve up link and flags.
> > ---
> >  gdb/btrace.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/gdb/btrace.c b/gdb/btrace.c
> > index 6d621e4..ddf6692 100644
> > --- a/gdb/btrace.c
> > +++ b/gdb/btrace.c
> > @@ -448,9 +448,11 @@ ftrace_new_switch (struct btrace_function *prev,
> >  {
> >    struct btrace_function *bfun;
> >
> > -  /* This is an unexplained function switch.  The call stack will likely
> > -     be wrong at this point.  */
> > +  /* This is an unexplained function switch.  We can't really be sure about the
> > +     call stack, yet the best I can think of right now is to preserve it.  */
> >    bfun = ftrace_new_function (prev, mfun, fun);
> > +  bfun->up = prev->up;
> > +  bfun->flags = prev->flags;
> >
> >    ftrace_debug (bfun, "new switch");
> >
> >
> 
> I don't know much about btrace, but the patch looks reasonable given the
> explanation.
> 
>  From what i understood, this adds an heuristic where previously there
> was none? We just declared defeat before the patch?
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