[review] Don't save the signal state in SIGSETJMP

[review] Don't save the signal state in SIGSETJMP

[Christian Biesinger (Code Review)]

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



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
 

[review] Allow not saving the signal state in SIGSETJMP

[Christian Biesinger (Code Review)]

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
 

[review] Allow not saving the signal state in SIGSETJMP

[Tom Tromey (Code Review)]

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.

[review] Allow not saving the signal state in SIGSETJMP

[Christian Biesinger (Code Review)]

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.

[review] Allow not saving the signal state in SIGSETJMP

[Christian Biesinger (Code Review)]

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!

[review] Allow not saving the signal state in SIGSETJMP

[Tom Tromey (Code Review)]

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.

[review] Allow not saving the signal state in SIGSETJMP

[Sourceware to Gerrit sync (Code Review)]

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
 

[review] Allow not saving the signal state in SIGSETJMP

[Sourceware to Gerrit sync (Code Review)]

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