Re: [PATCH] Supress SIGTTOU when handling errors

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Alan Hayward <Alan.Hayward@arm.com> [2019-05-16 15:51:53 +0000]:

> [I've seen this on and off over many months on AArch64 and Arm, and am
> assuming it isn't the intended behaviour. Not sure if this should be at
> tcdrain or it should be done at a higher level - eg in the terminal
> handling code]
> 
> Calls to error () can cause SIGTTOU to send gdb to the background.
> 
> For example, on an Arm build:
>   (gdb) b main
>   Breakpoint 1 at 0x10774: file /build/gdb/testsuite/../../../src/binutils-gdb/gdb/testsuite/gdb.base/watchpoint.c, line 174.
>   (gdb) r
>   Starting program: /build/gdb/testsuite/outputs/gdb.base/watchpoint/watchpoint
> 
>   [1]+  Stopped                 ../gdb ./outputs/gdb.base/watchpoint/watchpoint
>   localhost$ fg
>   ../gdb ./outputs/gdb.base/watchpoint/watchpoint
>   Cannot parse expression `.L1199 4@r4'.
>   warning: Probes-based dynamic linker interface failed.
>   Reverting to original interface.
> 
> The SIGTTOU is raised whilst inside a syscall during the call to tcdrain.
> Fix is to use scoped_ignore_sigttou to ensure SIGTTOU is blocked.
> 
> In addition fix include comments - job_control is not included via
> terminal.h

Maybe someone else knows this better, but this feels like the wrong
solution to me.

As I understand it the problem you're seeing could be resolved by
making sure that the terminal is correctly claimed by GDB at the point
tcdrain is called.  This would require a call to either
'target_terminal::ours ()' or 'target_terminal::ours_for_output ()'.

I've made several attempts to fix this in the past (non posted
though), one problem I've previously run into that I've not yet
understood is that calling ::ours_for_output doesn't seem to be enough
to prevent the SIGTTOU (I assume from the tcdrain) and only calling
::ours is enough.

What I've been trying to figure out is what the intended difference
between ::ours_for_output and ::ours actually is, if we can't always
write output after calling ::ours_for_output.  Part of me wonders if
we should just switch to using ::ours in all cases....

Anyway, I think most of the problems you're seeing should be fixed by
claiming the terminal either permanently (calling ::ours or
::ours_for_output) or temporarily using
target_terminal::scoped_restore_terminal_state.

Like I said, I'm not an expert on this code, so maybe I've
misunderstood the problem you're solving.

Thanks,
Andrew


> 
> gdb/ChangeLog:
> 
> 2019-05-15  Alan Hayward  <alan.hayward@arm.com>
> 
> 	* event-top.c: Remove include comment.
> 	* inflow.c (class scoped_ignore_sigttou): Move from here...
> 	* inflow.h (class scoped_ignore_sigttou): ...to here.
> 	* ser-unix.c (hardwire_drain_output): Block SIGTTOU during drain.
> 	* top.c:  Remove include comment.
> ---
>  gdb/event-top.c |  2 +-
>  gdb/inflow.c    | 29 -----------------------------
>  gdb/inflow.h    | 31 +++++++++++++++++++++++++++++++
>  gdb/ser-unix.c  |  4 ++++
>  gdb/top.c       |  2 +-
>  5 files changed, 37 insertions(+), 31 deletions(-)
> 
> diff --git a/gdb/event-top.c b/gdb/event-top.c
> index 3ccf136ff1..93b7d2d28b 100644
> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -24,7 +24,7 @@
>  #include "inferior.h"
>  #include "infrun.h"
>  #include "target.h"
> -#include "terminal.h"		/* for job_control */
> +#include "terminal.h"
>  #include "event-loop.h"
>  #include "event-top.h"
>  #include "interps.h"
> diff --git a/gdb/inflow.c b/gdb/inflow.c
> index 339b55c0bc..eba7a931f4 100644
> --- a/gdb/inflow.c
> +++ b/gdb/inflow.c
> @@ -106,35 +106,6 @@ static serial_ttystate initial_gdb_ttystate;
>  
>  static struct terminal_info *get_inflow_inferior_data (struct inferior *);
>  
> -/* RAII class used to ignore SIGTTOU in a scope.  */
> -
> -class scoped_ignore_sigttou
> -{
> -public:
> -  scoped_ignore_sigttou ()
> -  {
> -#ifdef SIGTTOU
> -    if (job_control)
> -      m_osigttou = signal (SIGTTOU, SIG_IGN);
> -#endif
> -  }
> -
> -  ~scoped_ignore_sigttou ()
> -  {
> -#ifdef SIGTTOU
> -    if (job_control)
> -      signal (SIGTTOU, m_osigttou);
> -#endif
> -  }
> -
> -  DISABLE_COPY_AND_ASSIGN (scoped_ignore_sigttou);
> -
> -private:
> -#ifdef SIGTTOU
> -  sighandler_t m_osigttou = NULL;
> -#endif
> -};
> -
>  /* While the inferior is running, we want SIGINT and SIGQUIT to go to the
>     inferior only.  If we have job control, that takes care of it.  If not,
>     we save our handlers in these two variables and set SIGINT and SIGQUIT
> diff --git a/gdb/inflow.h b/gdb/inflow.h
> index c32aa14433..5dd5c37bd2 100644
> --- a/gdb/inflow.h
> +++ b/gdb/inflow.h
> @@ -21,5 +21,36 @@
>  #define INFLOW_H
>  
>  #include <unistd.h>
> +#include <signal.h>
> +#include "common/job-control.h"
> +
> +/* RAII class used to ignore SIGTTOU in a scope.  */
> +
> +class scoped_ignore_sigttou
> +{
> +public:
> +  scoped_ignore_sigttou ()
> +  {
> +#ifdef SIGTTOU
> +    if (job_control)
> +      m_osigttou = signal (SIGTTOU, SIG_IGN);
> +#endif
> +  }
> +
> +  ~scoped_ignore_sigttou ()
> +  {
> +#ifdef SIGTTOU
> +    if (job_control)
> +      signal (SIGTTOU, m_osigttou);
> +#endif
> +  }
> +
> +  DISABLE_COPY_AND_ASSIGN (scoped_ignore_sigttou);
> +
> +private:
> +#ifdef SIGTTOU
> +  sighandler_t m_osigttou = NULL;
> +#endif
> +};
>  
>  #endif /* inflow.h */
> diff --git a/gdb/ser-unix.c b/gdb/ser-unix.c
> index 5a9965bf74..3492619f2d 100644
> --- a/gdb/ser-unix.c
> +++ b/gdb/ser-unix.c
> @@ -32,6 +32,7 @@
>  #include "gdbcmd.h"
>  #include "common/filestuff.h"
>  #include <termios.h>
> +#include "inflow.h"
>  
>  struct hardwire_ttystate
>    {
> @@ -164,6 +165,9 @@ hardwire_print_tty_state (struct serial *scb,
>  static int
>  hardwire_drain_output (struct serial *scb)
>  {
> +  /* Ignore SIGTTOU which may occur during the drain.  */
> +  scoped_ignore_sigttou ignore_sigttou;
> +
>    return tcdrain (scb->fd);
>  }
>  
> diff --git a/gdb/top.c b/gdb/top.c
> index bacd684dba..1e17ebee87 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -34,7 +34,7 @@
>  #include "expression.h"
>  #include "value.h"
>  #include "language.h"
> -#include "terminal.h"		/* For job_control.  */
> +#include "terminal.h"
>  #include "common/job-control.h"
>  #include "annotate.h"
>  #include "completer.h"
> -- 
> 2.20.1 (Apple Git-117)
>