[PATCH 1/3] Correctly initialize the TUI locator window

[PATCH 1/3] Correctly initialize the TUI locator window

[Patrick Palka]

The call to tui_alloc_content in tui_set_locator_info passes
locator->type as the type of the window whose content is being
allocated.  This may seem correct but it's actually not because when
this code path actually get executed locator->type has not yet been to
set LOCATOR_WIN so it defaults to 0 i.e. SRC_WIN.  Thus we allocate the
content of the locator window as if it was the source window.  This
oversight turns out not to be a big deal in practice but the patch that
follows depends on the locator's proc_name and full_name arrays to be
initialized to the empty string which is done by tui_alloc_content if
we pass to it LOCATOR_WIN.

This patch fixes this bug by explicitly passing LOCATOR_WIN to
tui_alloc_content.

gdb/ChangeLog:

	* tui/tui-stack.c (tui_set_locator_info): Explicitly pass
	LOCATOR_WIN to tui_alloc_content.
---
 gdb/tui/tui-stack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/tui/tui-stack.c b/gdb/tui/tui-stack.c
index 2870d70..b17d303 100644
--- a/gdb/tui/tui-stack.c
+++ b/gdb/tui/tui-stack.c
@@ -306,7 +306,7 @@ tui_set_locator_info (struct gdbarch *gdbarch,
   /* Allocate the locator content if necessary.  */
   if (locator->content_size <= 0)
     {
-      locator->content = tui_alloc_content (1, locator->type);
+      locator->content = tui_alloc_content (1, LOCATOR_WIN);
       locator->content_size = 1;
     }
 
-- 
2.5.0.rc0.5.g91e10c5.dirty

[PATCH 3/3] Replace TUI's select_frame hook (PR tui/13378)

[Patrick Palka]

The select_frame hook is used by TUI to update TUI's frame and register
information following changes to the selected frame.  The problem with
this hook is that it gets called after every single frame change, even
if the frame change is only temporary or internal.  This is the primary
cause of flickering and slowdown when running the inferior under TUI
with conditional breakpoints set.  Internal GDB events are the source of
many calls to select_frame and these internal events are triggered
frequently, especially when a few conditional breakpoints are set.

This patch removes the select_frame hook altogether and instead makes
the frame and register information get updated in two key places (using
observers): after an inferior stops, and right before displaying a
prompt.  The latter hook covers the case when frame information must be
updated following a call to "up", "down", "run", "continue" etc and the
former covers the case when frame information must be updated after the
inferior stops in async execution mode.  Together these hooks should
cover all the cases when frame information ought to be refreshed (and
when the relevant windows ought to be subsequently updated).

Effectively, with this patch, frame/PC changes that do not immediately
precede an inferior-stop event or a prompt display event no longer cause
TUI's frame and register information to be updated.

And as a result of this change and of the previous change to
tui_show_frame_info, the TUI is much more disciplined about updating the
screen, and so the flicker as described in the PR is totally gone.

gdb/ChangeLog:

	* frame.c (select_frame): Remove reference to
	deprecated_selected_frame_level_changed_hook.
	* frame.h (deprecated_selected_frame_level_changed_hook): Remove
	declaration.
	* stack.c (deprecated_selected_frame_level_changed_hook):
	Likewise.
	* tui/tui-hooks.c (tui_selected_frame_level_changed_hook):
	Rename to ...
	(tui_refresh_frame_and_register_information): ... this.  Handle
	the case where there is no stack.
	(tui_before_prompt): New function.
	(tui_before_prompt_observer): New observer.
	(tui_install_hooks): Register tui_before_prompt_observer to
	call tui_before_prompt.  Remove reference to
	deprecated_selected_frame_level_changed_hook.
	(tui_remove_hooks): Detach and unset tui_before_prompt_observer.
	Remove reference to
	deprecated_selected_frame_level_changed_hook.
	* tui/tui-hooks.h (tui_refresh_frame_and_register_information):
	Export.
	* tui/tui-interp.c: Include "tui/tui-hooks.h".
	(tui_on_sync_execution_done): Call
	tui_refresh_frame_and_register_information if tui_active.
---
 gdb/frame.c          |  2 --
 gdb/frame.h          |  2 --
 gdb/stack.c          |  2 --
 gdb/tui/tui-hooks.c  | 64 ++++++++++++++++++++++++++++++----------------------
 gdb/tui/tui-hooks.h  |  5 ++++
 gdb/tui/tui-interp.c |  4 ++++
 6 files changed, 46 insertions(+), 33 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index b3cbf23..da5bfb9 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1576,8 +1576,6 @@ select_frame (struct frame_info *fi)
   selected_frame = fi;
   /* NOTE: cagney/2002-05-04: FI can be NULL.  This occurs when the
      frame is being invalidated.  */
-  if (deprecated_selected_frame_level_changed_hook)
-    deprecated_selected_frame_level_changed_hook (frame_relative_level (fi));
 
   /* FIXME: kseitz/2002-08-28: It would be nice to call
      selected_frame_level_changed_event() right here, but due to limitations
diff --git a/gdb/frame.h b/gdb/frame.h
index 53a93e0..be64c57 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -763,8 +763,6 @@ extern void args_info (char *, int);
 
 extern void locals_info (char *, int);
 
-extern void (*deprecated_selected_frame_level_changed_hook) (int);
-
 extern void return_command (char *, int);
 
 /* Set FRAME's unwinder temporarily, so that we can call a sniffer.
diff --git a/gdb/stack.c b/gdb/stack.c
index eea575a..39803d9 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -52,8 +52,6 @@
 #include "symfile.h"
 #include "extension.h"
 
-void (*deprecated_selected_frame_level_changed_hook) (int);
-
 /* The possible choices of "set print frame-arguments", and the value
    of this setting.  */
 
diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index 8d84551..a0994c4 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -119,46 +119,46 @@ tui_about_to_proceed (void)
   tui_target_has_run = 1;
 }
 
-/* The selected frame has changed.  This is happens after a target
-   stop or when the user explicitly changes the frame
-   (up/down/thread/...).  */
-static void
-tui_selected_frame_level_changed_hook (int level)
+/* See tui-hooks.h.  */
+
+void
+tui_refresh_frame_and_register_information (void)
 {
   struct frame_info *fi;
   CORE_ADDR pc;
   struct cleanup *old_chain;
 
-  /* Negative level means that the selected frame was cleared.  */
-  if (level < 0)
-    return;
-
   old_chain = make_cleanup_restore_target_terminal ();
   target_terminal_ours_for_output ();
 
-  fi = get_selected_frame (NULL);
-  /* Ensure that symbols for this frame are read in.  Also, determine
-     the source language of this frame, and switch to it if
-     desired.  */
-  if (get_frame_pc_if_available (fi, &pc))
+  if (has_stack_frames ())
     {
-      struct symtab *s;
-
-      s = find_pc_line_symtab (pc);
-      /* elz: This if here fixes the problem with the pc not being
-	 displayed in the tui asm layout, with no debug symbols.  The
-	 value of s would be 0 here, and select_source_symtab would
-	 abort the command by calling the 'error' function.  */
-      if (s)
-	select_source_symtab (s);
+      fi = get_selected_frame (NULL);
+      /* Ensure that symbols for this frame are read in.  Also, determine
+	 the source language of this frame, and switch to it if
+	 desired.  */
+      if (get_frame_pc_if_available (fi, &pc))
+	{
+	  struct symtab *s;
+
+	  s = find_pc_line_symtab (pc);
+	  /* elz: This if here fixes the problem with the pc not being
+	     displayed in the tui asm layout, with no debug symbols.  The
+	     value of s would be 0 here, and select_source_symtab would
+	     abort the command by calling the 'error' function.  */
+	  if (s)
+	    select_source_symtab (s);
+	}
     }
+  else
+    fi = NULL;
 
   /* Display the frame position (even if there is no symbols or the PC
      is not known).  */
   tui_show_frame_info (fi);
 
   /* Refresh the register window if it's visible.  */
-  if (tui_is_window_visible (DATA_WIN))
+  if (fi != NULL && tui_is_window_visible (DATA_WIN))
     {
       tui_refreshing_registers = 1;
       tui_check_data_values (fi);
@@ -191,19 +191,26 @@ tui_inferior_exit (struct inferior *inf)
   tui_display_main ();
 }
 
+/* Observer for the before_prompt notification.  */
+
+static void
+tui_before_prompt (const char *current_gdb_prompt)
+{
+  tui_refresh_frame_and_register_information ();
+}
+
 /* Observers created when installing TUI hooks.  */
 static struct observer *tui_bp_created_observer;
 static struct observer *tui_bp_deleted_observer;
 static struct observer *tui_bp_modified_observer;
 static struct observer *tui_inferior_exit_observer;
 static struct observer *tui_about_to_proceed_observer;
+static struct observer *tui_before_prompt_observer;
 
 /* Install the TUI specific hooks.  */
 void
 tui_install_hooks (void)
 {
-  deprecated_selected_frame_level_changed_hook
-    = tui_selected_frame_level_changed_hook;
   deprecated_print_frame_info_listing_hook
     = tui_print_frame_info_listing_hook;
 
@@ -218,6 +225,8 @@ tui_install_hooks (void)
     = observer_attach_inferior_exit (tui_inferior_exit);
   tui_about_to_proceed_observer
     = observer_attach_about_to_proceed (tui_about_to_proceed);
+  tui_before_prompt_observer
+    = observer_attach_before_prompt (tui_before_prompt);
 
   deprecated_register_changed_hook = tui_register_changed_hook;
 }
@@ -226,7 +235,6 @@ tui_install_hooks (void)
 void
 tui_remove_hooks (void)
 {
-  deprecated_selected_frame_level_changed_hook = 0;
   deprecated_print_frame_info_listing_hook = 0;
   deprecated_query_hook = 0;
   deprecated_register_changed_hook = 0;
@@ -242,6 +250,8 @@ tui_remove_hooks (void)
   tui_inferior_exit_observer = NULL;
   observer_detach_about_to_proceed (tui_about_to_proceed_observer);
   tui_about_to_proceed_observer = NULL;
+  observer_detach_before_prompt (tui_before_prompt_observer);
+  tui_before_prompt_observer = NULL;
 }
 
 void _initialize_tui_hooks (void);
diff --git a/gdb/tui/tui-hooks.h b/gdb/tui/tui-hooks.h
index ab6afc9..303d9bb 100644
--- a/gdb/tui/tui-hooks.h
+++ b/gdb/tui/tui-hooks.h
@@ -20,6 +20,11 @@
 #ifndef TUI_HOOKS_H
 #define TUI_HOOKS_H
 
+/* Refresh TUI's frame and register information.  This is a hook intended to be
+   used to update the screen after potential frame and register changes.  */
+
+extern void tui_refresh_frame_and_register_information (void);
+
 extern void tui_install_hooks (void);
 extern void tui_remove_hooks (void);
 
diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
index 1a5639d..4284037 100644
--- a/gdb/tui/tui-interp.c
+++ b/gdb/tui/tui-interp.c
@@ -27,6 +27,7 @@
 #include "tui/tui-data.h"
 #include "readline/readline.h"
 #include "tui/tui-win.h"
+#include "tui/tui-hooks.h"
 #include "tui/tui.h"
 #include "tui/tui-io.h"
 #include "infrun.h"
@@ -107,6 +108,9 @@ tui_on_sync_execution_done (void)
 {
   if (!interp_quiet_p (tui_interp))
     display_gdb_prompt (NULL);
+
+  if (tui_active)
+    tui_refresh_frame_and_register_information ();
 }
 
 /* Observer for the command_error notification.  */
-- 
2.5.0.rc0.5.g91e10c5.dirty

[PATCH 2/3] Be lazy about refreshing the windows in tui_show_frame_info (PR tui/13378)

[Patrick Palka]

tui_show_frame_info is responsible for updating the visible windows
following a change in frame information (that being the currently
selected frame, PC, line number, etc).  Currently it always redraws and
refreshes each window even if frame information has not changed.  This
behavior is inefficient and helps contribute to the occassional
flickering of the TUI as described in the mentioned PR.

This patch makes tui_show_frame_info refresh the windows only if frame
information has changed.  Determining whether frame information has
changed is done indirectly by determining whether the locator has
changed.  This approach is convenient and yet sensible because the
locator contains all the relevant info we need to check anyway: the
current PC, the line number, the name of the executable and the name of
the current function.  Probably only the PC is really necessary to
check, but it doesn't hurt to check every field.

Effectively, with this patch, consecutive calls to select_frame with the
same frame/PC no longer cause TUI's frame information to be updated
multiple times.

gdb/ChangeLog:

	* tui/tui-stack.c (tui_set_locator_info): Change prototype to
	return an int instead of void.  Return whether the locator
	window has changed.
	(tui_show_frame_info): If the locator info has not changed, then
	bail out early to avoid refreshing the windows.
---
 gdb/tui/tui-stack.c | 67 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 51 insertions(+), 16 deletions(-)

diff --git a/gdb/tui/tui-stack.c b/gdb/tui/tui-stack.c
index b17d303..d43d39b 100644
--- a/gdb/tui/tui-stack.c
+++ b/gdb/tui/tui-stack.c
@@ -48,10 +48,10 @@ static char *tui_get_function_from_frame (struct frame_info *fi);
 static void tui_set_locator_fullname (const char *fullname);
 
 /* Update the locator, with the provided arguments.  */
-static void tui_set_locator_info (struct gdbarch *gdbarch,
-				  const char *fullname,
-				  const char *procname,
-                                  int lineno, CORE_ADDR addr);
+static int tui_set_locator_info (struct gdbarch *gdbarch,
+				 const char *fullname,
+				 const char *procname,
+                                 int lineno, CORE_ADDR addr);
 
 static void tui_update_command (char *, int);
 \f
@@ -292,8 +292,12 @@ tui_set_locator_fullname (const char *fullname)
   strcat_to_buf (element->full_name, MAX_LOCATOR_ELEMENT_LEN, fullname);
 }
 
