[PATCH] Supress SIGTTOU when handling errors

[PATCH] Supress SIGTTOU when handling errors

[Alan Hayward]

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

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)

Re: [PATCH] Supress SIGTTOU when handling errors

[Andrew Burgess]

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

Re: [PATCH] Supress SIGTTOU when handling errors

[Andrew Burgess]

Below is one of my attempts at looking into this issue.  This isn't a
patch for merging, it's just some random hacking at this point, but it
shows what I'm thinking...

With this patch applied set the environment variable 'GDB_FAKE_ERROR'
before starting GDB, set a breakpoint and run and you'll hit the
SIGTTOU issue.

  $ GDB_FAKE_ERROR=y ./gdb ./gdb
  (gdb) b main
  Breakpoint 1 at 0x410236: main. (24 locations)
  (gdb) r
  Starting program: ....
  Warning:
  Cannot insert breakpoint 1: fake error
  ...

Thanks,
Andrew

---

commit 395c148903ba4a96a55c9ade4b809b9df2ccbcb8
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Tue Sep 4 15:44:48 2018 +0100

    Remove change of terminal ownership before throw
    
    Calling target_terminal::ours_for_output before throwing an error
    seems wrong, surely the site at which the terminal is passed to the
    inferior should take care of reclaiming the terminal using a RAII
    object within that scope.
    
    If the error is caught before we reclaim the terminal _then_ we can
    call target_terminal::ours_for_output, but I don't think we should be
    calling this at the throw site.
    
    Issues:
    
      1. Why do I need to use target_terminal::ours instead of
      target_terminal::ours_for_output in event-loop.c???
    
      2. The change in event-loop.c shouldn't be there anyway, I should be
      just asserting that the terminal is owned by GDB at this point, the
      terminal should be returned to GDB using RAII at some other point in
      the stack (presumably the same frame level as we give up the
      terminal).
    
      3. Should be able to assert in the every output routing that GDB
      owns the terminal - but this is broken so badly by our debug.
      Maybe we can get GDB to automatically reclaim the terminal before
      writing out each debug.  I assume in some cases debug output would
      not work due to not owning the terminal???
    
    gdb/ChangeLog:
    
            * breakpoint.c (update_inserted_breakpoint_locations): Remove call
            to target_terminal::ours_for_output before throwing an error.
            (insert_breakpoint_locations): Likewise.
            (bkpt_insert_location): ***REMOVE*** Add code to raise a fake
            error.
            * event-loop.c: Add 'target.h' include.
            (start_event_loop): Claim terminal before printing the exception.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5b0a9fde61f..3a5396f7725 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2019-05-12  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* breakpoint.c (update_inserted_breakpoint_locations): Remove call
+	to target_terminal::ours_for_output before throwing an error.
+	(insert_breakpoint_locations): Likewise.
+	(bkpt_insert_location): ***REMOVE*** Add code to raise a fake
+	error.
+	* event-loop.c: Add 'target.h' include.
+	(start_event_loop): Claim terminal before printing the exception.
+
 2019-05-08  Tom Tromey  <tom@tromey.com>
 
 	* gdbtypes.c (objfile_type_data): Change type.
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 35da97bd041..053d75dcfd4 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2913,10 +2913,7 @@ update_inserted_breakpoint_locations (void)
     }
 
   if (error_flag)
-    {
-      target_terminal::ours_for_output ();
-      error_stream (tmp_error_stream);
-    }
+    error_stream (tmp_error_stream);
 }
 
 /* Used when starting or continuing the program.  */
@@ -3013,7 +3010,6 @@ insert_breakpoint_locations (void)
 	  tmp_error_stream.printf ("Could not insert hardware breakpoints:\n\
 You may have requested too many hardware breakpoints/watchpoints.\n");
 	}
-      target_terminal::ours_for_output ();
       error_stream (tmp_error_stream);
     }
 }
