| Message ID | 20190208094145.GA31267@delia.microfocus.com |
|---|---|
| State | New |
| Headers | show |
| Series | [libbacktrace] Declare external backtrace fns noinline | expand |
On Fri, Feb 8, 2019 at 10:41 AM Tom de Vries <tdevries@suse.de> wrote: > > Hi, > > The backtrace functions backtrace_full, backtrace_print and backtrace_simple > walk the call stack, but make sure to skip the first entry, in order to skip > over the functions themselves, and start the backtrace at the caller of the > functions. > > When compiling with -flto, the functions may be inlined, causing them to skip > over the caller instead. > > Fix this by declaring the functions with __attribute__((noinline)). > > OK for trunk? OK. Richard. > Thanks, > - Tom > > [libbacktrace] Declare external backtrace fns noinline > > 2019-02-08 Tom de Vries <tdevries@suse.de> > > * backtrace.c (backtrace_full): Declare with __attribute__((noinline)). > * print.c (backtrace_print): Same. > * simple.c (backtrace_simple): Same. > > --- > libbacktrace/backtrace.c | 2 +- > libbacktrace/print.c | 2 +- > libbacktrace/simple.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/libbacktrace/backtrace.c b/libbacktrace/backtrace.c > index 29204c63313..c579e803825 100644 > --- a/libbacktrace/backtrace.c > +++ b/libbacktrace/backtrace.c > @@ -98,7 +98,7 @@ unwind (struct _Unwind_Context *context, void *vdata) > > /* Get a stack backtrace. */ > > -int > +int __attribute__((noinline)) > backtrace_full (struct backtrace_state *state, int skip, > backtrace_full_callback callback, > backtrace_error_callback error_callback, void *data) > diff --git a/libbacktrace/print.c b/libbacktrace/print.c > index b2f45446443..0767facecae 100644 > --- a/libbacktrace/print.c > +++ b/libbacktrace/print.c > @@ -80,7 +80,7 @@ error_callback (void *data, const char *msg, int errnum) > > /* Print a backtrace. */ > > -void > +void __attribute__((noinline)) > backtrace_print (struct backtrace_state *state, int skip, FILE *f) > { > struct print_data data; > diff --git a/libbacktrace/simple.c b/libbacktrace/simple.c > index d439fcee8e2..118936397da 100644 > --- a/libbacktrace/simple.c > +++ b/libbacktrace/simple.c > @@ -90,7 +90,7 @@ simple_unwind (struct _Unwind_Context *context, void *vdata) > > /* Get a simple stack backtrace. */ > > -int > +int __attribute__((noinline)) > backtrace_simple (struct backtrace_state *state, int skip, > backtrace_simple_callback callback, > backtrace_error_callback error_callback, void *data)
Hi Tom! On Fri, 8 Feb 2019 10:41:47 +0100, Tom de Vries <tdevries@suse.de> wrote: > The backtrace functions backtrace_full, backtrace_print and backtrace_simple > walk the call stack, but make sure to skip the first entry, in order to skip > over the functions themselves, and start the backtrace at the caller of the > functions. > > When compiling with -flto, the functions may be inlined, causing them to skip > over the caller instead. So, when recently working on the OpenACC Profiling Interface implementation in libgomp, where I'm using libbacktrace to figure out the caller of certain libgomp functions, I recently wondered about the very same issue, that we reliably have to skip a few initial frames. So, "noinline" is how to do that reliably... ;-/ That might be non-obvious for the casual reader, so they might not understand... > Fix this by declaring the functions with __attribute__((noinline)). ... this alone. I'd suggest to have a common "#define LIBBACKTRACE_NOINLINE [...]" (or similar), together with the explanatory comment given above, and use that at the respective definition (or declaration?) sites. Can that go into the public libbacktrace "*.h" file, so that it can also be used elsewhere, as described above? If you agree, want me to prepare a patch? Grüße Thomas > [libbacktrace] Declare external backtrace fns noinline > > 2019-02-08 Tom de Vries <tdevries@suse.de> > > * backtrace.c (backtrace_full): Declare with __attribute__((noinline)). > * print.c (backtrace_print): Same. > * simple.c (backtrace_simple): Same. > > --- > libbacktrace/backtrace.c | 2 +- > libbacktrace/print.c | 2 +- > libbacktrace/simple.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/libbacktrace/backtrace.c b/libbacktrace/backtrace.c > index 29204c63313..c579e803825 100644 > --- a/libbacktrace/backtrace.c > +++ b/libbacktrace/backtrace.c > @@ -98,7 +98,7 @@ unwind (struct _Unwind_Context *context, void *vdata) > > /* Get a stack backtrace. */ > > -int > +int __attribute__((noinline)) > backtrace_full (struct backtrace_state *state, int skip, > backtrace_full_callback callback, > backtrace_error_callback error_callback, void *data) > diff --git a/libbacktrace/print.c b/libbacktrace/print.c > index b2f45446443..0767facecae 100644 > --- a/libbacktrace/print.c > +++ b/libbacktrace/print.c > @@ -80,7 +80,7 @@ error_callback (void *data, const char *msg, int errnum) > > /* Print a backtrace. */ > > -void > +void __attribute__((noinline)) > backtrace_print (struct backtrace_state *state, int skip, FILE *f) > { > struct print_data data; > diff --git a/libbacktrace/simple.c b/libbacktrace/simple.c > index d439fcee8e2..118936397da 100644 > --- a/libbacktrace/simple.c > +++ b/libbacktrace/simple.c > @@ -90,7 +90,7 @@ simple_unwind (struct _Unwind_Context *context, void *vdata) > > /* Get a simple stack backtrace. */ > > -int > +int __attribute__((noinline)) > backtrace_simple (struct backtrace_state *state, int skip, > backtrace_simple_callback callback, > backtrace_error_callback error_callback, void *data)
On 08-02-19 18:25, Thomas Schwinge wrote: > Hi Tom! > > On Fri, 8 Feb 2019 10:41:47 +0100, Tom de Vries <tdevries@suse.de> wrote: >> The backtrace functions backtrace_full, backtrace_print and backtrace_simple >> walk the call stack, but make sure to skip the first entry, in order to skip >> over the functions themselves, and start the backtrace at the caller of the >> functions. >> >> When compiling with -flto, the functions may be inlined, causing them to skip >> over the caller instead. > > So, when recently working on the OpenACC Profiling Interface > implementation in libgomp, where I'm using libbacktrace to figure out the > caller of certain libgomp functions, I recently wondered about the very > same issue, that we reliably have to skip a few initial frames. > > So, "noinline" is how to do that reliably... ;-/ That might be > non-obvious for the casual reader, so they might not understand... > >> Fix this by declaring the functions with __attribute__((noinline)). > > ... this alone. > > I'd suggest to have a common "#define LIBBACKTRACE_NOINLINE [...]" (or > similar), together with the explanatory comment given above, and use that > at the respective definition (or declaration?) sites. Can that go into > the public libbacktrace "*.h" file, so that it can also be used > elsewhere, as described above? > > If you agree, want me to prepare a patch? Hi Thomas, I suppose adding an explanatory comment at the places where I added "__attribute__((noinline))" could be an improvement. But to me, this is just an implementation detail of the library, and I would avoid changing the public header file. Thanks, - Tom
On Fri, Feb 8, 2019 at 9:26 AM Thomas Schwinge <thomas@codesourcery.com> wrote: > > On Fri, 8 Feb 2019 10:41:47 +0100, Tom de Vries <tdevries@suse.de> wrote: > > The backtrace functions backtrace_full, backtrace_print and backtrace_simple > > walk the call stack, but make sure to skip the first entry, in order to skip > > over the functions themselves, and start the backtrace at the caller of the > > functions. > > > > When compiling with -flto, the functions may be inlined, causing them to skip > > over the caller instead. > > So, when recently working on the OpenACC Profiling Interface > implementation in libgomp, where I'm using libbacktrace to figure out the > caller of certain libgomp functions, I recently wondered about the very > same issue, that we reliably have to skip a few initial frames. > > So, "noinline" is how to do that reliably... ;-/ That might be > non-obvious for the casual reader, so they might not understand... > > > Fix this by declaring the functions with __attribute__((noinline)). > > ... this alone. > > I'd suggest to have a common "#define LIBBACKTRACE_NOINLINE [...]" (or > similar), together with the explanatory comment given above, and use that > at the respective definition (or declaration?) sites. Can that go into > the public libbacktrace "*.h" file, so that it can also be used > elsewhere, as described above? > > If you agree, want me to prepare a patch? I think that at least for backtrace_full and backtrace_print we are arguably looking at the SKIP parameter in the wrong place. We shouldn't look at it in unwind before calling backtrace_pcinfo. We should count the inlined functions found by backtrace_pcinfo against the SKIP parameter. Ian
On 09-02-19 22:07, Ian Lance Taylor wrote: > On Fri, Feb 8, 2019 at 9:26 AM Thomas Schwinge <thomas@codesourcery.com> wrote: >> >> On Fri, 8 Feb 2019 10:41:47 +0100, Tom de Vries <tdevries@suse.de> wrote: >>> The backtrace functions backtrace_full, backtrace_print and backtrace_simple >>> walk the call stack, but make sure to skip the first entry, in order to skip >>> over the functions themselves, and start the backtrace at the caller of the >>> functions. >>> >>> When compiling with -flto, the functions may be inlined, causing them to skip >>> over the caller instead. >> >> So, when recently working on the OpenACC Profiling Interface >> implementation in libgomp, where I'm using libbacktrace to figure out the >> caller of certain libgomp functions, I recently wondered about the very >> same issue, that we reliably have to skip a few initial frames. >> >> So, "noinline" is how to do that reliably... ;-/ That might be >> non-obvious for the casual reader, so they might not understand... >> >>> Fix this by declaring the functions with __attribute__((noinline)). >> >> ... this alone. >> >> I'd suggest to have a common "#define LIBBACKTRACE_NOINLINE [...]" (or >> similar), together with the explanatory comment given above, and use that >> at the respective definition (or declaration?) sites. Can that go into >> the public libbacktrace "*.h" file, so that it can also be used >> elsewhere, as described above? >> >> If you agree, want me to prepare a patch? > > I think that at least for backtrace_full and backtrace_print we are > arguably looking at the SKIP parameter in the wrong place. We > shouldn't look at it in unwind before calling backtrace_pcinfo. We > should count the inlined functions found by backtrace_pcinfo against > the SKIP parameter. That change makes sense to me. It would stabilise the cut-off point independent of whether inlining happens or not. Though the documentation of both functions list SKIP as "SKIP is the number of frames to skip", and inlined function do not have their own frame, so AFAIU changing things in the way described above would require changing this documentation as well. Btw, I think we'd still need the inline attributes, in order to make libbacktrace behave the same with and without debug info. Thanks, - Tom
On 09-02-19 22:07, Ian Lance Taylor wrote: > On Fri, Feb 8, 2019 at 9:26 AM Thomas Schwinge <thomas@codesourcery.com> wrote: >> >> On Fri, 8 Feb 2019 10:41:47 +0100, Tom de Vries <tdevries@suse.de> wrote: >>> The backtrace functions backtrace_full, backtrace_print and backtrace_simple >>> walk the call stack, but make sure to skip the first entry, in order to skip >>> over the functions themselves, and start the backtrace at the caller of the >>> functions. >>> >>> When compiling with -flto, the functions may be inlined, causing them to skip >>> over the caller instead. >> >> So, when recently working on the OpenACC Profiling Interface >> implementation in libgomp, where I'm using libbacktrace to figure out the >> caller of certain libgomp functions, I recently wondered about the very >> same issue, that we reliably have to skip a few initial frames. >> >> So, "noinline" is how to do that reliably... ;-/ That might be >> non-obvious for the casual reader, so they might not understand... >> >>> Fix this by declaring the functions with __attribute__((noinline)). >> >> ... this alone. >> >> I'd suggest to have a common "#define LIBBACKTRACE_NOINLINE [...]" (or >> similar), together with the explanatory comment given above, and use that >> at the respective definition (or declaration?) sites. Can that go into >> the public libbacktrace "*.h" file, so that it can also be used >> elsewhere, as described above? >> >> If you agree, want me to prepare a patch? > > I think that at least for backtrace_full and backtrace_print we are > arguably looking at the SKIP parameter in the wrong place. We > shouldn't look at it in unwind before calling backtrace_pcinfo. We > should count the inlined functions found by backtrace_pcinfo against > the SKIP parameter. FTR, filed as PR89273 - "Count inlined functions in skip parm for backtrace_full and backtrace_print" ( https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89273 ). Thanks, - Tom
diff --git a/libbacktrace/backtrace.c b/libbacktrace/backtrace.c index 29204c63313..c579e803825 100644 --- a/libbacktrace/backtrace.c +++ b/libbacktrace/backtrace.c @@ -98,7 +98,7 @@ unwind (struct _Unwind_Context *context, void *vdata) /* Get a stack backtrace. */ -int +int __attribute__((noinline)) backtrace_full (struct backtrace_state *state, int skip, backtrace_full_callback callback, backtrace_error_callback error_callback, void *data) diff --git a/libbacktrace/print.c b/libbacktrace/print.c index b2f45446443..0767facecae 100644 --- a/libbacktrace/print.c +++ b/libbacktrace/print.c @@ -80,7 +80,7 @@ error_callback (void *data, const char *msg, int errnum) /* Print a backtrace. */ -void +void __attribute__((noinline)) backtrace_print (struct backtrace_state *state, int skip, FILE *f) { struct print_data data; diff --git a/libbacktrace/simple.c b/libbacktrace/simple.c index d439fcee8e2..118936397da 100644 --- a/libbacktrace/simple.c +++ b/libbacktrace/simple.c @@ -90,7 +90,7 @@ simple_unwind (struct _Unwind_Context *context, void *vdata) /* Get a simple stack backtrace. */ -int +int __attribute__((noinline)) backtrace_simple (struct backtrace_state *state, int skip, backtrace_simple_callback callback, backtrace_error_callback error_callback, void *data)