[PATCH][gdb/testsuite] Fix gdb.base/style.exp with -m32

[PATCH][gdb/testsuite] Fix gdb.base/style.exp with -m32

[Tom de Vries]

Hi,

When running test-case gdb.base/style.exp with target board unix/-m32, we run
into (stripped styling from output, shortened file name):
...
(gdb) frame
    argv=0xffffc714) at src/gdb/testsuite/gdb.base/style.c:45
45        return some_called_function (); /* break here */
(gdb) FAIL: gdb.base/style.exp: frame when width=20
...
while with native we have instead:
...
(gdb) frame
    argv=0x7fffffffd478)
    at src/gdb/testsuite/gdb.base/style.c:45
45        return some_called_function (); /* break here */
(gdb) PASS: gdb.base/style.exp: frame when width=20
...

The problem is that due to argv having a different length for -m32, we get a
different layout, and the test-case doesn't accommodate for that.

Fix this by first printing the frame without styling and checking the layout,
and then printing the frame with styling and verifying that the layout is the
same.

Tested on x86_64-linux.

Any comments?

Thanks,
- Tom

[gdb/testsuite] Fix gdb.base/style.exp with -m32

gdb/testsuite/ChangeLog:

2021-01-13  Tom de Vries  <tdevries@suse.de>

	PR testsuite/24590
	* gdb.base/style.exp: Handle shorter argv in frame command output.

---
 gdb/testsuite/gdb.base/style.exp | 66 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 60 insertions(+), 6 deletions(-)

diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp
index cc143b43292..848d4d337d4 100644
--- a/gdb/testsuite/gdb.base/style.exp
+++ b/gdb/testsuite/gdb.base/style.exp
@@ -70,21 +70,75 @@ save_vars { env(TERM) } {
     # Regression test for a bug where line-wrapping would occur at the
     # wrong spot with styling.  There were different bugs at different
     # widths, so try two.
-    foreach width {20 30} {
-	gdb_test_no_output "set width $width"
+    foreach_with_prefix width {20 30} {
+
 	# There was also a bug where the styling could be wrong in the
 	# line listing; this is why the words from the source code are
 	# spelled out in the final result line of the test.
-	gdb_test "frame" \
+	set re1 \
+	    [multi_line \
+		 "#0 *main.*argc.*" \
+		 ".*argv.*" \
+		 ".* at .*style\\.c.*" \
+		 "\[0-9\]+.*return.* break here .*"]
+	set re1_styled \
 	    [multi_line \
 		 "#0 *$main_expr.*$arg_expr.*" \
 		 ".*$arg_expr.*" \
 		 ".* at .*$file_expr.*" \
-		 "\[0-9\]+.*return.* break here .*"
-	    ] \
-	    "frame when width=$width"
+		 "\[0-9\]+.*return.* break here .*"]
+	set re2 \
+	    [multi_line \
+		 "#0 *main.*argc.*" \
+		 ".*argv.* at .*style\\.c.*" \
+		 "\[0-9\]+.*return.* break here .*"]
+	set re2_styled \
+	    [multi_line \
+		 "#0 *$main_expr.*$arg_expr.*" \
+		 ".*$arg_expr.* at .*$file_expr.*" \
+		 "\[0-9\]+.*return.* break here .*"]
+
+	set re_styled ""
+	with_test_prefix "style=off" {
+	    # We temporarily set width to 0 to be able to use
+	    # gdb_test_no_output for "set style enabled on".
+	    gdb_test_no_output "set width 0"
+	    gdb_test_no_output "set style enabled off"
+	    gdb_test_no_output "set width $width"
+
+	    # Depending on the width and the length of argv, there may be
+	    # a different layout.
+	    gdb_test_multiple "frame" "" {
+		-re -wrap $re1 {
+		    set re_styled $re1_styled
+		    pass $gdb_test_name
+		}
+		-re -wrap $re2 {
+		    set re_styled $re2_styled
+		    pass $gdb_test_name
+		}
+	    }
+	}
+	if { $re_styled == "" } {
+	    continue
+	}
+
+	with_test_prefix "style=on" {
+	    # We temporarily set width to 0 to be able to use
+	    # gdb_test_no_output for "set style enabled on".
+	    gdb_test_no_output "set width 0"
+	    gdb_test_no_output "set style enabled on"
+	    gdb_test_no_output "set width $width"
+
+	    # Check that the frame has the same layout with "style enabled on"
+	    # as with "style enabled off".
+	    gdb_test "frame" $re_styled
+	}
     }
 
+    # Reset width back to 0.
+    gdb_test_no_output "set width 0"
+
     if {$test_macros} {
 	set macro_line [gdb_get_line_number "\#define SOME_MACRO"]
 	gdb_test "info macro SOME_MACRO" \

Re: [PATCH][gdb/testsuite] Fix gdb.base/style.exp with -m32

[Andrew Burgess]

* Tom de Vries <tdevries@suse.de> [2021-01-13 01:09:30 +0100]:

> Hi,
> 
> When running test-case gdb.base/style.exp with target board unix/-m32, we run
> into (stripped styling from output, shortened file name):
> ...
> (gdb) frame
>     argv=0xffffc714) at src/gdb/testsuite/gdb.base/style.c:45
> 45        return some_called_function (); /* break here */
> (gdb) FAIL: gdb.base/style.exp: frame when width=20
> ...
> while with native we have instead:
> ...
> (gdb) frame
>     argv=0x7fffffffd478)
>     at src/gdb/testsuite/gdb.base/style.c:45
> 45        return some_called_function (); /* break here */
> (gdb) PASS: gdb.base/style.exp: frame when width=20
> ...
> 
> The problem is that due to argv having a different length for -m32, we get a
> different layout, and the test-case doesn't accommodate for that.
> 
> Fix this by first printing the frame without styling and checking the layout,
> and then printing the frame with styling and verifying that the layout is the
> same.
> 
> Tested on x86_64-linux.
> 
> Any comments?
> 
> Thanks,
> - Tom
> 
> [gdb/testsuite] Fix gdb.base/style.exp with -m32
> 
> gdb/testsuite/ChangeLog:
> 
> 2021-01-13  Tom de Vries  <tdevries@suse.de>
> 
> 	PR testsuite/24590
> 	* gdb.base/style.exp: Handle shorter argv in frame command output.
> 
> ---
>  gdb/testsuite/gdb.base/style.exp | 66 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 60 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp
> index cc143b43292..848d4d337d4 100644
> --- a/gdb/testsuite/gdb.base/style.exp
> +++ b/gdb/testsuite/gdb.base/style.exp
> @@ -70,21 +70,75 @@ save_vars { env(TERM) } {
>      # Regression test for a bug where line-wrapping would occur at the
>      # wrong spot with styling.  There were different bugs at different
>      # widths, so try two.
> -    foreach width {20 30} {
> -	gdb_test_no_output "set width $width"
> +    foreach_with_prefix width {20 30} {
> +
>  	# There was also a bug where the styling could be wrong in the
>  	# line listing; this is why the words from the source code are
>  	# spelled out in the final result line of the test.
> -	gdb_test "frame" \
> +	set re1 \
> +	    [multi_line \
> +		 "#0 *main.*argc.*" \
> +		 ".*argv.*" \
> +		 ".* at .*style\\.c.*" \
> +		 "\[0-9\]+.*return.* break here .*"]
> +	set re1_styled \
>  	    [multi_line \
>  		 "#0 *$main_expr.*$arg_expr.*" \
>  		 ".*$arg_expr.*" \
>  		 ".* at .*$file_expr.*" \
> -		 "\[0-9\]+.*return.* break here .*"
> -	    ] \
> -	    "frame when width=$width"
> +		 "\[0-9\]+.*return.* break here .*"]
> +	set re2 \
> +	    [multi_line \
> +		 "#0 *main.*argc.*" \
> +		 ".*argv.* at .*style\\.c.*" \
> +		 "\[0-9\]+.*return.* break here .*"]
> +	set re2_styled \
> +	    [multi_line \
> +		 "#0 *$main_expr.*$arg_expr.*" \
> +		 ".*$arg_expr.* at .*$file_expr.*" \
> +		 "\[0-9\]+.*return.* break here .*"]
> +
> +	set re_styled ""
> +	with_test_prefix "style=off" {
> +	    # We temporarily set width to 0 to be able to use
> +	    # gdb_test_no_output for "set style enabled on".
> +	    gdb_test_no_output "set width 0"
> +	    gdb_test_no_output "set style enabled off"
> +	    gdb_test_no_output "set width $width"
> +
> +	    # Depending on the width and the length of argv, there may be
> +	    # a different layout.
> +	    gdb_test_multiple "frame" "" {
> +		-re -wrap $re1 {
> +		    set re_styled $re1_styled
> +		    pass $gdb_test_name
> +		}
> +		-re -wrap $re2 {
> +		    set re_styled $re2_styled
> +		    pass $gdb_test_name
> +		}
> +	    }
> +	}
> +	if { $re_styled == "" } {
> +	    continue
> +	}

All of the above is really just a way to select between two different
output patterns, the choice of which is based on the length of argv.

So, could we not just skip the complexity, and instead do:

    set argv_value [get_hexadecimal_valueof "argv" "??"]
    if { [string length $argv_value] > 12 } {
       # Choose the first pattern...
    } else {
       # Choose the second pattern...
    }
    foreach_with_prefix width {20 30} {
      # Run the tests using the selected pattern...
    }

Seems like this might be a simpler solution?

Thanks,
Andrew


> +
> +	with_test_prefix "style=on" {
> +	    # We temporarily set width to 0 to be able to use
> +	    # gdb_test_no_output for "set style enabled on".
> +	    gdb_test_no_output "set width 0"
> +	    gdb_test_no_output "set style enabled on"
> +	    gdb_test_no_output "set width $width"
> +
> +	    # Check that the frame has the same layout with "style enabled on"
> +	    # as with "style enabled off".
> +	    gdb_test "frame" $re_styled
> +	}
>      }
>  
> +    # Reset width back to 0.
> +    gdb_test_no_output "set width 0"
> +
>      if {$test_macros} {
>  	set macro_line [gdb_get_line_number "\#define SOME_MACRO"]
>  	gdb_test "info macro SOME_MACRO" \

Re: [PATCH][gdb/testsuite] Fix gdb.base/style.exp with -m32

[Tom de Vries]

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

On 1/13/21 8:47 PM, Andrew Burgess wrote:
> All of the above is really just a way to select between two different
> output patterns, the choice of which is based on the length of argv.
> 
> So, could we not just skip the complexity, and instead do:
> 
>     set argv_value [get_hexadecimal_valueof "argv" "??"]
>     if { [string length $argv_value] > 12 } {
>        # Choose the first pattern...
>     } else {
>        # Choose the second pattern...
>     }
>     foreach_with_prefix width {20 30} {
>       # Run the tests using the selected pattern...
>     }
> 
> Seems like this might be a simpler solution?

In my mind, the pattern matching is the easier solution, and the picking
apart the line buildup and determining where it is wrapped the more
complex (and more fragile) one.  But agreed, the resulting code is less
complex.

Updated the patch to use the approach you suggested, and committed.

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-testsuite-Fix-gdb.base-style.exp-with-m32.patch --]
[-- Type: text/x-patch, Size: 3864 bytes --]

[gdb/testsuite] Fix gdb.base/style.exp with -m32

When running test-case gdb.base/style.exp with target board unix/-m32, we run
into (stripped styling from output, shortened file name):
...
(gdb) frame
    argv=0xffffc714) at src/gdb/testsuite/gdb.base/style.c:45