-/* Update the locator, with the provided arguments.  */
-static void
+/* Update the locator, with the provided arguments.
+
+   Returns 1 if any of the locator's fields were actually changed,
+   and 0 otherwise.  */
+
+static int
 tui_set_locator_info (struct gdbarch *gdbarch,
 		      const char *fullname,
 		      const char *procname, 
@@ -302,6 +306,7 @@ tui_set_locator_info (struct gdbarch *gdbarch,
 {
   struct tui_gen_win_info *locator = tui_locator_win_info_ptr ();
   struct tui_locator_element *element;
+  int locator_changed_p = 0;
 
   /* Allocate the locator content if necessary.  */
   if (locator->content_size <= 0)
@@ -311,12 +316,25 @@ tui_set_locator_info (struct gdbarch *gdbarch,
     }
 
   element = &locator->content[0]->which_element.locator;
+
+  if (procname != NULL)
+    locator_changed_p |= strncmp (element->proc_name, procname,
+				  MAX_LOCATOR_ELEMENT_LEN) != 0;
+  locator_changed_p |= lineno != element->line_no;
+  locator_changed_p |= addr != element->addr;
+  locator_changed_p |= gdbarch != element->gdbarch;
+  if (fullname != NULL)
+    locator_changed_p |= strncmp (element->full_name, fullname,
+				  MAX_LOCATOR_ELEMENT_LEN) != 0;
+
   element->proc_name[0] = (char) 0;
   strcat_to_buf (element->proc_name, MAX_LOCATOR_ELEMENT_LEN, procname);
   element->line_no = lineno;
   element->addr = addr;
   element->gdbarch = gdbarch;
   tui_set_locator_fullname (fullname);
+
+  return locator_changed_p;
 }
 
 /* Update only the full_name portion of the locator.  */
@@ -327,11 +345,14 @@ tui_update_locator_fullname (const char *fullname)
   tui_show_locator_content ();
 }
 
-/* Function to print the frame information for the TUI.  */
+/* Function to print the frame information for the TUI.  The windows are
+   refreshed only if frame information has changed since the last refresh.  */
+
 void
 tui_show_frame_info (struct frame_info *fi)
 {
   struct tui_win_info *win_info;
+  int locator_changed_p;
   int i;
 
   if (fi)
@@ -342,6 +363,7 @@ tui_show_frame_info (struct frame_info *fi)
       int source_already_displayed;
       struct symtab_and_line sal;
       CORE_ADDR pc;
+      int should_update_windows_p = 1;
 
       find_frame_sal (fi, &sal);
 
@@ -349,15 +371,23 @@ tui_show_frame_info (struct frame_info *fi)
         && tui_source_is_displayed (symtab_to_fullname (sal.symtab));
 
       if (get_frame_pc_if_available (fi, &pc))
-	tui_set_locator_info (get_frame_arch (fi),
-			      (sal.symtab == 0
-			       ? "??" : symtab_to_fullname (sal.symtab)),
-			      tui_get_function_from_frame (fi),
-			      sal.line,
-			      pc);
+	locator_changed_p
+	  = tui_set_locator_info (get_frame_arch (fi),
+				  (sal.symtab == 0
+				   ? "??" : symtab_to_fullname (sal.symtab)),
+				  tui_get_function_from_frame (fi),
+				  sal.line,
+				  pc);
       else
-	tui_set_locator_info (get_frame_arch (fi),
-			      "??", _("<unavailable>"), sal.line, 0);
+	locator_changed_p
+	  = tui_set_locator_info (get_frame_arch (fi),
+				  "??", _("<unavailable>"), sal.line, 0);
+
+      /* If the locator information has not changed, then frame information has
+	 not changed.  If frame information has not changed, then the windows'
+	 contents will not change.  So don't bother refreshing the windows.  */
+      if (!locator_changed_p)
+	return;
 
       tui_show_locator_content ();
       start_line = 0;
@@ -431,7 +461,12 @@ tui_show_frame_info (struct frame_info *fi)
     }
   else
     {
-      tui_set_locator_info (NULL, NULL, NULL, 0, (CORE_ADDR) 0);
+      locator_changed_p
+	= tui_set_locator_info (NULL, NULL, NULL, 0, (CORE_ADDR) 0);
+
+      if (!locator_changed_p)
+	return;
+
       tui_show_locator_content ();
       for (i = 0; i < (tui_source_windows ())->count; i++)
 	{
-- 
2.5.0.rc0.5.g91e10c5.dirty

[PATCH 3/3] Replace TUI's select_frame hook (PR tui/13378)

[Patrick Palka]

Actually, we should not call tui_show_frame_info(NULL) when there are no
stack frames, because doing so empties the source window after invoking
"continue&". That was a last-minute change with no justification anyway.

Here is a revised patch with a much smaller diffstat.

gdb/ChangeLog:

	* frame.c (select_frame): Remove reference to
	deprecated_selected_frame_level_changed_hook.
	* frame.h (deprecated_selected_frame_level_changed_hook): Remove
	declaration.
	* stack.c (deprecated_selected_frame_level_changed_hook):
	Likewise.
	* tui/tui-hooks.c (tui_selected_frame_level_changed_hook):
	Rename to ...
	(tui_refresh_frame_and_register_information): ... this.  Bail
	out if there is no stack.
	(tui_before_prompt): New function.
	(tui_before_prompt_observer): New observer.
	(tui_install_hooks): Register tui_before_prompt_observer to
	call tui_before_prompt.  Remove reference to
	deprecated_selected_frame_level_changed_hook.
	(tui_remove_hooks): Detach and unset tui_before_prompt_observer.
	Remove reference to
	deprecated_selected_frame_level_changed_hook.
	* tui/tui-hooks.h (tui_refresh_frame_and_register_information):
	Export.
	* tui/tui-interp.c: Include "tui/tui-hooks.h".
	(tui_on_sync_execution_done): Call
	tui_refresh_frame_and_register_information if tui_active.
---
 gdb/frame.c          |  2 --
 gdb/frame.h          |  2 --
 gdb/stack.c          |  2 --
 gdb/tui/tui-hooks.c  | 28 ++++++++++++++++++----------
 gdb/tui/tui-hooks.h  |  5 +++++
 gdb/tui/tui-interp.c |  4 ++++
 6 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index b3cbf23..da5bfb9 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1576,8 +1576,6 @@ select_frame (struct frame_info *fi)
   selected_frame = fi;
   /* NOTE: cagney/2002-05-04: FI can be NULL.  This occurs when the
      frame is being invalidated.  */
-  if (deprecated_selected_frame_level_changed_hook)
-    deprecated_selected_frame_level_changed_hook (frame_relative_level (fi));
 
   /* FIXME: kseitz/2002-08-28: It would be nice to call
      selected_frame_level_changed_event() right here, but due to limitations
diff --git a/gdb/frame.h b/gdb/frame.h
index 53a93e0..be64c57 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -763,8 +763,6 @@ extern void args_info (char *, int);
 
 extern void locals_info (char *, int);
 
-extern void (*deprecated_selected_frame_level_changed_hook) (int);
-
 extern void return_command (char *, int);
 
 /* Set FRAME's unwinder temporarily, so that we can call a sniffer.
diff --git a/gdb/stack.c b/gdb/stack.c
index eea575a..39803d9 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -52,8 +52,6 @@
 #include "symfile.h"
 #include "extension.h"
 
-void (*deprecated_selected_frame_level_changed_hook) (int);
-
 /* The possible choices of "set print frame-arguments", and the value
    of this setting.  */
 
diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index 8d84551..16c177b 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -119,18 +119,16 @@ tui_about_to_proceed (void)
   tui_target_has_run = 1;
 }
 
-/* The selected frame has changed.  This is happens after a target
-   stop or when the user explicitly changes the frame
-   (up/down/thread/...).  */
-static void
-tui_selected_frame_level_changed_hook (int level)
+/* See tui-hooks.h.  */
+
+void
+tui_refresh_frame_and_register_information (void)
 {
   struct frame_info *fi;
   CORE_ADDR pc;
   struct cleanup *old_chain;
 
-  /* Negative level means that the selected frame was cleared.  */
-  if (level < 0)
+  if (!has_stack_frames ())
     return;
 
   old_chain = make_cleanup_restore_target_terminal ();
@@ -191,19 +189,26 @@ tui_inferior_exit (struct inferior *inf)
   tui_display_main ();
 }
 
+/* Observer for the before_prompt notification.  */
+
+static void
+tui_before_prompt (const char *current_gdb_prompt)
+{
+  tui_refresh_frame_and_register_information ();
+}
+
 /* Observers created when installing TUI hooks.  */
 static struct observer *tui_bp_created_observer;
 static struct observer *tui_bp_deleted_observer;
 static struct observer *tui_bp_modified_observer;
 static struct observer *tui_inferior_exit_observer;
 static struct observer *tui_about_to_proceed_observer;
+static struct observer *tui_before_prompt_observer;
 
 /* Install the TUI specific hooks.  */
 void
 tui_install_hooks (void)
 {
-  deprecated_selected_frame_level_changed_hook
-    = tui_selected_frame_level_changed_hook;
   deprecated_print_frame_info_listing_hook
     = tui_print_frame_info_listing_hook;
 
@@ -218,6 +223,8 @@ tui_install_hooks (void)
     = observer_attach_inferior_exit (tui_inferior_exit);
   tui_about_to_proceed_observer
     = observer_attach_about_to_proceed (tui_about_to_proceed);
+  tui_before_prompt_observer
+    = observer_attach_before_prompt (tui_before_prompt);
 
   deprecated_register_changed_hook = tui_register_changed_hook;
 }
@@ -226,7 +233,6 @@ tui_install_hooks (void)
 void
 tui_remove_hooks (void)
 {
-  deprecated_selected_frame_level_changed_hook = 0;
   deprecated_print_frame_info_listing_hook = 0;
   deprecated_query_hook = 0;
   deprecated_register_changed_hook = 0;
@@ -242,6 +248,8 @@ tui_remove_hooks (void)
   tui_inferior_exit_observer = NULL;
   observer_detach_about_to_proceed (tui_about_to_proceed_observer);
   tui_about_to_proceed_observer = NULL;
+  observer_detach_before_prompt (tui_before_prompt_observer);
+  tui_before_prompt_observer = NULL;
 }
 
 void _initialize_tui_hooks (void);
diff --git a/gdb/tui/tui-hooks.h b/gdb/tui/tui-hooks.h
index ab6afc9..303d9bb 100644
--- a/gdb/tui/tui-hooks.h
+++ b/gdb/tui/tui-hooks.h
@@ -20,6 +20,11 @@
 #ifndef TUI_HOOKS_H
 #define TUI_HOOKS_H
 
+/* Refresh TUI's frame and register information.  This is a hook intended to be
+   used to update the screen after potential frame and register changes.  */
+
+extern void tui_refresh_frame_and_register_information (void);
+
 extern void tui_install_hooks (void);
 extern void tui_remove_hooks (void);
 
diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
index 1a5639d..4284037 100644
--- a/gdb/tui/tui-interp.c
+++ b/gdb/tui/tui-interp.c
@@ -27,6 +27,7 @@
 #include "tui/tui-data.h"
 #include "readline/readline.h"
 #include "tui/tui-win.h"
+#include "tui/tui-hooks.h"
 #include "tui/tui.h"
 #include "tui/tui-io.h"
 #include "infrun.h"
@@ -107,6 +108,9 @@ tui_on_sync_execution_done (void)
 {
   if (!interp_quiet_p (tui_interp))
     display_gdb_prompt (NULL);
+
+  if (tui_active)
+    tui_refresh_frame_and_register_information ();
 }
 
 /* Observer for the command_error notification.  */
-- 
2.5.0.rc0.5.g91e10c5.dirty

[PATCH] Be lazy about refreshing the windows in tui_show_frame_info (PR tui/13378)

[Patrick Palka]

This revised patch makes sure that tui_set_locator_info returns 1 when the
locator is first constructed, just in case none of the later checks trigger
for some reason.

gdb/ChangeLog:

	* tui/tui-stack.c (tui_set_locator_info): Change prototype to
	return an int instead of void.  Return whether the locator
	window has changed.
	(tui_show_frame_info): If the locator info has not changed, then
	bail out early to avoid refreshing the windows.
---
 gdb/tui/tui-stack.c | 67 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 51 insertions(+), 16 deletions(-)

diff --git a/gdb/tui/tui-stack.c b/gdb/tui/tui-stack.c
index b17d303..e077245 100644
--- a/gdb/tui/tui-stack.c
+++ b/gdb/tui/tui-stack.c
@@ -48,10 +48,10 @@ static char *tui_get_function_from_frame (struct frame_info *fi);
 static void tui_set_locator_fullname (const char *fullname);
 
 /* Update the locator, with the provided arguments.  */
-static void tui_set_locator_info (struct gdbarch *gdbarch,
-				  const char *fullname,
-				  const char *procname,
-                                  int lineno, CORE_ADDR addr);
+static int tui_set_locator_info (struct gdbarch *gdbarch,
+				 const char *fullname,
+				 const char *procname,
+                                 int lineno, CORE_ADDR addr);
 
 static void tui_update_command (char *, int);
 \f
@@ -292,8 +292,12 @@ tui_set_locator_fullname (const char *fullname)
   strcat_to_buf (element->full_name, MAX_LOCATOR_ELEMENT_LEN, fullname);
 }
 
-/* Update the locator, with the provided arguments.  */
-static void
+/* Update the locator, with the provided arguments.
+
+   Returns 1 if any of the locator's fields were actually changed,
+   and 0 otherwise.  */
+
+static int
 tui_set_locator_info (struct gdbarch *gdbarch,
 		      const char *fullname,
 		      const char *procname, 
@@ -302,21 +306,36 @@ tui_set_locator_info (struct gdbarch *gdbarch,
 {
   struct tui_gen_win_info *locator = tui_locator_win_info_ptr ();
   struct tui_locator_element *element;
+  int locator_changed_p = 0;
 
   /* Allocate the locator content if necessary.  */
   if (locator->content_size <= 0)
     {
       locator->content = tui_alloc_content (1, LOCATOR_WIN);
       locator->content_size = 1;
+      locator_changed_p = 1;
     }
 
   element = &locator->content[0]->which_element.locator;
+
+  if (procname != NULL)
+    locator_changed_p |= strncmp (element->proc_name, procname,
+				  MAX_LOCATOR_ELEMENT_LEN) != 0;
+  locator_changed_p |= lineno != element->line_no;
+  locator_changed_p |= addr != element->addr;
+  locator_changed_p |= gdbarch != element->gdbarch;
+  if (fullname != NULL)
+    locator_changed_p |= strncmp (element->full_name, fullname,
+				  MAX_LOCATOR_ELEMENT_LEN) != 0;
+
   element->proc_name[0] = (char) 0;
   strcat_to_buf (element->proc_name, MAX_LOCATOR_ELEMENT_LEN, procname);
   element->line_no = lineno;
   element->addr = addr;
   element->gdbarch = gdbarch;
   tui_set_locator_fullname (fullname);
+
+  return locator_changed_p;
 }
 
 /* Update only the full_name portion of the locator.  */
@@ -327,11 +346,14 @@ tui_update_locator_fullname (const char *fullname)
   tui_show_locator_content ();
 }
 
-/* Function to print the frame information for the TUI.  */
+/* Function to print the frame information for the TUI.  The windows are
+   refreshed only if frame information has changed since the last refresh.  */
+
 void
 tui_show_frame_info (struct frame_info *fi)
 {
   struct tui_win_info *win_info;
+  int locator_changed_p;
   int i;
 
   if (fi)
@@ -349,15 +371,23 @@ tui_show_frame_info (struct frame_info *fi)
         && tui_source_is_displayed (symtab_to_fullname (sal.symtab));
 
       if (get_frame_pc_if_available (fi, &pc))
-	tui_set_locator_info (get_frame_arch (fi),
-			      (sal.symtab == 0
-			       ? "??" : symtab_to_fullname (sal.symtab)),
-			      tui_get_function_from_frame (fi),
-			      sal.line,
-			      pc);
+	locator_changed_p
+	  = tui_set_locator_info (get_frame_arch (fi),
+				  (sal.symtab == 0
+				   ? "??" : symtab_to_fullname (sal.symtab)),
+				  tui_get_function_from_frame (fi),
+				  sal.line,
+				  pc);
       else
-	tui_set_locator_info (get_frame_arch (fi),
-			      "??", _("<unavailable>"), sal.line, 0);
+	locator_changed_p
+	  = tui_set_locator_info (get_frame_arch (fi),
+				  "??", _("<unavailable>"), sal.line, 0);
+
+      /* If the locator information has not changed, then frame information has
+	 not changed.  If frame information has not changed, then the windows'
+	 contents will not change.  So don't bother refreshing the windows.  */
+      if (!locator_changed_p)
+	return;
 
       tui_show_locator_content ();
       start_line = 0;
@@ -431,7 +461,12 @@ tui_show_frame_info (struct frame_info *fi)
     }
   else
     {
-      tui_set_locator_info (NULL, NULL, NULL, 0, (CORE_ADDR) 0);
+      locator_changed_p
+	= tui_set_locator_info (NULL, NULL, NULL, 0, (CORE_ADDR) 0);
+
+      if (!locator_changed_p)
+	return;
+
       tui_show_locator_content ();
       for (i = 0; i < (tui_source_windows ())->count; i++)
 	{
-- 
2.5.0.rc0.5.g91e10c5.dirty

Re: [PATCH 1/3] Correctly initialize the TUI locator window

[Pedro Alves]

On 06/27/2015 03:35 AM, Patrick Palka wrote:
> The call to tui_alloc_content in tui_set_locator_info passes
> locator->type as the type of the window whose content is being
> allocated.  This may seem correct but it's actually not because when
> this code path actually get executed locator->type has not yet been to
> set LOCATOR_WIN so it defaults to 0 i.e. SRC_WIN.  Thus we allocate the
> content of the locator window as if it was the source window.  This
> oversight turns out not to be a big deal in practice but the patch that
> follows depends on the locator's proc_name and full_name arrays to be
> initialized to the empty string which is done by tui_alloc_content if
> we pass to it LOCATOR_WIN.
> 
> This patch fixes this bug by explicitly passing LOCATOR_WIN to
> tui_alloc_content.

OK.

Thanks,
Pedro Alves

Re: [PATCH 3/3] Replace TUI's select_frame hook (PR tui/13378)

[Pedro Alves]

On 06/27/2015 03:50 AM, Patrick Palka wrote:
> Actually, we should not call tui_show_frame_info(NULL) when there are no
> stack frames, because doing so empties the source window after invoking
> "continue&". That was a last-minute change with no justification anyway.
> 
> Here is a revised patch with a much smaller diffstat.

On 06/27/2015 03:35 AM, Patrick Palka wrote:

> Effectively, with this patch, frame/PC changes that do not immediately
> precede an inferior-stop event or a prompt display event no longer cause
> TUI's frame and register information to be updated.

It seems to me that this doesn't update the info when the
program stops after continue& ...

> @@ -107,6 +108,9 @@ tui_on_sync_execution_done (void)
>  {
>    if (!interp_quiet_p (tui_interp))
>      display_gdb_prompt (NULL);
> +
> +  if (tui_active)
> +    tui_refresh_frame_and_register_information ();
>  }
>

... when sync/foreground execution is done, we show the
prompt (display_gdb_prompt above), so the before_prompt observer
is already taking care of refreshing.

But when background/async execution is done, we don't call
sync_execution_done observers, so this hook isn't called.
So I'm not seeing what this bit in tui_on_sync_execution_done
is for.

I think that for for stops (either after background or foreground
execution), the TUI refreshes itself from within the
deprecated_print_frame_info_listing_hook, called from:

#0  tui_show_frame_info (fi=0xf20b50) at /home/pedro/gdb/mygit/build/../src/gdb/tui/tui-stack.c:337
#1  0x00000000005266c5 in tui_print_frame_info_listing_hook (s=0x1978100, line=92, stopline=93, noerror=0) at /home/pedro/gdb/mygit/build/../src/gdb/tui/tui-hooks.c:177
#2  0x000000000063a6f7 in print_frame_info (frame=0xf20b50, print_level=0, print_what=SRC_AND_LOC, print_args=1, set_current_sal=1)
    at /home/pedro/gdb/mygit/build/../src/gdb/stack.c:895
#3  0x0000000000638e2f in print_stack_frame (frame=0xf20b50, print_level=0, print_what=SRC_AND_LOC, set_current_sal=1)
    at /home/pedro/gdb/mygit/build/../src/gdb/stack.c:162
#4  0x0000000000632c14 in print_stop_event (ws=0x7fffffffd120) at /home/pedro/gdb/mygit/build/../src/gdb/infrun.c:6536
#5  0x0000000000632fa5 in normal_stop () at /home/pedro/gdb/mygit/build/../src/gdb/infrun.c:6701

I think this hooking in deprecated_print_frame_info_listing_hook
is also a too-low-level place, and is likely contributing
to flicker in other situations.  I think we should move
to doing whatever we do in it to a normal_stop observer instead.
(tbc, doesn't have to be this patch.)

> And as a result of this change and of the previous change to
> tui_show_frame_info, the TUI is much more disciplined about updating the
> screen, and so the flicker as described in the PR is totally gone.

I think we should sort out patch 3 before patch 2.  AFAICS, the
important one is this one, right?

Thanks,
Pedro Alves

Re: [PATCH 3/3] Replace TUI's select_frame hook (PR tui/13378)

[Patrick Palka]

On Tue, Jun 30, 2015 at 5:32 AM, Pedro Alves <palves@redhat.com> wrote:
> On 06/27/2015 03:50 AM, Patrick Palka wrote:
>> Actually, we should not call tui_show_frame_info(NULL) when there are no
>> stack frames, because doing so empties the source window after invoking
>> "continue&". That was a last-minute change with no justification anyway.
>>
>> Here is a revised patch with a much smaller diffstat.
>
> On 06/27/2015 03:35 AM, Patrick Palka wrote:
>
>> Effectively, with this patch, frame/PC changes that do not immediately
>> precede an inferior-stop event or a prompt display event no longer cause
>> TUI's frame and register information to be updated.
>
> It seems to me that this doesn't update the info when the
> program stops after continue& ...

Oops, yeah..

>
>> @@ -107,6 +108,9 @@ tui_on_sync_execution_done (void)
>>  {
>>    if (!interp_quiet_p (tui_interp))
>>      display_gdb_prompt (NULL);
>> +
>> +  if (tui_active)
>> +    tui_refresh_frame_and_register_information ();
>>  }
>>
>
> ... when sync/foreground execution is done, we show the
> prompt (display_gdb_prompt above), so the before_prompt observer
> is already taking care of refreshing.
>
> But when background/async execution is done, we don't call
> sync_execution_done observers, so this hook isn't called.
> So I'm not seeing what this bit in tui_on_sync_execution_done
> is for.

Good point... the sync_execution_done addendum is useless.

>
> I think that for for stops (either after background or foreground
> execution), the TUI refreshes itself from within the
> deprecated_print_frame_info_listing_hook, called from:
>
> #0  tui_show_frame_info (fi=0xf20b50) at /home/pedro/gdb/mygit/build/../src/gdb/tui/tui-stack.c:337
> #1  0x00000000005266c5 in tui_print_frame_info_listing_hook (s=0x1978100, line=92, stopline=93, noerror=0) at /home/pedro/gdb/mygit/build/../src/gdb/tui/tui-hooks.c:177
> #2  0x000000000063a6f7 in print_frame_info (frame=0xf20b50, print_level=0, print_what=SRC_AND_LOC, print_args=1, set_current_sal=1)
>     at /home/pedro/gdb/mygit/build/../src/gdb/stack.c:895
> #3  0x0000000000638e2f in print_stack_frame (frame=0xf20b50, print_level=0, print_what=SRC_AND_LOC, set_current_sal=1)
>     at /home/pedro/gdb/mygit/build/../src/gdb/stack.c:162
> #4  0x0000000000632c14 in print_stop_event (ws=0x7fffffffd120) at /home/pedro/gdb/mygit/build/../src/gdb/infrun.c:6536
> #5  0x0000000000632fa5 in normal_stop () at /home/pedro/gdb/mygit/build/../src/gdb/infrun.c:6701
>
> I think this hooking in deprecated_print_frame_info_listing_hook
> is also a too-low-level place, and is likely contributing
> to flicker in other situations.  I think we should move
> to doing whatever we do in it to a normal_stop observer instead.
> (tbc, doesn't have to be this patch.)

That make sense.

>
>> And as a result of this change and of the previous change to
>> tui_show_frame_info, the TUI is much more disciplined about updating the
>> screen, and so the flicker as described in the PR is totally gone.
>
> I think we should sort out patch 3 before patch 2.  AFAICS, the
> important one is this one, right?

This one is more important, but by itself it causes source/asm-window
scrolling to be reset after each command (due to the before_prompt
observer) even if the frame information hasn't been changed in the
meantime.  Other than that they are mostly unrelated.

I'll post a revised patch in a bit.

>
> Thanks,
> Pedro Alves
>

[PATCH 3/3] Replace TUI's select_frame hook (PR tui/13378)

[Patrick Palka]

This version adds a tui_normal_stop observer in place of augmenting the
tui_on_sync_execution_done observer.  And tui_refresh_frame_and_register_information
can now be made a static function.

The observer takes a print_frame parameter that is supposed to inform us
whether the frame should be printed.  This boolean seems to only be true for
when the inferior has exited.  Since tui_refresh_frame_and_register_information
already handles this case by checking has_stack_frames() this patch elects to
ignore this parameter in the observer.

gdb/ChangeLog:

	* frame.c (select_frame): Remove reference to
	deprecated_selected_frame_level_changed_hook.
	* frame.h (deprecated_selected_frame_level_changed_hook): Remove
	declaration.
	* stack.c (deprecated_selected_frame_level_changed_hook):
	Likewise.
	* tui/tui-hooks.c (tui_selected_frame_level_changed_hook):
	Rename to ...
	(tui_refresh_frame_and_register_information): ... this.  Bail
	out if there is no stack.
	(tui_before_prompt): New function.
	(tui_normal_stop): New function.
	(tui_before_prompt_observer): New observer.
	(tui_normal_stop_observer): New observer.
	(tui_install_hooks): Register tui_before_prompt_observer to
	call tui_before_prompt and tui_normal_stop_observer to call
	tui_normal_stop.  Remove reference to
	deprecated_selected_frame_level_changed_hook.
	(tui_remove_hooks): Detach and unset tui_before_prompt_observer
	and tui_normal_stop_observer.  Remove reference to
	deprecated_selected_frame_level_changed_hook.
---
 gdb/frame.c         |  2 --
 gdb/frame.h         |  2 --
 gdb/stack.c         |  2 --
 gdb/tui/tui-hooks.c | 40 +++++++++++++++++++++++++++++++---------
 4 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index b3cbf23..da5bfb9 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1576,8 +1576,6 @@ select_frame (struct frame_info *fi)
   selected_frame = fi;
   /* NOTE: cagney/2002-05-04: FI can be NULL.  This occurs when the
      frame is being invalidated.  */
-  if (deprecated_selected_frame_level_changed_hook)
-    deprecated_selected_frame_level_changed_hook (frame_relative_level (fi));
 
   /* FIXME: kseitz/2002-08-28: It would be nice to call
      selected_frame_level_changed_event() right here, but due to limitations
diff --git a/gdb/frame.h b/gdb/frame.h
index 53a93e0..be64c57 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -763,8 +763,6 @@ extern void args_info (char *, int);
 
 extern void locals_info (char *, int);
 
-extern void (*deprecated_selected_frame_level_changed_hook) (int);
-
 extern void return_command (char *, int);
 
 /* Set FRAME's unwinder temporarily, so that we can call a sniffer.
diff --git a/gdb/stack.c b/gdb/stack.c
index eea575a..39803d9 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -52,8 +52,6 @@
 #include "symfile.h"
 #include "extension.h"
 
-void (*deprecated_selected_frame_level_changed_hook) (int);
-
 /* The possible choices of "set print frame-arguments", and the value
    of this setting.  */
 
diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index 8d84551..b7218ff 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -119,18 +119,17 @@ tui_about_to_proceed (void)
   tui_target_has_run = 1;
 }
 
-/* The selected frame has changed.  This is happens after a target
-   stop or when the user explicitly changes the frame
-   (up/down/thread/...).  */
+/* Refresh TUI's frame and register information.  This is a hook intended to be
+   used to update the screen after potential frame and register changes.  */
+
 static void
-tui_selected_frame_level_changed_hook (int level)
+tui_refresh_frame_and_register_information (void)
 {
   struct frame_info *fi;
   CORE_ADDR pc;
   struct cleanup *old_chain;
 
-  /* Negative level means that the selected frame was cleared.  */
-  if (level < 0)
+  if (!has_stack_frames ())
     return;
 
   old_chain = make_cleanup_restore_target_terminal ();
@@ -191,19 +190,35 @@ tui_inferior_exit (struct inferior *inf)
   tui_display_main ();
 }
 
+/* Observer for the before_prompt notification.  */
+
+static void
+tui_before_prompt (const char *current_gdb_prompt)
+{
+  tui_refresh_frame_and_register_information ();
+}
+
+/* Observer for the normal_stop notification.  */
+
+static void
+tui_normal_stop (struct bpstats *bs, int print_frame)
+{
+  tui_refresh_frame_and_register_information ();
+}
+
 /* Observers created when installing TUI hooks.  */
 static struct observer *tui_bp_created_observer;
 static struct observer *tui_bp_deleted_observer;
 static struct observer *tui_bp_modified_observer;
 static struct observer *tui_inferior_exit_observer;
 static struct observer *tui_about_to_proceed_observer;
+static struct observer *tui_before_prompt_observer;
+static struct observer *tui_normal_stop_observer;
 
 /* Install the TUI specific hooks.  */
 void
 tui_install_hooks (void)
 {
-  deprecated_selected_frame_level_changed_hook
-    = tui_selected_frame_level_changed_hook;
   deprecated_print_frame_info_listing_hook
     = tui_print_frame_info_listing_hook;
 
@@ -218,6 +233,10 @@ tui_install_hooks (void)
     = observer_attach_inferior_exit (tui_inferior_exit);
   tui_about_to_proceed_observer
     = observer_attach_about_to_proceed (tui_about_to_proceed);
+  tui_before_prompt_observer
+    = observer_attach_before_prompt (tui_before_prompt);
+  tui_normal_stop_observer
+    = observer_attach_normal_stop (tui_normal_stop);
 
   deprecated_register_changed_hook = tui_register_changed_hook;
 }
@@ -226,7 +245,6 @@ tui_install_hooks (void)
 void
 tui_remove_hooks (void)
 {
-  deprecated_selected_frame_level_changed_hook = 0;
   deprecated_print_frame_info_listing_hook = 0;
   deprecated_query_hook = 0;
   deprecated_register_changed_hook = 0;
@@ -242,6 +260,10 @@ tui_remove_hooks (void)
   tui_inferior_exit_observer = NULL;
   observer_detach_about_to_proceed (tui_about_to_proceed_observer);
   tui_about_to_proceed_observer = NULL;
+  observer_detach_before_prompt (tui_before_prompt_observer);
+  tui_before_prompt_observer = NULL;
+  observer_detach_normal_stop (tui_normal_stop_observer);
+  tui_normal_stop_observer = NULL;
 }
 
 void _initialize_tui_hooks (void);
-- 
2.5.0.rc0.5.g91e10c5.dirty

Re: [PATCH 3/3] Replace TUI's select_frame hook (PR tui/13378)

[Pedro Alves]

On 06/30/2015 01:16 PM, Patrick Palka wrote:

>>> And as a result of this change and of the previous change to
>>> tui_show_frame_info, the TUI is much more disciplined about updating the
>>> screen, and so the flicker as described in the PR is totally gone.
>>
>> I think we should sort out patch 3 before patch 2.  AFAICS, the
>> important one is this one, right?
> 
> This one is more important, but by itself it causes source/asm-window
> scrolling to be reset after each command (due to the before_prompt
> observer) even if the frame information hasn't been changed in the
> meantime.  Other than that they are mostly unrelated.

OK, I see.

I think I missed that because it only happens if you scroll enough to
make the highlighted source line _not_ visible.  Just scrolling a bit
and entering a command is not enough to trigger it.

Thanks,
Pedro Alves

Re: [PATCH 3/3] Replace TUI's select_frame hook (PR tui/13378)

[Patrick Palka]

On Tue, Jun 30, 2015 at 9:23 AM, Pedro Alves <palves@redhat.com> wrote:
> On 06/30/2015 01:16 PM, Patrick Palka wrote:
>
>>>> And as a result of this change and of the previous change to
>>>> tui_show_frame_info, the TUI is much more disciplined about updating the
>>>> screen, and so the flicker as described in the PR is totally gone.
>>>
>>> I think we should sort out patch 3 before patch 2.  AFAICS, the
>>> important one is this one, right?
>>
>> This one is more important, but by itself it causes source/asm-window
>> scrolling to be reset after each command (due to the before_prompt
>> observer) even if the frame information hasn't been changed in the
>> meantime.  Other than that they are mostly unrelated.
>
> OK, I see.
>
> I think I missed that because it only happens if you scroll enough to
> make the highlighted source line _not_ visible.  Just scrolling a bit
> and entering a command is not enough to trigger it.

Yep -- sorry, should've mentioned that bit.

>
> Thanks,
> Pedro Alves
>

Re: [PATCH 3/3] Replace TUI's select_frame hook (PR tui/13378)

[Pedro Alves]

On 06/30/2015 01:37 PM, Patrick Palka wrote:
> This version adds a tui_normal_stop observer in place of augmenting the
> tui_on_sync_execution_done observer.  And tui_refresh_frame_and_register_information
> can now be made a static function.
> 
> The observer takes a print_frame parameter that is supposed to inform us
> whether the frame should be printed.  This boolean seems to only be true for

s/only be true/only be false/

> when the inferior has exited.  Since tui_refresh_frame_and_register_information
> already handles this case by checking has_stack_frames() this patch elects to
> ignore this parameter in the observer.

This is OK.  I'll take a look at patch 2 soon.

Did you find that we still need deprecated_print_frame_info_listing_hook?

Thanks,
Pedro Alves

Re: [PATCH] Be lazy about refreshing the windows in tui_show_frame_info (PR tui/13378)

[Pedro Alves]

On 06/30/2015 03:32 AM, Patrick Palka wrote:
> This revised patch makes sure that tui_set_locator_info returns 1 when the
> locator is first constructed, just in case none of the later checks trigger
> for some reason.

I have a couple questions below, but I'm fine with this approach.

> @@ -302,21 +306,36 @@ tui_set_locator_info (struct gdbarch *gdbarch,
>  {
>    struct tui_gen_win_info *locator = tui_locator_win_info_ptr ();
>    struct tui_locator_element *element;
> +  int locator_changed_p = 0;
>  
>    /* Allocate the locator content if necessary.  */
>    if (locator->content_size <= 0)
>      {
>        locator->content = tui_alloc_content (1, LOCATOR_WIN);
>        locator->content_size = 1;
> +      locator_changed_p = 1;
>      }
>  
>    element = &locator->content[0]->which_element.locator;
> +
> +  if (procname != NULL)
> +    locator_changed_p |= strncmp (element->proc_name, procname,
> +				  MAX_LOCATOR_ELEMENT_LEN) != 0;

Can't element->proc_name be NULL here?

For the string fields, do we also need to compare
whether we go from NULL <-> non-NULL ?

  locator_changed_p |= ((fullname == NULL) != (element->full_name == NULL));

etc.?

Thanks,
Pedro Alves

Re: [PATCH] Be lazy about refreshing the windows in tui_show_frame_info (PR tui/13378)

[Patrick Palka]

On Tue, Jun 30, 2015 at 10:27 AM, Pedro Alves <palves@redhat.com> wrote:
> On 06/30/2015 03:32 AM, Patrick Palka wrote:
>> This revised patch makes sure that tui_set_locator_info returns 1 when the
>> locator is first constructed, just in case none of the later checks trigger
>> for some reason.
>
> I have a couple questions below, but I'm fine with this approach.
>
>> @@ -302,21 +306,36 @@ tui_set_locator_info (struct gdbarch *gdbarch,
>>  {
>>    struct tui_gen_win_info *locator = tui_locator_win_info_ptr ();
>>    struct tui_locator_element *element;
>> +  int locator_changed_p = 0;
>>
>>    /* Allocate the locator content if necessary.  */
>>    if (locator->content_size <= 0)
>>      {
>>        locator->content = tui_alloc_content (1, LOCATOR_WIN);
>>        locator->content_size = 1;
>> +      locator_changed_p = 1;
>>      }
>>
>>    element = &locator->content[0]->which_element.locator;
>> +
>> +  if (procname != NULL)
>> +    locator_changed_p |= strncmp (element->proc_name, procname,
>> +                               MAX_LOCATOR_ELEMENT_LEN) != 0;
>
> Can't element->proc_name be NULL here?

Don't think so, since it is an inline array.  It's defined as:

struct tui_locator_element
{
  ...
  char full_name[MAX_LOCATOR_ELEMENT_LEN];
  char proc_name[MAX_LOCATOR_ELEMENT_LEN];
}

(and tui_alloc_content makes sure to set full_name[0] = proc_name[0] = '\0').

>
> For the string fields, do we also need to compare
> whether we go from NULL <-> non-NULL ?
>
>   locator_changed_p |= ((fullname == NULL) != (element->full_name == NULL));
>
> etc.?

Yeah, that would be more correct I think.  But I think the logic would
have to look like "if (procname == NULL) locator_changed_p |= strlen
(element->proc_name) != 0;" because proc_name cannot be NULL.  When
procname is NULL, proc_name[0] gets set to 0.

>
> Thanks,
> Pedro Alves
>

Re: [PATCH 3/3] Replace TUI's select_frame hook (PR tui/13378)

[Patrick Palka]

On Tue, Jun 30, 2015 at 10:08 AM, Pedro Alves <palves@redhat.com> wrote:
> On 06/30/2015 01:37 PM, Patrick Palka wrote:
>> This version adds a tui_normal_stop observer in place of augmenting the
>> tui_on_sync_execution_done observer.  And tui_refresh_frame_and_register_information
>> can now be made a static function.
>>
>> The observer takes a print_frame parameter that is supposed to inform us
>> whether the frame should be printed.  This boolean seems to only be true for
>
> s/only be true/only be false/
>
>> when the inferior has exited.  Since tui_refresh_frame_and_register_information
>> already handles this case by checking has_stack_frames() this patch elects to
>> ignore this parameter in the observer.
>
> This is OK.  I'll take a look at patch 2 soon.
>
> Did you find that we still need deprecated_print_frame_info_listing_hook?

It still seems to be "necessary" -- at least, I can't outright remove it.

The only caller of deprecated_print_frame_info_listing_hook is in
print_frame_info and its use looks like this:

 if (deprecated_print_frame_info_listing_hook)
    (*deprecated_print_frame_info_listing_hook) (...);
 else
   { ... other code ... }

If I remove the hook by replacing the above code with

  { ... other code ... }

Then a regression occurs: the TUI decides to make sure that the
currently executing line always sits at the top of the window instead
of only scrolling the screen when the currently executing line is not
invisible.

But if I disable the hook by replacing the body of code in print_frame_info with

  if (deprecated_print_frame_info_listing_hook)
    ;
  else
    { ... other code ... }

Then everything seems to be OK.  So the code in  the else branch is
interfering with TUI somehow.  I will investigate further.

>
> Thanks,
> Pedro Alves
>

Re: [PATCH 3/3] Replace TUI's select_frame hook (PR tui/13378)

[Patrick Palka]

On Tue, Jun 30, 2015 at 10:54 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Tue, Jun 30, 2015 at 10:08 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 06/30/2015 01:37 PM, Patrick Palka wrote:
>>> This version adds a tui_normal_stop observer in place of augmenting the
>>> tui_on_sync_execution_done observer.  And tui_refresh_frame_and_register_information
>>> can now be made a static function.
>>>
>>> The observer takes a print_frame parameter that is supposed to inform us
>>> whether the frame should be printed.  This boolean seems to only be true for
>>
>> s/only be true/only be false/
>>
>>> when the inferior has exited.  Since tui_refresh_frame_and_register_information
>>> already handles this case by checking has_stack_frames() this patch elects to
>>> ignore this parameter in the observer.
>>
>> This is OK.  I'll take a look at patch 2 soon.
>>
>> Did you find that we still need deprecated_print_frame_info_listing_hook?
>
> It still seems to be "necessary" -- at least, I can't outright remove it.
>
> The only caller of deprecated_print_frame_info_listing_hook is in
> print_frame_info and its use looks like this:
>
>  if (deprecated_print_frame_info_listing_hook)
>     (*deprecated_print_frame_info_listing_hook) (...);
>  else
>    { ... other code ... }
>
> If I remove the hook by replacing the above code with
>
>   { ... other code ... }
>
> Then a regression occurs: the TUI decides to make sure that the
> currently executing line always sits at the top of the window instead
> of only scrolling the screen when the currently executing line is not
> invisible.

I meant to say not _visible_.

Re: [PATCH] Be lazy about refreshing the windows in tui_show_frame_info (PR tui/13378)

[Pedro Alves]

On 06/30/2015 03:44 PM, Patrick Palka wrote:
> On Tue, Jun 30, 2015 at 10:27 AM, Pedro Alves <palves@redhat.com> wrote:

>> Can't element->proc_name be NULL here?
> 
> Don't think so, since it is an inline array.  It's defined as:
> 
> struct tui_locator_element
> {
>   ...
>   char full_name[MAX_LOCATOR_ELEMENT_LEN];
>   char proc_name[MAX_LOCATOR_ELEMENT_LEN];
> }
> 
> (and tui_alloc_content makes sure to set full_name[0] = proc_name[0] = '\0').

Ah.

> 
>>
>> For the string fields, do we also need to compare
>> whether we go from NULL <-> non-NULL ?
>>
>>   locator_changed_p |= ((fullname == NULL) != (element->full_name == NULL));
>>
>> etc.?
> 
> Yeah, that would be more correct I think.  But I think the logic would
> have to look like "if (procname == NULL) locator_changed_p |= strlen
> (element->proc_name) != 0;" because proc_name cannot be NULL.  When
> procname is NULL, proc_name[0] gets set to 0.
> 

Or alternatively:

  if (fullname == NULL)
    fullname = "";
  locator_changed_p |= strncmp (element->proc_name, procname,
				MAX_LOCATOR_ELEMENT_LEN) != 0;
  ...

Thanks,
Pedro Alves

Re: [PATCH 3/3] Replace TUI's select_frame hook (PR tui/13378)

[Patrick Palka]

On Tue, Jun 30, 2015 at 10:54 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Tue, Jun 30, 2015 at 10:08 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 06/30/2015 01:37 PM, Patrick Palka wrote:
>>> This version adds a tui_normal_stop observer in place of augmenting the
>>> tui_on_sync_execution_done observer.  And tui_refresh_frame_and_register_information
>>> can now be made a static function.
>>>
>>> The observer takes a print_frame parameter that is supposed to inform us
>>> whether the frame should be printed.  This boolean seems to only be true for
>>
>> s/only be true/only be false/
>>
>>> when the inferior has exited.  Since tui_refresh_frame_and_register_information
>>> already handles this case by checking has_stack_frames() this patch elects to
>>> ignore this parameter in the observer.
>>
>> This is OK.  I'll take a look at patch 2 soon.
>>
>> Did you find that we still need deprecated_print_frame_info_listing_hook?
>
> It still seems to be "necessary" -- at least, I can't outright remove it.
>
> The only caller of deprecated_print_frame_info_listing_hook is in
> print_frame_info and its use looks like this:
>
>  if (deprecated_print_frame_info_listing_hook)
>     (*deprecated_print_frame_info_listing_hook) (...);
>  else
>    { ... other code ... }
>
> If I remove the hook by replacing the above code with
>
>   { ... other code ... }
>
> Then a regression occurs: the TUI decides to make sure that the
> currently executing line always sits at the top of the window instead
> of only scrolling the screen when the currently executing line is not
> invisible.
>
> But if I disable the hook by replacing the body of code in print_frame_info with
>
>   if (deprecated_print_frame_info_listing_hook)
>     ;
>   else
>     { ... other code ... }
>
> Then everything seems to be OK.  So the code in  the else branch is
> interfering with TUI somehow.  I will investigate further.

The call to "print_source_lines (sal.symtab, sal.line, sal.line + 1,
0);" in the else branch eventually calls "tui_show_source (sal.line);"
which adjusts the source window so that sal.line is the very first
line visible.

I'm not sure how easy this would be to fix properly.  We want to avoid
calling print_source_lines in print_frame_info when the TUI is active.
Of course, I can just guard the code with "if (tui_active)" but that's
not a good fix.  Instead of removing the hook yet, what about making
it (tui_print_frame_info_listing_hook) a no-op in the interim?

BTW, in the CLI, this call to print_source_lines is responsible for
printing the stopped-at source line to stdout, e.g.

(gdb) start
....
Temporary breakpoint 2, main () at 13378.c:9
9         int i = 0;  // THIS LINE
(gdb)

Re: [PATCH] Be lazy about refreshing the windows in tui_show_frame_info (PR tui/13378)

[Patrick Palka]

On Tue, Jun 30, 2015 at 11:11 AM, Pedro Alves <palves@redhat.com> wrote:
> On 06/30/2015 03:44 PM, Patrick Palka wrote:
>> On Tue, Jun 30, 2015 at 10:27 AM, Pedro Alves <palves@redhat.com> wrote:
>
>>> Can't element->proc_name be NULL here?
>>
>> Don't think so, since it is an inline array.  It's defined as:
>>
>> struct tui_locator_element
>> {
>>   ...
>>   char full_name[MAX_LOCATOR_ELEMENT_LEN];
>>   char proc_name[MAX_LOCATOR_ELEMENT_LEN];
>> }
>>
>> (and tui_alloc_content makes sure to set full_name[0] = proc_name[0] = '\0').
>
> Ah.
>
>>
>>>
>>> For the string fields, do we also need to compare
>>> whether we go from NULL <-> non-NULL ?
>>>
>>>   locator_changed_p |= ((fullname == NULL) != (element->full_name == NULL));
>>>
>>> etc.?
>>
>> Yeah, that would be more correct I think.  But I think the logic would
>> have to look like "if (procname == NULL) locator_changed_p |= strlen
>> (element->proc_name) != 0;" because proc_name cannot be NULL.  When
>> procname is NULL, proc_name[0] gets set to 0.
>>
>
> Or alternatively:
>
>   if (fullname == NULL)
>     fullname = "";
>   locator_changed_p |= strncmp (element->proc_name, procname,
>                                 MAX_LOCATOR_ELEMENT_LEN) != 0;

I'll do that.

>   ...
>
> Thanks,
> Pedro Alves
>

Re: [PATCH 3/3] Replace TUI's select_frame hook (PR tui/13378)

[Pedro Alves]

On 06/30/2015 04:12 PM, Patrick Palka wrote:
> On Tue, Jun 30, 2015 at 10:54 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:

> The call to "print_source_lines (sal.symtab, sal.line, sal.line + 1,
> 0);" in the else branch eventually calls "tui_show_source (sal.line);"
> which adjusts the source window so that sal.line is the very first
> line visible.
> 
> I'm not sure how easy this would be to fix properly.  We want to avoid
> calling print_source_lines in print_frame_info when the TUI is active.
> Of course, I can just guard the code with "if (tui_active)" but that's
> not a good fix. 

Indeed.

> Instead of removing the hook yet, what about making
> it (tui_print_frame_info_listing_hook) a no-op in the interim?

If that works, fine with me.  Fine with me to leave it be as is too.

> 
> BTW, in the CLI, this call to print_source_lines is responsible for
> printing the stopped-at source line to stdout, e.g.

Yeah.  Maybe if we move the print_stop_event call out of normal_stop
into TUI/CLI normal_stop observers, then we can tailor CLI/TUI/MI to
print what they need.  I actually moved it here:

 https://github.com/palves/gdb/commits/palves/merge-more-async-and-sync
 https://github.com/palves/gdb/commit/c0a88ed037b645fb9f072290000dc11044524639

That's WIP (still ugly) post 7.10 material.

> 
> (gdb) start
> ....
> Temporary breakpoint 2, main () at 13378.c:9
> 9         int i = 0;  // THIS LINE
> (gdb)

Thanks,
Pedro Alves

[PATCH 3/3] Replace TUI's select_frame hook (PR tui/13378)

[Patrick Palka]

[ I elected to go with making the print_frame_info_listing hook a no-op, since
  it does not seem to regress anything.

  I noticed that "layout regs" was somewhat broken now that
  tui_refresh_frame_and_register_information is called twice following a normal
  stop: once in tui_normal_stop and then in tui_before_prompt.  The second call
  removes any highlights done to the individual registers during the first
  call, because the function notices that the current snapshot of register
  values is the same as the one taken during the first call.  So effectively
  register changes are no longer highlighted in "layout regs" since the
  highlights immediately get removed.

  I don't think we should refresh register information at all in
  tui_before_prompt since this observer is intended to only update frame
  information following a call to "up", "down", "frame", etc.  Only after the
  inferior has run for a bit could registers have changed.  So this patch adds
  the parameter registers_too_p to tui_refresh_frame_and_register_information
  to indicate wheher we should update registers too, and updates
  tui_before_prompt and tui_normal_stop accordingly.  ]

The select_frame hook is used by TUI to update TUI's frame and register
information following changes to the selected frame.  The problem with
this hook is that it gets called after every single frame change, even
if the frame change is only temporary or internal.  This is the primary
cause of flickering and slowdown when running the inferior under TUI
with conditional breakpoints set.  Internal GDB events are the source of
many calls to select_frame and these internal events are triggered
frequently, especially when a few conditional breakpoints are set.

This patch removes the select_frame hook altogether and instead makes
the frame and register information get updated in two key places (using
observers): after an inferior stops, and right before displaying a
prompt.  The latter hook covers the case when frame information must be
updated following a call to "up", "down" or "frame", and the former
covers the case when frame and register information must be updated
after a call to "continue", "step", etc. or after the inferior stops in
async execution mode.  Together these hooks should cover all the cases
when frame information ought to be refreshed (and when the relevant
windows ought to be subsequently updated).

The print_frame_info_listing hook is also effectively obsolete now, but
it still must be set while the TUI is active because its caller
print_frame_info will otherwise assume that the CLI is active, and will
print the frame informaion accordingly.  So this patch also sets the
print_frame_info_listing hook to a dummy callback, in lieu of outright
removing it yet.

Effectively, with this patch, frame/PC changes that do not immediately
precede an inferior-stop event or a prompt display event no longer cause
TUI's frame and register information to be updated.

And as a result of this change and of the previous change to
tui_show_frame_info, the TUI is much more disciplined about updating the
screen, and so the flicker as described in the PR is totally gone.

gdb/ChangeLog:

	* frame.c (select_frame): Remove reference to
	deprecated_selected_frame_level_changed_hook.
	* frame.h (deprecated_selected_frame_level_changed_hook): Remove
	declaration.
	* stack.c (deprecated_selected_frame_level_changed_hook):
	Likewise.
	* tui/tui-hooks.c (tui_selected_frame_level_changed_hook):
	Rename to ...
	(tui_refresh_frame_and_register_information): ... this.  Bail
	out if there is no stack.
	(tui_print_frame_info_listing_hook): Rename to ...
	(tui_dummy_print_frame_info_listing_hook): ... this.
	(tui_before_prompt): New function.
	(tui_normal_stop): New function.
	(tui_before_prompt_observer): New observer.
	(tui_normal_stop_observer): New observer.
	(tui_install_hooks): Set
	deprecated_print_frame_info_listing_hook to
	tui_dummy_print_frame_info_listing_hook.  Register
	tui_before_prompt_observer to call tui_before_prompt and
	tui_normal_stop_observer to call tui_normal_stop.  Remove
	reference to deprecated_selected_frame_level_changed_hook.
	(tui_remove_hooks): Detach and unset tui_before_prompt_observer
	and tui_normal_stop_observer.  Remove reference to
	deprecated_selected_frame_level_changed_hook.
---
 gdb/frame.c         |  2 --
 gdb/frame.h         |  2 --
 gdb/stack.c         |  2 --
 gdb/tui/tui-hooks.c | 70 +++++++++++++++++++++++++++++++++++++++--------------
 4 files changed, 52 insertions(+), 24 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index b3cbf23..da5bfb9 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1576,8 +1576,6 @@ select_frame (struct frame_info *fi)
   selected_frame = fi;
   /* NOTE: cagney/2002-05-04: FI can be NULL.  This occurs when the
      frame is being invalidated.  */
-  if (deprecated_selected_frame_level_changed_hook)
-    deprecated_selected_frame_level_changed_hook (frame_relative_level (fi));
 
   /* FIXME: kseitz/2002-08-28: It would be nice to call
      selected_frame_level_changed_event() right here, but due to limitations
diff --git a/gdb/frame.h b/gdb/frame.h
index 53a93e0..be64c57 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -763,8 +763,6 @@ extern void args_info (char *, int);
 
 extern void locals_info (char *, int);
 
-extern void (*deprecated_selected_frame_level_changed_hook) (int);
-
 extern void return_command (char *, int);
 
 /* Set FRAME's unwinder temporarily, so that we can call a sniffer.
diff --git a/gdb/stack.c b/gdb/stack.c
index eea575a..39803d9 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -52,8 +52,6 @@
 #include "symfile.h"
 #include "extension.h"
 
-void (*deprecated_selected_frame_level_changed_hook) (int);
-
 /* The possible choices of "set print frame-arguments", and the value
    of this setting.  */
 
diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index 8d84551..d4a57c8 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -119,18 +119,19 @@ tui_about_to_proceed (void)
   tui_target_has_run = 1;
 }
 
-/* The selected frame has changed.  This is happens after a target
-   stop or when the user explicitly changes the frame
-   (up/down/thread/...).  */
+/* Refresh TUI's frame and register information.  This is a hook intended to be
+   used to update the screen after potential frame and register changes.
+
+   REGISTERS_TOO_P controls whether to refresh our register information.  */
+
 static void
-tui_selected_frame_level_changed_hook (int level)
+tui_refresh_frame_and_register_information (int registers_too_p)
 {
   struct frame_info *fi;
   CORE_ADDR pc;
   struct cleanup *old_chain;
 
-  /* Negative level means that the selected frame was cleared.  */
-  if (level < 0)
+  if (!has_stack_frames ())
     return;
 
   old_chain = make_cleanup_restore_target_terminal ();
@@ -158,7 +159,7 @@ tui_selected_frame_level_changed_hook (int level)
   tui_show_frame_info (fi);
 
   /* Refresh the register window if it's visible.  */
-  if (tui_is_window_visible (DATA_WIN))
+  if (tui_is_window_visible (DATA_WIN) && registers_too_p)
     {
       tui_refreshing_registers = 1;
       tui_check_data_values (fi);
@@ -168,15 +169,15 @@ tui_selected_frame_level_changed_hook (int level)
   do_cleanups (old_chain);
 }
 
-/* Called from print_frame_info to list the line we stopped in.  */
+/* Dummy callback for deprecated_print_frame_info_listing_hook which is called
+   from print_frame_info.  */
+
 static void
-tui_print_frame_info_listing_hook (struct symtab *s,
-				   int line,
-                                   int stopline, 
-				   int noerror)
+tui_dummy_print_frame_info_listing_hook (struct symtab *s,
+					 int line,
+					 int stopline, 
+					 int noerror)
 {
-  select_source_symtab (s);
-  tui_show_frame_info (get_selected_frame (NULL));
 }
 
 /* Perform all necessary cleanups regarding our module's inferior data
@@ -191,21 +192,47 @@ tui_inferior_exit (struct inferior *inf)
   tui_display_main ();
 }
 
+/* Observer for the before_prompt notification.  */
+
+static void
+tui_before_prompt (const char *current_gdb_prompt)
+{
+  /* This refresh is intended to catch changes to the selected frame following
+     a call to "up", "down" or "frame".  As such we don't necessarily want to
+     refresh registers here as they could not have changed.  Registers will be
+     refreshed after a normal stop.  */
+  tui_refresh_frame_and_register_information (/*registers_too_p=*/0);
+}
+
+/* Observer for the normal_stop notification.  */
+
+static void
+tui_normal_stop (struct bpstats *bs, int print_frame)
+{
+  /* This refresh is intended to catch changes to the selected frame and to
+     registers following a normal stop.  */
+  tui_refresh_frame_and_register_information (/*registers_too_p=*/1);
+}
+
 /* Observers created when installing TUI hooks.  */
 static struct observer *tui_bp_created_observer;
 static struct observer *tui_bp_deleted_observer;
 static struct observer *tui_bp_modified_observer;
 static struct observer *tui_inferior_exit_observer;
 static struct observer *tui_about_to_proceed_observer;
+static struct observer *tui_before_prompt_observer;
+static struct observer *tui_normal_stop_observer;
 
 /* Install the TUI specific hooks.  */
 void
 tui_install_hooks (void)
 {
-  deprecated_selected_frame_level_changed_hook
-    = tui_selected_frame_level_changed_hook;
+  /* If this hook is not set to something then print_frame_info will
+     assume that the CLI, not the TUI, is active, and will print the frame info
+     for us in such a way that we are not prepared to handle.  This hook is
+     otherwise effectively obsolete.  */
   deprecated_print_frame_info_listing_hook
-    = tui_print_frame_info_listing_hook;
+    = tui_dummy_print_frame_info_listing_hook;
 
   /* Install the event hooks.  */
   tui_bp_created_observer
@@ -218,6 +245,10 @@ tui_install_hooks (void)
     = observer_attach_inferior_exit (tui_inferior_exit);
   tui_about_to_proceed_observer
     = observer_attach_about_to_proceed (tui_about_to_proceed);
+  tui_before_prompt_observer
+    = observer_attach_before_prompt (tui_before_prompt);
+  tui_normal_stop_observer
+    = observer_attach_normal_stop (tui_normal_stop);
 
   deprecated_register_changed_hook = tui_register_changed_hook;
 }
@@ -226,7 +257,6 @@ tui_install_hooks (void)
 void
 tui_remove_hooks (void)
 {
-  deprecated_selected_frame_level_changed_hook = 0;
   deprecated_print_frame_info_listing_hook = 0;
   deprecated_query_hook = 0;
   deprecated_register_changed_hook = 0;
@@ -242,6 +272,10 @@ tui_remove_hooks (void)
   tui_inferior_exit_observer = NULL;
   observer_detach_about_to_proceed (tui_about_to_proceed_observer);
   tui_about_to_proceed_observer = NULL;
+  observer_detach_before_prompt (tui_before_prompt_observer);
+  tui_before_prompt_observer = NULL;
+  observer_detach_normal_stop (tui_normal_stop_observer);
+  tui_normal_stop_observer = NULL;
 }
 
 void _initialize_tui_hooks (void);
-- 
2.5.0.rc0.5.g91e10c5.dirty

[PATCH 2/3] Be lazy about refreshing the windows in tui_show_frame_info (PR tui/13378)

[Patrick Palka]

[ This version just changes tui_set_locator_info as you suggested.  ]

tui_show_frame_info is responsible for updating the visible windows
following a change in frame information (that being the currently
selected frame, PC, line number, etc).  Currently it always redraws and
refreshes each window even if frame information has not changed.  This
behavior is inefficient and helps contribute to the occassional
flickering of the TUI as described in the mentioned PR.

This patch makes tui_show_frame_info refresh the windows only if frame
information has changed.  Determining whether frame information has
changed is done indirectly by determining whether the locator has
changed.  This approach is convenient and yet sensible because the
locator contains all the relevant info we need to check anyway: the
current PC, the line number, the name of the executable and the name of
the current function.  Probably only the PC is really necessary to
check, but it doesn't hurt to check every field.

Effectively, with this patch, consecutive calls to select_frame with the
same frame/PC no longer cause TUI's frame information to be updated
multiple times.

gdb/ChangeLog:

	* tui/tui-stack.c (tui_set_locator_info): Change prototype to
	return an int instead of void.  Return whether the locator
	window has changed.
	(tui_show_frame_info): If the locator info has not changed, then
	bail out early to avoid refreshing the windows.
---
 gdb/tui/tui-stack.c | 71 +++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 55 insertions(+), 16 deletions(-)

diff --git a/gdb/tui/tui-stack.c b/gdb/tui/tui-stack.c
index b17d303..65d18fe 100644
--- a/gdb/tui/tui-stack.c
+++ b/gdb/tui/tui-stack.c
@@ -48,10 +48,10 @@ static char *tui_get_function_from_frame (struct frame_info *fi);
 static void tui_set_locator_fullname (const char *fullname);
 
 /* Update the locator, with the provided arguments.  */
-static void tui_set_locator_info (struct gdbarch *gdbarch,
-				  const char *fullname,
-				  const char *procname,
-                                  int lineno, CORE_ADDR addr);
+static int tui_set_locator_info (struct gdbarch *gdbarch,
+				 const char *fullname,
+				 const char *procname,
+                                 int lineno, CORE_ADDR addr);
 
 static void tui_update_command (char *, int);
 \f
@@ -292,8 +292,12 @@ tui_set_locator_fullname (const char *fullname)
   strcat_to_buf (element->full_name, MAX_LOCATOR_ELEMENT_LEN, fullname);
 }
 
-/* Update the locator, with the provided arguments.  */
-static void
+/* Update the locator, with the provided arguments.
+
+   Returns 1 if any of the locator's fields were actually changed,
+   and 0 otherwise.  */
+
+static int
 tui_set_locator_info (struct gdbarch *gdbarch,
 		      const char *fullname,
 		      const char *procname, 
@@ -302,21 +306,40 @@ tui_set_locator_info (struct gdbarch *gdbarch,
 {
   struct tui_gen_win_info *locator = tui_locator_win_info_ptr ();
   struct tui_locator_element *element;
+  int locator_changed_p = 0;
 
   /* Allocate the locator content if necessary.  */
   if (locator->content_size <= 0)
     {
       locator->content = tui_alloc_content (1, LOCATOR_WIN);
       locator->content_size = 1;
+      locator_changed_p = 1;
     }
 
+  if (procname == NULL)
+    procname = "";
+
+  if (fullname == NULL)
+    fullname = "";
+
   element = &locator->content[0]->which_element.locator;
+
+  locator_changed_p |= strncmp (element->proc_name, procname,
+				MAX_LOCATOR_ELEMENT_LEN) != 0;
+  locator_changed_p |= lineno != element->line_no;
+  locator_changed_p |= addr != element->addr;
+  locator_changed_p |= gdbarch != element->gdbarch;
+  locator_changed_p |= strncmp (element->full_name, fullname,
+				MAX_LOCATOR_ELEMENT_LEN) != 0;
+
   element->proc_name[0] = (char) 0;
   strcat_to_buf (element->proc_name, MAX_LOCATOR_ELEMENT_LEN, procname);
   element->line_no = lineno;
   element->addr = addr;
   element->gdbarch = gdbarch;
   tui_set_locator_fullname (fullname);
+
+  return locator_changed_p;
 }
 
 /* Update only the full_name portion of the locator.  */
@@ -327,11 +350,14 @@ tui_update_locator_fullname (const char *fullname)
   tui_show_locator_content ();
 }
 
-/* Function to print the frame information for the TUI.  */
+/* Function to print the frame information for the TUI.  The windows are
+   refreshed only if frame information has changed since the last refresh.  */
+
 void
 tui_show_frame_info (struct frame_info *fi)
 {
   struct tui_win_info *win_info;
+  int locator_changed_p;
   int i;
 
   if (fi)
@@ -349,15 +375,23 @@ tui_show_frame_info (struct frame_info *fi)
         && tui_source_is_displayed (symtab_to_fullname (sal.symtab));
 
       if (get_frame_pc_if_available (fi, &pc))
-	tui_set_locator_info (get_frame_arch (fi),
-			      (sal.symtab == 0
-			       ? "??" : symtab_to_fullname (sal.symtab)),
-			      tui_get_function_from_frame (fi),
-			      sal.line,
-			      pc);
+	locator_changed_p
+	  = tui_set_locator_info (get_frame_arch (fi),
+				  (sal.symtab == 0
+				   ? "??" : symtab_to_fullname (sal.symtab)),
+				  tui_get_function_from_frame (fi),
+				  sal.line,
+				  pc);
       else
-	tui_set_locator_info (get_frame_arch (fi),
-			      "??", _("<unavailable>"), sal.line, 0);
+	locator_changed_p
+	  = tui_set_locator_info (get_frame_arch (fi),
+				  "??", _("<unavailable>"), sal.line, 0);
+
+      /* If the locator information has not changed, then frame information has
+	 not changed.  If frame information has not changed, then the windows'
+	 contents will not change.  So don't bother refreshing the windows.  */
+      if (!locator_changed_p)
+	return;
 
       tui_show_locator_content ();
       start_line = 0;
@@ -431,7 +465,12 @@ tui_show_frame_info (struct frame_info *fi)
     }
   else
     {
-      tui_set_locator_info (NULL, NULL, NULL, 0, (CORE_ADDR) 0);
+      locator_changed_p
+	= tui_set_locator_info (NULL, NULL, NULL, 0, (CORE_ADDR) 0);
+
+      if (!locator_changed_p)
+	return;
+
       tui_show_locator_content ();
       for (i = 0; i < (tui_source_windows ())->count; i++)
 	{
-- 
2.5.0.rc0.5.g91e10c5.dirty

Re: [PATCH 3/3] Replace TUI's select_frame hook (PR tui/13378)

[Pedro Alves]

On 06/30/2015 05:40 PM, Patrick Palka wrote:
> [ I elected to go with making the print_frame_info_listing hook a no-op, since
>   it does not seem to regress anything.
> 
>   I noticed that "layout regs" was somewhat broken now that
>   tui_refresh_frame_and_register_information is called twice following a normal
>   stop: once in tui_normal_stop and then in tui_before_prompt.  The second call
>   removes any highlights done to the individual registers during the first
>   call, because the function notices that the current snapshot of register
>   values is the same as the one taken during the first call.  So effectively
>   register changes are no longer highlighted in "layout regs" since the
>   highlights immediately get removed.
> 
>   I don't think we should refresh register information at all in
>   tui_before_prompt since this observer is intended to only update frame
>   information following a call to "up", "down", "frame", etc.  Only after the
>   inferior has run for a bit could registers have changed.  So this patch adds
>   the parameter registers_too_p to tui_refresh_frame_and_register_information
>   to indicate wheher we should update registers too, and updates
>   tui_before_prompt and tui_normal_stop accordingly.  ]

Hmm, what about when the user changes registers with "print $rax = 1" etc.?
Do we end up with stale contents?

Thanks,
Pedro Alves

Re: [PATCH 3/3] Replace TUI's select_frame hook (PR tui/13378)

[Patrick Palka]

On Tue, Jun 30, 2015 at 1:07 PM, Pedro Alves <palves@redhat.com> wrote:
> On 06/30/2015 05:40 PM, Patrick Palka wrote:
>> [ I elected to go with making the print_frame_info_listing hook a no-op, since
>>   it does not seem to regress anything.
>>
>>   I noticed that "layout regs" was somewhat broken now that
>>   tui_refresh_frame_and_register_information is called twice following a normal
>>   stop: once in tui_normal_stop and then in tui_before_prompt.  The second call
>>   removes any highlights done to the individual registers during the first
>>   call, because the function notices that the current snapshot of register
>>   values is the same as the one taken during the first call.  So effectively
>>   register changes are no longer highlighted in "layout regs" since the
>>   highlights immediately get removed.
>>
>>   I don't think we should refresh register information at all in
>>   tui_before_prompt since this observer is intended to only update frame
>>   information following a call to "up", "down", "frame", etc.  Only after the
>>   inferior has run for a bit could registers have changed.  So this patch adds
>>   the parameter registers_too_p to tui_refresh_frame_and_register_information
>>   to indicate wheher we should update registers too, and updates
>>   tui_before_prompt and tui_normal_stop accordingly.  ]
>
> Hmm, what about when the user changes registers with "print $rax = 1" etc.?
> Do we end up with stale contents?

Apparently not, thanks to our deprecated_register_changed_hook called
from value_assign.  So many hooks!

Re: [PATCH 2/3] Be lazy about refreshing the windows in tui_show_frame_info (PR tui/13378)

[Pedro Alves]

On 06/30/2015 05:51 PM, Patrick Palka wrote:
> [ This version just changes tui_set_locator_info as you suggested.  ]
> 
> tui_show_frame_info is responsible for updating the visible windows
> following a change in frame information (that being the currently
> selected frame, PC, line number, etc).  Currently it always redraws and
> refreshes each window even if frame information has not changed.  This
> behavior is inefficient and helps contribute to the occassional
> flickering of the TUI as described in the mentioned PR.
> 
> This patch makes tui_show_frame_info refresh the windows only if frame
> information has changed.  Determining whether frame information has
> changed is done indirectly by determining whether the locator has
> changed.  This approach is convenient and yet sensible because the
> locator contains all the relevant info we need to check anyway: the
> current PC, the line number, the name of the executable and the name of
> the current function.  Probably only the PC is really necessary to
> check, but it doesn't hurt to check every field.
> 
> Effectively, with this patch, consecutive calls to select_frame with the
> same frame/PC no longer cause TUI's frame information to be updated
> multiple times.
> 
> gdb/ChangeLog:
> 
> 	* tui/tui-stack.c (tui_set_locator_info): Change prototype to
> 	return an int instead of void.  Return whether the locator
> 	window has changed.
> 	(tui_show_frame_info): If the locator info has not changed, then
> 	bail out early to avoid refreshing the windows.

OK.

Thanks,
Pedro Alves

Re: [PATCH 3/3] Replace TUI's select_frame hook (PR tui/13378)

[Pedro Alves]

On 06/30/2015 06:10 PM, Patrick Palka wrote:

>> Hmm, what about when the user changes registers with "print $rax = 1" etc.?
>> Do we end up with stale contents?
> 
> Apparently not, thanks to our deprecated_register_changed_hook called
> from value_assign.  So many hooks!

Phew!  And luckily there's an equivalent registers_changed observer
we could use instead too.

Patch is OK, just please mention tui_register_changed_hook ...

> +/* Observer for the before_prompt notification.  */
> +
> +static void
> +tui_before_prompt (const char *current_gdb_prompt)
> +{
> +  /* This refresh is intended to catch changes to the selected frame following
> +     a call to "up", "down" or "frame".  As such we don't necessarily want to
> +     refresh registers here as they could not have changed.  Registers will be
> +     refreshed after a normal stop.  */

... here too.

Awesome.  Glad that this is finally fixed.

Thanks,
Pedro Alves

Re: [PATCH 3/3] Replace TUI's select_frame hook (PR tui/13378)

[Patrick Palka]

On Tue, Jun 30, 2015 at 1:32 PM, Pedro Alves <palves@redhat.com> wrote:
> On 06/30/2015 06:10 PM, Patrick Palka wrote:
>
>>> Hmm, what about when the user changes registers with "print $rax = 1" etc.?
>>> Do we end up with stale contents?
>>
>> Apparently not, thanks to our deprecated_register_changed_hook called
>> from value_assign.  So many hooks!
>
> Phew!  And luckily there's an equivalent registers_changed observer
> we could use instead too.

How convenient.

>
> Patch is OK, just please mention tui_register_changed_hook ...
>
>> +/* Observer for the before_prompt notification.  */
>> +
>> +static void
>> +tui_before_prompt (const char *current_gdb_prompt)
>> +{
>> +  /* This refresh is intended to catch changes to the selected frame following
>> +     a call to "up", "down" or "frame".  As such we don't necessarily want to
>> +     refresh registers here as they could not have changed.  Registers will be
>> +     refreshed after a normal stop.  */
>
> ... here too.
>
> Awesome.  Glad that this is finally fixed.

Me too.  The results are quite nice.  TUI is silky smooth.

>
> Thanks,
> Pedro Alves
>

Re: [PATCH 3/3] Replace TUI's select_frame hook (PR tui/13378)

[Patrick Palka]

On Tue, Jun 30, 2015 at 1:07 PM, Pedro Alves <palves@redhat.com> wrote:
> On 06/30/2015 05:40 PM, Patrick Palka wrote:
>> [ I elected to go with making the print_frame_info_listing hook a no-op, since
>>   it does not seem to regress anything.
>>
>>   I noticed that "layout regs" was somewhat broken now that
>>   tui_refresh_frame_and_register_information is called twice following a normal
>>   stop: once in tui_normal_stop and then in tui_before_prompt.  The second call
>>   removes any highlights done to the individual registers during the first
>>   call, because the function notices that the current snapshot of register
>>   values is the same as the one taken during the first call.  So effectively
>>   register changes are no longer highlighted in "layout regs" since the
>>   highlights immediately get removed.
>>
>>   I don't think we should refresh register information at all in
>>   tui_before_prompt since this observer is intended to only update frame
>>   information following a call to "up", "down", "frame", etc.  Only after the
>>   inferior has run for a bit could registers have changed.  So this patch adds
>>   the parameter registers_too_p to tui_refresh_frame_and_register_information
>>   to indicate wheher we should update registers too, and updates
>>   tui_before_prompt and tui_normal_stop accordingly.  ]
>
> Hmm, what about when the user changes registers with "print $rax = 1" etc.?
> Do we end up with stale contents?

Unfortunately registers _do_ change following a call to "up", "down"
or "frame", because the snapshot of register values is per-frame.  So
this patch introduced a regression -- it made it so that TUI does not
update register information following such a call even though earlier
versions do.  I sent a patch to fix this.