[PATCH] Remove use of deprecated_init_ui_hook from quit_confirm.

[PATCH] Remove use of deprecated_init_ui_hook from quit_confirm.

[Andrew Burgess]

There are only two places that deprecated_init_ui_hook is set
(to !NULL) that I can find:

 1. In gdbtk, deprecated_init_ui_hook is used to grab a copy of
    argv0, but is then immediately set back to NULL, and

 2. In windows-nat.c, deprecated_init_ui_hook is used to solve
    an order of initialisation problem when creating a command
    alias, in this case deprecated_init_ui_hook is left set.

In top.c:quit_confirm we check deprecated_init_ui_hook to detect if
there's a GUI running.  For (1) above this will not kick in, but
for (2) it does.... however... I don't see why this is a good thing,
as I understand it the windows-nat.c code is not a GUI frontend for
gdb, but is just "running-gdb-on-windows-hosts".  I find it hard to
believe that the shorter, less informative, quit message is really
desired... but maybe I've missed something.

The following patch removes the use of deprecated_init_ui_hook
from quit_confirm, the only change I expect from this is that the
quit message on windows hosts will fall into line with other hosts.

OK to apply?

Thanks,
Andrew

gdb/ChangeLog

2013-09-06  Andrew Burgess  <aburgess@broadcom.com>

	* top.c (quit_confirm): Remove use of deprecated_init_ui_hook.

diff --git a/gdb/top.c b/gdb/top.c
index b3e7d37..d9128a3 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1355,18 +1355,9 @@ quit_confirm (void)
   stb = mem_fileopen ();
   old_chain = make_cleanup_ui_file_delete (stb);
 
-  /* This is something of a hack.  But there's no reliable way to see
-     if a GUI is running.  The `use_windows' variable doesn't cut
-     it.  */
-  if (deprecated_init_ui_hook)
-    fprintf_filtered (stb, _("A debugging session is active.\n"
-			     "Do you still want to close the debugger?"));
-  else
-    {
-      fprintf_filtered (stb, _("A debugging session is active.\n\n"));
-      iterate_over_inferiors (print_inferior_quit_action, stb);
-      fprintf_filtered (stb, _("\nQuit anyway? "));
-    }
+  fprintf_filtered (stb, _("A debugging session is active.\n\n"));
+  iterate_over_inferiors (print_inferior_quit_action, stb);
+  fprintf_filtered (stb, _("\nQuit anyway? "));
 
   str = ui_file_xstrdup (stb, NULL);
   make_cleanup (xfree, str);

Re: [PATCH] Remove use of deprecated_init_ui_hook from quit_confirm.

[Eli Zaretskii]

> Date: Fri, 6 Sep 2013 13:29:24 +0100
> From: "Andrew Burgess" <aburgess@broadcom.com>
> 
> There are only two places that deprecated_init_ui_hook is set
> (to !NULL) that I can find:
> 
>  1. In gdbtk, deprecated_init_ui_hook is used to grab a copy of
>     argv0, but is then immediately set back to NULL, and
> 
>  2. In windows-nat.c, deprecated_init_ui_hook is used to solve
>     an order of initialisation problem when creating a command
>     alias, in this case deprecated_init_ui_hook is left set.
> 
> In top.c:quit_confirm we check deprecated_init_ui_hook to detect if
> there's a GUI running.  For (1) above this will not kick in, but
> for (2) it does.... however... I don't see why this is a good thing,
> as I understand it the windows-nat.c code is not a GUI frontend for
> gdb, but is just "running-gdb-on-windows-hosts".  I find it hard to
> believe that the shorter, less informative, quit message is really
> desired... but maybe I've missed something.
> 
> The following patch removes the use of deprecated_init_ui_hook
> from quit_confirm, the only change I expect from this is that the
> quit message on windows hosts will fall into line with other hosts.

What situation would trigger the different quit message on Windows?
In GDB 7.6.1 built as MinGW native debugger for Windows, I only see
the "normal" quit message, viz.:

  (gdb) q
  A debugging session is active.

	  Inferior 1 [process 628] will be killed.

  Quit anyway? (y or n) y

So it looks like the allegedly Windows-specific quit message is never
used, although deprecated_init_ui_hook is non-NULL.  What am I
missing?

> OK to apply?

I'd suggest to wait until we fully understand the situation with this
hook.

Thanks.

Re: [PATCH] Remove use of deprecated_init_ui_hook from quit_confirm.