@@ -12343,6 +12339,9 @@ bkpt_insert_location (struct bp_location *bl)
 {
   CORE_ADDR addr = bl->target_info.reqstd_address;
 
+  if (getenv ("GDB_FAKE_ERROR") != NULL)
+    error ("fake error");
+
   bl->target_info.kind = breakpoint_kind (bl, &addr);
   bl->target_info.placed_address = addr;
 
diff --git a/gdb/event-loop.c b/gdb/event-loop.c
index caeb5f38d9b..611f63b5942 100644
--- a/gdb/event-loop.c
+++ b/gdb/event-loop.c
@@ -21,6 +21,7 @@
 #include "event-loop.h"
 #include "event-top.h"
 #include "ser-event.h"
+#include "target.h"
 
 #ifdef HAVE_POLL
 #if defined (HAVE_POLL_H)
@@ -371,6 +372,13 @@ start_event_loop (void)
 	}
       catch (const gdb_exception &ex)
 	{
+	  /* Ideally the terminal should have been restored to GDB as part
+	     of unwinding the stack to get back to here, but things are
+	     not ideal.  Further, based on the name alone we should be able
+	     to call target_terminal::ours_for_output () here, but that's
+	     not enough, we need to call full target_terminal::ours ().  */
+	  target_terminal::ours ();
+
 	  exception_print (gdb_stderr, ex);
 
 	  /* If any exception escaped to here, we better enable

Re: [PATCH] Supress SIGTTOU when handling errors

[Pedro Alves]

On 5/16/19 7:30 PM, Andrew Burgess wrote:
> Below is one of my attempts at looking into this issue.  This isn't a
> patch for merging, it's just some random hacking at this point, but it
> shows what I'm thinking...
> 
> With this patch applied set the environment variable 'GDB_FAKE_ERROR'
> before starting GDB, set a breakpoint and run and you'll hit the
> SIGTTOU issue.
> 
>   $ GDB_FAKE_ERROR=y ./gdb ./gdb
>   (gdb) b main
>   Breakpoint 1 at 0x410236: main. (24 locations)
>   (gdb) r
>   Starting program: ....
>   Warning:
>   Cannot insert breakpoint 1: fake error
>   ...
> 
> Thanks,
> Andrew
> 
> ---
> 
> commit 395c148903ba4a96a55c9ade4b809b9df2ccbcb8
> Author: Andrew Burgess <andrew.burgess@embecosm.com>
> Date:   Tue Sep 4 15:44:48 2018 +0100
> 
>     Remove change of terminal ownership before throw
>     
>     Calling target_terminal::ours_for_output before throwing an error
>     seems wrong, surely the site at which the terminal is passed to the
>     inferior should take care of reclaiming the terminal using a RAII
>     object within that scope.

That's not true/correct, because run control / event handle is asynchronous.
You put the inferior in the foreground, resume the inferior, and then
go back to the event loop.  The stack frame that put the terminal
in the foreground is long gone, it isn't there to restore the terminal
state anymore.  Instead, we (should) switch the terminal back to gdb when
the target stops and/or when we give back the prompt to the user.

>     
>     If the error is caught before we reclaim the terminal _then_ we can
>     call target_terminal::ours_for_output, but I don't think we should be
>     calling this at the throw site.


"I don't think we should be calling this at the throw site"

Agreed with that part.


>     
>     Issues:
>     
>       1. Why do I need to use target_terminal::ours instead of
>       target_terminal::ours_for_output in event-loop.c???

Because target_terminal::ours_for_output does not put gdb in
the foreground -- the tcsetpgrp call in child_terminal_ours_1.

>     
>       2. The change in event-loop.c shouldn't be there anyway, I should be
>       just asserting that the terminal is owned by GDB at this point, the
>       terminal should be returned to GDB using RAII at some other point in
>       the stack (presumably the same frame level as we give up the
>       terminal).

Fully disagreed.  :-)

>     
>       3. Should be able to assert in the every output routing that GDB
>       owns the terminal - but this is broken so badly by our debug.

Right, I tried that once too, in the context of that branch I pointed at
above -- we don't switch to ours_for_output before log output.  I ended
up just shrugging and thinking that at least debug output is not meant for
the users, so if debug output comes out a bit messed up it's not _that_ bad.

>       Maybe we can get GDB to automatically reclaim the terminal before
>       writing out each debug.  I assume in some cases debug output would
>       not work due to not owning the terminal???

I think output should always work.  If you miss an ours_for_output call,
all that happens is that you end up printing in whatever terminal
mode/settings the inferior configured for the terminal, instead of in
gdb's mode/settings.  But then again, if the inferior prints at
the same time as gdb, or queries/saves the terminal settings, it'll
see gdb's settings...  That's another thing that branch fixes, because
with that branch, gdb is in control of flushing the output from the
inferior's terminal to gdb's terminal at the right times, and the
inferior's terminal settings are never tweaked by gdb.

>     
>     gdb/ChangeLog:
>     
>             * breakpoint.c (update_inserted_breakpoint_locations): Remove call
>             to target_terminal::ours_for_output before throwing an error.
>             (insert_breakpoint_locations): Likewise.
>             (bkpt_insert_location): ***REMOVE*** Add code to raise a fake
>             error.
>             * event-loop.c: Add 'target.h' include.
>             (start_event_loop): Claim terminal before printing the exception.
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 5b0a9fde61f..3a5396f7725 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,13 @@
> +2019-05-12  Andrew Burgess  <andrew.burgess@embecosm.com>
> +
> +	* breakpoint.c (update_inserted_breakpoint_locations): Remove call
> +	to target_terminal::ours_for_output before throwing an error.
> +	(insert_breakpoint_locations): Likewise.
> +	(bkpt_insert_location): ***REMOVE*** Add code to raise a fake
> +	error.
> +	* event-loop.c: Add 'target.h' include.
> +	(start_event_loop): Claim terminal before printing the exception.
> +
>  2019-05-08  Tom Tromey  <tom@tromey.com>
>  
>  	* gdbtypes.c (objfile_type_data): Change type.
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 35da97bd041..053d75dcfd4 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -2913,10 +2913,7 @@ update_inserted_breakpoint_locations (void)
>      }
>  
>    if (error_flag)
> -    {
> -      target_terminal::ours_for_output ();
> -      error_stream (tmp_error_stream);
> -    }
> +    error_stream (tmp_error_stream);

This bit is OK.

>  }
>  
>  /* Used when starting or continuing the program.  */
> @@ -3013,7 +3010,6 @@ insert_breakpoint_locations (void)
>  	  tmp_error_stream.printf ("Could not insert hardware breakpoints:\n\
>  You may have requested too many hardware breakpoints/watchpoints.\n");
>  	}
> -      target_terminal::ours_for_output ();
>        error_stream (tmp_error_stream);

This bit is OK.

