[PATCH] Fix PR gdb/18653: gdb disturbs inferior's inherited signal dispositions

[PATCH] Fix PR gdb/18653: gdb disturbs inferior's inherited signal dispositions

[Pedro Alves]

Today I was helping Phil debug something on the new C++ compile
support, and we noticed that when debugging gdb under gdb, the
inferior gdb behaved differently compared to when it was not being
debugged.  Turned out to be a manifestation of PR gdb/18653.

Since the exec family of functions do not reset the signal disposition
of signals that are set to SIG_IGN:

  http://pubs.opengroup.org/onlinepubs/7908799/xsh/execve.html

  Signals set to the default action (SIG_DFL) in the calling process
  image are set to the default action in the new process
  image. Signals set to be ignored (SIG_IGN) by the calling process
  image are set to be ignored by the new process image. Signals set to
  be caught by the calling process image are set to the default action
  in the new process image (see <signal.h>).

gdb's (or gdbserver's) own signal handling should not interfere with
the signal dispositions their spawned children inherit.  However, it
currently does.  For example, some paths in gdb cause SIGPIPE to be
set to SIG_IGN, and as consequence, the child starts with SIGPIPE to
set to SIG_IGN too, even though gdb was started with SIGPIPE set to
SIG_DFL.

In order to be transparent, when spawning new child processes to debug
(with "run", etc.), reset all signal dispositions back to what was
originally inherited from gdb/gdbserver's parent, just before execing
the target program to debug.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	PR gdb/18653
	* Makefile.in (SFILES): Add
	common/signal-dispositions-save-restore.c.
	(HFILES_NO_SRCDIR): Add common/signal-dispositions-save-restore.h.
	(COMMON_OBS): Add signal-dispositions-save-restore.o.
	(signal-dispositions-save-restore.o): New rule.
	* configure: Regenerate.
	* fork-child.c: Include "signal-dispositions-save-restore.h".
	(fork_inferior): Call restore_original_signal_dispositions.
	* main.c: Include "signal-dispositions-save-restore.h".
	(captured_main): Call save_original_signal_dispositions.
	* common/common.m4: Add sigaction to AC_CHECK_FUNCS checks.
	* common/signal-dispositions-save-restore.c: New file.
	* common/signal-dispositions-save-restore.h: New file.

gdb/gdbserver/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	PR gdb/18653
	* Makefile.in (OBS): Add signal-dispositions-save-restore.o.
	(signal-dispositions-save-restore.o): New rule.
	* config.in: Regenerate.
	* configure: Regenerate.
	* linux-low.c: Include "signal-dispositions-save-restore.h".
	(linux_create_inferior): Call
	restore_original_signal_dispositions.
	* server.c: Include "signal-dispositions-save-restore.h".
	(captured_main): Call save_original_signal_dispositions.

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	PR gdb/18653
	* gdb.base/signal-dispositions-child.c: New file.
	* gdb.base/signal-dispositions-child.exp: New file.
	* gdb.gdb/selftest.exp (do_steps_and_nexts): Add new pattern.
---
 gdb/Makefile.in                                    |  9 ++-
 gdb/common/common.m4                               |  2 +-
 gdb/common/signal-dispositions-save-restore.c      | 65 ++++++++++++++++++
 gdb/common/signal-dispositions-save-restore.h      | 40 +++++++++++
 gdb/configure                                      |  2 +-
 gdb/fork-child.c                                   |  4 +-
 gdb/gdbserver/Makefile.in                          |  4 ++
 gdb/gdbserver/config.in                            |  3 +
 gdb/gdbserver/configure                            |  2 +-
 gdb/gdbserver/linux-low.c                          |  4 +-
 gdb/gdbserver/server.c                             |  4 +-
 gdb/main.c                                         |  2 +
 gdb/testsuite/gdb.base/signal-dispositions-child.c | 62 +++++++++++++++++
 .../gdb.base/signal-dispositions-child.exp         | 80 ++++++++++++++++++++++
 gdb/testsuite/gdb.gdb/selftest.exp                 |  4 ++
 15 files changed, 280 insertions(+), 7 deletions(-)
 create mode 100644 gdb/common/signal-dispositions-save-restore.c
 create mode 100644 gdb/common/signal-dispositions-save-restore.h
 create mode 100644 gdb/testsuite/gdb.base/signal-dispositions-child.c
 create mode 100644 gdb/testsuite/gdb.base/signal-dispositions-child.exp

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 5af6103..20549de 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -897,6 +897,7 @@ SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-valprint.c ada-tasks.c \
 	target/waitstatus.c common/print-utils.c common/rsp-low.c \
 	common/errors.c common/common-debug.c common/common-exceptions.c \
 	common/btrace-common.c common/fileio.c common/common-regcache.c \
+	common/signal-dispositions-save-restore.c \
 	$(SUBDIR_GCC_COMPILE_SRCS)
 
 LINTFILES = $(SFILES) $(YYFILES) $(CONFIG_SRCS) init.c
@@ -989,7 +990,8 @@ common/common-exceptions.h target/target.h common/symbol.h \
 common/common-regcache.h fbsd-tdep.h nat/linux-personality.h \
 common/fileio.h nat/x86-linux.h nat/x86-linux-dregs.h nat/amd64-linux-siginfo.h\
 nat/linux-namespaces.h arch/arm.h common/gdb_sys_time.h arch/aarch64-insn.h \
-tid-parse.h ser-event.h
+tid-parse.h ser-event.h \
+common/signal-dispositions-save-restore.h
 
 # Header files that already have srcdir in them, or which are in objdir.
 
@@ -1049,6 +1051,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
 	build-id.o buildsym.o \
 	findcmd.o \
 	std-regs.o \
+	signal-dispositions-save-restore.o \
 	signals.o \
 	exec.o reverse.o \
 	bcache.o objfiles.o observer.o minsyms.o maint.o demangle.o \
@@ -2286,6 +2289,10 @@ common-regcache.o: ${srcdir}/common/common-regcache.c
 	$(COMPILE) $(srcdir)/common/common-regcache.c
 	$(POSTCOMPILE)
 
+signal-dispositions-save-restore.o: $(srcdir)/common/signal-dispositions-save-restore.c
+	$(COMPILE) $(srcdir)/common/signal-dispositions-save-restore.c
+	$(POSTCOMPILE)
+
 #
 # gdb/target/ dependencies
 #
