[PATCH] Call target_terminal_ours in quit_force

[PATCH] Call target_terminal_ours in quit_force

[Patrick Palka]

We should make sure our terminal settings are in effect before finally
quitting GDB.  Our terminal settings may not be in effect at this point
if we are e.g. quitting due to a SIGTERM.

gdb/ChangeLog:

	* top.c (quit_force): Call target_terminal_ours.
---
 gdb/top.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gdb/top.c b/gdb/top.c
index 1e30b1c..1a31194 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1494,6 +1494,8 @@ quit_force (char *args, int from_tty)
   int exit_code = 0;
   struct qt_args qt;
 
+  target_terminal_ours ();
+
   /* An optional expression may be used to cause gdb to terminate with the 
      value of that expression.  */
   if (args)
-- 
2.5.0.rc2.22.g69b1679.dirty

Re: [PATCH] Call target_terminal_ours in quit_force

[Patrick Palka]

On Mon, Jul 27, 2015 at 12:08 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> We should make sure our terminal settings are in effect before finally
> quitting GDB.  Our terminal settings may not be in effect at this point
> if we are e.g. quitting due to a SIGTERM.

I should add, "quitting due to a SIGTERM while an inferior an inferior
is running in the foreground."

Re: [PATCH] Call target_terminal_ours in quit_force

[Andreas Schwab]

Patrick Palka <patrick@parcs.ath.cx> writes:

> On Mon, Jul 27, 2015 at 12:08 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>> We should make sure our terminal settings are in effect before finally
>> quitting GDB.  Our terminal settings may not be in effect at this point
>> if we are e.g. quitting due to a SIGTERM.
>
> I should add, "quitting due to a SIGTERM while an inferior an inferior

s/an inferior an inferior/an inferior/

> is running in the foreground."

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Call target_terminal_ours in quit_force

[Pedro Alves]

On 07/27/2015 05:11 PM, Patrick Palka wrote:
> On Mon, Jul 27, 2015 at 12:08 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>> We should make sure our terminal settings are in effect before finally
>> quitting GDB.  Our terminal settings may not be in effect at this point
>> if we are e.g. quitting due to a SIGTERM.
> 
> I should add, "quitting due to a SIGTERM while an inferior an inferior
> is running in the foreground."

Looks OK, though I notice that the settings are broken even if we
we're not debugging anything:

$ stty
speed 38400 baud; line = 0;
iutf8

$ ./gdb
GNU gdb (GDB) 7.10.50.20150726-cvs
(gdb)
*sent SIGTERM from another terminal, gdb exits*
$
$ stty (echo is off)
speed 38400 baud; line = 0;
lnext = <undef>; min = 1; time = 0;
-icrnl iutf8
-icanon -echo
$

Do you also see this?

Can I convince you to add a test?  :-)  You wouldn't have to write
it from scratch; we already have a test that checks that terminal
settings are preserved:
 gdb.base/batch-preserve-term-settings.exp

Thanks,
Pedro Alves

Re: [PATCH] Call target_terminal_ours in quit_force

[Patrick Palka]

On Mon, Jul 27, 2015 at 12:39 PM, Pedro Alves <palves@redhat.com> wrote:
> On 07/27/2015 05:11 PM, Patrick Palka wrote:
>> On Mon, Jul 27, 2015 at 12:08 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>>> We should make sure our terminal settings are in effect before finally
>>> quitting GDB.  Our terminal settings may not be in effect at this point
>>> if we are e.g. quitting due to a SIGTERM.
>>
>> I should add, "quitting due to a SIGTERM while an inferior an inferior
>> is running in the foreground."
>
> Looks OK, though I notice that the settings are broken even if we
> we're not debugging anything:
>
> $ stty
> speed 38400 baud; line = 0;
> iutf8
>
> $ ./gdb
> GNU gdb (GDB) 7.10.50.20150726-cvs
> (gdb)
> *sent SIGTERM from another terminal, gdb exits*
> $
> $ stty (echo is off)
> speed 38400 baud; line = 0;
> lnext = <undef>; min = 1; time = 0;
> -icrnl iutf8
> -icanon -echo
> $
>
> Do you also see this?

Yeah, even with this patch...

$ stty
speed 38400 baud; line = 0;
-brkint -imaxbel iutf8
$ gdb -q
(gdb) *SIGTERM*
$ stty
speed 38400 baud; line = 0;
lnext = <undef>;
-brkint -icrnl -imaxbel iutf8

