[PATCH 1/3] [gdb/tui] Add maint set/show tui-left-margin-verbose

[PATCH 1/3] [gdb/tui] Add maint set/show tui-left-margin-verbose

[Tom de Vries]

The TUI has two types of windows derived from tui_source_window_base:
- tui_source_window (the source window), and
- tui_disasm_window (the disassembly window).

The two windows share a common concept: the left margin.

With a hello world a.out, we can see the source window:
...
┌─/home/vries/hello.c───────────────────────────────────────┐
│        5  {                                               │
│B+>     6    printf ("hello\n");                           │
│        7    return 0;                                     │
│        8  }                                               │
│        9                                                  │
│
...
where the left margin is the part holding "B+>" and the line number, and the
disassembly window:
...
┌───────────────────────────────────────────────────────────┐
│    0x555555555149 <main>           endbr64                │
│    0x55555555514d <main+4>         push   %rbp            │
│    0x55555555514e <main+5>         mov    %rsp,%rbp       │
│B+> 0x555555555151 <main+8>         lea    0xeac(%rip),%rax│
│    0x555555555158 <main+15>        mov    %rax,%rdi       │
...
where the left margin is just the bit holding "B+>".

Because the left margin contains some spaces, it's not clear where it starts
and ends, making it harder to observe problems related to it.

Add a new maintenance command "maint set tui-left-margin-verbose", that when
set to on replaces the spaces in the left margin with either '_' or '0',
giving us this for the source window:
...
┌─/home/vries/hello.c───────────────────────────────────────┐
│___000005__{                                               │
│B+>000006__  printf ("hello\n");                           │
│___000007__  return 0;                                     │
│___000008__}                                               │
...
and this for the disassembly window:
...
┌───────────────────────────────────────────────────────────┐
│___ 0x555555555149 <main>           endbr64                │
│___ 0x55555555514d <main+4>         push   %rbp            │
│___ 0x55555555514e <main+5>         mov    %rsp,%rbp       │
│B+> 0x555555555151 <main+8>         lea    0xeac(%rip),%rax│
│___ 0x555555555158 <main+15>        mov    %rax,%rdi       │
...

Note the space between "B+>" and 0x555555555151.  The space shows that a bit
of the left margin is not written, a problem reported as PR tui/30325.

Specifically, PR tui/30325 is about the fact that the '[' character from the
string "[ No Assembly Available ]" ends up in that same spot:
...
│B+>[0x555555555151 <main+8>         lea    0xeac(%rip),%rax│
...
which only happens for certain window widths.

The new command allows us to spot the problem with any window width.

Likewise, when we revert the fix from commit 1b6d4bb2232 ("Redraw both spaces
between line numbers and source code"), we have:
...
┌─/home/vries/hello.c───────────────────────────────────────┐
│___000005_ {                                               │
│B+>000006_   printf ("hello\n");                           │
│___000007_   return 0;                                     │
│___000008_ }                                               │
...
showing a similar problem at the space between '_' and '{'.

Tested on x86_64-linux.
---
 gdb/doc/gdb.texinfo     |  8 ++++++++
 gdb/tui/tui-source.c    |  5 ++++-
 gdb/tui/tui-win.c       | 16 ++++++++++++++++
 gdb/tui/tui-win.h       |  3 +++
 gdb/tui/tui-winsource.c |  7 ++++++-
 5 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 2d5358a792b..bb96ddb2d52 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -41242,6 +41242,14 @@ has been resized.  This setting is intended for use by the test suite,
 where it would otherwise be difficult to determine when a resize and
 refresh has been completed.
 
+@kindex maint set tui-left-margin-verbose
+@kindex maint show tui-left-margin-verbose
+@item maint set tui-left-margin-verbose
+@item maint show tui-left-margin-verbose
+Control whether the left margin of the TUI source and disassembly windows
+uses @samp{_} and @samp{0} at locations where otherwise there would be a
+space.  The default is @code{off}, which means spaces are used.
+
 @kindex maint set per-command
 @kindex maint show per-command
 @item maint set per-command
diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
index 73d44417533..aa3e58407c4 100644
--- a/gdb/tui/tui-source.c
+++ b/gdb/tui/tui-source.c
@@ -234,6 +234,9 @@ tui_source_window::show_line_number (int offset) const
   char text[20];
   /* To completely overwrite the previous border when the source window height
      is increased, both spaces after the line number have to be redrawn.  */
-  xsnprintf (text, sizeof (text), "%*d  ", m_digits - 1, lineno);
+  char space = tui_left_margin_verbose ? '_' : ' ';
+  xsnprintf (text, sizeof (text),
+	     tui_left_margin_verbose ? "%0*d%c%c" : "%*d%c%c", m_digits - 1,
+	     lineno, space, space);
   waddstr (handle.get (), text);
 }
diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
index 008189eb99b..3b17cb8dd29 100644
--- a/gdb/tui/tui-win.c
+++ b/gdb/tui/tui-win.c
@@ -1111,6 +1111,10 @@ tui_window_command (const char *args, int from_tty)
   help_list (tui_window_cmds, "tui window ", all_commands, gdb_stdout);
 }
 