diff --git a/gdb/common/common.m4 b/gdb/common/common.m4
index 1342503..68afc65 100644
--- a/gdb/common/common.m4
+++ b/gdb/common/common.m4
@@ -30,7 +30,7 @@ AC_DEFUN([GDB_AC_COMMON], [
 		   sys/un.h sys/wait.h dnl
 		   thread_db.h wait.h)
 
-  AC_CHECK_FUNCS([fdwalk getrlimit pipe pipe2 socketpair])
+  AC_CHECK_FUNCS([fdwalk getrlimit pipe pipe2 socketpair sigaction])
 
   AC_CHECK_DECLS([strerror, strstr])
 
diff --git a/gdb/common/signal-dispositions-save-restore.c b/gdb/common/signal-dispositions-save-restore.c
new file mode 100644
index 0000000..422ebf2
--- /dev/null
+++ b/gdb/common/signal-dispositions-save-restore.c
@@ -0,0 +1,65 @@
+/* Copyright (C) 2016 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "common-defs.h"
+#include "signal-dispositions-save-restore.h"
+
+#include <signal.h>
+
+/* The original signal handlers.  */
+
+#ifdef HAVE_SIGACTION
+static sighandler_t original_signal_handlers[NSIG];
+#endif
+
+/* See signal-dispositions-save-restore.h.   */
+
+void
+save_original_signal_dispositions (void)
+{
+#ifdef HAVE_SIGACTION
+  int i;
+
+  for (i = 1; i < NSIG; i++)
+    {
+      struct sigaction oldact;
+
+      sigaction (i, NULL, &oldact);
+
+      original_signal_handlers[i] = oldact.sa_handler;
+
+      /* If we find a signal handler already installed, then this
+	 function was called too late.  */
+      if (oldact.sa_handler != SIG_DFL
+	  && oldact.sa_handler != SIG_IGN)
+	internal_error (__FILE__, __LINE__, _("unexpected signal handler"));
+    }
+#endif
+}
+
+/* See signal-dispositions-save-restore.h.   */
+
+void
+restore_original_signal_dispositions (void)
+{
+#ifdef HAVE_SIGACTION
+  int i;
+
+  for (i = 1; i < NSIG; i++)
+    signal (i, original_signal_handlers[i]);
+#endif
+}
diff --git a/gdb/common/signal-dispositions-save-restore.h b/gdb/common/signal-dispositions-save-restore.h
new file mode 100644
index 0000000..71c3365
--- /dev/null
+++ b/gdb/common/signal-dispositions-save-restore.h
@@ -0,0 +1,40 @@
+/* Copyright (C) 2016 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef COMMON_SIGNAL_DISPOSITIONS_SAVE_RESTORE_H
+#define COMMON_SIGNAL_DISPOSITIONS_SAVE_RESTORE_H
+
+/* Save/restore the signal disposition of all signals.
+
+   Since the exec family of functions does not reset the signal
+   disposition of signals set to SIG_IGN, in order to be transparent,
+   when spawning new child processes to debug (with "run", etc.), we
+   must reset all signal dispositions back to what was originally
+   inherited from gdb/gdbserver's parent, just before execing the
+   target program to debug. */
+
+/* Save the signal disposition of all signals, to be restored by
+   restore_original_signal_dispositions.  */
+
+extern void save_original_signal_dispositions (void);
+
+/* Restore the signal disposition of all signals, as saved by a call
+   to save_original_signals_disposition.  */
+
+extern void restore_original_signal_dispositions (void);
+
+#endif /* COMMON_SIGNAL_DISPOSITIONS_SAVE_RESTORE_H */
diff --git a/gdb/configure b/gdb/configure
index 067f86e..fde6503 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -12243,7 +12243,7 @@ fi
 done
 
 
-  for ac_func in fdwalk getrlimit pipe pipe2 socketpair
+  for ac_func in fdwalk getrlimit pipe pipe2 socketpair sigaction
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/gdb/fork-child.c b/gdb/fork-child.c
index 8ac3bef..1c1a4bf 100644
--- a/gdb/fork-child.c
+++ b/gdb/fork-child.c
@@ -32,7 +32,7 @@
 #include "solib.h"
 #include "filestuff.h"
 #include "top.h"
-
+#include "signal-dispositions-save-restore.h"
 #include <signal.h>
 
 /* This just gets used as a default if we can't find SHELL.  */
@@ -366,6 +366,8 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env,
         saying "not parent".  Sorry; you'll have to use print
         statements!  */
 
+      restore_original_signal_dispositions ();
+
       /* There is no execlpe call, so we have to set the environment
          for our child in the global variable.  If we've vforked, this
          clobbers the parent, but environ is restored a few lines down
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index 2be61ef..4e4279a 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -200,6 +200,7 @@ OBS = agent.o ax.o inferiors.o regcache.o remote-utils.o server.o signals.o \
       common-utils.o ptid.o buffer.o format.o filestuff.o dll.o notif.o \
       tdesc.o print-utils.o rsp-low.o errors.o common-debug.o cleanups.o \
       common-exceptions.o symbol.o btrace-common.o fileio.o common-regcache.o \
+      signal-dispositions-save-restore.o \
       $(XML_BUILTIN) $(DEPFILES) $(LIBOBJS)
 GDBREPLAY_OBS = gdbreplay.o version.o
 GDBSERVER_LIBS = @GDBSERVER_LIBS@
@@ -715,6 +716,9 @@ fileio.o: ../common/fileio.c
 common-regcache.o: ../common/common-regcache.c
 	$(COMPILE) $<
 	$(POSTCOMPILE)
+signal-dispositions-save-restore.o: ../common/signal-dispositions-save-restore.c
+	$(COMPILE) $<
+	$(POSTCOMPILE)
 
 # Arch object files rules form ../arch
 
diff --git a/gdb/gdbserver/config.in b/gdb/gdbserver/config.in
index 2c3a69a..04072cf 100644
--- a/gdb/gdbserver/config.in
+++ b/gdb/gdbserver/config.in
@@ -202,6 +202,9 @@
 /* Define to 1 if you have the <sgtty.h> header file. */
 #undef HAVE_SGTTY_H
 
+/* Define to 1 if you have the `sigaction' function. */
+#undef HAVE_SIGACTION
+
 /* Define to 1 if you have the <signal.h> header file. */
 #undef HAVE_SIGNAL_H
 
diff --git a/gdb/gdbserver/configure b/gdb/gdbserver/configure
index 2926deb..0a3436e 100755
--- a/gdb/gdbserver/configure
+++ b/gdb/gdbserver/configure
@@ -5912,7 +5912,7 @@ fi
 done
 
 
-  for ac_func in fdwalk getrlimit pipe pipe2 socketpair
+  for ac_func in fdwalk getrlimit pipe pipe2 socketpair sigaction
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 1839f99..6bc6189 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -22,7 +22,7 @@
 #include "agent.h"
 #include "tdesc.h"
 #include "rsp-low.h"
-
+#include "signal-dispositions-save-restore.h"
 #include "nat/linux-nat.h"
 #include "nat/linux-waitpid.h"
 #include "gdb_wait.h"
@@ -975,6 +975,8 @@ linux_create_inferior (char *program, char **allargs)
 	    }
 	}
 
+      restore_original_signal_dispositions ();
+
       execv (program, allargs);
       if (errno == ENOENT)
 	execvp (program, allargs);
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 6d6cb09..d520947 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -22,7 +22,7 @@
 #include "notif.h"
 #include "tdesc.h"
 #include "rsp-low.h"
-
+#include "signal-dispositions-save-restore.h"
 #include <ctype.h>
 #include <unistd.h>
 #if HAVE_SIGNAL_H
@@ -3611,6 +3611,8 @@ captured_main (int argc, char *argv[])
      opened by remote_prepare.  */
   notice_open_fds ();
 
