libbacktrace PATCH: improve comment for backtrace_create_state

libbacktrace PATCH: improve comment for backtrace_create_state

[basile]

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]

Hello All,

I just discovered that backtrace_create_state should be called once, 
that it is returning some heap-allocated data (which cannot be free-d, 
because there is no
backtrace_destroy_state routine).

I suggest the attached patch (against GCC trunk r246678) which just 
improves the comment describing that function.

libgcc/ChangeLog entry:

2017-04-04  Basile Starynkevitch  <basile@starynkevitch.net>

        * backtrace.h (backtrace_create_state): Improve comment, since 
should be called once.

Comments are welcome. Otherwise, ok for trunk?

-- 
Basile Starynkevitch (France)
http://starynkevitch.net/Basile/

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-r246678-backtrace-comment.diff --]
[-- Type: text/x-diff; name=gcc-r246678-backtrace-comment.diff, Size: 1782 bytes --]

Index: libbacktrace/backtrace.h
===================================================================
--- libbacktrace/backtrace.h	(revision 246678)
+++ libbacktrace/backtrace.h	(working copy)
@@ -83,16 +83,17 @@
 					  int errnum);
 
 /* Create state information for the backtrace routines.  This must be
-   called before any of the other routines, and its return value must
-   be passed to all of the other routines.  FILENAME is the path name
-   of the executable file; if it is NULL the library will try
-   system-specific path names.  If not NULL, FILENAME must point to a
-   permanent buffer.  If THREADED is non-zero the state may be
-   accessed by multiple threads simultaneously, and the library will
-   use appropriate atomic operations.  If THREADED is zero the state
-   may only be accessed by one thread at a time.  This returns a state
-   pointer on success, NULL on error.  If an error occurs, this will
-   call the ERROR_CALLBACK routine.  */
+   called once before any of the other routines (probably at startup,
+   e.g. early in main), and its return value must be passed to all of
+   the other routines.  FILENAME is the path name of the executable
+   file; if it is NULL the library will try system-specific path
+   names.  If not NULL, FILENAME must point to a permanent buffer.  If
+   THREADED is non-zero the state may be accessed by multiple threads
+   simultaneously, and the library will use appropriate atomic
+   operations.  If THREADED is zero the state may only be accessed by
+   one thread at a time.  This returns a state pointer on success,
+   NULL on error.  If an error occurs, this will call the
+   ERROR_CALLBACK routine.  */
 
 extern struct backtrace_state *backtrace_create_state (
     const char *filename, int threaded,

Re: libbacktrace PATCH: improve comment for backtrace_create_state

[Ian Lance Taylor]

On Tue, Apr 4, 2017 at 5:05 AM,  <basile@starynkevitch.net> wrote:
>
> I just discovered that backtrace_create_state should be called once, that it
> is returning some heap-allocated data (which cannot be free-d, because there
> is no
> backtrace_destroy_state routine).
>
> I suggest the attached patch (against GCC trunk r246678) which just improves
> the comment describing that function.

You are adding that backtrace_create_state should be called "(probably
at startup, e.g. early in main)"?  But that is not accurate.  It's
perfectly reasonable to do what GCC itself does, which is call
backtrace_create_state only when it encounters an internal compiler
error (in diagnostic_action_after_output in gcc/diagnostic.c).

How about we just add backtrace_destroy_state?

Ian

Re: libbacktrace PATCH: improve comment for backtrace_create_state

[basile]

On 2017-04-04 15:38, Ian Lance Taylor wrote:
> On Tue, Apr 4, 2017 at 5:05 AM,  <basile@starynkevitch.net> wrote:
>> 
>> I just discovered that backtrace_create_state should be called once, 
>> that it
>> is returning some heap-allocated data (which cannot be free-d, because 
>> there
>> is no
>> backtrace_destroy_state routine).
>> 
>> I suggest the attached patch (against GCC trunk r246678) which just 
>> improves
>> the comment describing that function.
> 
> You are adding that backtrace_create_state should be called "(probably
> at startup, e.g. early in main)"?  But that is not accurate.  It's
> perfectly reasonable to do what GCC itself does, which is call
> backtrace_create_state only when it encounters an internal compiler
> error (in diagnostic_action_after_output in gcc/diagnostic.c).
> 
> How about we just add backtrace_destroy_state?

I don't know how to code that. In my 
https://github.com/bstarynk/melt-monitor I observed that calling free on 
such
a struct backtrace_state pointer is breaking things.

