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: 6267 bytes --]



> 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

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