[PATCH 1/3] Introduce function for directly updating GDB's screen dimensions

[PATCH 1/3] Introduce function for directly updating GDB's screen dimensions

[Patrick Palka]

... to replace the roundabout pattern of

  execute_command ("set width %d");
  execute_command ("set height %d");

for doing the same thing.

gdb/ChangeLog

	* utils.h (set_screen_width_and_height): Declare.
	* utils.c (set_screen_width_and_height): Define.
	* tui/tui-win.c (tui_update_gdb_sizes): Use it.
---
 gdb/tui/tui-win.c | 21 +++++++++++++--------
 gdb/utils.c       | 12 ++++++++++++
 gdb/utils.h       |  2 ++
 3 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
index 3cf38fc..6830977 100644
--- a/gdb/tui/tui-win.c
+++ b/gdb/tui/tui-win.c
@@ -465,15 +465,20 @@ bold-standout   use extra bright or bold with standout mode"),
 void
 tui_update_gdb_sizes (void)
 {
-  char cmd[50];
+  int width, height;
 
-  /* Set to TUI command window dimension or use readline values.  */
-  xsnprintf (cmd, sizeof (cmd), "set width %d",
-           tui_active ? TUI_CMD_WIN->generic.width : tui_term_width());
-  execute_command (cmd, 0);
-  xsnprintf (cmd, sizeof (cmd), "set height %d",
-           tui_active ? TUI_CMD_WIN->generic.height : tui_term_height());
-  execute_command (cmd, 0);
+  if (tui_active)
+    {
+      width = TUI_CMD_WIN->generic.width;
+      height = TUI_CMD_WIN->generic.height;
+    }
+  else
+    {
+      width = tui_term_width ();
+      height = tui_term_height ();
+    }
+
+  set_screen_width_and_height (width, height);
 }
 
 
diff --git a/gdb/utils.c b/gdb/utils.c
index a9350d9..ec2fd87 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1795,6 +1795,18 @@ set_height_command (char *args, int from_tty, struct cmd_list_element *c)
   set_screen_size ();
 }
 
+/* Set the screen dimensions to WIDTH and HEIGHT.  */
+
+void
+set_screen_width_and_height (int width, int height)
+{
+  lines_per_page = height;
+  chars_per_line = width;
+
+  set_screen_size ();
+  set_width ();
+}
+
 /* Wait, so the user can read what's on the screen.  Prompt the user
    to continue by pressing RETURN.  */
 
diff --git a/gdb/utils.h b/gdb/utils.h
index b8e1aff..9c1af78 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -174,6 +174,8 @@ extern struct ui_file *gdb_stdtarg;
 extern struct ui_file *gdb_stdtargerr;
 extern struct ui_file *gdb_stdtargin;
 
+extern void set_screen_width_and_height (int width, int height);
+
 /* More generic printf like operations.  Filtered versions may return
    non-locally on error.  */
 
-- 
2.4.0.rc2.31.g7c597ef

[PATCH 2/3] Update our idea of our terminal's dimensions even outside of TUI

[Patrick Palka]

When in the CLI, GDB's "width" and "height" variables are not kept in sync
when the underlying terminal gets resized.

This patch fixes this issue by making sure sure to update GDB's "width"
and "height" variables in the !tui_active case of our SIGWINCH handler.

gdb/ChangeLog:

	* tui/tui-win.c (tui_sigwinch_handler): Remove now-stale comment.
	(tui_sigwinch_handler): Still update our idea of
	the terminal's width and height even when TUI is not active.
---
 gdb/tui/tui-win.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
index 6830977..2de73ed 100644
--- a/gdb/tui/tui-win.c
+++ b/gdb/tui/tui-win.c
@@ -836,12 +836,6 @@ static struct async_signal_handler *tui_sigwinch_token;
 static void
 tui_sigwinch_handler (int signal)
 {
-  /* Set win_resized to TRUE and asynchronously invoke our resize callback.  If
-     the callback is invoked while TUI is active then it ought to successfully
-     resize the screen, resetting win_resized to FALSE.  Of course, if the
-     callback is invoked while TUI is inactive then it will do nothing; in that
-     case, win_resized will remain TRUE until we get a chance to synchronously
-     resize the screen from tui_enable().  */
   mark_async_signal_handler (tui_sigwinch_token);
   tui_set_win_resized_to (TRUE);
 }