+  save_original_signal_dispositions ();
+
   /* We need to know whether the remote connection is stdio before
      starting the inferior.  Inferiors created in this scenario have
      stdin,stdout redirected.  So do this here before we call
diff --git a/gdb/main.c b/gdb/main.c
index 5477379..a018d84 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -44,6 +44,7 @@
 #include <signal.h>
 #include "event-top.h"
 #include "infrun.h"
+#include "signal-dispositions-save-restore.h"
 
 /* The selected interpreter.  This will be used as a set command
    variable, so it should always be malloc'ed - since
@@ -505,6 +506,7 @@ captured_main (void *data)
 
   bfd_init ();
   notice_open_fds ();
+  save_original_signal_dispositions ();
 
   make_cleanup (VEC_cleanup (cmdarg_s), &cmdarg_vec);
   dirsize = 1;
diff --git a/gdb/testsuite/gdb.base/signal-dispositions-child.c b/gdb/testsuite/gdb.base/signal-dispositions-child.c
new file mode 100644
index 0000000..2e93d62
--- /dev/null
+++ b/gdb/testsuite/gdb.base/signal-dispositions-child.c
@@ -0,0 +1,62 @@
+/* Copyright 2016 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <signal.h>
+
+#ifndef OUTPUT_TXT
+# define OUTPUT_TXT "output.txt"
+#endif
+
+int
+main (int argc, char **argv)
+{
+  int i;
+  FILE *out;
+
+  if (argc > 1)
+    out = stdout;
+  else
+    {
+      out = fopen (OUTPUT_TXT, "w");
+      if (out == NULL)
+	{
+	  fprintf (stderr, "File open failed\n");
+	  exit (1);
+	}
+    }
+
+  for (i = 1; i < NSIG; i++)
+    {
+      struct sigaction oldact;
+
+      sigaction (i, NULL, &oldact);
+
+      if (oldact.sa_handler == SIG_DFL)
+	fprintf (out, "%d: SIG_DFL\n", i);
+      else if (oldact.sa_handler == SIG_IGN)
+	fprintf (out, "%d: SIG_IGN\n", i);
+      else
+	abort ();
+    }
+
+  if (out != stdout)
+    fclose (out);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/signal-dispositions-child.exp b/gdb/testsuite/gdb.base/signal-dispositions-child.exp
new file mode 100644
index 0000000..4a926b8
--- /dev/null
+++ b/gdb/testsuite/gdb.base/signal-dispositions-child.exp
@@ -0,0 +1,80 @@
+# Copyright 2016 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test that gdb's (or gdbserver's) own signal handling does not
+# interfere with the signal dispositions their spawned children
+# inherit.
+#
+# - If gdb inherits some signal set to SIG_IGN, so should the
+#   inferior, even if gdb itself chooses not to ignore the signal.
+#
+# - If gdb inherits some signal set to SIG_DFL, so should the inferior
+#   even if gdb itself ignores that signal.
+#
+# This requires special support in gdb/gdbserver because the exec
+# family of functions do not reset the signal disposition of signals
+# that are set to SIG_IGN.
+
+standard_testfile
+
+set gdb_txt [standard_output_file gdb.txt]
+set standalone_txt [standard_output_file standalone.txt]
+remote_exec host "rm -f $gdb_txt"
+remote_exec host "rm -f $standalone_txt"
+
+set options [list debug "additional_flags=-DOUTPUT_TXT=\"$gdb_txt\""]
+if {[build_executable $testfile.exp $testfile $srcfile $options]} {
+    untested $testfile.exp
+    return -1
+}
+
+set options [list debug "additional_flags=-DOUTPUT_TXT=\"$standalone_txt\""]
+if {[build_executable $testfile.exp $testfile-standalone $srcfile $options]} {
+    untested $testfile.exp
+    return -1
+}
+
+# Run the program directly, and dump its initial signal dispositions
+# in "standalone.txt".
+
+# Use remote_spawn instead of remote_exec, like how we spawn gdb.
+# This is in order to take the same code code paths in dejagnu
+# compared to when running the program through gdb.  E.g., because
+# local_exec uses -ignore SIGHUP, while remote_spawn does not, if we
+# used remote_exec, the test program would start with SIGHUP ignored
+# when run standalone, but not when run through gdb.
+set res [remote_spawn host "$binfile-standalone"]
+if { $res < 0 || $res == "" } {
+    perror "Spawning $binfile-standalone failed."
+    return 1
+}
+remote_close host
+
+# Now run the program through gdb, and dump its initial signal
+# dispositions in "gdb.txt".
+
+clean_restart $binfile
+
+if { ! [ runto_main ] } then {
+    untested $testfile.exp
+    return -1
+}
+
+gdb_continue_to_end
+
+# Diff the .txt files.  They should be identical.
+gdb_test "shell diff -s $standalone_txt $gdb_txt" \
+    "Files .* are identical.*" \
+    "diff signal dispositions"
diff --git a/gdb/testsuite/gdb.gdb/selftest.exp b/gdb/testsuite/gdb.gdb/selftest.exp
index 2fdd9e3..236f4b7 100644
--- a/gdb/testsuite/gdb.gdb/selftest.exp
+++ b/gdb/testsuite/gdb.gdb/selftest.exp
@@ -162,6 +162,10 @@ proc do_steps_and_nexts {} {
 		set description "next over notice_open_fds"
 		set command "next"
 	    }
+	    -re ".*save_original_signal_dispositions ..;.*$gdb_prompt $" {
+		set description "next over save_original_signal_dispositions"
+		set command "next"
+	    }
 	    -re ".*VEC_cleanup .cmdarg_s.*$gdb_prompt $" {
 		set description "next over cmdarg_s VEC_cleanup"
 		set command "next"
-- 
2.5.5

Re: [PATCH] Fix PR gdb/18653: gdb disturbs inferior's inherited signal dispositions

[John Baldwin]

On Thursday, August 04, 2016 06:37:31 PM Pedro Alves wrote:
> Today I was helping Phil debug something on the new C++ compile
> support, and we noticed that when debugging gdb under gdb, the
> inferior gdb behaved differently compared to when it was not being
> debugged.  Turned out to be a manifestation of PR gdb/18653.
> 
> Since the exec family of functions do not reset the signal disposition
> of signals that are set to SIG_IGN:
> 
>   http://pubs.opengroup.org/onlinepubs/7908799/xsh/execve.html
> 
>   Signals set to the default action (SIG_DFL) in the calling process
>   image are set to the default action in the new process
>   image. Signals set to be ignored (SIG_IGN) by the calling process
>   image are set to be ignored by the new process image. Signals set to
>   be caught by the calling process image are set to the default action
>   in the new process image (see <signal.h>).
> 
> gdb's (or gdbserver's) own signal handling should not interfere with
> the signal dispositions their spawned children inherit.  However, it
> currently does.  For example, some paths in gdb cause SIGPIPE to be
> set to SIG_IGN, and as consequence, the child starts with SIGPIPE to
> set to SIG_IGN too, even though gdb was started with SIGPIPE set to
> SIG_DFL.
> 
> In order to be transparent, when spawning new child processes to debug
> (with "run", etc.), reset all signal dispositions back to what was
> originally inherited from gdb/gdbserver's parent, just before execing
> the target program to debug.

Perhaps consider saving/restore the entire 'struct sigaction' instead
of just the sa_handler field given you are already requiring
sigaction?

-- 
John Baldwin

Re: [PATCH] Fix PR gdb/18653: gdb disturbs inferior's inherited signal dispositions

[Pedro Alves]

On 08/04/2016 07:37 PM, John Baldwin wrote:

> Perhaps consider saving/restore the entire 'struct sigaction' instead
> of just the sa_handler field given you are already requiring sigaction?

Indeed, I think I should, because signal masks are not reset
at exec time either.  Forgot about that.  Thanks for suggesting this.

I'll give it a try soon.

Thanks,
Pedro Alves

[PATCH v2] Fix PR gdb/18653: gdb disturbs inferior's inherited signal dispositions

[Pedro Alves]

On 08/05/2016 02:04 AM, Pedro Alves wrote:
> On 08/04/2016 07:37 PM, John Baldwin wrote:
> 
>> Perhaps consider saving/restore the entire 'struct sigaction' instead
>> of just the sa_handler field given you are already requiring sigaction?
> 
> Indeed, I think I should, because signal masks are not reset
> at exec time either.  Forgot about that.  Thanks for suggesting this.
> 
> I'll give it a try soon.
> 

Like this?  

I renamed the new files and functions to avoid "disposition" since this
saves/restores more than than now. I also applied a bit more polish to
comments and error handling.

----------

[PATCH v2] Fix PR gdb/18653: gdb disturbs inferior's inherited signal
 dispositions

gdb's (or gdbserver's) own signal handling should not interfere with
the signal dispositions their spawned children inherit.  However, it
currently does.  For example, some paths in gdb cause SIGPIPE to be
set to SIG_IGN, and as consequence, the child starts with SIGPIPE to
set to SIG_IGN too, even though gdb was started with SIGPIPE set to
SIG_DFL.

This is because the exec family of functions does not reset the signal
disposition of signals that are set to SIG_IGN:

  http://pubs.opengroup.org/onlinepubs/7908799/xsh/execve.html

  Signals set to the default action (SIG_DFL) in the calling process
  image are set to the default action in the new process
  image. Signals set to be ignored (SIG_IGN) by the calling process
  image are set to be ignored by the new process image. Signals set to
  be caught by the calling process image are set to the default action
  in the new process image (see <signal.h>).

And neither does it reset signal masks or flags.

In order to be transparent, when spawning new child processes to debug
(with "run", etc.), reset signal actions and mask back to what was
originally inherited from gdb/gdbserver's parent, just before execing
the target program to debug.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	PR gdb/18653
	* Makefile.in (SFILES): Add
	common/signals-state-save-restore.c.
	(HFILES_NO_SRCDIR): Add common/signals-state-save-restore.h.
	(COMMON_OBS): Add signals-state-save-restore.o.
	(signals-state-save-restore.o): New rule.
	* configure: Regenerate.
	* fork-child.c: Include "signals-state-save-restore.h".
	(fork_inferior): Call restore_original_signals_state.
	* main.c: Include "signals-state-save-restore.h".
	(captured_main): Call save_original_signals_state.
	* common/common.m4: Add sigaction to AC_CHECK_FUNCS checks.
	* common/signals-state-save-restore.c: New file.
	* common/signals-state-save-restore.h: New file.

gdb/gdbserver/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	PR gdb/18653
	* Makefile.in (OBS): Add signals-state-save-restore.o.
	(signals-state-save-restore.o): New rule.
	* config.in: Regenerate.
	* configure: Regenerate.
	* linux-low.c: Include "signals-state-save-restore.h".
	(linux_create_inferior): Call
	restore_original_signals_state.
	* server.c: Include "dispositions-save-restore.h".
	(captured_main): Call save_original_signals_state.

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	PR gdb/18653
	* gdb.base/signals-state-child.c: New file.
	* gdb.base/signals-state-child.exp: New file.
	* gdb.gdb/selftest.exp (do_steps_and_nexts): Add new pattern.
---
 gdb/Makefile.in                                |   9 ++-
 gdb/common/common.m4                           |   2 +-
 gdb/common/signals-state-save-restore.c        |  94 +++++++++++++++++++++++
 gdb/common/signals-state-save-restore.h        |  39 ++++++++++
 gdb/configure                                  |   2 +-
 gdb/fork-child.c                               |   4 +-
 gdb/gdbserver/Makefile.in                      |   4 +
 gdb/gdbserver/config.in                        |   3 +
 gdb/gdbserver/configure                        |   2 +-
 gdb/gdbserver/linux-low.c                      |   4 +-
 gdb/gdbserver/server.c                         |   4 +-
 gdb/main.c                                     |   2 +
 gdb/testsuite/gdb.base/signals-state-child.c   | 101 +++++++++++++++++++++++++
 gdb/testsuite/gdb.base/signals-state-child.exp |  82 ++++++++++++++++++++
 gdb/testsuite/gdb.gdb/selftest.exp             |   4 +
 15 files changed, 349 insertions(+), 7 deletions(-)
 create mode 100644 gdb/common/signals-state-save-restore.c
 create mode 100644 gdb/common/signals-state-save-restore.h
 create mode 100644 gdb/testsuite/gdb.base/signals-state-child.c
 create mode 100644 gdb/testsuite/gdb.base/signals-state-child.exp

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 5af6103..7b2df86 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -897,6 +897,7 @@ SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-valprint.c ada-tasks.c \
 	target/waitstatus.c common/print-utils.c common/rsp-low.c \
 	common/errors.c common/common-debug.c common/common-exceptions.c \
 	common/btrace-common.c common/fileio.c common/common-regcache.c \
+	common/signals-state-save-restore.c \
 	$(SUBDIR_GCC_COMPILE_SRCS)
 
 LINTFILES = $(SFILES) $(YYFILES) $(CONFIG_SRCS) init.c
@@ -989,7 +990,8 @@ common/common-exceptions.h target/target.h common/symbol.h \
 common/common-regcache.h fbsd-tdep.h nat/linux-personality.h \
 common/fileio.h nat/x86-linux.h nat/x86-linux-dregs.h nat/amd64-linux-siginfo.h\
 nat/linux-namespaces.h arch/arm.h common/gdb_sys_time.h arch/aarch64-insn.h \
-tid-parse.h ser-event.h
+tid-parse.h ser-event.h \
+common/signals-state-save-restore.h
 
 # Header files that already have srcdir in them, or which are in objdir.
 
@@ -1049,6 +1051,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
 	build-id.o buildsym.o \
 	findcmd.o \
 	std-regs.o \
+	signals-state-save-restore.o \
 	signals.o \
 	exec.o reverse.o \
 	bcache.o objfiles.o observer.o minsyms.o maint.o demangle.o \
@@ -2286,6 +2289,10 @@ common-regcache.o: ${srcdir}/common/common-regcache.c
 	$(COMPILE) $(srcdir)/common/common-regcache.c
 	$(POSTCOMPILE)
 
+signals-state-save-restore.o: $(srcdir)/common/signals-state-save-restore.c
+	$(COMPILE) $(srcdir)/common/signals-state-save-restore.c
+	$(POSTCOMPILE)
+
 #
 # gdb/target/ dependencies
 #
diff --git a/gdb/common/common.m4 b/gdb/common/common.m4
index 1342503..68afc65 100644
--- a/gdb/common/common.m4
+++ b/gdb/common/common.m4
@@ -30,7 +30,7 @@ AC_DEFUN([GDB_AC_COMMON], [
 		   sys/un.h sys/wait.h dnl
 		   thread_db.h wait.h)
 
-  AC_CHECK_FUNCS([fdwalk getrlimit pipe pipe2 socketpair])
+  AC_CHECK_FUNCS([fdwalk getrlimit pipe pipe2 socketpair sigaction])
 
   AC_CHECK_DECLS([strerror, strstr])
 
diff --git a/gdb/common/signals-state-save-restore.c b/gdb/common/signals-state-save-restore.c
new file mode 100644
index 0000000..1c00cdd
--- /dev/null
+++ b/gdb/common/signals-state-save-restore.c
@@ -0,0 +1,94 @@
+/* Copyright (C) 2016 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "common-defs.h"
+#include "signals-state-save-restore.h"
+
+#include <signal.h>
+
+/* The original signal actions and mask.  */
+
+#ifdef HAVE_SIGACTION
+static struct sigaction original_signal_actions[NSIG];
+
+/* Note that we use sigprocmask without worrying about threads because
+   the save/restore functions are called either from main, or after a
+   fork.  In both cases, we know the calling process is single
+   threaded.  */
+static sigset_t original_signal_mask;
+#endif
+
+/* See signals-state-save-restore.h.   */
+
+void
+save_original_signals_state (void)
+{
+#ifdef HAVE_SIGACTION
+  int i;
+  int res;
+
+  res = sigprocmask (0,  NULL, &original_signal_mask);
+  if (res == -1)
+    perror_with_name ("sigprocmask");
+
+  for (i = 1; i < NSIG; i++)
+    {
+      struct sigaction *oldact = &original_signal_actions[i];
+
+      res = sigaction (i, NULL, oldact);
+      if (res == -1 && errno == EINVAL)
+	{
+	  /* Some signal numbers in the range are invalid.  */
+	  continue;
+	}
+      else if (res == -1)
+	perror_with_name ("sigaction");
+
+      /* If we find a custom signal handler already installed, then
+	 this function was called too late.  */
+      if (oldact->sa_handler != SIG_DFL && oldact->sa_handler != SIG_IGN)
+	internal_error (__FILE__, __LINE__, _("unexpected signal handler"));
+    }
+#endif
+}
+
+/* See signals-state-save-restore.h.   */
+
+void
+restore_original_signals_state (void)
+{
+#ifdef HAVE_SIGACTION
+  int i;
+  int res;
+
+  for (i = 1; i < NSIG; i++)
+    {
+      res = sigaction (i, &original_signal_actions[i], NULL);
+      if (res == -1 && errno == EINVAL)
+	{
+	  /* Some signal numbers in the range are invalid.  */
+	  continue;
+	}
+      else if (res == -1)
+	perror_with_name ("sigaction");
+    }
+
+  res = sigprocmask (SIG_SETMASK,  &original_signal_mask, NULL);
+  if (res == -1)
+    perror_with_name ("sigprocmask");
+#endif
+}
diff --git a/gdb/common/signals-state-save-restore.h b/gdb/common/signals-state-save-restore.h
new file mode 100644
index 0000000..bed77fe
--- /dev/null
+++ b/gdb/common/signals-state-save-restore.h
@@ -0,0 +1,39 @@
+/* Copyright (C) 2016 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef COMMON_SIGNALS_STATE_SAVE_RESTORE_H
+#define COMMON_SIGNALS_STATE_SAVE_RESTORE_H
+
+/* Save/restore the signal actions of all signals, and the signal
+   mask.
+
+   Since the exec family of functions does not reset the signal
+   disposition of signals set to SIG_IGN, nor does it reset the signal
+   mask, in order to be transparent, when spawning new child processes
+   to debug (with "run", etc.), we must reset signal actions and mask
+   back to what was originally inherited from gdb/gdbserver's parent,
+   just before execing the target program to debug.  */
+
+/* Save the signal state of all signals.  */
+
+extern void save_original_signals_state (void);
+
+/* Restore the signal state of all signals.  */
+
+extern void restore_original_signals_state (void);
+
+#endif /* COMMON_SIGNALS_STATE_SAVE_RESTORE_H */
diff --git a/gdb/configure b/gdb/configure
index 067f86e..fde6503 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -12243,7 +12243,7 @@ fi
 done
 
 
