| Message ID | 3defaafeedc9fa6b42e61401d4c8c84e@starynkevitch.net |
|---|---|
| State | New |
| Headers | show |
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
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
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
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.
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,
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?