+/* See tui-win.h.  */
+
+bool tui_left_margin_verbose = false;
+
 /* Function to initialize gdb commands, for tui window
    manipulation.  */
 
@@ -1284,6 +1288,18 @@ position indicator is styled."),
 			   &style_set_list,
 			   &style_show_list);
 
+  add_setshow_boolean_cmd ("tui-left-margin-verbose", class_maintenance,
+			   &tui_left_margin_verbose, _("\
+Set whether the left margin should use '_' and '0' instead of spaces."),
+			   _("\
+Show whether the left margin should use '_' and '0' instead of spaces."),
+			   _("\
+When enabled, the left margin will use '_' and '0' instead of spaces."),
+			   nullptr,
+			   nullptr,
+			   &maintenance_set_cmdlist,
+			   &maintenance_show_cmdlist);
+
   tui_border_style.changed.attach (tui_rehighlight_all, "tui-win");
   tui_active_border_style.changed.attach (tui_rehighlight_all, "tui-win");
 }
diff --git a/gdb/tui/tui-win.h b/gdb/tui/tui-win.h
index 4b33f1f2b54..3d35f1dfb7f 100644
--- a/gdb/tui/tui-win.h
+++ b/gdb/tui/tui-win.h
@@ -55,4 +55,7 @@ extern bool compact_source;
    current position indicator.  */
 extern bool style_tui_current_position;
 
+/* Whether to replace the spaces in the left margin with '_' and '0'.  */
+extern bool tui_left_margin_verbose;
+
 #endif /* TUI_TUI_WIN_H */