Quitting via the "quit" command is OK though... strange.

Re: [PATCH] Call target_terminal_ours in quit_force

[Patrick Palka]

On Mon, Jul 27, 2015 at 2:49 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Mon, Jul 27, 2015 at 12:39 PM, Pedro Alves <palves@redhat.com> wrote:
>> On 07/27/2015 05:11 PM, Patrick Palka wrote:
>>> On Mon, Jul 27, 2015 at 12:08 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>>>> We should make sure our terminal settings are in effect before finally
>>>> quitting GDB.  Our terminal settings may not be in effect at this point
>>>> if we are e.g. quitting due to a SIGTERM.
>>>
>>> I should add, "quitting due to a SIGTERM while an inferior an inferior
>>> is running in the foreground."
>>
>> Looks OK, though I notice that the settings are broken even if we
>> we're not debugging anything:
>>
>> $ stty
>> speed 38400 baud; line = 0;
>> iutf8
>>
>> $ ./gdb
>> GNU gdb (GDB) 7.10.50.20150726-cvs
>> (gdb)
>> *sent SIGTERM from another terminal, gdb exits*
>> $
>> $ stty (echo is off)
>> speed 38400 baud; line = 0;
>> lnext = <undef>; min = 1; time = 0;
>> -icrnl iutf8
>> -icanon -echo
>> $
>>
>> Do you also see this?
>
> Yeah, even with this patch...
>
> $ stty
> speed 38400 baud; line = 0;
> -brkint -imaxbel iutf8
> $ gdb -q
> (gdb) *SIGTERM*
> $ stty
> speed 38400 baud; line = 0;
> lnext = <undef>;
> -brkint -icrnl -imaxbel iutf8
>
> Quitting via the "quit" command is OK though... strange.

This happens because when quitting via SIGTERM a readline callback
handler remains installed which means that the terminal is still
prepped by readline.  The readline callback handler is temporarily
removed during the execution of a command (thus deprepping the
terminal) which is why quitting via "quit" does not leak our terminal
settings.

Re: [PATCH] Call target_terminal_ours in quit_force

[Pedro Alves]

On 07/27/2015 08:12 PM, Patrick Palka wrote:
> On Mon, Jul 27, 2015 at 2:49 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>> On Mon, Jul 27, 2015 at 12:39 PM, Pedro Alves <palves@redhat.com> wrote:
>>> On 07/27/2015 05:11 PM, Patrick Palka wrote:
>>>> On Mon, Jul 27, 2015 at 12:08 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>>>>> We should make sure our terminal settings are in effect before finally
>>>>> quitting GDB.  Our terminal settings may not be in effect at this point
>>>>> if we are e.g. quitting due to a SIGTERM.
>>>>
>>>> I should add, "quitting due to a SIGTERM while an inferior an inferior
>>>> is running in the foreground."
>>>
>>> Looks OK, though I notice that the settings are broken even if we
>>> we're not debugging anything:
>>>
>>> $ stty
>>> speed 38400 baud; line = 0;
>>> iutf8
>>>
>>> $ ./gdb
>>> GNU gdb (GDB) 7.10.50.20150726-cvs
>>> (gdb)
>>> *sent SIGTERM from another terminal, gdb exits*
>>> $
>>> $ stty (echo is off)
>>> speed 38400 baud; line = 0;
>>> lnext = <undef>; min = 1; time = 0;
>>> -icrnl iutf8
>>> -icanon -echo
>>> $
>>>
>>> Do you also see this?
>>
>> Yeah, even with this patch...
>>
>> $ stty
>> speed 38400 baud; line = 0;
>> -brkint -imaxbel iutf8
>> $ gdb -q
>> (gdb) *SIGTERM*
>> $ stty
>> speed 38400 baud; line = 0;
>> lnext = <undef>;
>> -brkint -icrnl -imaxbel iutf8
>>
>> Quitting via the "quit" command is OK though... strange.
> 
> This happens because when quitting via SIGTERM a readline callback
> handler remains installed which means that the terminal is still
> prepped by readline.  The readline callback handler is temporarily
> removed during the execution of a command (thus deprepping the
> terminal) which is why quitting via "quit" does not leak our terminal
> settings.

Yeah.  Sounds like we should call gdb_rl_callback_handler_remove.
And remove the input fd from the event loop too, otherwise pending input
may end up waking some nested event loop and end up in readline with
no callback installed, which aborts.   Looks like gdb_disable_readline
would do?

Thanks,
Pedro Alves