[PATCH] switch away from using specify_exec_file_hook

[PATCH] switch away from using specify_exec_file_hook

[Andrew Burgess]

I had a go at testing this patch using make check-gdb
TESTS="gdb.gdbtk/*.exp", but ran into numerous errors, I suspect that
the gdbtk testing has not been run in a while, and it certainly
doesn't appear to have kept up with changes to the GDB testsuite.

If there's any particular testing that I should be running then just
let me know.

Otherwise, if we could merge this patch, then we can remove a couple
of functions from GDB that are only used to support this one hook.

---

Stop using specify_exec_file_hook to spot when the executable GDB is
debugging changes, and instead use gdb::observers::executable_changed.

The specify_exec_file_hook would call a function with the name of the
executable as specified by the user, so long as the name of the
executable wasn't empty, e.g.:

  (gdb) file exec_name

would call the hook with 'exec_name', while:

  (gdb) file

would not call the hook at all.

The observer I'm proposing we switch too is slightly different.  The
observer is called with the program_space in which the executable
changed, even in the empty filename case.  The observer also receives
a flag which indicates if GDB is loading a new executable, or is
loading the same executable because the on-disk file changed.

To reduce the chance of breakage, within insight, if the new observer
function is called in the empty filename case I don't call into the
TCL code, this ensures the TCL code is only called in the same cases a
previously.

The TCL code will be called with the full path to the executable
rather than the partial string the user specified, however, the TCL
code would (in some cases at least) look up the full executable path
anyway, so I don't think the change should have a negative impact.

For now I'm ignoring the reload_p flag, that is, I call into the TCL
code both when the executable has completely changed, and when it's
the same executable filename, but the on-disk file changed, I think
this matches the behaviour before this patch.

And finally, while looking at the TCL code, I spotted that a comment
in interface.tcl relating to this code was slightly out of date, I've
updated the comment.
---
 gdbtk/generic/gdbtk-hooks.c | 16 +++++++++++-----
 gdbtk/library/interface.tcl |  2 +-
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/gdbtk/generic/gdbtk-hooks.c b/gdbtk/generic/gdbtk-hooks.c
index 75e89e7..fe42e7d 100644
--- a/gdbtk/generic/gdbtk-hooks.c
+++ b/gdbtk/generic/gdbtk-hooks.c
@@ -83,7 +83,7 @@ static void gdbtk_trace_start_stop (int, int);
 static void gdbtk_attach (void);
 static void gdbtk_detach (void);
 static void gdbtk_file_changed (const char *);
-static void gdbtk_exec_file_display (const char *);
+static void gdbtk_executable_changed (program_space *, bool);
 static void gdbtk_call_command (struct cmd_list_element *, const char *, int);
 static void gdbtk_target_pre_wait (ptid_t ptid);
 static void gdbtk_target_post_wait (ptid_t event_ptid);
@@ -140,6 +140,8 @@ gdbtk_add_hooks (void)
                                           "gdbtk-hooks");
   gdb::observers::target_post_wait.attach (gdbtk_target_post_wait,
                                            "gdbtk-hooks");
+  gdb::observers::executable_changed.attach (gdbtk_executable_changed,
+                                             "gdbtk-hooks");
 
   /* Hooks */
   deprecated_call_command_hook = gdbtk_call_command;
@@ -157,7 +159,6 @@ gdbtk_add_hooks (void)
   deprecated_pre_add_symbol_hook = gdbtk_pre_add_symbol;
   deprecated_post_add_symbol_hook = gdbtk_post_add_symbol;
   deprecated_file_changed_hook = gdbtk_file_changed;
-  specify_exec_file_hook (gdbtk_exec_file_display);
 
   deprecated_attach_hook            = gdbtk_attach;
   deprecated_detach_hook            = gdbtk_detach;
@@ -732,11 +733,16 @@ gdbtk_file_changed (const char *filename)
   gdbtk_two_elem_cmd ("gdbtk_tcl_file_changed", filename);
 }
 
-/* Called from exec_file_command */
+/* Called from exec_file_attach when the executable changes.  PSPACE is
+   the program space in which the executable was changed.  RELOAD_P is
+   true if the executable didn't actually change, but instead GDB detected
+   that the on-disk file change, so reloaded the current executable.  */
 static void