I also find the code of backtrace_create_state a bit complex (maybe for 
historical reasons). Why does it call backtrace_alloc instead of just 
calling malloc?
And why would it call the error_callback on failure? (I would just 
return NULL in that case, leaving the caller of backtrace_create_state 
to handle
that out-of-memory error itself).

Actually, I tend to believe that backtrace_create_state should have its 
signature changed to just:

    struct backtrace_state *backtrace_create_state (const char *filename, 
int threaded);

Or maybe the above should be called backtrace_create_simple_state?

BTW, I guess that changing the API is not possible in current stage 
(that it why I suggested just a comment change).

Cheers

-- 
Basile Starynkevitch (France)    http://starynkevitch.net/Basile/

Re: libbacktrace PATCH: improve comment for backtrace_create_state

[Ian Lance Taylor]

On Tue, Apr 4, 2017 at 6:50 AM,  <basile@starynkevitch.net> wrote:
> On 2017-04-04 15:38, Ian Lance Taylor wrote:
>
>> How about we just add backtrace_destroy_state?
>
> I don't know how to code that. In my
> https://github.com/bstarynk/melt-monitor I observed that calling free on
> such
> a struct backtrace_state pointer is breaking things.

Well, yes, it would have to call backtrace_free.  But more than that
it would have to munmap the free list.  So you're right that it's not
trivial.

> I also find the code of backtrace_create_state a bit complex (maybe for
> historical reasons). Why does it call backtrace_alloc instead of just
> calling malloc?

Because backtrace_create_state, like all the backtrace functions, can
be called from a signal handler.  The backtrace code can never call
malloc.  (It does call malloc on systems that do not support anonymous
mmap, because there is no other choice, which means that the backtrace
library does not really work entirely correctly on such systems.
Fortunately such systems are rare these days.  See
BACKTRACE_USES_MALLOC.)

> And why would it call the error_callback on failure? (I would just return
> NULL in that case, leaving the caller of backtrace_create_state to handle
> that out-of-memory error itself).

It will call the error_callback on mmap failure, or an attempt to pass
threaded as true when it is not supported.

> Actually, I tend to believe that backtrace_create_state should have its
> signature changed to just:
>
>    struct backtrace_state *backtrace_create_state (const char *filename, int
> threaded);
>
> Or maybe the above should be called backtrace_create_simple_state?

We could just document and implement passing error_callback as NULL.

Ian

Re: libbacktrace PATCH: improve comment for backtrace_create_state

[basile]

On 2017-04-04 16:04, Ian Lance Taylor wrote:
> On Tue, Apr 4, 2017 at 6:50 AM,  <basile@starynkevitch.net> wrote:
>> On 2017-04-04 15:38, Ian Lance Taylor wrote:
>> 
>>> How about we just add backtrace_destroy_state?
>> 
>> I don't know how to code that. In my
>> https://github.com/bstarynk/melt-monitor I observed that calling free 
>> on
>> such
>> a struct backtrace_state pointer is breaking things.
> 
> Well, yes, it would have to call backtrace_free.  But more than that
> it would have to munmap the free list.  So you're right that it's not
> trivial.
> 
>> I also find the code of backtrace_create_state a bit complex (maybe 
>> for
>> historical reasons). Why does it call backtrace_alloc instead of just
>> calling malloc?
> 
> Because backtrace_create_state, like all the backtrace functions, can
> be called from a signal handler.  The backtrace code can never call
> malloc.  (It does call malloc on systems that do not support anonymous
> mmap, because there is no other choice, which means that the backtrace
> library does not really work entirely correctly on such systems.
> Fortunately such systems are rare these days.  See
> BACKTRACE_USES_MALLOC.)


This is great news! I would be tempted to suggest another comment change 
in backtrace.h saying that the functions are
(on most recent systems like Linux and those with anonymous mmap) 
async-signal-safe (when the user callbacks are also that).
I was unaware of that delicious property of your libbacktrace. Ian, you 
are impressing me even more than usual!


(I'm not a native English speaker or a POSIX guru, but to me the three 
words async-signal-safe means something important;
I am borrowing them from 
http://man7.org/linux/man-pages/man7/signal.7.html which might be my 
favorite man page, with time(7)).

Cheers.

-- 
Basile Starynkevitch   (France)    http://starynkevitch/net/Basile/