[Pedro Alves]

On 09/06/2013 02:25 PM, Eli Zaretskii wrote:

> So it looks like the allegedly Windows-specific quit message is never
> used, although deprecated_init_ui_hook is non-NULL.  What am I
> missing?

gdb_init is called early, and calls the _initialize routines, and also
does:

  /* Allow another UI to initialize.  If the UI fails to initialize,
     and it wants GDB to revert to the CLI, it should clear
     deprecated_init_ui_hook.  */
  if (deprecated_init_ui_hook)
    deprecated_init_ui_hook (argv0);

And then afterwards, clear_interpreter_hooks is called:

#0  clear_interpreter_hooks () at ../../src/gdb/interps.c:371
#1  0x00000000005e4d1d in interp_set (interp=0xda7530, top_level=1) at ../../src/gdb/interps.c:195
#2  0x00000000005e68a0 in captured_main (data=0x7fffffffda50) at ../../src/gdb/main.c:868
#3  0x00000000005e34a2 in catch_errors (func=0x5e5d1e <captured_main>, func_args=0x7fffffffda50, errstring=0x8a5c54 "", mask=RETURN_MASK_ALL)
    at ../../src/gdb/exceptions.c:524
#4  0x00000000005e6eda in gdb_main (args=0x7fffffffda50) at ../../src/gdb/main.c:1069
#5  0x000000000045afba in main (argc=1, argv=0x7fffffffdb58) at ../../src/gdb/gdb.c:34

And that clears the hook.

-- 
Pedro Alves

Re: [PATCH] Remove use of deprecated_init_ui_hook from quit_confirm.

[Eli Zaretskii]

> Date: Fri, 06 Sep 2013 14:44:08 +0100
> From: Pedro Alves <palves@redhat.com>
> CC: Andrew Burgess <aburgess@broadcom.com>, gdb-patches@sourceware.org
> 
> On 09/06/2013 02:25 PM, Eli Zaretskii wrote:
> 
> > So it looks like the allegedly Windows-specific quit message is never
> > used, although deprecated_init_ui_hook is non-NULL.  What am I
> > missing?
> 
> gdb_init is called early, and calls the _initialize routines, and also
> does:
> 
>   /* Allow another UI to initialize.  If the UI fails to initialize,
>      and it wants GDB to revert to the CLI, it should clear
>      deprecated_init_ui_hook.  */
>   if (deprecated_init_ui_hook)
>     deprecated_init_ui_hook (argv0);
> 
> And then afterwards, clear_interpreter_hooks is called:
> 
> #0  clear_interpreter_hooks () at ../../src/gdb/interps.c:371
> #1  0x00000000005e4d1d in interp_set (interp=0xda7530, top_level=1) at ../../src/gdb/interps.c:195
> #2  0x00000000005e68a0 in captured_main (data=0x7fffffffda50) at ../../src/gdb/main.c:868
> #3  0x00000000005e34a2 in catch_errors (func=0x5e5d1e <captured_main>, func_args=0x7fffffffda50, errstring=0x8a5c54 "", mask=RETURN_MASK_ALL)
>     at ../../src/gdb/exceptions.c:524
> #4  0x00000000005e6eda in gdb_main (args=0x7fffffffda50) at ../../src/gdb/main.c:1069
> #5  0x000000000045afba in main (argc=1, argv=0x7fffffffdb58) at ../../src/gdb/gdb.c:34
> 
> And that clears the hook.

Thanks.  So that code in top.c is indeed never exercised on Windows,
right?

Re: [PATCH] Remove use of deprecated_init_ui_hook from quit_confirm.

[Pedro Alves]

On 09/06/2013 02:50 PM, Eli Zaretskii wrote:

> Thanks.  So that code in top.c is indeed never exercised on Windows,
> right?

That's what it looks like to me from reading the code.

-- 
Pedro Alves

Re: [PATCH] Remove use of deprecated_init_ui_hook from quit_confirm.

[Pedro Alves]

On 09/06/2013 01:29 PM, Andrew Burgess wrote:
> There are only two places that deprecated_init_ui_hook is set
> (to !NULL) that I can find:
> 
>  1. In gdbtk, deprecated_init_ui_hook is used to grab a copy of
>     argv0, but is then immediately set back to NULL, and

Then, was that setting back to NULL a recent change that had
that side effect and nobody noticed?