diff --git a/gdb/tui/tui-winsource.c b/gdb/tui/tui-winsource.c
index 52a0f7af00f..6c69fb7a907 100644
--- a/gdb/tui/tui-winsource.c
+++ b/gdb/tui/tui-winsource.c
@@ -666,7 +666,12 @@ tui_source_window_base::update_exec_info (bool refresh_p)
   for (int i = 0; i < m_content.size (); i++)
     {
       struct tui_source_element *src_element = &m_content[i];
-      char element[TUI_EXECINFO_SIZE] = "   ";
+      char element[TUI_EXECINFO_SIZE];
+      /* Initialize all but last element.  */
+      char space = tui_left_margin_verbose ? '_' : ' ';
+      memset (element, space, TUI_EXECINFO_SIZE - 1);
+      /* Initialize last element.  */
+      element[TUI_EXECINFO_SIZE - 1] = '\0';
 
       /* Now update the exec info content based upon the state
 	 of each line as indicated by the source content.  */

base-commit: 44019209faf3db952a6a04aaeeaa779a8ff7e661
-- 
2.35.3

[PATCH 2/3] [gdb/tui] Fix left margin in disassembly window

[Tom de Vries]

With a hello world a.out, and maint set tui-left-margin-verbose on, we have
this disassembly window:
...
┌───────────────────────────────────────────────────────────┐
│___ 0x555555555149 <main>           endbr64                │
│___ 0x55555555514d <main+4>         push   %rbp            │
│___ 0x55555555514e <main+5>         mov    %rsp,%rbp       │
│B+> 0x555555555151 <main+8>         lea    0xeac(%rip),%rax│
│___ 0x555555555158 <main+15>        mov    %rax,%rdi       │
...
Note the space between "B+>" and 0x555555555151.  The space shows that a bit
of the left margin is not written, which is a problem because that location is
showing a character previously written, which happens to be a space, but also
may be something else, for instance a '[' as reported in PR tui/30325.

The problem is caused by confusion about the meaning of:
...
 #define TUI_EXECINFO_SIZE 4
...

There's the meaning of defining the size of this zero-terminated char array:
...
      char element[TUI_EXECINFO_SIZE];
...
which is used to print the "B+>" bit, which is 3 chars wide.

And there's the meaning of defining part of the size of the left margin:
...
  int left_margin () const
  { return 1 + TUI_EXECINFO_SIZE + extra_margin (); }
...
where it represents 4 chars.

The discrepancy between the two causes the space between "B+>" and
"0x555555555151".

Fix this by redefining TUI_EXECINFO_SIZE to 3, and using:
...
      char element[TUI_EXECINFO_SIZE + 1];
...
such that we have:
...
|B+>0x555555555151 <main+8>         lea    0xeac(%rip),%rax │
...

This changes the layout of the disassembly window back to what it was before
commit 9e820dec13e ("Use a curses pad for source and disassembly windows"),
the commit that introduced the PR30325 regression.

This also changes the source window from:
...
│___000005__{                                               |
...
to:
...
│___000005_{                                                |
...

Tested on x86_64-linux.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30325
---
 gdb/tui/tui-winsource.c | 7 ++++---
 gdb/tui/tui-winsource.h | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/gdb/tui/tui-winsource.c b/gdb/tui/tui-winsource.c
index 6c69fb7a907..84f9d97c554 100644
--- a/gdb/tui/tui-winsource.c
+++ b/gdb/tui/tui-winsource.c
@@ -666,12 +666,13 @@ tui_source_window_base::update_exec_info (bool refresh_p)
   for (int i = 0; i < m_content.size (); i++)
     {
       struct tui_source_element *src_element = &m_content[i];
-      char element[TUI_EXECINFO_SIZE];
+      /* Add 1 for '\0'.  */
+      char element[TUI_EXECINFO_SIZE + 1];
       /* Initialize all but last element.  */
       char space = tui_left_margin_verbose ? '_' : ' ';
-      memset (element, space, TUI_EXECINFO_SIZE - 1);
+      memset (element, space, TUI_EXECINFO_SIZE);
       /* Initialize last element.  */
-      element[TUI_EXECINFO_SIZE - 1] = '\0';
+      element[TUI_EXECINFO_SIZE] = '\0';
 
       /* Now update the exec info content based upon the state
 	 of each line as indicated by the source content.  */
diff --git a/gdb/tui/tui-winsource.h b/gdb/tui/tui-winsource.h
index 7370ae95d8b..a8ff94f5769 100644
--- a/gdb/tui/tui-winsource.h
+++ b/gdb/tui/tui-winsource.h
@@ -58,7 +58,7 @@ DEF_ENUM_FLAGS_TYPE (enum tui_bp_flag, tui_bp_flags);
 #define TUI_BP_HIT_POS      0
 #define TUI_BP_BREAK_POS    1
 #define TUI_EXEC_POS        2
-#define TUI_EXECINFO_SIZE   4
+#define TUI_EXECINFO_SIZE   3
 
 /* Elements in the Source/Disassembly Window.  */
 struct tui_source_element
-- 
2.35.3

[PATCH 3/3] [gdb/tui] Revert workaround in tui_source_window::show_line_number

[Tom de Vries]

The m_digits member of tui_source_window is documented as having semantics:
...
  /* How many digits to use when formatting the line number.  This
     includes the trailing space.  */
...

The commit 1b6d4bb2232 ("Redraw both spaces between line numbers and source
code") started printing two trailing spaces instead:
...
-  xsnprintf (text, sizeof (text), "%*d ", m_digits - 1, lineno);
+  xsnprintf (text, sizeof (text), "%*d  ", m_digits - 1, lineno);
...

Now that PR30325 is fixed, this no longer has any effect.

Fix this by reverting to the original behaviour: print one trailing space
char.

Tested on x86_64-linux.
---
 gdb/tui/tui-source.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
index aa3e58407c4..3d08ea8b7cd 100644
--- a/gdb/tui/tui-source.c
+++ b/gdb/tui/tui-source.c
@@ -232,11 +232,9 @@ tui_source_window::show_line_number (int offset) const
 {
   int lineno = m_content[0].line_or_addr.u.line_no + offset;
   char text[20];
-  /* To completely overwrite the previous border when the source window height
-     is increased, both spaces after the line number have to be redrawn.  */
   char space = tui_left_margin_verbose ? '_' : ' ';
   xsnprintf (text, sizeof (text),
-	     tui_left_margin_verbose ? "%0*d%c%c" : "%*d%c%c", m_digits - 1,
-	     lineno, space, space);
+	     tui_left_margin_verbose ? "%0*d%c" : "%*d%c", m_digits - 1,
+	     lineno, space);
   waddstr (handle.get (), text);
 }
-- 
2.35.3

Re: [PATCH 1/3] [gdb/tui] Add maint set/show tui-left-margin-verbose

[Eli Zaretskii]

> Cc: Tom Tromey <tom@tromey.com>
> Date: Tue, 11 Apr 2023 10:23:30 +0200
> From: Tom de Vries via Gdb-patches <gdb-patches@sourceware.org>
> 
>  gdb/doc/gdb.texinfo     |  8 ++++++++
>  gdb/tui/tui-source.c    |  5 ++++-
>  gdb/tui/tui-win.c       | 16 ++++++++++++++++
>  gdb/tui/tui-win.h       |  3 +++
>  gdb/tui/tui-winsource.c |  7 ++++++-
>  5 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 2d5358a792b..bb96ddb2d52 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -41242,6 +41242,14 @@ has been resized.  This setting is intended for use by the test suite,
>  where it would otherwise be difficult to determine when a resize and
>  refresh has been completed.
>  
> +@kindex maint set tui-left-margin-verbose
> +@kindex maint show tui-left-margin-verbose
> +@item maint set tui-left-margin-verbose
> +@item maint show tui-left-margin-verbose
> +Control whether the left margin of the TUI source and disassembly windows
> +uses @samp{_} and @samp{0} at locations where otherwise there would be a
> +space.  The default is @code{off}, which means spaces are used.

This is okay, but I suggest adding a sentence explaining the rationale
for using this command.  Otherwise, its purpose is unclear, not
without reading the description you included in the commit log.

Thanks.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 1/3] [gdb/tui] Add maint set/show tui-left-margin-verbose

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Add a new maintenance command "maint set tui-left-margin-verbose", that when
Tom> set to on replaces the spaces in the left margin with either '_' or '0',
Tom> giving us this for the source window:

This looks good to me.  Thank you.

Tom

Re: [PATCH 2/3] [gdb/tui] Fix left margin in disassembly window

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Fix this by redefining TUI_EXECINFO_SIZE to 3, and using:
Tom> ...
Tom>       char element[TUI_EXECINFO_SIZE + 1];
Tom> ...
Tom> such that we have:
Tom> ...
Tom> |B+>0x555555555151 <main+8>         lea    0xeac(%rip),%rax │
Tom> ...

Looks good.  Thank you for finding this.

Tom

Re: [PATCH 3/3] [gdb/tui] Revert workaround in tui_source_window::show_line_number

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Fix this by reverting to the original behaviour: print one trailing space
Tom> char.

Looks good.  Thanks for doing this.

Tom

Re: [PATCH 2/3] [gdb/tui] Fix left margin in disassembly window

[Andrew Burgess]

Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

> With a hello world a.out, and maint set tui-left-margin-verbose on, we have
> this disassembly window:
> ...
> ┌───────────────────────────────────────────────────────────┐
> │___ 0x555555555149 <main>           endbr64                │
> │___ 0x55555555514d <main+4>         push   %rbp            │
> │___ 0x55555555514e <main+5>         mov    %rsp,%rbp       │
> │B+> 0x555555555151 <main+8>         lea    0xeac(%rip),%rax│
> │___ 0x555555555158 <main+15>        mov    %rax,%rdi       │
> ...
> Note the space between "B+>" and 0x555555555151.  The space shows that a bit
> of the left margin is not written, which is a problem because that location is
> showing a character previously written, which happens to be a space, but also
> may be something else, for instance a '[' as reported in PR tui/30325.
>
> The problem is caused by confusion about the meaning of:
> ...
>  #define TUI_EXECINFO_SIZE 4
> ...
>
> There's the meaning of defining the size of this zero-terminated char array:
> ...
>       char element[TUI_EXECINFO_SIZE];
> ...
> which is used to print the "B+>" bit, which is 3 chars wide.
>
> And there's the meaning of defining part of the size of the left margin:
> ...
>   int left_margin () const
>   { return 1 + TUI_EXECINFO_SIZE + extra_margin (); }
> ...
> where it represents 4 chars.
>
> The discrepancy between the two causes the space between "B+>" and
> "0x555555555151".
>
> Fix this by redefining TUI_EXECINFO_SIZE to 3, and using:
> ...
>       char element[TUI_EXECINFO_SIZE + 1];
> ...
> such that we have:
> ...
> |B+>0x555555555151 <main+8>         lea    0xeac(%rip),%rax │
> ...
>
> This changes the layout of the disassembly window back to what it was before
> commit 9e820dec13e ("Use a curses pad for source and disassembly windows"),
> the commit that introduced the PR30325 regression.
>
> This also changes the source window from:
> ...
> │___000005__{                                               |
> ...
> to:
> ...
> │___000005_{                                                |
> ...
>
> Tested on x86_64-linux.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30325
> ---
>  gdb/tui/tui-winsource.c | 7 ++++---
>  gdb/tui/tui-winsource.h | 2 +-
>  2 files changed, 5 insertions(+), 4 deletions(-)

I'm surprised there's no new test for this.  I kind of assumed the new
maint command (from patch #1) was to make writing the test easier...

Thanks,
Andrew


>
> diff --git a/gdb/tui/tui-winsource.c b/gdb/tui/tui-winsource.c
> index 6c69fb7a907..84f9d97c554 100644
> --- a/gdb/tui/tui-winsource.c
> +++ b/gdb/tui/tui-winsource.c
> @@ -666,12 +666,13 @@ tui_source_window_base::update_exec_info (bool refresh_p)
>    for (int i = 0; i < m_content.size (); i++)
>      {
>        struct tui_source_element *src_element = &m_content[i];
> -      char element[TUI_EXECINFO_SIZE];
> +      /* Add 1 for '\0'.  */
> +      char element[TUI_EXECINFO_SIZE + 1];
>        /* Initialize all but last element.  */
>        char space = tui_left_margin_verbose ? '_' : ' ';
> -      memset (element, space, TUI_EXECINFO_SIZE - 1);
> +      memset (element, space, TUI_EXECINFO_SIZE);
>        /* Initialize last element.  */
> -      element[TUI_EXECINFO_SIZE - 1] = '\0';
> +      element[TUI_EXECINFO_SIZE] = '\0';
>  
>        /* Now update the exec info content based upon the state
>  	 of each line as indicated by the source content.  */
> diff --git a/gdb/tui/tui-winsource.h b/gdb/tui/tui-winsource.h
> index 7370ae95d8b..a8ff94f5769 100644
> --- a/gdb/tui/tui-winsource.h
> +++ b/gdb/tui/tui-winsource.h
> @@ -58,7 +58,7 @@ DEF_ENUM_FLAGS_TYPE (enum tui_bp_flag, tui_bp_flags);
>  #define TUI_BP_HIT_POS      0
>  #define TUI_BP_BREAK_POS    1
>  #define TUI_EXEC_POS        2
> -#define TUI_EXECINFO_SIZE   4
> +#define TUI_EXECINFO_SIZE   3
>  
>  /* Elements in the Source/Disassembly Window.  */
>  struct tui_source_element
> -- 
> 2.35.3

Re: [PATCH 1/3] [gdb/tui] Add maint set/show tui-left-margin-verbose

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1488 bytes --]

On 4/11/23 10:47, Eli Zaretskii wrote:
>> Cc: Tom Tromey <tom@tromey.com>
>> Date: Tue, 11 Apr 2023 10:23:30 +0200
>> From: Tom de Vries via Gdb-patches <gdb-patches@sourceware.org>
>>
>>   gdb/doc/gdb.texinfo     |  8 ++++++++
>>   gdb/tui/tui-source.c    |  5 ++++-
>>   gdb/tui/tui-win.c       | 16 ++++++++++++++++
>>   gdb/tui/tui-win.h       |  3 +++
>>   gdb/tui/tui-winsource.c |  7 ++++++-
>>   5 files changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>> index 2d5358a792b..bb96ddb2d52 100644
>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -41242,6 +41242,14 @@ has been resized.  This setting is intended for use by the test suite,
>>   where it would otherwise be difficult to determine when a resize and
>>   refresh has been completed.
>>   
>> +@kindex maint set tui-left-margin-verbose
>> +@kindex maint show tui-left-margin-verbose
>> +@item maint set tui-left-margin-verbose
>> +@item maint show tui-left-margin-verbose
>> +Control whether the left margin of the TUI source and disassembly windows
>> +uses @samp{_} and @samp{0} at locations where otherwise there would be a
>> +space.  The default is @code{off}, which means spaces are used.
> 
> This is okay, but I suggest adding a sentence explaining the rationale
> for using this command.  Otherwise, its purpose is unclear, not
> without reading the description you included in the commit log.

Done, and committed as attached.

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-tui-Add-maint-set-show-tui-left-margin-verbose.patch --]
[-- Type: text/x-patch, Size: 8614 bytes --]

From 345b95b017fc7db8225cf9feacdb8ddc80a884b4 Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Mon, 10 Apr 2023 10:09:36 +0200
Subject: [PATCH 1/3] [gdb/tui] Add maint set/show tui-left-margin-verbose
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The TUI has two types of windows derived from tui_source_window_base:
- tui_source_window (the source window), and
- tui_disasm_window (the disassembly window).

The two windows share a common concept: the left margin.

With a hello world a.out, we can see the source window:
...
┌─/home/vries/hello.c───────────────────────────────────────┐
│        5  {                                               │
│B+>     6    printf ("hello\n");                           │
│        7    return 0;                                     │
│        8  }                                               │
│        9                                                  │
│
...
where the left margin is the part holding "B+>" and the line number, and the
disassembly window:
...
┌───────────────────────────────────────────────────────────┐
│    0x555555555149 <main>           endbr64                │
│    0x55555555514d <main+4>         push   %rbp            │
│    0x55555555514e <main+5>         mov    %rsp,%rbp       │
│B+> 0x555555555151 <main+8>         lea    0xeac(%rip),%rax│
│    0x555555555158 <main+15>        mov    %rax,%rdi       │
...
where the left margin is just the bit holding "B+>".

Because the left margin contains some spaces, it's not clear where it starts
and ends, making it harder to observe problems related to it.

Add a new maintenance command "maint set tui-left-margin-verbose", that when
set to on replaces the spaces in the left margin with either '_' or '0',
giving us this for the source window:
...
┌─/home/vries/hello.c───────────────────────────────────────┐
│___000005__{                                               │
│B+>000006__  printf ("hello\n");                           │
│___000007__  return 0;                                     │
│___000008__}                                               │
...
and this for the disassembly window:
...
┌───────────────────────────────────────────────────────────┐
│___ 0x555555555149 <main>           endbr64                │
│___ 0x55555555514d <main+4>         push   %rbp            │
│___ 0x55555555514e <main+5>         mov    %rsp,%rbp       │
│B+> 0x555555555151 <main+8>         lea    0xeac(%rip),%rax│
│___ 0x555555555158 <main+15>        mov    %rax,%rdi       │
...

Note the space between "B+>" and 0x555555555151.  The space shows that a bit
of the left margin is not written, a problem reported as PR tui/30325.

Specifically, PR tui/30325 is about the fact that the '[' character from the
string "[ No Assembly Available ]" ends up in that same spot:
...
│B+>[0x555555555151 <main+8>         lea    0xeac(%rip),%rax│
...
which only happens for certain window widths.

The new command allows us to spot the problem with any window width.

Likewise, when we revert the fix from commit 1b6d4bb2232 ("Redraw both spaces
between line numbers and source code"), we have:
...
┌─/home/vries/hello.c───────────────────────────────────────┐
│___000005_ {                                               │
│B+>000006_   printf ("hello\n");                           │
│___000007_   return 0;                                     │
│___000008_ }                                               │
...
showing a similar problem at the space between '_' and '{'.

Tested on x86_64-linux.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Approved-By: Tom Tromey <tom@tromey.com>
---
 gdb/doc/gdb.texinfo     | 11 +++++++++++
 gdb/tui/tui-source.c    |  5 ++++-
 gdb/tui/tui-win.c       | 16 ++++++++++++++++
 gdb/tui/tui-win.h       |  3 +++
 gdb/tui/tui-winsource.c |  7 ++++++-
 5 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 45a0580bc29..c11457952d2 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -41249,6 +41249,17 @@ has been resized.  This setting is intended for use by the test suite,
 where it would otherwise be difficult to determine when a resize and
 refresh has been completed.
 
+@kindex maint set tui-left-margin-verbose
+@kindex maint show tui-left-margin-verbose
+@item maint set tui-left-margin-verbose
+@item maint show tui-left-margin-verbose
+Control whether the left margin of the TUI source and disassembly windows
+uses @samp{_} and @samp{0} at locations where otherwise there would be a
+space.  The default is @code{off}, which means spaces are used.  The
+setting is intended to make it clear where the left margin begins and
+ends, to avoid incorrectly interpreting a space as being part of the
+the left margin.
+
 @kindex maint set per-command
 @kindex maint show per-command
 @item maint set per-command
diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
index 73d44417533..aa3e58407c4 100644
--- a/gdb/tui/tui-source.c
+++ b/gdb/tui/tui-source.c
@@ -234,6 +234,9 @@ tui_source_window::show_line_number (int offset) const
   char text[20];
   /* To completely overwrite the previous border when the source window height
      is increased, both spaces after the line number have to be redrawn.  */
-  xsnprintf (text, sizeof (text), "%*d  ", m_digits - 1, lineno);
+  char space = tui_left_margin_verbose ? '_' : ' ';
+  xsnprintf (text, sizeof (text),
+	     tui_left_margin_verbose ? "%0*d%c%c" : "%*d%c%c", m_digits - 1,
+	     lineno, space, space);
   waddstr (handle.get (), text);
 }
diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
index 008189eb99b..3b17cb8dd29 100644
--- a/gdb/tui/tui-win.c
+++ b/gdb/tui/tui-win.c
@@ -1111,6 +1111,10 @@ tui_window_command (const char *args, int from_tty)
   help_list (tui_window_cmds, "tui window ", all_commands, gdb_stdout);
 }
 
+/* See tui-win.h.  */
+
+bool tui_left_margin_verbose = false;
+
 /* Function to initialize gdb commands, for tui window
    manipulation.  */
 
@@ -1284,6 +1288,18 @@ position indicator is styled."),
 			   &style_set_list,
 			   &style_show_list);
 
+  add_setshow_boolean_cmd ("tui-left-margin-verbose", class_maintenance,
+			   &tui_left_margin_verbose, _("\
+Set whether the left margin should use '_' and '0' instead of spaces."),
+			   _("\
+Show whether the left margin should use '_' and '0' instead of spaces."),
+			   _("\
+When enabled, the left margin will use '_' and '0' instead of spaces."),
+			   nullptr,
+			   nullptr,
+			   &maintenance_set_cmdlist,
+			   &maintenance_show_cmdlist);
+
   tui_border_style.changed.attach (tui_rehighlight_all, "tui-win");
   tui_active_border_style.changed.attach (tui_rehighlight_all, "tui-win");
 }
diff --git a/gdb/tui/tui-win.h b/gdb/tui/tui-win.h
index 4b33f1f2b54..3d35f1dfb7f 100644
--- a/gdb/tui/tui-win.h
+++ b/gdb/tui/tui-win.h
@@ -55,4 +55,7 @@ extern bool compact_source;
    current position indicator.  */
 extern bool style_tui_current_position;
 
+/* Whether to replace the spaces in the left margin with '_' and '0'.  */
+extern bool tui_left_margin_verbose;
+
 #endif /* TUI_TUI_WIN_H */
diff --git a/gdb/tui/tui-winsource.c b/gdb/tui/tui-winsource.c
index 52a0f7af00f..6c69fb7a907 100644
--- a/gdb/tui/tui-winsource.c
+++ b/gdb/tui/tui-winsource.c
@@ -666,7 +666,12 @@ tui_source_window_base::update_exec_info (bool refresh_p)
   for (int i = 0; i < m_content.size (); i++)
     {
       struct tui_source_element *src_element = &m_content[i];
-      char element[TUI_EXECINFO_SIZE] = "   ";
+      char element[TUI_EXECINFO_SIZE];
+      /* Initialize all but last element.  */
+      char space = tui_left_margin_verbose ? '_' : ' ';
+      memset (element, space, TUI_EXECINFO_SIZE - 1);
+      /* Initialize last element.  */
+      element[TUI_EXECINFO_SIZE - 1] = '\0';
 
       /* Now update the exec info content based upon the state
 	 of each line as indicated by the source content.  */

base-commit: fade906daab13c99fc329c1e08447f2aac168b62
-- 
2.35.3

Re: [PATCH 2/3] [gdb/tui] Fix left margin in disassembly window

[Tom de Vries]

On 4/12/23 23:06, Andrew Burgess wrote:
> I'm surprised there's no new test for this.  I kind of assumed the new
> maint command (from patch #1) was to make writing the test easier...
> 

It certainly could be used for that, yes.  My primary purpose though was 
to make the problem easily visible.

I haven't added a test because I'm still fixing issues I find with the 
current testsuite...

I might add a test-case for this eventually, or hijack one of the 
existing tests.

Thanks,
- Tom

Re: [PATCH 2/3] [gdb/tui] Fix left margin in disassembly window

[Andrew Burgess]

Tom de Vries <tdevries@suse.de> writes:

> On 4/12/23 23:06, Andrew Burgess wrote:
>> I'm surprised there's no new test for this.  I kind of assumed the new
>> maint command (from patch #1) was to make writing the test easier...
>> 
>
> It certainly could be used for that, yes.  My primary purpose though was 
> to make the problem easily visible.
>
> I haven't added a test because I'm still fixing issues I find with the 
> current testsuite...
>
> I might add a test-case for this eventually, or hijack one of the 
> existing tests.

I don't understand -- aren't you proposing this patch for inclusion now?

Patches should be accompanied by a corresponding test.

I really think this should have a test before it's merged.

Thanks,
Andrew

Re: [PATCH 2/3] [gdb/tui] Fix left margin in disassembly window

[Andrew Burgess]

Andrew Burgess <aburgess@redhat.com> writes:

> Tom de Vries <tdevries@suse.de> writes:
>
>> On 4/12/23 23:06, Andrew Burgess wrote:
>>> I'm surprised there's no new test for this.  I kind of assumed the new
>>> maint command (from patch #1) was to make writing the test easier...
>>> 
>>
>> It certainly could be used for that, yes.  My primary purpose though was 
>> to make the problem easily visible.
>>
>> I haven't added a test because I'm still fixing issues I find with the 
>> current testsuite...
>>
>> I might add a test-case for this eventually, or hijack one of the 
>> existing tests.
>
> I don't understand -- aren't you proposing this patch for inclusion now?
>
> Patches should be accompanied by a corresponding test.
>
> I really think this should have a test before it's merged.

Oh, I just noticed you merged this anyway.

GDB development is usually done by consensus.  That would mean if
someone raises a concern -- like a test is missing -- then you don't
merge a patch until the concern has been discussed and addressed.

I've had plenty of patches on the list that have had an approval, and
then a concern raised by someone else.  I don't just choose to see the
approval and merge the patch regardless -- that's just disrespectful to
a pretty legitimate concern I had about this patch.

I know writing tests can be a real drag, but it really is critical to
making GDB a solid product.

Hopefully you can prioritise getting a test written for this patch.

Thanks,
Andrew

Re: [PATCH 2/3] [gdb/tui] Fix left margin in disassembly window

[Tom de Vries]

On 4/13/23 11:25, Andrew Burgess wrote:
> Andrew Burgess <aburgess@redhat.com> writes:
> 
>> Tom de Vries <tdevries@suse.de> writes:
>>
>>> On 4/12/23 23:06, Andrew Burgess wrote:
>>>> I'm surprised there's no new test for this.  I kind of assumed the new
>>>> maint command (from patch #1) was to make writing the test easier...
>>>>
>>>
>>> It certainly could be used for that, yes.  My primary purpose though was
>>> to make the problem easily visible.
>>>
>>> I haven't added a test because I'm still fixing issues I find with the
>>> current testsuite...
>>>
>>> I might add a test-case for this eventually, or hijack one of the
>>> existing tests.
>>
>> I don't understand -- aren't you proposing this patch for inclusion now?
>>
>> Patches should be accompanied by a corresponding test.
>>
>> I really think this should have a test before it's merged.
> 
> Oh, I just noticed you merged this anyway.
> 
> GDB development is usually done by consensus.  That would mean if
> someone raises a concern -- like a test is missing -- then you don't
> merge a patch until the concern has been discussed and addressed.
> 
> I've had plenty of patches on the list that have had an approval, and
> then a concern raised by someone else.  I don't just choose to see the
> approval and merge the patch regardless -- that's just disrespectful to
> a pretty legitimate concern I had about this patch.
> 

Hi Andrew,

I understand that the sequence of events can be read as such, but I went 
ahead and committed after seeing the approval by Tom, without seeing 
your reply, sorry about that.  I only saw it when replying to Eli.

> I know writing tests can be a real drag, but it really is critical to
> making GDB a solid product.
> 

I agree.  And I don't consider it a drag, it's just that I felt a bit 
overwhelmed with other work (that also tries to make GDB a solid product).

> Hopefully you can prioritise getting a test written for this patch.

Yes, now that this ( 
https://sourceware.org/pipermail/gdb-patches/2023-April/198848.html ) is 
out of the door, I'll take a look.

Thanks,
- Tom