@@ -850,15 +844,26 @@ tui_sigwinch_handler (int signal)
 static void
 tui_async_resize_screen (gdb_client_data arg)
 {
-  if (!tui_active)
-    return;
-
   rl_resize_terminal ();
-  tui_resize_all ();
-  tui_refresh_all_win ();
-  tui_update_gdb_sizes ();
-  tui_set_win_resized_to (FALSE);
-  tui_redisplay_readline ();
+
+  if (!tui_active)
+    {
+      int screen_height, screen_width;
+
+      rl_get_screen_size (&screen_height, &screen_width);
+      set_screen_width_and_height (screen_width, screen_height);
+
+      /* win_resized will be untoggled and the windows resized in the next call
+	 to tui_enable().  */
+    }
+  else
+    {
+      tui_resize_all ();
+      tui_refresh_all_win ();
+      tui_update_gdb_sizes ();
+      tui_set_win_resized_to (FALSE);
+      tui_redisplay_readline ();
+    }
 }
 #endif
 
-- 
2.4.0.rc2.31.g7c597ef

Re: [PATCH 2/3] Update our idea of our terminal's dimensions even outside of TUI

[Joel Brobecker]

> When in the CLI, GDB's "width" and "height" variables are not kept in sync
> when the underlying terminal gets resized.
> 
> This patch fixes this issue by making sure sure to update GDB's "width"
> and "height" variables in the !tui_active case of our SIGWINCH handler.
> 
> gdb/ChangeLog:
> 
> 	* tui/tui-win.c (tui_sigwinch_handler): Remove now-stale comment.
> 	(tui_sigwinch_handler): Still update our idea of
> 	the terminal's width and height even when TUI is not active.

Neat. Thanks for this nice little enhancement!

I've only glanced at the patch, as I'm not a specialist of this area
and Pedro has been pretty active there and would do a much better job
than I would. But if Pedro can't make it, I'm sure I could set some
time aside to review it a little more carefully.

-- 
Joel

Re: [PATCH 2/3] Update our idea of our terminal's dimensions even outside of TUI

[Pedro Alves]

On 04/24/2015 01:53 AM, Patrick Palka wrote:
> When in the CLI, GDB's "width" and "height" variables are not kept in sync
> when the underlying terminal gets resized.
> 
> This patch fixes this issue by making sure sure to update GDB's "width"
> and "height" variables in the !tui_active case of our SIGWINCH handler.
> 
> gdb/ChangeLog:
> 
> 	* tui/tui-win.c (tui_sigwinch_handler): Remove now-stale comment.
> 	(tui_sigwinch_handler): Still update our idea of
> 	the terminal's width and height even when TUI is not active.

(A next step for a rainy day would be to more the signal handler to
core code, and make this work even when the TUI is not compiled in.)

