* [review] Allow not saving the signal state in SIGSETJMP
2019-10-15 23:20 [review] Don't save the signal state in SIGSETJMP Christian Biesinger (Code Review)
@ 2019-10-15 23:22 ` Christian Biesinger (Code Review)
2019-10-16 15:30 ` Tom Tromey (Code Review)
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Christian Biesinger (Code Review) @ 2019-10-15 23:22 UTC (permalink / raw)
To: Christian Biesinger, gdb-patches
Christian Biesinger has uploaded a new patch set version (#2).
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/124
......................................................................
Allow not saving 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(-)
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index cd732b6..253369b 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -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
diff --git a/gdb/gdbsupport/gdb_setjmp.h b/gdb/gdbsupport/gdb_setjmp.h
index d4ebbfa..4995970 100644
--- a/gdb/gdbsupport/gdb_setjmp.h
+++ b/gdb/gdbsupport/gdb_setjmp.h
@@ -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
^ permalink raw reply [flat|nested] 8+ messages in thread* [review] Allow not saving the signal state in SIGSETJMP
2019-10-15 23:20 [review] Don't save the signal state in SIGSETJMP Christian Biesinger (Code Review)
2019-10-15 23:22 ` [review] Allow not saving " Christian Biesinger (Code Review)
@ 2019-10-16 15:30 ` Tom Tromey (Code Review)
2019-10-16 15:54 ` Christian Biesinger (Code Review)
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Tom Tromey (Code Review) @ 2019-10-16 15:30 UTC (permalink / raw)
To: Christian Biesinger, gdb-patches
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* [review] Allow not saving the signal state in SIGSETJMP
2019-10-15 23:20 [review] Don't save the signal state in SIGSETJMP Christian Biesinger (Code Review)
2019-10-15 23:22 ` [review] Allow not saving " Christian Biesinger (Code Review)
2019-10-16 15:30 ` Tom Tromey (Code Review)
@ 2019-10-16 15:54 ` Christian Biesinger (Code Review)
2019-10-16 16:10 ` Christian Biesinger (Code Review)
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Christian Biesinger (Code Review) @ 2019-10-16 15:54 UTC (permalink / raw)
To: Christian Biesinger, gdb-patches; +Cc: Tom Tromey
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* [review] Allow not saving the signal state in SIGSETJMP
2019-10-15 23:20 [review] Don't save the signal state in SIGSETJMP Christian Biesinger (Code Review)
` (2 preceding siblings ...)
2019-10-16 15:54 ` Christian Biesinger (Code Review)
@ 2019-10-16 16:10 ` Christian Biesinger (Code Review)
2019-10-16 17:59 ` Tom Tromey (Code Review)
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Christian Biesinger (Code Review) @ 2019-10-16 16:10 UTC (permalink / raw)
To: Christian Biesinger, gdb-patches; +Cc: Tom Tromey
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!
^ permalink raw reply [flat|nested] 8+ messages in thread* [review] Allow not saving the signal state in SIGSETJMP
2019-10-15 23:20 [review] Don't save the signal state in SIGSETJMP Christian Biesinger (Code Review)
` (3 preceding siblings ...)
2019-10-16 16:10 ` Christian Biesinger (Code Review)
@ 2019-10-16 17:59 ` Tom Tromey (Code Review)
2019-10-16 21:15 ` Sourceware to Gerrit sync (Code Review)
2019-10-16 21:15 ` Sourceware to Gerrit sync (Code Review)
6 siblings, 0 replies; 8+ messages in thread
From: Tom Tromey (Code Review) @ 2019-10-16 17:59 UTC (permalink / raw)
To: Christian Biesinger, gdb-patches
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* [review] Allow not saving the signal state in SIGSETJMP
2019-10-15 23:20 [review] Don't save the signal state in SIGSETJMP Christian Biesinger (Code Review)
` (4 preceding siblings ...)
2019-10-16 17:59 ` Tom Tromey (Code Review)
@ 2019-10-16 21:15 ` Sourceware to Gerrit sync (Code Review)
2019-10-16 21:15 ` Sourceware to Gerrit sync (Code Review)
6 siblings, 0 replies; 8+ messages in thread
From: Sourceware to Gerrit sync (Code Review) @ 2019-10-16 21:15 UTC (permalink / raw)
To: Christian Biesinger, gdb-patches; +Cc: Tom Tromey
Sourceware to Gerrit sync has submitted this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/124
......................................................................
Allow not saving 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-16 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/ChangeLog
M gdb/cp-support.c
M gdb/gdbsupport/gdb_setjmp.h
3 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d11dbfb..ba028ed 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2019-10-16 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.
+
2019-10-16 Keith Seitz <keiths@redhat.com>
PR gdb/23567
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index cd732b6..253369b 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -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
diff --git a/gdb/gdbsupport/gdb_setjmp.h b/gdb/gdbsupport/gdb_setjmp.h
index d4ebbfa..4995970 100644
--- a/gdb/gdbsupport/gdb_setjmp.h
+++ b/gdb/gdbsupport/gdb_setjmp.h
@@ -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
^ permalink raw reply [flat|nested] 8+ messages in thread* [review] Allow not saving the signal state in SIGSETJMP
2019-10-15 23:20 [review] Don't save the signal state in SIGSETJMP Christian Biesinger (Code Review)
` (5 preceding siblings ...)
2019-10-16 21:15 ` Sourceware to Gerrit sync (Code Review)
@ 2019-10-16 21:15 ` Sourceware to Gerrit sync (Code Review)
6 siblings, 0 replies; 8+ messages in thread
From: Sourceware to Gerrit sync (Code Review) @ 2019-10-16 21:15 UTC (permalink / raw)
To: Christian Biesinger, Tom Tromey, gdb-patches
Sourceware to Gerrit sync has uploaded a new patch set version (#3) to the change originally created by Christian Biesinger.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/124
......................................................................
Allow not saving 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-16 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/ChangeLog
M gdb/cp-support.c
M gdb/gdbsupport/gdb_setjmp.h
3 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d11dbfb..ba028ed 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2019-10-16 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.
+
2019-10-16 Keith Seitz <keiths@redhat.com>
PR gdb/23567
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index cd732b6..253369b 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -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
diff --git a/gdb/gdbsupport/gdb_setjmp.h b/gdb/gdbsupport/gdb_setjmp.h
index d4ebbfa..4995970 100644
--- a/gdb/gdbsupport/gdb_setjmp.h
+++ b/gdb/gdbsupport/gdb_setjmp.h
@@ -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
^ permalink raw reply [flat|nested] 8+ messages in thread