Messages in this thread | | | From | Milian Wolff <> | Subject | Re: [PATCH v2] perf report: fix off-by-one for non-activation frames | Date | Tue, 16 May 2017 18:17:26 +0200 |
| |
On Dienstag, 16. Mai 2017 16:38:29 CEST Namhyung Kim wrote: > On Tue, May 16, 2017 at 10:59:51AM +0200, Milian Wolff wrote: > > As the documentation for dwfl_frame_pc says, frames that > > are no activation frames need to have their program counter > > decremented by one to properly find the function of the caller. > > > > This fixes many cases where perf report currently attributes > > the cost to the next line. I.e. I have code like this: > > > > ~~~~~~~~~~~~~~~ > > > > #include <thread> > > #include <chrono> > > > > using namespace std; > > > > int main() > > { > > > > this_thread::sleep_for(chrono::milliseconds(1000)); > > this_thread::sleep_for(chrono::milliseconds(100)); > > this_thread::sleep_for(chrono::milliseconds(10)); > > > > return 0; > > > > } > > > > ~~~~~~~~~~~~~~~ > > It'd be nice if the test program has a signal frame for verification.
I have pretty much zero experience about signals. Would it be enough to add a signal handler for, say, SIGUSR1 to my test application and then trigger a sleep when that signal is delivered? If that should be enough, I'll write and test it out.
<snip>
> > diff --git a/tools/perf/util/unwind-libunwind-local.c > > b/tools/perf/util/unwind-libunwind-local.c index > > f8455bed6e65..30ab26375c80 100644 > > --- a/tools/perf/util/unwind-libunwind-local.c > > +++ b/tools/perf/util/unwind-libunwind-local.c > > @@ -690,8 +690,22 @@ static int get_entries(struct unwind_info *ui, > > unwind_entry_cb_t cb,> > > if (ret) > > > > display_error(ret); > > > > + bool previous_frame_was_signal = false; > > > > while (!ret && (unw_step(&c) > 0) && i < max_stack) { > > > > unw_get_reg(&c, UNW_REG_IP, &ips[i]); > > > > + > > + /* > > + * Decrement the IP for any non-activation frames. > > + * this is required to properly find the srcline > > + * for caller frames. > > + * See also the documentation for dwfl_frame_pc, > > + * which this code tries to replicate. > > + */ > > + bool frame_is_signal = unw_is_signal_frame(&c) > 0; > > + if (!previous_frame_was_signal && !frame_is_signal) > > + --ips[i]; > > + previous_frame_was_signal = frame_is_signal; > > Does it need to check previous frame too?
That's what dwfl_frame_pc does, if I'm not misunderstanding it's source code?
Bye
-- Milian Wolff | milian.wolff@kdab.com | Software Engineer KDAB (Deutschland) GmbH&Co KG, a KDAB Group company Tel: +49-30-521325470 KDAB - The Qt Experts[unhandled content-type:application/pkcs7-signature] | |