-  for ac_func in fdwalk getrlimit pipe pipe2 socketpair
+  for ac_func in fdwalk getrlimit pipe pipe2 socketpair sigaction
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/gdb/fork-child.c b/gdb/fork-child.c
index 8ac3bef..6856cf6 100644
--- a/gdb/fork-child.c
+++ b/gdb/fork-child.c
@@ -32,7 +32,7 @@
 #include "solib.h"
 #include "filestuff.h"
 #include "top.h"
-
+#include "signals-state-save-restore.h"
 #include <signal.h>
 
 /* This just gets used as a default if we can't find SHELL.  */
@@ -366,6 +366,8 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env,
         saying "not parent".  Sorry; you'll have to use print
         statements!  */
 
+      restore_original_signals_state ();
+
       /* There is no execlpe call, so we have to set the environment
          for our child in the global variable.  If we've vforked, this
          clobbers the parent, but environ is restored a few lines down
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index 2be61ef..bed2b1e 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -200,6 +200,7 @@ OBS = agent.o ax.o inferiors.o regcache.o remote-utils.o server.o signals.o \
       common-utils.o ptid.o buffer.o format.o filestuff.o dll.o notif.o \
       tdesc.o print-utils.o rsp-low.o errors.o common-debug.o cleanups.o \
       common-exceptions.o symbol.o btrace-common.o fileio.o common-regcache.o \
+      signals-state-save-restore.o \
       $(XML_BUILTIN) $(DEPFILES) $(LIBOBJS)
 GDBREPLAY_OBS = gdbreplay.o version.o
 GDBSERVER_LIBS = @GDBSERVER_LIBS@
@@ -715,6 +716,9 @@ fileio.o: ../common/fileio.c
 common-regcache.o: ../common/common-regcache.c
 	$(COMPILE) $<
 	$(POSTCOMPILE)
+signals-state-save-restore.o: ../common/signals-state-save-restore.c
+	$(COMPILE) $<
+	$(POSTCOMPILE)
 
 # Arch object files rules form ../arch
 
diff --git a/gdb/gdbserver/config.in b/gdb/gdbserver/config.in
index 2c3a69a..04072cf 100644
--- a/gdb/gdbserver/config.in
+++ b/gdb/gdbserver/config.in
@@ -202,6 +202,9 @@
 /* Define to 1 if you have the <sgtty.h> header file. */
 #undef HAVE_SGTTY_H
 
