Supress SIGTTOU when handling errors

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



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

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

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
  

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