> @@ -850,15 +844,26 @@ tui_sigwinch_handler (int signal)
>  static void
>  tui_async_resize_screen (gdb_client_data arg)
>  {
> -  if (!tui_active)
> -    return;
> -
>    rl_resize_terminal ();
> -  tui_resize_all ();
> -  tui_refresh_all_win ();
> -  tui_update_gdb_sizes ();
> -  tui_set_win_resized_to (FALSE);
> -  tui_redisplay_readline ();
> +
> +  if (!tui_active)
> +    {
> +      int screen_height, screen_width;
> +
> +      rl_get_screen_size (&screen_height, &screen_width);
> +      set_screen_width_and_height (screen_width, screen_height);
> +
> +      /* win_resized will be untoggled and the windows resized in the next call
> +	 to tui_enable().  */

Hmm, can we please avoid "untoggle"?  I'm not a native speaker, but it
game me pause, as assuming toggle is x^=1, then untoggle is x^=1 too,
thus toggle==untoggle.  :-)  I'd rather use "unset".

OK with that change.

FWIW, the comment gives a bit of pause.  I'd suggest something like this
instead:

     /* win_resized is left set so that the next call to tui_enable
        resizes windows.  */

> +    }
> +  else
> +    {
> +      tui_resize_all ();
> +      tui_refresh_all_win ();
> +      tui_update_gdb_sizes ();
> +      tui_set_win_resized_to (FALSE);
> +      tui_redisplay_readline ();

A preexisting problem (thus not be fixed by this patch), but
I note that this seems racy.  If another SIGWINCH comes in after
between tui_resize_all and tui_set_win_resized_to, we'll clear
tui_set_win_resized_to and lose the need to resize in tui_enable:

  /* Resize windows before anything might display/refresh a
     window.  */
  if (tui_win_resized ())
    {
      tui_resize_all ();
      tui_set_win_resized_to (FALSE);
    }

That's why the mainline code that handles a signal should always clear
the signal handler's control variable before actually reacting to
it, like:

      tui_set_win_resized_to (FALSE);
      tui_resize_all ();
      tui_refresh_all_win ();
      tui_update_gdb_sizes ();
      tui_redisplay_readline ();

Thanks,
Pedro Alves

Re: [PATCH 2/3] Update our idea of our terminal's dimensions even outside of TUI

[Patrick Palka]

On Mon, Apr 27, 2015 at 12:35 PM, Pedro Alves <palves@redhat.com> wrote:
> On 04/24/2015 01:53 AM, Patrick Palka wrote:
>> When in the CLI, GDB's "width" and "height" variables are not kept in sync
>> when the underlying terminal gets resized.
>>
>> This patch fixes this issue by making sure sure to update GDB's "width"
>> and "height" variables in the !tui_active case of our SIGWINCH handler.
>>
>> gdb/ChangeLog:
>>
>>       * tui/tui-win.c (tui_sigwinch_handler): Remove now-stale comment.
>>       (tui_sigwinch_handler): Still update our idea of
>>       the terminal's width and height even when TUI is not active.
>
> (A next step for a rainy day would be to more the signal handler to
> core code, and make this work even when the TUI is not compiled in.)

Good idea... I'll do this.

>
>> @@ -850,15 +844,26 @@ tui_sigwinch_handler (int signal)
>>  static void
>>  tui_async_resize_screen (gdb_client_data arg)
>>  {
>> -  if (!tui_active)
>> -    return;
>> -
>>    rl_resize_terminal ();
>> -  tui_resize_all ();
>> -  tui_refresh_all_win ();
>> -  tui_update_gdb_sizes ();
>> -  tui_set_win_resized_to (FALSE);
>> -  tui_redisplay_readline ();
>> +
>> +  if (!tui_active)
>> +    {
>> +      int screen_height, screen_width;
>> +
>> +      rl_get_screen_size (&screen_height, &screen_width);
>> +      set_screen_width_and_height (screen_width, screen_height);
>> +
>> +      /* win_resized will be untoggled and the windows resized in the next call
>> +      to tui_enable().  */
>
> Hmm, can we please avoid "untoggle"?  I'm not a native speaker, but it
> game me pause, as assuming toggle is x^=1, then untoggle is x^=1 too,
> thus toggle==untoggle.  :-)  I'd rather use "unset".
>
> OK with that change.
>
> FWIW, the comment gives a bit of pause.  I'd suggest something like this
> instead:
>
>      /* win_resized is left set so that the next call to tui_enable
>         resizes windows.  */

Good point, it's better to explicitly mention that win_resized is left set.

>
>> +    }
>> +  else
>> +    {
>> +      tui_resize_all ();
>> +      tui_refresh_all_win ();
>> +      tui_update_gdb_sizes ();
>> +      tui_set_win_resized_to (FALSE);
>> +      tui_redisplay_readline ();
>
> A preexisting problem (thus not be fixed by this patch), but
> I note that this seems racy.  If another SIGWINCH comes in after
> between tui_resize_all and tui_set_win_resized_to, we'll clear
> tui_set_win_resized_to and lose the need to resize in tui_enable:
>
>   /* Resize windows before anything might display/refresh a
>      window.  */
>   if (tui_win_resized ())
>     {
>       tui_resize_all ();
>       tui_set_win_resized_to (FALSE);
>     }
>
> That's why the mainline code that handles a signal should always clear
> the signal handler's control variable before actually reacting to
> it, like:
>
>       tui_set_win_resized_to (FALSE);
>       tui_resize_all ();
>       tui_refresh_all_win ();
>       tui_update_gdb_sizes ();
>       tui_redisplay_readline ();

Dang, I totally missed that.  I'll post a patch for this too.

Just an FYI, you have commented on patches 2/3 and 3/3 but not 1/3.

>
> Thanks,
> Pedro Alves
>

Re: [PATCH 2/3] Update our idea of our terminal's dimensions even outside of TUI

[Pedro Alves]

On 04/28/2015 02:08 AM, Patrick Palka wrote:

> Just an FYI, you have commented on patches 2/3 and 3/3 but not 1/3.

Whoops, I wrote the email, but forgot to press send yesterday; still
had the window open...  Done now.

Thanks,
Pedro Alves

[PATCH 3/3] Disable readline's SIGWINCH handler

[Patrick Palka]

We no longer need it as we handle SIGWINCH ourselves.  Also move the
call to init_page_info() from initialize_utils() to the latter
function's only caller, gdb_init().

gdb/ChangeLog:

	* utils.c (init_page_info): Set rl_catch_sigwinch to zero.
	(initialize_utils): Move call of init_page_info() to ...
	* top.c (gdb_init): ... here.
---
 gdb/top.c   | 2 ++
 gdb/utils.c | 5 +++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/gdb/top.c b/gdb/top.c
index 647d9fb..ddf5415 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1939,6 +1939,8 @@ gdb_init (char *argv0)
   initialize_targets ();    /* Setup target_terminal macros for utils.c.  */
   initialize_utils ();	    /* Make errors and warnings possible.  */
 
+  init_page_info ();
+
   /* Here is where we call all the _initialize_foo routines.  */
   initialize_all_files ();
 
diff --git a/gdb/utils.c b/gdb/utils.c
index ec2fd87..b4d8680 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1696,6 +1696,9 @@ init_page_info (void)
 #endif
     }
 
+  /* We handle SIGWINCH ourselves.  */
+  rl_catch_sigwinch = 0;
+
   set_screen_size ();
   set_width ();
 }