>      }
>  }
> @@ -12343,6 +12339,9 @@ bkpt_insert_location (struct bp_location *bl)
>  {
>    CORE_ADDR addr = bl->target_info.reqstd_address;
>  
> +  if (getenv ("GDB_FAKE_ERROR") != NULL)
> +    error ("fake error");
> +
>    bl->target_info.kind = breakpoint_kind (bl, &addr);
>    bl->target_info.placed_address = addr;
>  
> diff --git a/gdb/event-loop.c b/gdb/event-loop.c
> index caeb5f38d9b..611f63b5942 100644
> --- a/gdb/event-loop.c
> +++ b/gdb/event-loop.c
> @@ -21,6 +21,7 @@
>  #include "event-loop.h"
>  #include "event-top.h"
>  #include "ser-event.h"
> +#include "target.h"
>  
>  #ifdef HAVE_POLL
>  #if defined (HAVE_POLL_H)
> @@ -371,6 +372,13 @@ start_event_loop (void)
>  	}
>        catch (const gdb_exception &ex)
>  	{
> +	  /* Ideally the terminal should have been restored to GDB as part
> +	     of unwinding the stack to get back to here, but things are
> +	     not ideal.  Further, based on the name alone we should be able
> +	     to call target_terminal::ours_for_output () here, but that's
> +	     not enough, we need to call full target_terminal::ours ().  */

This comment is not OK.

> +	  target_terminal::ours ();

See comment just below exception_print.  It reads:

	  /* If any exception escaped to here, we better enable
	     stdin.  Otherwise, any command that calls async_disable_stdin,
	     and then throws, will leave stdin inoperable.  */

Also see async_enable_stdin:

/* Re-enable stdin after the end of an execution command in
   synchronous mode, or after an error from the target, and we aborted
   the exec operation.  */

void
async_enable_stdin (void)
{
  struct ui *ui = current_ui;

  if (ui->prompt_state == PROMPT_BLOCKED)
    {
      target_terminal::ours ();
      ui_register_input_event_handler (ui);
      ui->prompt_state = PROMPT_NEEDED;
    }
}

So it seems to be that we should call async_enable_stdin _before_
calling exception_print, instead of after.  Does that work?
If not, why not?

I would have tried it myself to confirm, but I couldn't reproduce
the SIGTTOU issue with your patch, for some reason.

> +
>  	  exception_print (gdb_stderr, ex);
>  
>  	  /* If any exception escaped to here, we better enable
> 

Thanks,
Pedro Alves

Re: [PATCH] Supress SIGTTOU when handling errors

[Alan Hayward]

> On 16 May 2019, at 19:06, Andrew Burgess <andrew.burgess@embecosm.com> wrote:
> 
> * 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 ()’.

That makes sense. Wasn’t aware about that code before. 

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


Playing about with the patch you posted, I also found that ::ours prevents
the signal, but ::ours_for_output doesn’t.  Looks like it’s due to the
following tcsetpgrp not happening:

inflow.c:child_terminal_ours_1 ()

if (job_control && desired_state == target_terminal_state::is_ours)
  {
#ifdef HAVE_TERMIOS_H
    result = tcsetpgrp (0, our_terminal_info.process_group);

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

Agreed, I’m not sure of the intended differences here.

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

The problem with that is finding all possible error cases and ensuring
they all all fixed up.

For example, my error was coming from the try/catch/exception_print
in solid-svr4.c:solib_event_probe_action ()

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

We’re definitely looking at the same issue.

Working back up the call trace from where my change was, all the error
prints first end up in exceptions.c::print_flush () which already has
a call to ::ours_for_output.

I’ve posted two patches below. Both of them fix all my issues, including
your GDB_FAKE_ERROR case.

If ::ours_for_output and ::ours are working as intended, then the first
patch is probably the correct fix.

But, if ::ours_for_output and ::ours are not working as intended, then
possibly more investigation is required to know why patch 2 works.

Alan.



PATCH 1:

gdb/ChangeLog:

2019-05-17  Alan Hayward  <alan.hayward@arm.com>

        * exceptions.c (print_flush): Use target_terminal::ours.

diff --git a/gdb/exceptions.c b/gdb/exceptions.c
index ebdc71d98d..47bfb95153 100644
--- a/gdb/exceptions.c
+++ b/gdb/exceptions.c
@@ -46,7 +46,7 @@ print_flush (void)
   if (current_top_target () != NULL && target_supports_terminal_ours ())
     {
       term_state.emplace ();
-      target_terminal::ours_for_output ();
+      target_terminal::ours ();
     }

   /* We want all output to appear now, before we print the error.  We




PATCH 2:


    gdb/ChangeLog:

    2019-05-17  Alan Hayward  <alan.hayward@arm.com>

            * inflow.c (child_terminal_ours_1): Call tcsetpgrp for
            is_ours_for_output.

diff --git a/gdb/inflow.c b/gdb/inflow.c
index 339b55c0bc..b376e24e86 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -509,7 +509,9 @@ child_terminal_ours_1 (target_terminal_state desired_state)
       /* If we only want output, then leave the inferior's pgrp in the
         foreground, so that Ctrl-C/Ctrl-Z reach the inferior
         directly.  */
-      if (job_control && desired_state == target_terminal_state::is_ours)
+      if (job_control
+         && (desired_state == target_terminal_state::is_ours
+             || desired_state == target_terminal_state::is_ours_for_output))
        {
 #ifdef HAVE_TERMIOS_H

Re: [PATCH] Supress SIGTTOU when handling errors

[Andrew Burgess]

* Alan Hayward <Alan.Hayward@arm.com> [2019-05-17 12:46:56 +0000]:

> 
> 
> > On 16 May 2019, at 19:06, Andrew Burgess <andrew.burgess@embecosm.com> wrote:
> > 
> > * 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 ()’.
> 
> That makes sense. Wasn’t aware about that code before. 
> 
> > 
> > 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.
> 
> 
> Playing about with the patch you posted, I also found that ::ours prevents
> the signal, but ::ours_for_output doesn’t.  Looks like it’s due to the
> following tcsetpgrp not happening:
> 
> inflow.c:child_terminal_ours_1 ()
> 
> if (job_control && desired_state == target_terminal_state::is_ours)
>   {
> #ifdef HAVE_TERMIOS_H
>     result = tcsetpgrp (0, our_terminal_info.process_group);
> 
> > 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….
> 
> Agreed, I’m not sure of the intended differences here.
> 
> > 
> > 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.
> 
> The problem with that is finding all possible error cases and ensuring
> they all all fixed up.

This has bothered me too.  The requirement really is that we can't
have a call to error when the terminal is not ours _unless_ there's a
catch handler in place that will reacquire the terminal.

I've been trying to figure out a good way we could test this
condition, but so far I've not come up with anything good.

I think you're correct - in the short term we should assume that GDB
doesn't hold the above requirement, and ensure we grab the terminal at
the point the error is caught / printed.

> 
> For example, my error was coming from the try/catch/exception_print
> in solid-svr4.c:solib_event_probe_action ()
> 
> > 
> > Like I said, I'm not an expert on this code, so maybe I've
> > misunderstood the problem you're solving.
> > 
> 
> We’re definitely looking at the same issue.
> 
> Working back up the call trace from where my change was, all the error
> prints first end up in exceptions.c::print_flush () which already has
> a call to ::ours_for_output.
> 
> I’ve posted two patches below. Both of them fix all my issues, including
> your GDB_FAKE_ERROR case.
> 
> If ::ours_for_output and ::ours are working as intended, then the first
> patch is probably the correct fix.
> 
> But, if ::ours_for_output and ::ours are not working as intended, then
> possibly more investigation is required to know why patch 2 works.
> 
> Alan.
> 
> 
> 
> PATCH 1:
> 
> gdb/ChangeLog:
> 
> 2019-05-17  Alan Hayward  <alan.hayward@arm.com>
> 
>         * exceptions.c (print_flush): Use target_terminal::ours.
> 
> diff --git a/gdb/exceptions.c b/gdb/exceptions.c
> index ebdc71d98d..47bfb95153 100644
> --- a/gdb/exceptions.c
> +++ b/gdb/exceptions.c
> @@ -46,7 +46,7 @@ print_flush (void)
>    if (current_top_target () != NULL && target_supports_terminal_ours ())
>      {
>        term_state.emplace ();
> -      target_terminal::ours_for_output ();
> +      target_terminal::ours ();
>      }
> 
>    /* We want all output to appear now, before we print the error.  We
> 

I think that this is my preferred patch, though I think using 'ours'
instead of the more obvious 'ours_for_output' is worth a comment.

> 
> 
> PATCH 2:
> 
> 
>     gdb/ChangeLog:
> 
>     2019-05-17  Alan Hayward  <alan.hayward@arm.com>
> 
>             * inflow.c (child_terminal_ours_1): Call tcsetpgrp for
>             is_ours_for_output.
> 
> diff --git a/gdb/inflow.c b/gdb/inflow.c
> index 339b55c0bc..b376e24e86 100644
> --- a/gdb/inflow.c
> +++ b/gdb/inflow.c
> @@ -509,7 +509,9 @@ child_terminal_ours_1 (target_terminal_state desired_state)
>        /* If we only want output, then leave the inferior's pgrp in the
>          foreground, so that Ctrl-C/Ctrl-Z reach the inferior
>          directly.  */
> -      if (job_control && desired_state == target_terminal_state::is_ours)
> +      if (job_control
> +         && (desired_state == target_terminal_state::is_ours
> +             || desired_state == target_terminal_state::is_ours_for_output))
>         {
>  #ifdef HAVE_TERMIOS_H
> 
> 

The only reason I don't like this is that I don't really understand
the wider impact it might have.  I guess there are places in GDB where
we call 'ours_for_output' and assume that Ctrl-C / Ctrl-Z will be
delivered to the inferior.  If these suddenly start arriving in GDB
then it's not clear if we'll have the correct infrastructure in place
to pass these signals on to the inferior.

You should probably wait for a while to see if any terminal experts
want to comment, but if nobody else comments within a week I think you
should go ahead with patch #1.

Thanks,
Andrew

Re: [PATCH] Supress SIGTTOU when handling errors

[Pedro Alves]

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

Re: [PATCH] Supress SIGTTOU when handling errors

[Alan Hayward]

> On 23 May 2019, at 21:32, Pedro Alves <palves@redhat.com> wrote:
> 
> 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.  


Looking back at my original patch again, I’m wondering if it’s better to
move the ignore higher up the call stack in print_flush, so that it’s set
across both flushes:


diff --git a/gdb/exceptions.c b/gdb/exceptions.c
index ebdc71d98d..cba6d78b0b 100644
--- a/gdb/exceptions.c
+++ b/gdb/exceptions.c
@@ -32,40 +32,44 @@
 static void
 print_flush (void)
 {
   struct ui *ui = current_ui;
   struct serial *gdb_stdout_serial;

   if (deprecated_error_begin_hook)
     deprecated_error_begin_hook ();

   gdb::optional<target_terminal::scoped_restore_terminal_state> term_state;
   /* While normally there's always something pushed on the target
      stack, the NULL check is needed here because we can get here very
      early during startup, before the target stack is first
      initialized.  */
   if (current_top_target () != NULL && target_supports_terminal_ours ())
     {
       term_state.emplace ();
       target_terminal::ours_for_output ();
     }

+   /* Ignore SIGTTOU which may occur during the drain due to the terminal being
+      in the background.  */
+   scoped_ignore_sigttou ignore_sigttou;
+
   /* We want all output to appear now, before we print the error.  We
      have 3 levels of buffering we have to flush (it's possible that
      some of these should be changed to flush the lower-level ones
      too):  */

   /* 1.  The _filtered buffer.  */
   if (filtered_printing_initialized ())
     wrap_here ("");

   /* 2.  The stdio buffer.  */
   gdb_flush (gdb_stdout);
   gdb_flush (gdb_stderr);

   /* 3.  The system-level buffer.  */
   gdb_stdout_serial = serial_fdopen (fileno (ui->outstream));
   if (gdb_stdout_serial)
     {
       serial_drain_output (gdb_stdout_serial);
       serial_un_fdopen (gdb_stdout_serial);
     }


...or if it really should be left just around the tcdrain.
Not quite sure what the behaviour is on non-Linux targets.


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

Minor issue is that once my patch is in, it’ll hide the missing ours_for_output
bugs (?)

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

Re: [PATCH] Supress SIGTTOU when handling errors

[Pedro Alves]

On 5/24/19 9:54 AM, Alan Hayward wrote:

> Looking back at my original patch again, I’m wondering if it’s better to
> move the ignore higher up the call stack in print_flush, so that it’s set
> across both flushes:

What are the two flushes?  I only see one, from the serial_drain_output call?

In any case, I think it's better to keep the SIGTTOU handling close to
the tcdrain call, to make to code a lot more obvious -- SIGTTOU suppression
is described in tcdrain manuals -- and I don't think we have to worry
about efficiency here?

> ...or if it really should be left just around the tcdrain.
> Not quite sure what the behaviour is on non-Linux targets.

The behavior should be the same on all POSIX systems:

https://pubs.opengroup.org/onlinepubs/009695399/functions/tcdrain.html

 "Any attempts to use tcdrain() from a process which is a member of a background
 process group on a fildes associated with its controlling terminal, shall cause the process
 group to be sent a SIGTTOU signal. If the calling process is blocking or ignoring
 SIGTTOU signals, the process shall be allowed to perform the operation, and
 no signal is sent."

On non-POSIX systems, the serial_drain_output call doesn't end up in
ser-unix.c:hardwire_drain_output, so from that perspective, putting
the SIGTTOU suppression in common code is a bit of an abstraction violation.

> 
> 
>>
>> 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.
>>
> 
> Minor issue is that once my patch is in, it’ll hide the missing ours_for_output
> bugs (?)
> 

Sure.  But we shouldn't avoid fixing one bug because of that.  The
palves/tty-always-separate-session branch on my github exposes such
bugs because with a missing ours_for_output call, gdb prints to
the screen while the terminal is in raw mode, resulting in output
like

this is line one
                this is line two
                                this is line three

instead of:

this is line one
this is line two
this is line three

Thanks,
Pedro Alves

Re: [PATCH] Supress SIGTTOU when handling errors

[Alan Hayward]

> On 24 May 2019, at 12:02, Pedro Alves <palves@redhat.com> wrote:
> 
> On 5/24/19 9:54 AM, Alan Hayward wrote:
> 
>> Looking back at my original patch again, I’m wondering if it’s better to
>> move the ignore higher up the call stack in print_flush, so that it’s set
>> across both flushes:
> 
> What are the two flushes?  I only see one, from the serial_drain_output call?

Sorry, I was forgot it was that call. I was thinking it was as part of the
gdb_flush call.
 
> 
> In any case, I think it's better to keep the SIGTTOU handling close to
> the tcdrain call, to make to code a lot more obvious -- SIGTTOU suppression
> is described in tcdrain manuals -- and I don't think we have to worry
> about efficiency here?

True - errors from gdb shouldn’t be frequent enough to be an issue.

> 
>> ...or if it really should be left just around the tcdrain.
>> Not quite sure what the behaviour is on non-Linux targets.
> 
> The behavior should be the same on all POSIX systems:
> 
> https://pubs.opengroup.org/onlinepubs/009695399/functions/tcdrain.html
> 
> "Any attempts to use tcdrain() from a process which is a member of a background
> process group on a fildes associated with its controlling terminal, shall cause the process
> group to be sent a SIGTTOU signal. If the calling process is blocking or ignoring
> SIGTTOU signals, the process shall be allowed to perform the operation, and
> no signal is sent."
> 
> On non-POSIX systems, the serial_drain_output call doesn't end up in
> ser-unix.c:hardwire_drain_output, so from that perspective, putting
> the SIGTTOU suppression in common code is a bit of an abstraction violation.

Ok, agreed.

Any objections to me pushing the original patch then?


> 
>> 
>> 
>>> 
>>> 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.
>>> 
>> 
>> Minor issue is that once my patch is in, it’ll hide the missing ours_for_output
>> bugs (?)
>> 
> 
> Sure.  But we shouldn't avoid fixing one bug because of that.  The
> palves/tty-always-separate-session branch on my github exposes such
> bugs because with a missing ours_for_output call, gdb prints to
> the screen while the terminal is in raw mode, resulting in output
> like
> 
> this is line one
>                this is line two
>                                this is line three
> 
> instead of:
> 
> this is line one
> this is line two
> this is line three
> 
> Thanks,
> Pedro Alves

Re: [PATCH] Supress SIGTTOU when handling errors

[Pedro Alves]

On 5/24/19 1:36 PM, Alan Hayward wrote:

> Any objections to me pushing the original patch then?

Not from me.

Thanks,
Pedro Alves

Re: [PATCH] Supress SIGTTOU when handling errors

[Andrew Burgess]

* Alan Hayward <Alan.Hayward@arm.com> [2019-05-24 12:36:43 +0000]:

> 
> 
> > On 24 May 2019, at 12:02, Pedro Alves <palves@redhat.com> wrote:
> > 
> > On 5/24/19 9:54 AM, Alan Hayward wrote:
> > 
> >> Looking back at my original patch again, I’m wondering if it’s better to
> >> move the ignore higher up the call stack in print_flush, so that it’s set
> >> across both flushes:
> > 
> > What are the two flushes?  I only see one, from the serial_drain_output call?
> 
> Sorry, I was forgot it was that call. I was thinking it was as part of the
> gdb_flush call.
>  
> > 
> > In any case, I think it's better to keep the SIGTTOU handling close to
> > the tcdrain call, to make to code a lot more obvious -- SIGTTOU suppression
> > is described in tcdrain manuals -- and I don't think we have to worry
> > about efficiency here?
> 
> True - errors from gdb shouldn’t be frequent enough to be an issue.
> 
> > 
> >> ...or if it really should be left just around the tcdrain.
> >> Not quite sure what the behaviour is on non-Linux targets.
> > 
> > The behavior should be the same on all POSIX systems:
> > 
> > https://pubs.opengroup.org/onlinepubs/009695399/functions/tcdrain.html
> > 
> > "Any attempts to use tcdrain() from a process which is a member of a background
> > process group on a fildes associated with its controlling terminal, shall cause the process
> > group to be sent a SIGTTOU signal. If the calling process is blocking or ignoring
> > SIGTTOU signals, the process shall be allowed to perform the operation, and
> > no signal is sent."
> > 
> > On non-POSIX systems, the serial_drain_output call doesn't end up in
> > ser-unix.c:hardwire_drain_output, so from that perspective, putting
> > the SIGTTOU suppression in common code is a bit of an abstraction violation.
> 
> Ok, agreed.
> 
> Any objections to me pushing the original patch then?

Not from me.

Sorry for any delay I caused in getting the patch merged.

Thanks,
Andrew


> 
> 
> > 
> >> 
> >> 
> >>> 
> >>> 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.
> >>> 
> >> 
> >> Minor issue is that once my patch is in, it’ll hide the missing ours_for_output
> >> bugs (?)
> >> 
> > 
> > Sure.  But we shouldn't avoid fixing one bug because of that.  The
> > palves/tty-always-separate-session branch on my github exposes such
> > bugs because with a missing ours_for_output call, gdb prints to
> > the screen while the terminal is in raw mode, resulting in output
> > like
> > 
> > this is line one
> >                this is line two
> >                                this is line three
> > 
> > instead of:
> > 
> > this is line one
> > this is line two
> > this is line three
> > 
> > Thanks,
> > Pedro Alves
> 

Re: [PATCH] Supress SIGTTOU when handling errors

[Pedro Alves]

On 5/26/19 11:42 PM, Andrew Burgess wrote:
> * Alan Hayward <Alan.Hayward@arm.com> [2019-05-24 12:36:43 +0000]:
> 

>> Ok, agreed.
>>
>> Any objections to me pushing the original patch then?
> 
> Not from me.
> 
> Sorry for any delay I caused in getting the patch merged.

Not at all.  It was an interesting discussion and revealed that we
need to clean up and clarify some comments at least.

Thanks,
Pedro Alves

Re: [PATCH] Supress SIGTTOU when handling errors

[Alan Hayward]

> On 27 May 2019, at 19:03, Pedro Alves <palves@redhat.com> wrote:
> 
> On 5/26/19 11:42 PM, Andrew Burgess wrote:
>> * Alan Hayward <Alan.Hayward@arm.com> [2019-05-24 12:36:43 +0000]:
>> 
> 
>>> Ok, agreed.
>>> 
>>> Any objections to me pushing the original patch then?
>> 
>> Not from me.
>> 
>> Sorry for any delay I caused in getting the patch merged.
> 
> Not at all.  It was an interesting discussion and revealed that we
> need to clean up and clarify some comments at least.
> 
> Thanks,
> Pedro Alves

Ditto with what Pedro said, and pushed.

Alan.

[8.3 backport] Supress SIGTTOU when handling errors

[Tom de Vries]

On 28-05-19 11:39, Alan Hayward wrote:
> 
> 
>> On 27 May 2019, at 19:03, Pedro Alves <palves@redhat.com> wrote:
>>
>> On 5/26/19 11:42 PM, Andrew Burgess wrote:
>>> * Alan Hayward <Alan.Hayward@arm.com> [2019-05-24 12:36:43 +0000]:
>>>
>>
>>>> Ok, agreed.
>>>>
>>>> Any objections to me pushing the original patch then?
>>>
>>> Not from me.
>>>
>>> Sorry for any delay I caused in getting the patch merged.
>>
>> Not at all.  It was an interesting discussion and revealed that we
>> need to clean up and clarify some comments at least.
>>
>> Thanks,
>> Pedro Alves
> 
> Ditto with what Pedro said, and pushed.
> 

Hi,

OK to backport to 8.3?

The patch applies cleanly, and causes no regressions on x86_64-linux.

Thanks,
- Tom

Re: [8.3 backport] Supress SIGTTOU when handling errors

[Alan Hayward]

> On 2 Aug 2019, at 17:05, Tom de Vries <tdevries@suse.de> wrote:
> 
> On 28-05-19 11:39, Alan Hayward wrote:
>> 
>> 
>>> On 27 May 2019, at 19:03, Pedro Alves <palves@redhat.com> wrote:
>>> 
>>> On 5/26/19 11:42 PM, Andrew Burgess wrote:
>>>> * Alan Hayward <Alan.Hayward@arm.com> [2019-05-24 12:36:43 +0000]:
>>>> 
>>> 
>>>>> Ok, agreed.
>>>>> 
>>>>> Any objections to me pushing the original patch then?
>>>> 
>>>> Not from me.
>>>> 
>>>> Sorry for any delay I caused in getting the patch merged.
>>> 
>>> Not at all.  It was an interesting discussion and revealed that we
>>> need to clean up and clarify some comments at least.
>>> 
>>> Thanks,
>>> Pedro Alves
>> 
>> Ditto with what Pedro said, and pushed.
>> 
> 
> Hi,
> 
> OK to backport to 8.3?
> 
> The patch applies cleanly, and causes no regressions on x86_64-linux.
> 

I’ve got no issues with this one either, but don’t have the powers to approve.


> Thanks,
> - Tom
> 
> 

Re: [8.3 backport] Supress SIGTTOU when handling errors

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> OK to backport to 8.3?

Tom> The patch applies cleanly, and causes no regressions on x86_64-linux.

This is fine, thanks.

Tom

Re: [PATCH] Supress SIGTTOU when handling errors

[Andreas Schwab]

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.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] Supress SIGTTOU when handling errors

[Andrew Burgess]

* Andreas Schwab <schwab@linux-m68k.org> [2019-05-18 15:41:56 +0200]:

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

Andreas,

Thanks for tracking this down.

It appears that the change in this patch that seems to be responsible
would correspond to Alan's patch #2 option.

I wonder if we should just apply something like the below to revert
part of Pedro's patch?  This will fix this problems we're seeing (as
Alan already pointed out) as this effectively makes 'ours_for_output
()' the same as 'ours ()' again.

My concern would be whether there's going to be some place in GDB that
calls 'ours_for_output ()' and assumes Ctrl-C / Ctrl-Z will be
automatically passed to the inferior.  This change means they are now
passed to GDB instead, will GDB always forward these to the inferior
correctly?

Thanks,
Andrew



--

diff --git a/gdb/inflow.c b/gdb/inflow.c
index 339b55c0bc6..6ed22c14b6b 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -506,10 +506,11 @@ child_terminal_ours_1 (target_terminal_state desired_state)
       /* Set tty state to our_ttystate.  */
       serial_set_tty_state (stdin_serial, our_terminal_info.ttystate);
 
-      /* If we only want output, then leave the inferior's pgrp in the
-	 foreground, so that Ctrl-C/Ctrl-Z reach the inferior
-	 directly.  */
-      if (job_control && desired_state == target_terminal_state::is_ours)
+      /* If might be tempting to think that we can leave the inferior's
+	 pgrp in the foreground if we only want ours_for_output, however,
+	 calls to tcdrain within GDB will result in SIGTTOU unless GDB's
+	 process group is in the foreground.  */
+      if (job_control)
 	{
 #ifdef HAVE_TERMIOS_H
 	  result = tcsetpgrp (0, our_terminal_info.process_group);
@@ -526,7 +527,7 @@ child_terminal_ours_1 (target_terminal_state desired_state)
 #endif /* termios */
 	}
 
-      if (!job_control && desired_state == target_terminal_state::is_ours)
+      if (!job_control)
 	{
 	  signal (SIGINT, sigint_ours);
 #ifdef SIGQUIT

Re: [PATCH] Supress SIGTTOU when handling errors

[Alan Hayward]

> On 19 May 2019, at 23:06, Andrew Burgess <andrew.burgess@embecosm.com> wrote:
> 
> * Andreas Schwab <schwab@linux-m68k.org> [2019-05-18 15:41:56 +0200]:
> 
>> 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.
> 
> Andreas,
> 
> Thanks for tracking this down.

+1

> 
> It appears that the change in this patch that seems to be responsible
> would correspond to Alan's patch #2 option.
> 
> I wonder if we should just apply something like the below to revert
> part of Pedro's patch?  This will fix this problems we're seeing (as
> Alan already pointed out) as this effectively makes 'ours_for_output
> ()' the same as 'ours ()' again.
> 
> My concern would be whether there's going to be some place in GDB that
> calls 'ours_for_output ()' and assumes Ctrl-C / Ctrl-Z will be
> automatically passed to the inferior.  This change means they are now
> passed to GDB instead, will GDB always forward these to the inferior
> correctly?

I’m wary about changing the behaviour of ours_for_output for everyone. With
patch #2 / your version, then it’s making ::ours_for_output meaningless
because it’s just the same as ::ours.

Looking around the code, ::ours_for_output is only(?) used directly before
printing to the terminal. It looks like print_flush is the only case where
output is flushed before printing.

Therefore is this just a edge case? - ours_for_output works fine as long
as you don’t want to flush.

print_flush uses scoped_restore_terminal_state, so that means the terminal
state is restored at the end of the function.

I’m wondering then, if this version is better. Only use ::ours around the
flush, and then switch to ours_for_output for the printing.


diff --git a/gdb/exceptions.c b/gdb/exceptions.c
index ebdc71d98d..d4e3197d21 100644
--- a/gdb/exceptions.c
+++ b/gdb/exceptions.c
@@ -46,7 +46,9 @@ print_flush (void)
   if (current_top_target () != NULL && target_supports_terminal_ours ())
     {
       term_state.emplace ();
-      target_terminal::ours_for_output ();
+      /* Use ::ours instead of ::ours_for_output to prevent a SIGTTOU during the
+        flush.  */
+      target_terminal::ours ();
     }

   /* We want all output to appear now, before we print the error.  We
@@ -70,6 +72,13 @@ print_flush (void)
       serial_un_fdopen (gdb_stdout_serial);
     }

+  /* Now that the output has been flushed, switch to ::ours_for_output.  */
+  if (current_top_target () != NULL && target_supports_terminal_ours ())
+    {
+      term_state.emplace ();
+      target_terminal::ours_for_output ();
+    }
+
   annotate_error_begin ();
 }


> 
> Thanks,
> Andrew
> 
> 
> 
> --
> 
> diff --git a/gdb/inflow.c b/gdb/inflow.c
> index 339b55c0bc6..6ed22c14b6b 100644
> --- a/gdb/inflow.c
> +++ b/gdb/inflow.c
> @@ -506,10 +506,11 @@ child_terminal_ours_1 (target_terminal_state desired_state)
>       /* Set tty state to our_ttystate.  */
>       serial_set_tty_state (stdin_serial, our_terminal_info.ttystate);
> 
> -      /* If we only want output, then leave the inferior's pgrp in the
> -	 foreground, so that Ctrl-C/Ctrl-Z reach the inferior
> -	 directly.  */
> -      if (job_control && desired_state == target_terminal_state::is_ours)
> +      /* If might be tempting to think that we can leave the inferior's
> +	 pgrp in the foreground if we only want ours_for_output, however,
> +	 calls to tcdrain within GDB will result in SIGTTOU unless GDB's
> +	 process group is in the foreground.  */
> +      if (job_control)
> 	{
> #ifdef HAVE_TERMIOS_H
> 	  result = tcsetpgrp (0, our_terminal_info.process_group);
> @@ -526,7 +527,7 @@ child_terminal_ours_1 (target_terminal_state desired_state)
> #endif /* termios */
> 	}
> 
> -      if (!job_control && desired_state == target_terminal_state::is_ours)
> +      if (!job_control)
> 	{
> 	  signal (SIGINT, sigint_ours);
> #ifdef SIGQUIT

Re: [PATCH] Supress SIGTTOU when handling errors

[Andrew Burgess]

* Alan Hayward <Alan.Hayward@arm.com> [2019-05-20 08:44:27 +0000]:

> 
> 
> > On 19 May 2019, at 23:06, Andrew Burgess <andrew.burgess@embecosm.com> wrote:
> > 
> > * Andreas Schwab <schwab@linux-m68k.org> [2019-05-18 15:41:56 +0200]:
> > 
> >> 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.
> > 
> > Andreas,
> > 
> > Thanks for tracking this down.
> 
> +1
> 
> > 
> > It appears that the change in this patch that seems to be responsible
> > would correspond to Alan's patch #2 option.
> > 
> > I wonder if we should just apply something like the below to revert
> > part of Pedro's patch?  This will fix this problems we're seeing (as
> > Alan already pointed out) as this effectively makes 'ours_for_output
> > ()' the same as 'ours ()' again.
> > 
> > My concern would be whether there's going to be some place in GDB that
> > calls 'ours_for_output ()' and assumes Ctrl-C / Ctrl-Z will be
> > automatically passed to the inferior.  This change means they are now
> > passed to GDB instead, will GDB always forward these to the inferior
> > correctly?
> 
> I’m wary about changing the behaviour of ours_for_output for everyone. With
> patch #2 / your version, then it’s making ::ours_for_output meaningless
> because it’s just the same as ::ours.

I share your concern, but...

If you check the comment on 'child_terminal_ours_for_output' you'll
see a little note left from before Pedro's commit e671cd59d74cec9f
which says:

    /* Put some of our terminal settings into effect,
       enough to get proper results from our output,
       but do not change into or out of RAW mode
       so that no input is discarded.

       After doing this, either terminal_ours or terminal_inferior
       should be called to get back to a normal state of affairs.

This next bit is interesting....

       N.B. The implementation is (currently) no different than
       child_terminal_ours.  See child_terminal_ours_1.  */

    void
    child_terminal_ours_for_output (struct target_ops *self)
    {
      child_terminal_ours_1 (1);
    }

So, until Pedro's change 'ours ()' and 'ours_for_output ()' were the
same.  Now that doesn't mean we should go back, but I think it means
I'd be willing to consider it (hence why I originally came our against
it, then changed my mind).

> 
> Looking around the code, ::ours_for_output is only(?) used directly before
> printing to the terminal. It looks like print_flush is the only case where
> output is flushed before printing.
> 
> Therefore is this just a edge case? - ours_for_output works fine as long
> as you don’t want to flush.
> 
> print_flush uses scoped_restore_terminal_state, so that means the terminal
> state is restored at the end of the function.
> 
> I’m wondering then, if this version is better. Only use ::ours around the
> flush, and then switch to ours_for_output for the printing.

Having claimed the terminal with ::ours I don't think there's any need
to switch to ours_for_output.  ours should surely always be a super
set of ours_for_output, and we're going to restore back anyway, so I
think just the first call to ours would be enough.

I'd be just as happy with this approach as with the patch I
suggested.  I'd like Pedro's input given he wrote the original
terminal patch that exposed this issue.

Thanks,
Andrew

> 
> 
> diff --git a/gdb/exceptions.c b/gdb/exceptions.c
> index ebdc71d98d..d4e3197d21 100644
> --- a/gdb/exceptions.c
> +++ b/gdb/exceptions.c
> @@ -46,7 +46,9 @@ print_flush (void)
>    if (current_top_target () != NULL && target_supports_terminal_ours ())
>      {
>        term_state.emplace ();
> -      target_terminal::ours_for_output ();
> +      /* Use ::ours instead of ::ours_for_output to prevent a SIGTTOU during the
> +        flush.  */
> +      target_terminal::ours ();
>      }
> 
>    /* We want all output to appear now, before we print the error.  We
> @@ -70,6 +72,13 @@ print_flush (void)
>        serial_un_fdopen (gdb_stdout_serial);
>      }
> 
> +  /* Now that the output has been flushed, switch to ::ours_for_output.  */
> +  if (current_top_target () != NULL && target_supports_terminal_ours ())
> +    {
> +      term_state.emplace ();
> +      target_terminal::ours_for_output ();
> +    }
> +
>    annotate_error_begin ();
>  }
> 
> 
> > 
> > Thanks,
> > Andrew
> > 
> > 
> > 
> > --
> > 
> > diff --git a/gdb/inflow.c b/gdb/inflow.c
> > index 339b55c0bc6..6ed22c14b6b 100644
> > --- a/gdb/inflow.c
> > +++ b/gdb/inflow.c
> > @@ -506,10 +506,11 @@ child_terminal_ours_1 (target_terminal_state desired_state)
> >       /* Set tty state to our_ttystate.  */
> >       serial_set_tty_state (stdin_serial, our_terminal_info.ttystate);
> > 
> > -      /* If we only want output, then leave the inferior's pgrp in the
> > -	 foreground, so that Ctrl-C/Ctrl-Z reach the inferior
> > -	 directly.  */
> > -      if (job_control && desired_state == target_terminal_state::is_ours)
> > +      /* If might be tempting to think that we can leave the inferior's
> > +	 pgrp in the foreground if we only want ours_for_output, however,
> > +	 calls to tcdrain within GDB will result in SIGTTOU unless GDB's
> > +	 process group is in the foreground.  */
> > +      if (job_control)
> > 	{
> > #ifdef HAVE_TERMIOS_H
> > 	  result = tcsetpgrp (0, our_terminal_info.process_group);
> > @@ -526,7 +527,7 @@ child_terminal_ours_1 (target_terminal_state desired_state)
> > #endif /* termios */
> > 	}
> > 
> > -      if (!job_control && desired_state == target_terminal_state::is_ours)
> > +      if (!job_control)
> > 	{
> > 	  signal (SIGINT, sigint_ours);
> > #ifdef SIGQUIT
> 

Re: [PATCH] Supress SIGTTOU when handling errors

[Pedro Alves]

On 5/20/19 10:11 AM, Andrew Burgess wrote:
> 
> I'd be just as happy with this approach as with the patch I
> suggested.  I'd like Pedro's input given he wrote the original
> terminal patch that exposed this issue.

I'd like a chance to give my input too.  :-)  I'll take a look
as soon as I have a chance.

Thanks,
Pedro Alves

Re: [PATCH] Supress SIGTTOU when handling errors

[Pedro Alves]

On 5/20/19 10:11 AM, Andrew Burgess wrote:
> I share your concern, but...
> 
> If you check the comment on 'child_terminal_ours_for_output' you'll
> see a little note left from before Pedro's commit e671cd59d74cec9f
> which says:
> 
>     /* Put some of our terminal settings into effect,
>        enough to get proper results from our output,
>        but do not change into or out of RAW mode
>        so that no input is discarded.
> 
>        After doing this, either terminal_ours or terminal_inferior
>        should be called to get back to a normal state of affairs.
> 
> This next bit is interesting....
> 
>        N.B. The implementation is (currently) no different than
>        child_terminal_ours.  See child_terminal_ours_1.  */
> 
>     void
>     child_terminal_ours_for_output (struct target_ops *self)
>     {
>       child_terminal_ours_1 (1);
>     }

Yeah.  Whooops, I meant to fix that comment, clearly I forgot...

> 
> So, until Pedro's change 'ours ()' and 'ours_for_output ()' were the
> same.  Now that doesn't mean we should go back, but I think it means
> I'd be willing to consider it (hence why I originally came our against
> it, then changed my mind).
> 

Thanks,
Pedro Alves