[PATCH] Fix for gdb.tui/tui-layout-asm.exp

[PATCH] Fix for gdb.tui/tui-layout-asm.exp

[Carl Love]

GDB maintainers:

The following patch fixes an issue seen on PPC64.  The issue is the
width of the columns of data in the terminal window changes based on
the amount of text in each column.  As the test scrolls thru the
window, the column widths change causing the check to see if the scroll
down one line works.  This patch makes the window a little wider so now
the column widths remain constant for the test.

This patch was tested on Power 9 and Intel without any regression
errors.

Please let me know if it is acceptable.

                            Carl Love
---------------------------------------------------------------

Fix for gdb.tui/tui-layout-asm.exp

The width of the window is too narrow to display the entire assembly line.
The width of the columns in the window changes as the test walks thru the
terminal window output.  The column change results in the first and second
reads of the same line to differ.  Increasing the width of the window keeps
the column width consistent thru the test.

gdb/testsuite/ChangeLog
2021-07-20  Carl Love  <cel@us.ibm.com>
---
 gdb/testsuite/gdb.tui/tui-layout-asm.exp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.tui/tui-layout-asm.exp b/gdb/testsuite/gdb.tui/tui-layout-asm.exp
index 19ce333ca9e..75e0d6b3a07 100644
--- a/gdb/testsuite/gdb.tui/tui-layout-asm.exp
+++ b/gdb/testsuite/gdb.tui/tui-layout-asm.exp
@@ -24,7 +24,7 @@ if {[build_executable "failed to prepare" ${testfile} ${srcfile}] == -1} {
     return -1
 }
 
