[libbacktrace] Declare external backtrace fns noinline

Message ID 20190208094145.GA31267@delia.microfocus.com
State New
Headers show
Series
  • [libbacktrace] Declare external backtrace fns noinline
Related show

Commit Message

Tom de Vries Feb. 8, 2019, 9:41 a.m.
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?

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(-)

Comments

Richard Biener Feb. 8, 2019, 9:43 a.m. | #1
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)
Thomas Schwinge Feb. 8, 2019, 5:25 p.m. | #2
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)
Tom de Vries Feb. 9, 2019, 3:47 p.m. | #3
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
Ian Lance Taylor Feb. 9, 2019, 9:07 p.m. | #4
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
Tom de Vries Feb. 9, 2019, 10:54 p.m. | #5
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
Tom de Vries Feb. 10, 2019, 4:06 a.m. | #6
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

Patch

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)