Re: [PATCH] Supress SIGTTOU when handling errors

[Alan Hayward <Alan.Hayward@arm.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4998 bytes --]



> 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

\x16º&Öéj×!zÊÞ¶êçמ¶ÛÙb²Ö«r\x18\x1dn–­r\x17¬