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

  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(-)
  

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>
  

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
  

>>>>> "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
  

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