Supress SIGTTOU when handling errors

  [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

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

-- 
2.20.1 (Apple Git-117)
  

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

On Mai 16 2019, Alan Hayward <Alan.Hayward@arm.com> wrote:

> [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

e671cd59d74cec9f53e110ce887128d1eeadb7f2 is the first bad commit
commit e671cd59d74cec9f53e110ce887128d1eeadb7f2
Author: Pedro Alves <palves@redhat.com>
Date:   Tue Jan 30 14:23:51 2018 +0000

    Per-inferior target_terminal state, fix PR gdb/13211, more

Andreas.
  

On 5/16/19 7:06 PM, Andrew Burgess wrote:

> 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, 

The point of ours_for_output is that while the inferior is still
running, leave it in the foreground, such that gdb doesn't interfere
with the terminal mode, Ctrl-C reaches the inferior directly, or such
that any keyboard/stdin input typed by the user goes to the inferior
directly, not to gdb.  The latter is particularly important for a
follow up series I was working on but never got a chance to
submit, here:

  https://github.com/palves/gdb/commits/palves/tty-always-separate-session

With that branch, gdb always puts the inferior in a separate session,
and then pumps stdin/stdout between the inferior's tty and gdb's tty
at the right times.  That branch solves problems like not being able
to Ctrl-C while the inferior is blocked in sigwait with SIGINT blocked
(gdb/14559, gdb/9425).  That's the fix I mentioned in the commit log
of e671cd59d74c.  Anyway, with that branch, if we switch to ours() while
the inferior is running in the foreground, then we miss forwarding the
input to the inferior.  See:

https://github.com/palves/gdb/blob/d3392b83086b0a541aa31fcff301281da7a73a0e/gdb/inflow.c#L762

Also, if we miss calling ours_for_output(), then we print output to the
terminal in raw mode.  What I'm saying is that that branch/fix exposes
more latent bugs around incorrect ours()/ours_for_output()/inferior()
handling, and the branch includes fixes in that direction, e.g.: the 
"target_terminal::ours_for_output(): received SIGINT" patch.

This is not unlike what old comment in remote.c suggests we could
do, but for local debugging:

static void
remote_terminal_inferior (struct target_ops *self)
{
  /* NOTE: At this point we could also register our selves as the
     recipient of all input.  Any characters typed could then be
     passed on down to the target.  */
}


> 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....

I'm thinking that Alan's original patch to disable SIGTTOU is correct.
When we're in ours_for_output mode, we should be able to write to the
terminal, and we can.  But, there are a couple functions that raise
a SIGTTOU if you write to the controlling terminal while in the
background, and your terminal has the TOSTOP attribute set, and tcdrain
is one of them.  

That isn't to say that your patch _isn't_ also correct.  We have many
latent bugs around this area.  Let me take a better look at that one too.

I think that even if we get something like your patch in, then
Alan's is still correct because we can have places doing
try/catch to swallow an error but still print it with exception_print,
all while the inferior is running.  Of course such spots should
call ours_for_output(), but that will run into the tcdrain issue.


> 
> 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,
Pedro Alves