@@ -2712,8 +2715,6 @@ Setting this to \"unlimited\" or zero causes GDB never pause during output."),
 			    show_lines_per_page,
 			    &setlist, &showlist);
 
-  init_page_info ();
-
   add_setshow_boolean_cmd ("pagination", class_support,
 			   &pagination_enabled, _("\
 Set state of GDB output pagination."), _("\
-- 
2.4.0.rc2.31.g7c597ef

Re: [PATCH 3/3] Disable readline's SIGWINCH handler

[Pedro Alves]

On 04/24/2015 01:53 AM, Patrick Palka wrote:
> We no longer need it as we handle SIGWINCH ourselves.  Also move the
> call to init_page_info() from initialize_utils() to the latter
> function's only caller, gdb_init().
> 
> gdb/ChangeLog:
> 
> 	* utils.c (init_page_info): Set rl_catch_sigwinch to zero.
> 	(initialize_utils): Move call of init_page_info() to ...
> 	* top.c (gdb_init): ... here.

OK.

Thanks,
Pedro Alves

Re: [PATCH 1/3] Introduce function for directly updating GDB's screen dimensions

[Pedro Alves]

Thanks for doing this.

LGTM.  One nit below.

On 04/24/2015 01:53 AM, Patrick Palka wrote:
>  
> diff --git a/gdb/utils.c b/gdb/utils.c
> index a9350d9..ec2fd87 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -1795,6 +1795,18 @@ set_height_command (char *args, int from_tty, struct cmd_list_element *c)
>    set_screen_size ();
>  }
>  
> +/* Set the screen dimensions to WIDTH and HEIGHT.  */
> +
> +void

We're putting public function comments in the headers nowadays.
So could you please move the describing comment to utils.h, and
put a breadcrumb here instead:

/* See utils.h.  */

> +set_screen_width_and_height (int width, int height)
> +{

Thanks,
Pedro Alves