[review] Don't save the signal state in SIGSETJMP
Commit Message
Christian Biesinger has uploaded a new change for review.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/124
......................................................................
Don't save the signal state in SIGSETJMP
Saving the signal state is very slow (this patch is a 14% speedup). The
reason we need this code is because signal handler will leave the
signal blocked when we longjmp out of it. But in this case we can
just manually unblock the signal instead of taking the unconditional
perf hit.
gdb/ChangeLog:
2019-10-15 Christian Biesinger <cbiesinger@google.com>
* gdbsupport/gdb_setjmp.h (SIGSETJMP): Allow passing in the value to
pass on to sigsetjmp's second argument.
* cp-support.c (gdb_demangle): Unblock SIGSEGV if we caught a crash.
Change-Id: Ib3010966050c64b4cc8b47d8cb45871652b0b3ea
---
M gdb/cp-support.c
M gdb/gdbsupport/gdb_setjmp.h
2 files changed, 22 insertions(+), 3 deletions(-)
Comments
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/124
......................................................................
Patch Set 2:
Thank you for doing this.
It seems reasonable to me. My only question is: how do we know it works?
I guess one way would be to find a mangled string that causes a crash
(there may be some in bugzilla), then demangle it twice (using "demangle"
should be enough) -- the second time should still report the error.
Christian Biesinger has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/124
......................................................................
Patch Set 2:
> Patch Set 2:
>
> Thank you for doing this.
>
> It seems reasonable to me. My only question is: how do we know it works?
>
> I guess one way would be to find a mangled string that causes a crash
> (there may be some in bugzilla), then demangle it twice (using "demangle"
> should be enough) -- the second time should still report the error.
I went the easy way:
+ *(int*)0=0;
And got this output:
../../gdb/cp-support.c:1589: demangler-warning: unable to demangle '__progname' (demangler failed with signal 11)
Unable to dump core, use `ulimit -c unlimited' before executing GDB next time.
../../gdb/cp-support.c:1603: demangler-warning: unable to demangle '__progname' (demangler failed with signal 11)
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) n
then:
(gdb) demangle __progname
Can't demangle "__progname"
The same as without this patch applied.
Although I am confused because I don't see where error_reported gets reset.
Christian Biesinger has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/124
......................................................................
Patch Set 2:
> Patch Set 2:
>
> > Patch Set 2:
> >
> > Thank you for doing this.
> >
> > It seems reasonable to me. My only question is: how do we know it works?
> >
> > I guess one way would be to find a mangled string that causes a crash
> > (there may be some in bugzilla), then demangle it twice (using "demangle"
> > should be enough) -- the second time should still report the error.
>
> I went the easy way:
> + *(int*)0=0;
>
> And got this output:
> ../../gdb/cp-support.c:1589: demangler-warning: unable to demangle '__progname' (demangler failed with signal 11)
> Unable to dump core, use `ulimit -c unlimited' before executing GDB next time.
> ../../gdb/cp-support.c:1603: demangler-warning: unable to demangle '__progname' (demangler failed with signal 11)
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) n
> then:
> (gdb) demangle __progname
> Can't demangle "__progname"
>
>
> The same as without this patch applied.
>
> Although I am confused because I don't see where error_reported gets reset.
In fact, with a printf in the signal handler, I get lots of output, so this does work.
Conversely, if I remove the sigprocmask call but keep the rest of the patch, I do get a segfault from gdb.
So this should be good!
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/124
......................................................................
Patch Set 2: Code-Review+2
Thank you for taking a look. I also appreciate that you're looking into
performance in this area. This patch is ok.
@@ -1539,7 +1539,16 @@
ofunc = signal (SIGSEGV, gdb_demangle_signal_handler);
#endif
- crash_signal = SIGSETJMP (gdb_demangle_jmp_buf);
+ /* The signal handler may keep the signal blocked when we longjmp out
+ of it. If we have sigprocmask, we can use it to unblock the signal
+ afterwards and we can avoid the performance overhead of saving the
+ signal mask just in case the signal gets triggered. Otherwise, just
+ tell sigsetjmp to save the mask. */
+#ifdef HAVE_SIGPROCMASK
+ crash_signal = SIGSETJMP (gdb_demangle_jmp_buf, 0);
+#else
+ crash_signal = SIGSETJMP (gdb_demangle_jmp_buf, 1);
+#endif
}
#endif
@@ -1559,6 +1568,14 @@
{
static int error_reported = 0;
+#ifdef HAVE_SIGPROCMASK
+ /* If we got the signal, SIGSEGV may still be blocked; restore it. */
+ sigset_t segv_sig_set;
+ sigemptyset (&segv_sig_set);
+ sigaddset (&segv_sig_set, SIGSEGV);
+ sigprocmask (SIG_UNBLOCK, &segv_sig_set, NULL);
+#endif
+
if (!error_reported)
{
std::string short_msg
@@ -23,11 +23,13 @@
#ifdef HAVE_SIGSETJMP
#define SIGJMP_BUF sigjmp_buf
-#define SIGSETJMP(buf) sigsetjmp((buf), 1)
+#define SIGSETJMP(buf,val) sigsetjmp((buf), val)
#define SIGLONGJMP(buf,val) siglongjmp((buf), (val))
#else
#define SIGJMP_BUF jmp_buf
-#define SIGSETJMP(buf) setjmp(buf)
+/* We ignore val here because that's safer and avoids having to check
+ whether _setjmp exists. */
+#define SIGSETJMP(buf,val) setjmp(buf)
#define SIGLONGJMP(buf,val) longjmp((buf), (val))
#endif