-gdbtk_exec_file_display (const char *filename)
+gdbtk_executable_changed (program_space *pspace, bool reload_p)
 {
-  gdbtk_two_elem_cmd ("gdbtk_tcl_exec_file_display", filename);
+  if (pspace->exec_filename.get () != nullptr)
+    gdbtk_two_elem_cmd ("gdbtk_tcl_exec_file_display",
+                        pspace->exec_filename.get ());
 }
 
 /* Called from error_begin, this hook is used to warn the gui
diff --git a/gdbtk/library/interface.tcl b/gdbtk/library/interface.tcl
index c69e2f0..da0bcae 100644
--- a/gdbtk/library/interface.tcl
+++ b/gdbtk/library/interface.tcl
@@ -798,7 +798,7 @@ proc gdbtk_tcl_file_changed {filename} {
 
 # ------------------------------------------------------------------
 #  PROCEDURE: gdbtk_tcl_exec_file_display
-#         This hook is called from exec_file_command. It's purpose
+#         This hook is called from exec_file_attach. It's purpose
 #         is to setup the gui for a new file. Note that we cannot
 #         look for main, since this hook is called BEFORE we
 #         read symbols. If the user used the "file" command,

base-commit: 0b35bf269b3625d05764ebaeec45b32a2ac03321
-- 
2.25.4

Ping: Re: [PATCH] switch away from using specify_exec_file_hook

[Andrew Burgess]

Ping!

Thanks,
Andrew

Andrew Burgess <aburgess@redhat.com> writes:

> I had a go at testing this patch using make check-gdb
> TESTS="gdb.gdbtk/*.exp", but ran into numerous errors, I suspect that
> the gdbtk testing has not been run in a while, and it certainly
> doesn't appear to have kept up with changes to the GDB testsuite.
>
> If there's any particular testing that I should be running then just
> let me know.
>
> Otherwise, if we could merge this patch, then we can remove a couple
> of functions from GDB that are only used to support this one hook.
>
> ---
>
> Stop using specify_exec_file_hook to spot when the executable GDB is
> debugging changes, and instead use gdb::observers::executable_changed.
>
> The specify_exec_file_hook would call a function with the name of the
> executable as specified by the user, so long as the name of the
> executable wasn't empty, e.g.:
>
>   (gdb) file exec_name
>
> would call the hook with 'exec_name', while:
>
>   (gdb) file
>
> would not call the hook at all.
>
> The observer I'm proposing we switch too is slightly different.  The
> observer is called with the program_space in which the executable
> changed, even in the empty filename case.  The observer also receives
> a flag which indicates if GDB is loading a new executable, or is
> loading the same executable because the on-disk file changed.
>
> To reduce the chance of breakage, within insight, if the new observer
> function is called in the empty filename case I don't call into the
> TCL code, this ensures the TCL code is only called in the same cases a
> previously.
>
> The TCL code will be called with the full path to the executable
> rather than the partial string the user specified, however, the TCL
> code would (in some cases at least) look up the full executable path
> anyway, so I don't think the change should have a negative impact.
>
> For now I'm ignoring the reload_p flag, that is, I call into the TCL
> code both when the executable has completely changed, and when it's
> the same executable filename, but the on-disk file changed, I think
> this matches the behaviour before this patch.
>
> And finally, while looking at the TCL code, I spotted that a comment
> in interface.tcl relating to this code was slightly out of date, I've
> updated the comment.
> ---
>  gdbtk/generic/gdbtk-hooks.c | 16 +++++++++++-----
>  gdbtk/library/interface.tcl |  2 +-
>  2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/gdbtk/generic/gdbtk-hooks.c b/gdbtk/generic/gdbtk-hooks.c
> index 75e89e7..fe42e7d 100644
> --- a/gdbtk/generic/gdbtk-hooks.c
> +++ b/gdbtk/generic/gdbtk-hooks.c
> @@ -83,7 +83,7 @@ static void gdbtk_trace_start_stop (int, int);
>  static void gdbtk_attach (void);
>  static void gdbtk_detach (void);
>  static void gdbtk_file_changed (const char *);
> -static void gdbtk_exec_file_display (const char *);
> +static void gdbtk_executable_changed (program_space *, bool);
>  static void gdbtk_call_command (struct cmd_list_element *, const char *, int);
>  static void gdbtk_target_pre_wait (ptid_t ptid);
>  static void gdbtk_target_post_wait (ptid_t event_ptid);
> @@ -140,6 +140,8 @@ gdbtk_add_hooks (void)
>                                            "gdbtk-hooks");
>    gdb::observers::target_post_wait.attach (gdbtk_target_post_wait,
>                                             "gdbtk-hooks");
> +  gdb::observers::executable_changed.attach (gdbtk_executable_changed,
> +                                             "gdbtk-hooks");
>  
>    /* Hooks */
>    deprecated_call_command_hook = gdbtk_call_command;
> @@ -157,7 +159,6 @@ gdbtk_add_hooks (void)
>    deprecated_pre_add_symbol_hook = gdbtk_pre_add_symbol;
>    deprecated_post_add_symbol_hook = gdbtk_post_add_symbol;
>    deprecated_file_changed_hook = gdbtk_file_changed;
> -  specify_exec_file_hook (gdbtk_exec_file_display);
>  
>    deprecated_attach_hook            = gdbtk_attach;
>    deprecated_detach_hook            = gdbtk_detach;
> @@ -732,11 +733,16 @@ gdbtk_file_changed (const char *filename)
>    gdbtk_two_elem_cmd ("gdbtk_tcl_file_changed", filename);
>  }
>  
> -/* Called from exec_file_command */
> +/* Called from exec_file_attach when the executable changes.  PSPACE is
> +   the program space in which the executable was changed.  RELOAD_P is
> +   true if the executable didn't actually change, but instead GDB detected
> +   that the on-disk file change, so reloaded the current executable.  */
>  static void
> -gdbtk_exec_file_display (const char *filename)
> +gdbtk_executable_changed (program_space *pspace, bool reload_p)
>  {
> -  gdbtk_two_elem_cmd ("gdbtk_tcl_exec_file_display", filename);
> +  if (pspace->exec_filename.get () != nullptr)
> +    gdbtk_two_elem_cmd ("gdbtk_tcl_exec_file_display",
> +                        pspace->exec_filename.get ());
>  }
>  
>  /* Called from error_begin, this hook is used to warn the gui
> diff --git a/gdbtk/library/interface.tcl b/gdbtk/library/interface.tcl
> index c69e2f0..da0bcae 100644
> --- a/gdbtk/library/interface.tcl
> +++ b/gdbtk/library/interface.tcl
> @@ -798,7 +798,7 @@ proc gdbtk_tcl_file_changed {filename} {
>  
>  # ------------------------------------------------------------------
>  #  PROCEDURE: gdbtk_tcl_exec_file_display
> -#         This hook is called from exec_file_command. It's purpose
> +#         This hook is called from exec_file_attach. It's purpose
>  #         is to setup the gui for a new file. Note that we cannot
>  #         look for main, since this hook is called BEFORE we
>  #         read symbols. If the user used the "file" command,
>
> base-commit: 0b35bf269b3625d05764ebaeec45b32a2ac03321
> -- 
> 2.25.4