Hmm, it's been that way for 10 years:

 https://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/gdbtk/generic/gdbtk.c?rev=1.36&content-type=text/x-cvsweb-markup&cvsroot=src
 https://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/gdbtk/generic/gdbtk.c.diff?r1=1.35&r2=1.36&cvsroot=src&f=h

Before, it used to be only cleared if not displaying a GUI, like:

static void
gdbtk_init (char *argv0)
{
...
  /* If there is no DISPLAY environment variable, Tk_Init below will fail,
     causing gdb to abort.  If instead we simply return here, gdb will
     gracefully degrade to using the command line interface. */

#ifndef _WIN32
  if (getenv ("DISPLAY") == NULL)
    {
      init_ui_hook = NULL;
      return;
    }
#endif

Guess if nobody complained thus far, people are fine with the
alternative text...

>  2. In windows-nat.c, deprecated_init_ui_hook is used to solve
>     an order of initialisation problem when creating a command
>     alias, in this case deprecated_init_ui_hook is left set.
> 
> In top.c:quit_confirm we check deprecated_init_ui_hook to detect if
> there's a GUI running.  For (1) above this will not kick in, but


> for (2) it does.... however... I don't see why this is a good thing,
> as I understand it the windows-nat.c code is not a GUI frontend for
> gdb, but is just "running-gdb-on-windows-hosts".  I find it hard to
> believe that the shorter, less informative, quit message is really
> desired... but maybe I've missed something.

Right.  That very much just looks like an unintentional side effect...

> The following patch removes the use of deprecated_init_ui_hook
> from quit_confirm, the only change I expect from this is that the
> quit message on windows hosts will fall into line with other hosts.
> 
> OK to apply?
> 

Looks fine to me.

-- 
Pedro Alves

Re: [PATCH] Remove use of deprecated_init_ui_hook from quit_confirm.

[Andrew Burgess]

On 06/09/2013 2:34 PM, Pedro Alves wrote:
> On 09/06/2013 01:29 PM, Andrew Burgess wrote:
>> There are only two places that deprecated_init_ui_hook is set
>> (to !NULL) that I can find:
>>
>>  1. In gdbtk, deprecated_init_ui_hook is used to grab a copy of
>>     argv0, but is then immediately set back to NULL, and
> 
> Then, was that setting back to NULL a recent change that had
> that side effect and nobody noticed?
> 
> Hmm, it's been that way for 10 years:
> 
>  https://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/gdbtk/generic/gdbtk.c?rev=1.36&content-type=text/x-cvsweb-markup&cvsroot=src
>  https://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/gdbtk/generic/gdbtk.c.diff?r1=1.35&r2=1.36&cvsroot=src&f=h
> 
> Before, it used to be only cleared if not displaying a GUI, like:
> 
> static void
> gdbtk_init (char *argv0)
> {
> ...
>   /* If there is no DISPLAY environment variable, Tk_Init below will fail,
>      causing gdb to abort.  If instead we simply return here, gdb will
>      gracefully degrade to using the command line interface. */
> 
> #ifndef _WIN32
>   if (getenv ("DISPLAY") == NULL)
>     {
>       init_ui_hook = NULL;
>       return;
>     }
> #endif
> 
> Guess if nobody complained thus far, people are fine with the
> alternative text...
> 
>>  2. In windows-nat.c, deprecated_init_ui_hook is used to solve
>>     an order of initialisation problem when creating a command
>>     alias, in this case deprecated_init_ui_hook is left set.
>>
>> In top.c:quit_confirm we check deprecated_init_ui_hook to detect if
>> there's a GUI running.  For (1) above this will not kick in, but
> 
> 
>> for (2) it does.... however... I don't see why this is a good thing,
>> as I understand it the windows-nat.c code is not a GUI frontend for
>> gdb, but is just "running-gdb-on-windows-hosts".  I find it hard to
>> believe that the shorter, less informative, quit message is really
>> desired... but maybe I've missed something.
> 
> Right.  That very much just looks like an unintentional side effect...
> 
>> The following patch removes the use of deprecated_init_ui_hook
>> from quit_confirm, the only change I expect from this is that the
>> quit message on windows hosts will fall into line with other hosts.
>>
>> OK to apply?
>>
> 
> Looks fine to me.
> 

Given that Pedro kindly addressed the issue raised by Eli (in another
mail), I've committed this patch.

Thanks all,
Andrew