45        return some_called_function (); /* break here */
(gdb) FAIL: gdb.base/style.exp: frame when width=20
...
while with native we have instead:
...
(gdb) frame
    argv=0x7fffffffd478)
    at src/gdb/testsuite/gdb.base/style.c:45
45        return some_called_function (); /* break here */
(gdb) PASS: gdb.base/style.exp: frame when width=20
...

The problem is that due to argv having a different length for -m32, we get a
different layout, and the test-case doesn't accommodate for that.

Fix this by using a different regexp depending on the length of argv.

Tested on x86_64-linux.

gdb/testsuite/ChangeLog:

2021-01-13  Tom de Vries  <tdevries@suse.de>

	PR testsuite/24590
	* gdb.base/style.exp: Handle shorter argv in frame command output.

---
 gdb/testsuite/gdb.base/style.exp | 49 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp
index cc143b43292..aec1d0f42ed 100644
--- a/gdb/testsuite/gdb.base/style.exp
+++ b/gdb/testsuite/gdb.base/style.exp
@@ -46,6 +46,16 @@ save_vars { env(TERM) } {
     set main_line [gdb_get_line_number "break here"]
     gdb_test "list $main_line,$main_line" "return.*some_called_function.*"
 
+    gdb_test_no_output "set style enabled off"
+
+    set argv ""
+    gdb_test_multiple "frame" "frame without styling" {
+	-re -wrap "main \\(argc=.*, (argv=$hex)\\).*style\\.c:\[0-9\].*" {
+	    set argv $expect_out(1,string)
+	    pass $gdb_test_name
+	}
+    }
+
     gdb_test_no_output "set style enabled on"
 
     set main_expr [style main function]
@@ -60,7 +70,7 @@ save_vars { env(TERM) } {
     gdb_test_no_output "set style sources off"
     gdb_test "frame" \
 	"\r\n\[^\033\]*break here.*" \
-	"frame without styling"
+	"frame without sources styling"
     gdb_test_no_output "set style sources on"
 
     gdb_test "break -q main" "file $base_file_expr.*"
@@ -71,20 +81,47 @@ save_vars { env(TERM) } {
     # wrong spot with styling.  There were different bugs at different
     # widths, so try two.
     foreach width {20 30} {
-	gdb_test_no_output "set width $width"
+	set argv_len [string length $argv]
+	if { $argv_len == 0 } {
+	    continue
+	}
+
 	# There was also a bug where the styling could be wrong in the
 	# line listing; this is why the words from the source code are
 	# spelled out in the final result line of the test.
-	gdb_test "frame" \
+	set re1_styled \
 	    [multi_line \
 		 "#0 *$main_expr.*$arg_expr.*" \
 		 ".*$arg_expr.*" \
 		 ".* at .*$file_expr.*" \
-		 "\[0-9\]+.*return.* break here .*"
-	    ] \
-	    "frame when width=$width"
+		 "\[0-9\]+.*return.* break here .*"]
+	set re2_styled \
+	    [multi_line \
+		 "#0 *$main_expr.*$arg_expr.*" \
+		 ".*$arg_expr.* at .*$file_expr.*" \
+		 "\[0-9\]+.*return.* break here .*"]
+
+	# The length of the line containing argv containing:
+	# - 4 leading spaces
+	# - argv string
+	# - closing parenthesis
+	set line_len [expr 4 + $argv_len + 1]
+
+	if { $line_len > $width } {
+	    # At on the next line.
+	    set re_styled $re1_styled
+	} else {
+	    # At on the same line as argv.
+	    set re_styled $re2_styled
+	}
+
+	gdb_test_no_output "set width $width"
+	gdb_test "frame" $re_styled "frame when width=$width"
     }
 
+    # Reset width back to 0.
+    gdb_test_no_output "set width 0"
+
     if {$test_macros} {
 	set macro_line [gdb_get_line_number "\#define SOME_MACRO"]
 	gdb_test "info macro SOME_MACRO" \