Re: [PATCH] Supress SIGTTOU when handling errors

[Pedro Alves <palves@redhat.com>]

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