-Term::clean_restart 24 80 $testfile
+Term::clean_restart 24 90 $testfile
 if {![Term::prepare_for_tui]} {
     unsupported "TUI not supported"
     return
@@ -32,7 +32,7 @@ if {![Term::prepare_for_tui]} {
 
 # This puts us into TUI mode, and should display the ASM window.
 Term::command_no_prompt_prefix "layout asm"
-Term::check_box_contents "check asm box contents" 0 0 80 15 "<main>"
+Term::check_box_contents "check asm box contents" 0 0 90 15 "<main>"
 
 # Scroll the ASM window down using the down arrow key.  In an ideal
 # world we'd like to use PageDown here, but currently our terminal
-- 
2.17.1

Re: [PATCH] Fix for gdb.tui/tui-layout-asm.exp

[Andrew Burgess]

* Carl Love <cel@us.ibm.com> [2021-07-21 10:30:55 -0700]:

> 
> GDB maintainers:
> 
> The following patch fixes an issue seen on PPC64.  The issue is the
> width of the columns of data in the terminal window changes based on
> the amount of text in each column.  As the test scrolls thru the
> window, the column widths change causing the check to see if the scroll
> down one line works.

I don't think this last sentence is correct, it feels like some words
are missing from the end maybe?  "...causing the check to see if the
scroll down one line works to fail." maybe?

>                        This patch makes the window a little wider so now
> the column widths remain constant for the test.

I can't think of a better way to solve this issue.  However, I wonder
if there is more we could do to help anyone in the future who might
hit this issue again?

Consider the patch I've attached below.  In this version the width is
still 80 (for now), so it should fail for you.  However, my hope is
that when you look at the log file you should see a big message
printed right next to the failure.  This message will direct you to
the test script, which (I hope) should then explain what's gone
wrong.

My expectation is that you would still need to adjust the window width
from 80 to 90 in a final version of this patch.

How would you feel with this?

Thanks,
Andrew

---

commit 405f91e36a3fa2b231b33e901bc39676539fda57
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Mon Jul 26 13:26:14 2021 +0100

    [wip] gdb/testsuite: issue with tui asm layout
    
    This relates to this patch:
    
      https://sourceware.org/pipermail/gdb-patches/2021-July/181052.html

diff --git a/gdb/testsuite/gdb.tui/tui-layout-asm.exp b/gdb/testsuite/gdb.tui/tui-layout-asm.exp
index 19ce333ca9e..16128032017 100644
--- a/gdb/testsuite/gdb.tui/tui-layout-asm.exp
+++ b/gdb/testsuite/gdb.tui/tui-layout-asm.exp
@@ -24,15 +24,22 @@ if {[build_executable "failed to prepare" ${testfile} ${srcfile}] == -1} {
     return -1
 }
 
-Term::clean_restart 24 80 $testfile
+set tui_asm_window_width 80
+
+Term::clean_restart 24 ${tui_asm_window_width} $testfile
 if {![Term::prepare_for_tui]} {
     unsupported "TUI not supported"
     return
 }
 
+# Helper proc, returns a count of the ' ' characters in STRING.
+proc count_whitespace { string } {
+    return [expr {[llength [split $string { }]] - 1}]
+}
+
 # This puts us into TUI mode, and should display the ASM window.
 Term::command_no_prompt_prefix "layout asm"
-Term::check_box_contents "check asm box contents" 0 0 80 15 "<main>"
+Term::check_box_contents "check asm box contents" 0 0 ${tui_asm_window_width} 15 "<main>"
 
 # Scroll the ASM window down using the down arrow key.  In an ideal
 # world we'd like to use PageDown here, but currently our terminal
@@ -65,6 +72,26 @@ while (1) {
 	    && [regexp $re_line [Term::get_line 1]]} {
 	# We scrolled successfully.
     } else {
+	if {[count_whitespace ${line}] != \
+		[count_whitespace [Term::get_line 1]]} {
+	    # GDB's TUI assembler display will widen columns based on
+	    # the longest item that appears in a column on any line.
+	    # As we have just scrolled, and so revealed a new line, it
+	    # is possible that the width of some columns has changed.
+	    #
+	    # As a result it is possible that part of the line we were
+	    # expected to see in the output is now off the screen. And
+	    # this test will fail.
+	    #
+	    # This is unfortunate, but, right now, there's no easy way
+	    # to "lock" the format of the TUI assembler window.  The
+	    # only option appears to be making the window width wider,
+	    # this can be done by adjusting TUI_ASM_WINDOW_WIDTH.
+	    verbose -log "WARNING: The following failure is probably due to the TUI window"
+	    verbose -log "         width.  See the comments in the test script for more"
+	    verbose -log "         details."
+	}
+
 	fail "$testname (scroll failed)"
 	Term::dump_screen
 	break

RE: [PATCH] Fix for gdb.tui/tui-layout-asm.exp

[Carl Love]

Andrew:

On Mon, 2021-07-26 at 13:33 +0100, Andrew Burgess wrote:
> I can't think of a better way to solve this issue.  However, I wonder
> if there is more we could do to help anyone in the future who might
> hit this issue again?
> 
> Consider the patch I've attached below.  In this version the width is
> still 80 (for now), so it should fail for you.  However, my hope is
> that when you look at the log file you should see a big message
> printed right next to the failure.  This message will direct you to
> the test script, which (I hope) should then explain what's gone
> wrong.
> 
> My expectation is that you would still need to adjust the window
> width
> from 80 to 90 in a final version of this patch.
> 
> How would you feel with this?

Yes, I like it.  The issue was really not obvious to me at first.  As
embaressing as it is to admit, it took me way too long to realize what
the error was.  I looked at a number of other things that I thought
could be the cause of the failure, timeout, read failure etc.

I wasn't all that happy with just changing the window from 80 to 90 as
it was a bit of an arbitrary choice.  There is no way to ensure that
will cover all possible cases in the future.  The additional checks and
messages should really help in the future if the issue occurs again.  I
did note in the patch that PPC needs it to be at least 90 characters
wide so someone would know why 90 was picked.  Otherwise, it is just an
arbitrary looking setting.

I tested the patch with a width of 80 to verify the test fails and the
warning messages are printed to the log.  I then changed the width to
90 and verified the test now works correctly.  I have attached you
patch with the minor PPC64 changes.  Please let me know if it looks ok.
Thanks for all the help.

                        Carl Love

------------------------------------------------------------
Fix for gdb.tui/tui-layout-asm.exp

The width of the window is too narrow to display the entire assembly line.
The width of the columns in the window changes as the test walks thru the
terminal window output.  The column change results in the first and second
reads of the same line to differ thus causing the test to fail.  Increasing
the width of the window keeps the column width consistent thru the test.

If the test fails, the added check prints an message to the log file if
the failure may be due to the window being too narrow.

gdb/testsuite/ChangeLog

	*gdb.tui/tui-layout-asm.exp: Replace window width of 80 with the
	 tui_asm_window_width variable for the width.
	Add count_whitespace proceedure.
	Add if count_whitespace check.
---
 gdb/testsuite/gdb.tui/tui-layout-asm.exp | 32 ++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.tui/tui-layout-asm.exp b/gdb/testsuite/gdb.tui/tui-layout-asm.exp
index 19ce333ca9e..3ff5347f5ed 100644
--- a/gdb/testsuite/gdb.tui/tui-layout-asm.exp
+++ b/gdb/testsuite/gdb.tui/tui-layout-asm.exp
@@ -24,15 +24,23 @@ if {[build_executable "failed to prepare" ${testfile} ${srcfile}] == -1} {
     return -1
 }
 
-Term::clean_restart 24 80 $testfile
+# PPC currently needs a minimum window width of 90 to work correctly.
+set tui_asm_window_width 90
+
+Term::clean_restart 24 ${tui_asm_window_width} $testfile
 if {![Term::prepare_for_tui]} {
     unsupported "TUI not supported"
     return
 }
 
+# Helper proc, returns a count of the ' ' characters in STRING.
+proc count_whitespace { string } {
+    return [expr {[llength [split $string { }]] - 1}]
+}
+
 # This puts us into TUI mode, and should display the ASM window.
 Term::command_no_prompt_prefix "layout asm"
-Term::check_box_contents "check asm box contents" 0 0 80 15 "<main>"
+Term::check_box_contents "check asm box contents" 0 0 ${tui_asm_window_width} 15 "<main>"
 
 # Scroll the ASM window down using the down arrow key.  In an ideal
 # world we'd like to use PageDown here, but currently our terminal
@@ -65,6 +73,26 @@ while (1) {
 	    && [regexp $re_line [Term::get_line 1]]} {
 	# We scrolled successfully.
     } else {
+	if {[count_whitespace ${line}] != \
+		[count_whitespace [Term::get_line 1]]} {
+	    # GDB's TUI assembler display will widen columns based on
+	    # the longest item that appears in a column on any line.
+	    # As we have just scrolled, and so revealed a new line, it
+	    # is possible that the width of some columns has changed.
+	    #
+	    # As a result it is possible that part of the line we were
+	    # expected to see in the output is now off the screen. And
+	    # this test will fail.
+	    #
+	    # This is unfortunate, but, right now, there's no easy way
+	    # to "lock" the format of the TUI assembler window.  The
+	    # only option appears to be making the window width wider,
+	    # this can be done by adjusting TUI_ASM_WINDOW_WIDTH.
+	    verbose -log "WARNING: The following failure is probably due to the TUI window"
+	    verbose -log "         width.  See the comments in the test script for more"
+	    verbose -log "         details."
+	}
+
 	fail "$testname (scroll failed)"
 	Term::dump_screen
 	break
-- 
2.17.1

Re: [PATCH] Fix for gdb.tui/tui-layout-asm.exp

[Andrew Burgess]

* Carl Love <cel@us.ibm.com> [2021-07-26 09:59:11 -0700]:

> Andrew:
> 
> On Mon, 2021-07-26 at 13:33 +0100, Andrew Burgess wrote:
> > I can't think of a better way to solve this issue.  However, I wonder
> > if there is more we could do to help anyone in the future who might
> > hit this issue again?
> > 
> > Consider the patch I've attached below.  In this version the width is
> > still 80 (for now), so it should fail for you.  However, my hope is
> > that when you look at the log file you should see a big message
> > printed right next to the failure.  This message will direct you to
> > the test script, which (I hope) should then explain what's gone
> > wrong.
> > 
> > My expectation is that you would still need to adjust the window
> > width
> > from 80 to 90 in a final version of this patch.
> > 
> > How would you feel with this?
> 
> Yes, I like it.  The issue was really not obvious to me at first.  As
> embaressing as it is to admit, it took me way too long to realize what
> the error was.  I looked at a number of other things that I thought
> could be the cause of the failure, timeout, read failure etc.
> 
> I wasn't all that happy with just changing the window from 80 to 90 as
> it was a bit of an arbitrary choice.  There is no way to ensure that
> will cover all possible cases in the future.  The additional checks and
> messages should really help in the future if the issue occurs again.  I
> did note in the patch that PPC needs it to be at least 90 characters
> wide so someone would know why 90 was picked.  Otherwise, it is just an
> arbitrary looking setting.
> 
> I tested the patch with a width of 80 to verify the test fails and the
> warning messages are printed to the log.  I then changed the width to
> 90 and verified the test now works correctly.  I have attached you
> patch with the minor PPC64 changes.  Please let me know if it looks ok.
> Thanks for all the help.
> 
>                         Carl Love
> 
> ------------------------------------------------------------
> Fix for gdb.tui/tui-layout-asm.exp
> 
> The width of the window is too narrow to display the entire assembly line.
> The width of the columns in the window changes as the test walks thru the
> terminal window output.  The column change results in the first and second
> reads of the same line to differ thus causing the test to fail.  Increasing
> the width of the window keeps the column width consistent thru the test.
> 
> If the test fails, the added check prints an message to the log file if
> the failure may be due to the window being too narrow.
> 
> gdb/testsuite/ChangeLog
> 
> 	*gdb.tui/tui-layout-asm.exp: Replace window width of 80 with the
> 	 tui_asm_window_width variable for the width.
> 	Add count_whitespace proceedure.
> 	Add if count_whitespace check.

The ChangeLog block should be formatted as:

	* gdb.tui/tui-layout-asm.exp: Replace window width of 80 with
	the tui_asm_window_width variable for the width.  Add if
	count_whitespace check.
	(count_whitespace): New proc.

Otherwise, this looks good to me.  I think it would be a good idea
though to wait for 48hrs before pushing this, just in case someone
else wants to comment.

Thanks,
Andrew

> ---
>  gdb/testsuite/gdb.tui/tui-layout-asm.exp | 32 ++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.tui/tui-layout-asm.exp b/gdb/testsuite/gdb.tui/tui-layout-asm.exp
> index 19ce333ca9e..3ff5347f5ed 100644
> --- a/gdb/testsuite/gdb.tui/tui-layout-asm.exp
> +++ b/gdb/testsuite/gdb.tui/tui-layout-asm.exp
> @@ -24,15 +24,23 @@ if {[build_executable "failed to prepare" ${testfile} ${srcfile}] == -1} {
>      return -1
>  }
>  
> -Term::clean_restart 24 80 $testfile
> +# PPC currently needs a minimum window width of 90 to work correctly.
> +set tui_asm_window_width 90
> +
> +Term::clean_restart 24 ${tui_asm_window_width} $testfile
>  if {![Term::prepare_for_tui]} {
>      unsupported "TUI not supported"
>      return
>  }
>  
> +# Helper proc, returns a count of the ' ' characters in STRING.
> +proc count_whitespace { string } {
> +    return [expr {[llength [split $string { }]] - 1}]
> +}
> +
>  # This puts us into TUI mode, and should display the ASM window.
>  Term::command_no_prompt_prefix "layout asm"
> -Term::check_box_contents "check asm box contents" 0 0 80 15 "<main>"
> +Term::check_box_contents "check asm box contents" 0 0 ${tui_asm_window_width} 15 "<main>"
>  
>  # Scroll the ASM window down using the down arrow key.  In an ideal
>  # world we'd like to use PageDown here, but currently our terminal
> @@ -65,6 +73,26 @@ while (1) {
>  	    && [regexp $re_line [Term::get_line 1]]} {
>  	# We scrolled successfully.
>      } else {
> +	if {[count_whitespace ${line}] != \
> +		[count_whitespace [Term::get_line 1]]} {
> +	    # GDB's TUI assembler display will widen columns based on
> +	    # the longest item that appears in a column on any line.
> +	    # As we have just scrolled, and so revealed a new line, it
> +	    # is possible that the width of some columns has changed.
> +	    #
> +	    # As a result it is possible that part of the line we were
> +	    # expected to see in the output is now off the screen. And
> +	    # this test will fail.
> +	    #
> +	    # This is unfortunate, but, right now, there's no easy way
> +	    # to "lock" the format of the TUI assembler window.  The
> +	    # only option appears to be making the window width wider,
> +	    # this can be done by adjusting TUI_ASM_WINDOW_WIDTH.
> +	    verbose -log "WARNING: The following failure is probably due to the TUI window"
> +	    verbose -log "         width.  See the comments in the test script for more"
> +	    verbose -log "         details."
> +	}
> +
>  	fail "$testname (scroll failed)"
>  	Term::dump_screen
>  	break
> -- 
> 2.17.1
> 
>