[PATCH] gdb.base/watchpoint-unaligned.exp: Fix TCL error.

[PATCH] gdb.base/watchpoint-unaligned.exp: Fix TCL error.

[Alexandra Hájková]

From: Alexandra Hajkova <ahajkova@redhat.com>

When running this test on aarch64 I've got TCL error:
ERROR:  can't read "wpoffset_to_wpnum(1)": no such element in array
ERROR:  tcl error code TCL READ VARNAME
ERROR:  tcl error info:
can't read "wpoffset_to_wpnum(1)": no such element in array ...

This patch adds checks whether the wpoffset_to_wpnum array member
exists to avoid the error.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30340
---
 .../gdb.base/watchpoint-unaligned.exp         | 38 ++++++++++---------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/gdb/testsuite/gdb.base/watchpoint-unaligned.exp b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
index ce5a1e5bf66..d470368a6e3 100644
--- a/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
+++ b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
@@ -121,27 +121,31 @@ foreach wpcount {4 7} {
     gdb_test_no_output -nopass "set variable offset = 1"
     set test "continue"
     set got_hit 0
-    gdb_test_multiple $test $test {
-	-re "\r\nCould not insert hardware watchpoint .*\r\n$gdb_prompt $" {
-	}
-	-re "$rwatch_exp $wpoffset_to_wpnum(1):.*alue = .*\r\n$gdb_prompt $" {
-	    set got_hit 1
-	    send_gdb "continue\n"
-	    exp_continue
-	}
-	-re " start_again .*\r\n$gdb_prompt $" {
-	}
+    if {[info exists wpoffset_to_wpnum(1)]} {
+	    gdb_test_multiple $test $test {
+		    -re "\r\nCould not insert hardware watchpoint .*\r\n$gdb_prompt $" {
+		    }
+		    -re "$rwatch_exp $wpoffset_to_wpnum(1):.*alue = .*\r\n$gdb_prompt $" {
+			    set got_hit 1
+			    send_gdb "continue\n"
+			    exp_continue
+		    }
+		    -re " start_again .*\r\n$gdb_prompt $" {
+		    }
+	    }
     }
     for {set wpoffset 1} {$wpoffset <= $wpcount} {incr wpoffset} {
-	if {$wpoffset_to_wpnum($wpoffset)} {
-	    gdb_test_no_output "delete $wpoffset_to_wpnum($wpoffset)" ""
-	}
+	    if {[info exists wpoffset_to_wpnum($wpoffset)] &&
+		    $wpoffset_to_wpnum($wpoffset)} {
+		    gdb_test_no_output "delete $wpoffset_to_wpnum($wpoffset)" ""
+	    }
     }
     set test "wpcount($wpcount)"
-    if {!$wpoffset_to_wpnum([expr $wpcount - 1])} {
-	untested $test
-	continue
-    }
+    if {[info exists wpoffset_to_wpnum([expr $wpcount - 1])] &&
+	    !$wpoffset_to_wpnum([expr $wpcount - 1])} {
+		    untested $test
+		    continue
+	    }
     if {$wpcount > 4} {
 	if {![istarget "s390*-*-*"]} {
 	    setup_kfail tdep/22389 *-*-*
-- 
2.39.2

Re: [PATCH] gdb.base/watchpoint-unaligned.exp: Fix TCL error.

[Luis Machado]

On 4/12/23 22:08, Alexandra Hájková via Gdb-patches wrote:
> From: Alexandra Hajkova <ahajkova@redhat.com>
> 
> When running this test on aarch64 I've got TCL error:
> ERROR:  can't read "wpoffset_to_wpnum(1)": no such element in array
> ERROR:  tcl error code TCL READ VARNAME
> ERROR:  tcl error info:
> can't read "wpoffset_to_wpnum(1)": no such element in array ...
> 
> This patch adds checks whether the wpoffset_to_wpnum array member
> exists to avoid the error.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30340
> ---
>   .../gdb.base/watchpoint-unaligned.exp         | 38 ++++++++++---------
>   1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/watchpoint-unaligned.exp b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
> index ce5a1e5bf66..d470368a6e3 100644
> --- a/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
> +++ b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
> @@ -121,27 +121,31 @@ foreach wpcount {4 7} {
>       gdb_test_no_output -nopass "set variable offset = 1"
>       set test "continue"
>       set got_hit 0
> -    gdb_test_multiple $test $test {
> -	-re "\r\nCould not insert hardware watchpoint .*\r\n$gdb_prompt $" {
> -	}
> -	-re "$rwatch_exp $wpoffset_to_wpnum(1):.*alue = .*\r\n$gdb_prompt $" {
> -	    set got_hit 1
> -	    send_gdb "continue\n"
> -	    exp_continue
> -	}
> -	-re " start_again .*\r\n$gdb_prompt $" {
> -	}
> +    if {[info exists wpoffset_to_wpnum(1)]} {
> +	    gdb_test_multiple $test $test {
> +		    -re "\r\nCould not insert hardware watchpoint .*\r\n$gdb_prompt $" {
> +		    }
> +		    -re "$rwatch_exp $wpoffset_to_wpnum(1):.*alue = .*\r\n$gdb_prompt $" {
> +			    set got_hit 1
> +			    send_gdb "continue\n"
> +			    exp_continue
> +		    }
> +		    -re " start_again .*\r\n$gdb_prompt $" {
> +		    }
> +	    }
>       }
>       for {set wpoffset 1} {$wpoffset <= $wpcount} {incr wpoffset} {
> -	if {$wpoffset_to_wpnum($wpoffset)} {
> -	    gdb_test_no_output "delete $wpoffset_to_wpnum($wpoffset)" ""
> -	}
> +	    if {[info exists wpoffset_to_wpnum($wpoffset)] &&
> +		    $wpoffset_to_wpnum($wpoffset)} {
> +		    gdb_test_no_output "delete $wpoffset_to_wpnum($wpoffset)" ""
> +	    }
>       }
>       set test "wpcount($wpcount)"
> -    if {!$wpoffset_to_wpnum([expr $wpcount - 1])} {
> -	untested $test
> -	continue
> -    }
> +    if {[info exists wpoffset_to_wpnum([expr $wpcount - 1])] &&
> +	    !$wpoffset_to_wpnum([expr $wpcount - 1])} {
> +		    untested $test
> +		    continue
> +	    }
>       if {$wpcount > 4} {
>   	if {![istarget "s390*-*-*"]} {
>   	    setup_kfail tdep/22389 *-*-*

As I commented on the bug ticket, I think a better approach is to check for errors and then bail out of the test if one is found.

If we can't insert watchpoints, it is probably not worth continuing to run checks. We will end up with a pile of failures.

[PATCH v2] gdb.base/watchpoint-unaligned.exp: Always initialize wpoffset_to_wpnum

[Alexandra Hájková]

to avoid TCL error which happens in some aarch64 types.

ERROR: in testcase /root/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
ERROR:  can't read "wpoffset_to_wpnum(1)": no such element in array
ERROR:  tcl error code TCL READ VARNAME
ERROR:  tcl error info:
can't read "wpoffset_to_wpnum(1)": no such element in array
    while executing

Fixes bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30340
---
The test uses gdb_test_multiple, and there are plenty of other reasons (see gdb_test_multiple in lib/gdb.exp) that a test using gdb_test_multiple might fail.  We're never going to recreate each of those in the gdb.base/watchpoint-unaligned.exp script just so that we can ensure the variable is initialized.  Instead we should just ensure the variable is always initialized.
 
 gdb/testsuite/gdb.base/watchpoint-unaligned.exp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/watchpoint-unaligned.exp b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
index ce5a1e5bf66..d31a9cdc2c8 100644
--- a/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
+++ b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
@@ -103,6 +103,8 @@ foreach wpcount {4 7} {
     for {set wpoffset 1} {$wpoffset <= $wpcount} {incr wpoffset} {
 	set test "$rwatch data.u.size1\[$wpoffset\]"
 	set wpnum ""
+	# Initialize the result incase the test fails.
+	set wpoffset_to_wpnum($wpoffset) 0
 	gdb_test_multiple $test $test {
 	    -re "$rwatch_exp (\[0-9\]+): .*\r\n$gdb_prompt $" {
 		set wpoffset_to_wpnum($wpoffset) $expect_out(1,string)
@@ -113,7 +115,6 @@ foreach wpcount {4 7} {
 		    setup_xfail breakpoints/23131 "arm*-*-*"
 		}
 		fail $test
-		set wpoffset_to_wpnum($wpoffset) 0
 	    }
 	}
     }
-- 
2.40.0

Re: [PATCH v2] gdb.base/watchpoint-unaligned.exp: Always initialize wpoffset_to_wpnum

[Luis Machado]

Hi,

On 4/25/23 11:47, Alexandra Hájková via Gdb-patches wrote:
> to avoid TCL error which happens in some aarch64 types.
> 
> ERROR: in testcase /root/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
> ERROR:  can't read "wpoffset_to_wpnum(1)": no such element in array
> ERROR:  tcl error code TCL READ VARNAME
> ERROR:  tcl error info:
> can't read "wpoffset_to_wpnum(1)": no such element in array
>      while executing
> 
> Fixes bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30340
> ---
> The test uses gdb_test_multiple, and there are plenty of other reasons (see gdb_test_multiple in lib/gdb.exp) that a test using gdb_test_multiple might fail.  We're never going to recreate each of those in the gdb.base/watchpoint-unaligned.exp script just so that we can ensure the variable is initialized.  Instead we should just ensure the variable is always initialized.
>   
>   gdb/testsuite/gdb.base/watchpoint-unaligned.exp | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/testsuite/gdb.base/watchpoint-unaligned.exp b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
> index ce5a1e5bf66..d31a9cdc2c8 100644
> --- a/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
> +++ b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
> @@ -103,6 +103,8 @@ foreach wpcount {4 7} {
>       for {set wpoffset 1} {$wpoffset <= $wpcount} {incr wpoffset} {
>   	set test "$rwatch data.u.size1\[$wpoffset\]"
>   	set wpnum ""
> +	# Initialize the result incase the test fails.
> +	set wpoffset_to_wpnum($wpoffset) 0
>   	gdb_test_multiple $test $test {
>   	    -re "$rwatch_exp (\[0-9\]+): .*\r\n$gdb_prompt $" {
>   		set wpoffset_to_wpnum($wpoffset) $expect_out(1,string)
> @@ -113,7 +115,6 @@ foreach wpcount {4 7} {
>   		    setup_xfail breakpoints/23131 "arm*-*-*"
>   		}
>   		fail $test
> -		set wpoffset_to_wpnum($wpoffset) 0
>   	    }
>   	}
>       }

Thanks. I think this approach is a reasonable one.

Reviewed-by: Luis Machado <luis.machado@arm.com>

Re: [PATCH v2] gdb.base/watchpoint-unaligned.exp: Always initialize wpoffset_to_wpnum

[Andrew Burgess]

Alexandra Hájková via Gdb-patches <gdb-patches@sourceware.org> writes:

> to avoid TCL error which happens in some aarch64 types.

Commit messages should ideally be written as proper sentences, so start
with a capital letter.

>
> ERROR: in testcase /root/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
> ERROR:  can't read "wpoffset_to_wpnum(1)": no such element in array
> ERROR:  tcl error code TCL READ VARNAME
> ERROR:  tcl error info:
> can't read "wpoffset_to_wpnum(1)": no such element in array
>     while executing
>
> Fixes bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30340

Looking through the commit log there's only one other instances of
'Fixes bug:', the GDB style is usually just 'Bug:' -- being consistent
makes it much easier to search for these labels.

Also, at one point we had to reference the traditional bug tag in order
to get this commit automatically linked with the bugzilla report - so we
needed to include something like 'PR gdb/30340'.  I don't know if this
is still needed or not -- I've never bothered to find out as I just
include both forms.

> ---
> The test uses gdb_test_multiple, and there are plenty of other reasons (see gdb_test_multiple in lib/gdb.exp) that a test using gdb_test_multiple might fail.  We're never going to recreate each of those in the gdb.base/watchpoint-unaligned.exp script just so that we can ensure the variable is initialized.  Instead we should just ensure the variable is always initialized.

It's not clear if this is intended to be part of the commit message or
not.  It seems like there's helpful text in here, but it's not wrapped
to a reasonable width.  That said, for such a small change, maybe we
don't need to include any additional text, folk can just figure out
what's going on from the code change.

>  
>  gdb/testsuite/gdb.base/watchpoint-unaligned.exp | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/testsuite/gdb.base/watchpoint-unaligned.exp b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
> index ce5a1e5bf66..d31a9cdc2c8 100644
> --- a/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
> +++ b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
> @@ -103,6 +103,8 @@ foreach wpcount {4 7} {
>      for {set wpoffset 1} {$wpoffset <= $wpcount} {incr wpoffset} {
>  	set test "$rwatch data.u.size1\[$wpoffset\]"
>  	set wpnum ""
> +	# Initialize the result incase the test fails.
> +	set wpoffset_to_wpnum($wpoffset) 0
>  	gdb_test_multiple $test $test {
>  	    -re "$rwatch_exp (\[0-9\]+): .*\r\n$gdb_prompt $" {
>  		set wpoffset_to_wpnum($wpoffset) $expect_out(1,string)
> @@ -113,7 +115,6 @@ foreach wpcount {4 7} {
>  		    setup_xfail breakpoints/23131 "arm*-*-*"
>  		}
>  		fail $test
> -		set wpoffset_to_wpnum($wpoffset) 0
>  	    }
>  	}
>      }

The change itself looks great.

Reviewed-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


> -- 
> 2.40.0

Re: [PATCH v2] gdb.base/watchpoint-unaligned.exp: Always initialize wpoffset_to_wpnum

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> Also, at one point we had to reference the traditional bug tag in order
Andrew> to get this commit automatically linked with the bugzilla report - so we
Andrew> needed to include something like 'PR gdb/30340'.  I don't know if this
Andrew> is still needed or not -- I've never bothered to find out as I just
Andrew> include both forms.

Either form is recognized by the commit script now.  I tend to leave out
the old form and just rely on the Bug trailer now, though I've found
that sometimes referencing the bug leads to clearer text.

Tom

Re: [PATCH v2] gdb.base/watchpoint-unaligned.exp: Always initialize wpoffset_to_wpnum

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Andrew> Also, at one point we had to reference the traditional bug tag in order
> Andrew> to get this commit automatically linked with the bugzilla report - so we
> Andrew> needed to include something like 'PR gdb/30340'.  I don't know if this
> Andrew> is still needed or not -- I've never bothered to find out as I just
> Andrew> include both forms.
>
> Either form is recognized by the commit script now.  I tend to leave out
> the old form and just rely on the Bug trailer now, though I've found
> that sometimes referencing the bug leads to clearer text.

Thanks, I missed when this changed.  I'll try keep this in mind for
future patches.

Thanks,
Andrew