Re: [PATCH] Supress SIGTTOU when handling errors

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

> 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