diff mbox

Supress SIGTTOU when handling errors

Message ID 3935437B-CD4F-4474-B84A-05859CE822DF@arm.com
State New, archived
Headers

Commit Message

Alan Hayward May 24, 2019, 8:54 a.m. UTC 
  > 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:




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

Comments

Pedro Alves May 24, 2019, 11:02 a.m. UTC | #1 
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
Alan Hayward May 24, 2019, 12:36 p.m. UTC | #2 
> 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
Pedro Alves May 24, 2019, 1:15 p.m. UTC | #3 
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
Andrew Burgess May 26, 2019, 10:42 p.m. UTC | #4 
* 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
>
Pedro Alves May 27, 2019, 6:03 p.m. UTC | #5 
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
Alan Hayward May 28, 2019, 9:39 a.m. UTC | #6 
> 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.
Tom de Vries Aug. 2, 2019, 4:05 p.m. UTC | #7 
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
Alan Hayward Aug. 5, 2019, 10:58 a.m. UTC | #8 
> 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

> 

>
Tom Tromey Aug. 5, 2019, 5:32 p.m. UTC | #9 
>>>>> "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
diff mbox

Patch

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);
     }