+/* Define to 1 if you have the `sigaction' function. */
+#undef HAVE_SIGACTION
+
 /* Define to 1 if you have the <signal.h> header file. */
 #undef HAVE_SIGNAL_H
 
diff --git a/gdb/gdbserver/configure b/gdb/gdbserver/configure
index 2926deb..0a3436e 100755
--- a/gdb/gdbserver/configure
+++ b/gdb/gdbserver/configure
@@ -5912,7 +5912,7 @@ fi
 done
 
 
-  for ac_func in fdwalk getrlimit pipe pipe2 socketpair
+  for ac_func in fdwalk getrlimit pipe pipe2 socketpair sigaction
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 1839f99..45061ac 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -22,7 +22,7 @@
 #include "agent.h"
 #include "tdesc.h"
 #include "rsp-low.h"
-
+#include "signals-state-save-restore.h"
 #include "nat/linux-nat.h"
 #include "nat/linux-waitpid.h"
 #include "gdb_wait.h"
@@ -975,6 +975,8 @@ linux_create_inferior (char *program, char **allargs)
 	    }
 	}
 
+      restore_original_signals_state ();
+
       execv (program, allargs);
       if (errno == ENOENT)
 	execvp (program, allargs);
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 6d6cb09..6fbd61d 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -22,7 +22,7 @@
 #include "notif.h"
 #include "tdesc.h"
 #include "rsp-low.h"
-
+#include "signals-state-save-restore.h"
 #include <ctype.h>
 #include <unistd.h>
 #if HAVE_SIGNAL_H
@@ -3611,6 +3611,8 @@ captured_main (int argc, char *argv[])
      opened by remote_prepare.  */
   notice_open_fds ();
 
+  save_original_signals_state ();
+
   /* We need to know whether the remote connection is stdio before
      starting the inferior.  Inferiors created in this scenario have
      stdin,stdout redirected.  So do this here before we call
diff --git a/gdb/main.c b/gdb/main.c
index 5477379..23d4ca0 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -44,6 +44,7 @@
 #include <signal.h>
 #include "event-top.h"
 #include "infrun.h"
+#include "signals-state-save-restore.h"
 
 /* The selected interpreter.  This will be used as a set command
    variable, so it should always be malloc'ed - since
@@ -505,6 +506,7 @@ captured_main (void *data)
 
   bfd_init ();
   notice_open_fds ();
+  save_original_signals_state ();
 
   make_cleanup (VEC_cleanup (cmdarg_s), &cmdarg_vec);
   dirsize = 1;
diff --git a/gdb/testsuite/gdb.base/signals-state-child.c b/gdb/testsuite/gdb.base/signals-state-child.c
new file mode 100644
index 0000000..e11fa93
--- /dev/null
+++ b/gdb/testsuite/gdb.base/signals-state-child.c
@@ -0,0 +1,101 @@
+/* Copyright 2016 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <signal.h>
+#include <assert.h>
+#include <errno.h>
+
+#ifndef OUTPUT_TXT
+# define OUTPUT_TXT "output.txt"
+#endif
+
+static void
+perror_and_exit (const char *s)
+{
+  perror (s);
+  exit (1);
+}
+
+int
+main (int argc, char **argv)
+{
+  int i;
+  FILE *out;
+  sigset_t sigset;
+  int res;
+
+  res = sigprocmask (0,  NULL, &sigset);
+  if (res != 0)
+    perror_and_exit ("sigprocmask");
+
+  if (argc > 1)
+    out = stdout;
+  else
+    {
+      out = fopen (OUTPUT_TXT, "w");
+      if (out == NULL)
+	perror_and_exit ("fopen");
+    }
+
+  for (i = 1; i < NSIG; i++)
+    {
+      struct sigaction oldact;
+
+      fprintf (out, "signal %d: ", i);
+
+      res = sigaction (i, NULL, &oldact);
+      if (res == -1 && errno == EINVAL)
+	{
+	  /* Some signal numbers in the range are invalid.  E.g.,
+	     signals 32 and 33 on GNU/Linux.  */
+	  fprintf (out, "invalid");
+	}
+      else if (res == -1)
+	{
+	  perror_and_exit ("sigaction");
+	}
+      else
+	{
+	  int m;
+
+	  fprintf (out, "sigaction={sa_handler=", i);
+
+	  if (oldact.sa_handler == SIG_DFL)
+	    fprintf (out, "SIG_DFL");
+	  else if (oldact.sa_handler == SIG_IGN)
+	    fprintf (out, "SIG_IGN");
+	  else
+	    abort ();
+
+	  fprintf (out, ", sa_mask=");
+	  for (m = 1; m < NSIG; m++)
+	    fprintf (out, "%c", sigismember (&oldact.sa_mask, m) ? '1' : '0');
+
+	  fprintf (out, ", sa_flags=%d", oldact.sa_flags);
+
+	  fprintf (out, "}, masked=%d", sigismember (&sigset, i));
+	}
+      fprintf (out, "\n");
+    }
+
+  if (out != stdout)
+    fclose (out);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/signals-state-child.exp b/gdb/testsuite/gdb.base/signals-state-child.exp
new file mode 100644
index 0000000..f5fdcb2
--- /dev/null
+++ b/gdb/testsuite/gdb.base/signals-state-child.exp
@@ -0,0 +1,82 @@
+# Copyright 2016 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test that gdb's (or gdbserver's) own signal handling does not
+# interfere with the signal actions (dispositions, etc.) and mask
+# their spawned children inherit.
+#
+# - If gdb inherits some signal set to SIG_IGN, so should the
+#   inferior, even if gdb itself chooses not to ignore the signal.
+#
+# - If gdb inherits some signal set to SIG_DFL, so should the inferior
+#   even if gdb itself ignores that signal.
+#
+# This requires special support in gdb/gdbserver because the exec
+# family of functions does not reset the signal disposition of signals
+# that are set to SIG_IGN, nor signal masks and flags.
+
+standard_testfile
+
+set gdb_txt [standard_output_file gdb.txt]
+set standalone_txt [standard_output_file standalone.txt]
+remote_exec host "rm -f $gdb_txt"
+remote_exec host "rm -f $standalone_txt"
+
+set options [list debug "additional_flags=-DOUTPUT_TXT=\"$gdb_txt\""]
+if {[build_executable $testfile.exp $testfile $srcfile $options]} {
+    untested $testfile.exp
+    return -1
+}
+
+set options [list debug "additional_flags=-DOUTPUT_TXT=\"$standalone_txt\""]
+if {[build_executable $testfile.exp $testfile-standalone $srcfile $options]} {
+    untested $testfile.exp
+    return -1
+}
+
+# Run the program directly, and dump its initial signal actions and
+# mask in "standalone.txt".
+
+# Use remote_spawn instead of remote_exec, like how we spawn gdb.
+# This is in order to take the same code code paths in dejagnu
+# compared to when running the program through gdb.  E.g., because
+# local_exec uses -ignore SIGHUP, while remote_spawn does not, if we
+# used remote_exec, the test program would start with SIGHUP ignored
+# when run standalone, but not when run through gdb.
+set res [remote_spawn host "$binfile-standalone"]
+if { $res < 0 || $res == "" } {
+    untested "spawning $binfile-standalone failed"
+    return 1
+} else {
+    pass "collect standalone signals state"
+}
+remote_close host
+
+# Now run the program through gdb, and dump its initial signal actions
+# and mask in "gdb.txt".
+
+clean_restart $binfile
+
+if { ! [ runto_main ] } then {
+    untested $testfile.exp
+    return -1
+}
+
+gdb_continue_to_end "collect signals state under gdb"
+
+# Diff the .txt files.  They should be identical.
+gdb_test "shell diff -s $standalone_txt $gdb_txt" \
+    "Files .* are identical.*" \
+    "signals states are identical"
diff --git a/gdb/testsuite/gdb.gdb/selftest.exp b/gdb/testsuite/gdb.gdb/selftest.exp
index 2fdd9e3..28d5493 100644
--- a/gdb/testsuite/gdb.gdb/selftest.exp
+++ b/gdb/testsuite/gdb.gdb/selftest.exp
@@ -162,6 +162,10 @@ proc do_steps_and_nexts {} {
 		set description "next over notice_open_fds"
 		set command "next"
 	    }
+	    -re ".*save_original_signals_state ..;.*$gdb_prompt $" {
+		set description "next over save_original_signals_state"
+		set command "next"
+	    }
 	    -re ".*VEC_cleanup .cmdarg_s.*$gdb_prompt $" {
 		set description "next over cmdarg_s VEC_cleanup"
 		set command "next"
-- 
2.5.5

Re: [PATCH v2] Fix PR gdb/18653: gdb disturbs inferior's inherited signal dispositions

[John Baldwin]

On Friday, August 05, 2016 11:56:56 AM Pedro Alves wrote:
> On 08/05/2016 02:04 AM, Pedro Alves wrote:
> > On 08/04/2016 07:37 PM, John Baldwin wrote:
> > 
> >> Perhaps consider saving/restore the entire 'struct sigaction' instead
> >> of just the sa_handler field given you are already requiring sigaction?
> > 
> > Indeed, I think I should, because signal masks are not reset
> > at exec time either.  Forgot about that.  Thanks for suggesting this.
> > 
> > I'll give it a try soon.
> > 
> 
> Like this?  
> 
> I renamed the new files and functions to avoid "disposition" since this
> saves/restores more than than now. I also applied a bit more polish to
> comments and error handling.

Yep.  Thanks, looks good to me.

-- 
John Baldwin

Re: [PATCH v2] Fix PR gdb/18653: gdb disturbs inferior's inherited signal dispositions

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> +      else
> +	{
> +	  int m;
> +
> +	  fprintf (out, "sigaction={sa_handler=", i);

The redundant "i" causes a compilation warning,

gdb/testsuite/gdb.base/signals-state-child.c:77:4: warning: too many arguments for format [-Wformat-extra-args]
    fprintf (out, "sigaction={sa_handler=", i);
    ^

The patch blow fixes this.  With this patch applied, the test case can
be compiled successfully.  However test fails,

shell diff -s /scratch/yao/gdb/build-git/x86_64/gdb/testsuite/outputs/gdb.base/signals-state-child/standalone.txt /scratch/yao/gdb/build-git/x86_64/gdb/testsuite/outputs/gdb.base/signals-state-child/gdb.txt^M
13c13^M
< signal 13: sigaction={sa_handler=SIG_DFL, sa_mask=0000000000000000000000000000000000000000000000000000000000000000, sa_flags=0}, masked=0^M
---^M
> signal 13: sigaction={sa_handler=SIG_IGN, sa_mask=0000000000000000000000000000000000000000000000000000000000000000, sa_flags=0}, masked=0^M
25c25^M
< signal 25: sigaction={sa_handler=SIG_DFL, sa_mask=0000000000000000000000000000000000000000000000000000000000000000, sa_flags=0}, masked=0^M
---^M
> signal 25: sigaction={sa_handler=SIG_IGN, sa_mask=0000000000000000000000000000000000000000000000000000000000000000, sa_flags=0}, masked=0^M
(gdb) FAIL: gdb.base/signals-state-child.exp: signals states are identical

-- 
Yao (齐尧)
From e1a1f81d96575dfda32022c599652cefae762798 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Fri, 12 Aug 2016 09:08:44 +0100
Subject: [PATCH] Fix warning in gdb.base/signals-state-child.c

I see the following warning when running signals-state-child.exp.

gdb/testsuite/gdb.base/signals-state-child.c:77:4: warning: too many arguments for format [-Wformat-extra-args]
    fprintf (out, "sigaction={sa_handler=", i);
    ^

this patch is to remove the argument from fprintf.

gdb/testsuite:

2016-08-12  Yao Qi  <yao.qi@linaro.org>

	* gdb.base/signals-state-child.c (main): Remove "i" from fprintf's
	argument list.

diff --git a/gdb/testsuite/gdb.base/signals-state-child.c b/gdb/testsuite/gdb.base/signals-state-child.c
index e11fa93..f934c86 100644
--- a/gdb/testsuite/gdb.base/signals-state-child.c
+++ b/gdb/testsuite/gdb.base/signals-state-child.c
@@ -74,7 +74,7 @@ main (int argc, char **argv)
 	{
 	  int m;
 
-	  fprintf (out, "sigaction={sa_handler=", i);
+	  fprintf (out, "sigaction={sa_handler=");
 
 	  if (oldact.sa_handler == SIG_DFL)
 	    fprintf (out, "SIG_DFL");

Re: [PATCH v2] Fix PR gdb/18653: gdb disturbs inferior's inherited signal dispositions

[Pedro Alves]

On 08/12/2016 09:21 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> +      else
>> +	{
>> +	  int m;
>> +
>> +	  fprintf (out, "sigaction={sa_handler=", i);
> 
> The redundant "i" causes a compilation warning,
> 
> gdb/testsuite/gdb.base/signals-state-child.c:77:4: warning: too many arguments for format [-Wformat-extra-args]
>     fprintf (out, "sigaction={sa_handler=", i);
>     ^
> 
> The patch blow fixes this.  With this patch applied, the test case can
> be compiled successfully.  

Thanks.  It's simply that v1 used it like this:

+      if (oldact.sa_handler == SIG_DFL)
+	fprintf (out, "%d: SIG_DFL\n", i);
+      else if (oldact.sa_handler == SIG_IGN)
+	fprintf (out, "%d: SIG_IGN\n", i);

and in v2 the signal number printing go split to a separate fprintf:

+      fprintf (out, "signal %d: ", i);

... I missed the 'i' that was left behind.

Please push.

> However test fails,
> 
> shell diff -s /scratch/yao/gdb/build-git/x86_64/gdb/testsuite/outputs/gdb.base/signals-state-child/standalone.txt /scratch/yao/gdb/build-git/x86_64/gdb/testsuite/outputs/gdb.base/signals-state-child/gdb.txt^M
> 13c13^M
> < signal 13: sigaction={sa_handler=SIG_DFL, sa_mask=0000000000000000000000000000000000000000000000000000000000000000, sa_flags=0}, masked=0^M
> ---^M
>> signal 13: sigaction={sa_handler=SIG_IGN, sa_mask=0000000000000000000000000000000000000000000000000000000000000000, sa_flags=0}, masked=0^M
> 25c25^M
> < signal 25: sigaction={sa_handler=SIG_DFL, sa_mask=0000000000000000000000000000000000000000000000000000000000000000, sa_flags=0}, masked=0^M
> ---^M
>> signal 25: sigaction={sa_handler=SIG_IGN, sa_mask=0000000000000000000000000000000000000000000000000000000000000000, sa_flags=0}, masked=0^M
> (gdb) FAIL: gdb.base/signals-state-child.exp: signals states are identical
> 

First thing to check is whether running the standalone vs through-gdb check
outside dejagnu also shows differences. 

I realize now that NSIG is not the right upper bound, since it
doesn't cover the realtime signals.  Maybe it should be _NSIG if
available.  But that shouldn't explain this particular problem,
since NSIG is 32.

Thanks,
Pedro Alves

Re: [PATCH v2] Fix PR gdb/18653: gdb disturbs inferior's inherited signal dispositions

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> diff --git a/gdb/testsuite/gdb.base/signals-state-child.exp b/gdb/testsuite/gdb.base/signals-state-child.exp
> new file mode 100644
> index 0000000..f5fdcb2

> +
> +set gdb_txt [standard_output_file gdb.txt]
> +set standalone_txt [standard_output_file standalone.txt]
> +remote_exec host "rm -f $gdb_txt"
> +remote_exec host "rm -f $standalone_txt"
> +
> +set options [list debug "additional_flags=-DOUTPUT_TXT=\"$gdb_txt\""]
> +if {[build_executable $testfile.exp $testfile $srcfile $options]} {
> +    untested $testfile.exp
> +    return -1
> +}

$gdb_txt and $standalone_txt is the file location on build, but we pass
them to the test file, which will be run on target.  It won't work if
build != target.

> +
> +set options [list debug "additional_flags=-DOUTPUT_TXT=\"$standalone_txt\""]
> +if {[build_executable $testfile.exp $testfile-standalone $srcfile $options]} {
> +    untested $testfile.exp
> +    return -1
> +}
> +
> +# Run the program directly, and dump its initial signal actions and
> +# mask in "standalone.txt".
> +
> +# Use remote_spawn instead of remote_exec, like how we spawn gdb.
> +# This is in order to take the same code code paths in dejagnu
> +# compared to when running the program through gdb.  E.g., because
> +# local_exec uses -ignore SIGHUP, while remote_spawn does not, if we
> +# used remote_exec, the test program would start with SIGHUP ignored
> +# when run standalone, but not when run through gdb.
> +set res [remote_spawn host "$binfile-standalone"]

s/host/target/

> +if { $res < 0 || $res == "" } {
> +    untested "spawning $binfile-standalone failed"
> +    return 1
> +} else {
> +    pass "collect standalone signals state"
> +}
> +remote_close host

How about the patch below?  It follows the way we fix the problem of
this kind in mi-traceframe-changed.exp and tfile.exp.

How about the patch below?  It should go to master and 7.12 branch.

-- 
Yao (齐尧)

From 6be8c11c9dc2f733449c1a988dee5f8354f5dfc6 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Fri, 19 Aug 2016 14:49:31 +0100
Subject: [PATCH] Fix signals-state-child.exp in remote testing

Remote testing isn't considered in signals-state-child.exp, so the it
fails like

shell diff -s /scratch/yao/gdb/build-git/aarch64-linux-gnu/gdb/testsuite/outputs/gdb.base/signals-state-child/standalone.txt /scratch/yao/gdb/build-git/aarch64-linux-gnu/gdb/testsuite/outputs/gdb.base/signals-state-child/gdb.txt^M
diff: /scratch/yao/gdb/build-git/aarch64-linux-gnu/gdb/testsuite/outputs/gdb.base/signals-state-child/standalone.txt: No such file or directory^M
(gdb) FAIL: gdb.base/signals-state-child.exp: signals states are identical

This patch is to fix it.

gdb/testsuite:

2016-08-22  Yao Qi  <yao.qi@linaro.org>

	* gdb.base/signals-state-child.exp: Set variables gdb_txt and
	standalone_txt.  Delete gdb_txt and standalone_txt on host
	and target.  Spawn the binary on target.  Copy files from
	target to host.

diff --git a/gdb/testsuite/gdb.base/signals-state-child.exp b/gdb/testsuite/gdb.base/signals-state-child.exp
index f5fdcb2..cd6a9a5 100644
--- a/gdb/testsuite/gdb.base/signals-state-child.exp
+++ b/gdb/testsuite/gdb.base/signals-state-child.exp
@@ -29,10 +29,20 @@
 
 standard_testfile
 
-set gdb_txt [standard_output_file gdb.txt]
-set standalone_txt [standard_output_file standalone.txt]
-remote_exec host "rm -f $gdb_txt"
-remote_exec host "rm -f $standalone_txt"
+if {![is_remote host] && ![is_remote target]} {
+    set gdb_txt [standard_output_file gdb.txt]
+    set standalone_txt [standard_output_file standalone.txt]
+    set purely_local 1
+} else {
+    set gdb_txt gdb.txt
+    set standalone_txt standalone.txt
+    set purely_local 0
+}
+
+remote_file host delete $gdb_txt
+remote_file host delete $standalone_txt
+remote_file target delete $gdb_txt
+remote_file target delete $standalone_txt
 
 set options [list debug "additional_flags=-DOUTPUT_TXT=\"$gdb_txt\""]
 if {[build_executable $testfile.exp $testfile $srcfile $options]} {
@@ -55,14 +65,14 @@ if {[build_executable $testfile.exp $testfile-standalone $srcfile $options]} {
 # local_exec uses -ignore SIGHUP, while remote_spawn does not, if we
 # used remote_exec, the test program would start with SIGHUP ignored
 # when run standalone, but not when run through gdb.
-set res [remote_spawn host "$binfile-standalone"]
+set res [remote_spawn target "$binfile-standalone"]
 if { $res < 0 || $res == "" } {
     untested "spawning $binfile-standalone failed"
     return 1
 } else {
     pass "collect standalone signals state"
 }
-remote_close host
+remote_close target
 
 # Now run the program through gdb, and dump its initial signal actions
 # and mask in "gdb.txt".
@@ -76,6 +86,12 @@ if { ! [ runto_main ] } then {
 
 gdb_continue_to_end "collect signals state under gdb"
 
+if {!$purely_local} {
+    # Copy file from target to host through build.
+    remote_download host [remote_upload target gdb.txt] gdb.txt
+    remote_download host [remote_upload target standalone.txt] standalone.txt
+}
+
 # Diff the .txt files.  They should be identical.
 gdb_test "shell diff -s $standalone_txt $gdb_txt" \
     "Files .* are identical.*" \

Re: [PATCH v2] Fix PR gdb/18653: gdb disturbs inferior's inherited signal dispositions

[Pedro Alves]

On 08/22/2016 03:28 PM, Yao Qi wrote:

> How about the patch below?  It follows the way we fix the problem of
> this kind in mi-traceframe-changed.exp and tfile.exp.
> 
> How about the patch below?  It should go to master and 7.12 branch.

LGTM.

Thanks,
Pedro Alves

Re: [PATCH v2] Fix PR gdb/18653: gdb disturbs inferior's inherited signal dispositions

[Yao Qi]

On Mon, Aug 22, 2016 at 3:34 PM, Pedro Alves <palves@redhat.com> wrote:
>> How about the patch below?  It should go to master and 7.12 branch.
>
> LGTM.
>

Patch is pushed in to master and 7.12.

-- 
Yao (齐尧)