[3/5,gdb/testsuite] Fix some test-cases for check-read1 (-lbl)

Message ID 20241010072510.25545-3-tdevries@suse.de
State Committed
Headers
Series [1/5,gdb/testsuite] Fix some test-cases for check-read1 (gdb_test_lines) |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Tom de Vries Oct. 10, 2024, 7:25 a.m. UTC
  I ran the testsuite in an environment simulating a stressed system in
combination with check-read1.  This exposes a few more FAILs.

Fix some by using -lbl.

Tested on x86_64-linux.
---
 gdb/testsuite/gdb.base/sect-cmd.exp | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)
  

Comments

Andrew Burgess Oct. 26, 2024, 10:33 a.m. UTC | #1
Tom de Vries <tdevries@suse.de> writes:

> I ran the testsuite in an environment simulating a stressed system in
> combination with check-read1.  This exposes a few more FAILs.
>
> Fix some by using -lbl.
>
> Tested on x86_64-linux.
> ---
>  gdb/testsuite/gdb.base/sect-cmd.exp | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)

After this commit I noticed that sect-cmd.exp would sometimes fail.  The
fix seemed pretty obvious, so I went ahead and pushed it.  But if anyone
feels this isn't the correct solution, let me know and I can rework
things.

Thanks,
Andrew

----

commit 2bba46058789196c1c384896933cbc9692ef4933
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Sat Oct 26 11:18:40 2024 +0100

    gdb/testsuite: fix test pattern after switch to -lbl matching
    
    After commit:
    
      commit a1ccc78ea7ba8cad3ff37cbde9b5d3bba0194796
      Date:   Fri Oct 25 06:14:03 2024 +0200
    
          [gdb/testsuite] Fix some test-cases for check-read1 (-lbl)
    
    I notice that gdb.base/sect-cmd.exp would sometimes fail.  The problem
    is that by switching to line by line matching we now need to ensure
    that the gdb_test_multiple patterns match up to the end of the line,
    but don't actually include the trailing \r\n (yeah, our line by line
    matching is weird).  We need to be especially careful anywhere '.*' is
    used as this can potentially match content on a subsequent line.
    
    I have replaced '.*' with '\[^\r\n\]*(?=\r\n)', matching everything up
    to the end of the line, but not the end of line itself, and I've made
    use of '(?=\r\n)' in a couple of other places to ensure we match up to
    the end of the line, but don't match the line terminator itself.

diff --git a/gdb/testsuite/gdb.base/sect-cmd.exp b/gdb/testsuite/gdb.base/sect-cmd.exp
index 2a5758a94c6..49a94cd9019 100644
--- a/gdb/testsuite/gdb.base/sect-cmd.exp
+++ b/gdb/testsuite/gdb.base/sect-cmd.exp
@@ -37,7 +37,7 @@ set section_name ""
 
 set ok 0
 gdb_test_multiple "info files" "" -lbl {
-    -re "\\s+($hex) - ($hex) is (\\\$CODE\\\$|\\.text\\S*) in .*" {
+    -re "\\s+($hex) - ($hex) is (\\\$CODE\\\$|\\.text\\S*) in \[^\r\n\]*(?=\r\n)" {
 	if { ! $ok } {
 	    set address1 $expect_out(1,string)
 	    set address2 $expect_out(2,string)
@@ -64,11 +64,11 @@ if { $address1 == "" || $address2 == "" || $section_name == "" } {
 set saw_section_address_line false
 gdb_test_multiple "section $section_name $address1" \
     "set section $section_name to original address" -lbl {
-	-re ".*$address1 \- $address2 is $section_name in \[^\r\n\]*" {
+	-re "\[^\r\n\]*$address1 \- $address2 is $section_name in \[^\r\n\]*(?=\r\n)" {
 	    set saw_section_address_line true
 	    exp_continue
 	}
-	-re "Section \[^\r\n\]+ not found\r\n" {
+	-re "Section \[^\r\n\]+ not found(?=\r\n)" {
 	    fail "$gdb_test_name (saw not found marker)"
 	    exp_continue
 	}
  
Tom de Vries Oct. 26, 2024, 1:02 p.m. UTC | #2
On 10/26/24 12:33, Andrew Burgess wrote:
> Tom de Vries <tdevries@suse.de> writes:
> 
>> I ran the testsuite in an environment simulating a stressed system in
>> combination with check-read1.  This exposes a few more FAILs.
>>
>> Fix some by using -lbl.
>>
>> Tested on x86_64-linux.
>> ---
>>   gdb/testsuite/gdb.base/sect-cmd.exp | 21 ++++++++++++++-------
>>   1 file changed, 14 insertions(+), 7 deletions(-)
> 
> After this commit I noticed that sect-cmd.exp would sometimes fail.  The
> fix seemed pretty obvious, so I went ahead and pushed it.  But if anyone
> feels this isn't the correct solution, let me know and I can rework
> things.
> 

Hi Andrew,

sorry for the breakage, and thanks for catching and fixing it.

I see now that I could have reproduced the failure using the readmore 
functionality.

I often wonder about adding both read1 and readmore to make-check-all.sh 
(and once we go down that road, maybe clang as well), which would make 
it easier to not forget to run those.

The current behaviour is "Run make check with all boards from 
gdb/testsuite/boards", and read1 and readmore are not target boards, so 
sofar I've refrained from adding those.  Perhaps you have an opinion on 
this?

> Thanks,
> Andrew
> 
> ----
> 
> commit 2bba46058789196c1c384896933cbc9692ef4933
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Sat Oct 26 11:18:40 2024 +0100
> 
>      gdb/testsuite: fix test pattern after switch to -lbl matching
>      
>      After commit:
>      
>        commit a1ccc78ea7ba8cad3ff37cbde9b5d3bba0194796
>        Date:   Fri Oct 25 06:14:03 2024 +0200
>      
>            [gdb/testsuite] Fix some test-cases for check-read1 (-lbl)
>      
>      I notice that gdb.base/sect-cmd.exp would sometimes fail.  The problem
>      is that by switching to line by line matching we now need to ensure
>      that the gdb_test_multiple patterns match up to the end of the line,
>      but don't actually include the trailing \r\n (yeah, our line by line
>      matching is weird).  We need to be especially careful anywhere '.*' is

AFAIR, the line-by-line matching was designed to be compatible with 
-wrap (which requires a \r\n before the prompt), which itself was 
designed to be compatible with gdb_test.

Thanks,
- Tom

>      used as this can potentially match content on a subsequent line.
>      
>      I have replaced '.*' with '\[^\r\n\]*(?=\r\n)', matching everything up
>      to the end of the line, but not the end of line itself, and I've made
>      use of '(?=\r\n)' in a couple of other places to ensure we match up to
>      the end of the line, but don't match the line terminator itself.
> 
> diff --git a/gdb/testsuite/gdb.base/sect-cmd.exp b/gdb/testsuite/gdb.base/sect-cmd.exp
> index 2a5758a94c6..49a94cd9019 100644
> --- a/gdb/testsuite/gdb.base/sect-cmd.exp
> +++ b/gdb/testsuite/gdb.base/sect-cmd.exp
> @@ -37,7 +37,7 @@ set section_name ""
>   
>   set ok 0
>   gdb_test_multiple "info files" "" -lbl {
> -    -re "\\s+($hex) - ($hex) is (\\\$CODE\\\$|\\.text\\S*) in .*" {
> +    -re "\\s+($hex) - ($hex) is (\\\$CODE\\\$|\\.text\\S*) in \[^\r\n\]*(?=\r\n)" {
>   	if { ! $ok } {
>   	    set address1 $expect_out(1,string)
>   	    set address2 $expect_out(2,string)
> @@ -64,11 +64,11 @@ if { $address1 == "" || $address2 == "" || $section_name == "" } {
>   set saw_section_address_line false
>   gdb_test_multiple "section $section_name $address1" \
>       "set section $section_name to original address" -lbl {
> -	-re ".*$address1 \- $address2 is $section_name in \[^\r\n\]*" {
> +	-re "\[^\r\n\]*$address1 \- $address2 is $section_name in \[^\r\n\]*(?=\r\n)" {
>   	    set saw_section_address_line true
>   	    exp_continue
>   	}
> -	-re "Section \[^\r\n\]+ not found\r\n" {
> +	-re "Section \[^\r\n\]+ not found(?=\r\n)" {
>   	    fail "$gdb_test_name (saw not found marker)"
>   	    exp_continue
>   	}
>
  
Andrew Burgess Oct. 28, 2024, 2:08 p.m. UTC | #3
Tom de Vries <tdevries@suse.de> writes:

> On 10/26/24 12:33, Andrew Burgess wrote:
>> Tom de Vries <tdevries@suse.de> writes:
>> 
>>> I ran the testsuite in an environment simulating a stressed system in
>>> combination with check-read1.  This exposes a few more FAILs.
>>>
>>> Fix some by using -lbl.
>>>
>>> Tested on x86_64-linux.
>>> ---
>>>   gdb/testsuite/gdb.base/sect-cmd.exp | 21 ++++++++++++++-------
>>>   1 file changed, 14 insertions(+), 7 deletions(-)
>> 
>> After this commit I noticed that sect-cmd.exp would sometimes fail.  The
>> fix seemed pretty obvious, so I went ahead and pushed it.  But if anyone
>> feels this isn't the correct solution, let me know and I can rework
>> things.
>> 
>
> Hi Andrew,
>
> sorry for the breakage, and thanks for catching and fixing it.
>
> I see now that I could have reproduced the failure using the readmore 
> functionality.
>
> I often wonder about adding both read1 and readmore to make-check-all.sh 
> (and once we go down that road, maybe clang as well), which would make 
> it easier to not forget to run those.
>
> The current behaviour is "Run make check with all boards from 
> gdb/testsuite/boards", and read1 and readmore are not target boards, so 
> sofar I've refrained from adding those.  Perhaps you have an opinion on 
> this?

This would certainly get a +100 from me.  The more testing that can be
wrapped up in one place the less I forget to check with.  I'll be
honest, I wasn't even aware of the 'readmore' testing until you
mentioned it here.

Thanks,
Andrew
  
Tom de Vries Oct. 29, 2024, 9:51 a.m. UTC | #4
On 10/28/24 15:08, Andrew Burgess wrote:
> Tom de Vries <tdevries@suse.de> writes:
> 
>> On 10/26/24 12:33, Andrew Burgess wrote:
>>> Tom de Vries <tdevries@suse.de> writes:
>>>
>>>> I ran the testsuite in an environment simulating a stressed system in
>>>> combination with check-read1.  This exposes a few more FAILs.
>>>>
>>>> Fix some by using -lbl.
>>>>
>>>> Tested on x86_64-linux.
>>>> ---
>>>>    gdb/testsuite/gdb.base/sect-cmd.exp | 21 ++++++++++++++-------
>>>>    1 file changed, 14 insertions(+), 7 deletions(-)
>>>
>>> After this commit I noticed that sect-cmd.exp would sometimes fail.  The
>>> fix seemed pretty obvious, so I went ahead and pushed it.  But if anyone
>>> feels this isn't the correct solution, let me know and I can rework
>>> things.
>>>
>>
>> Hi Andrew,
>>
>> sorry for the breakage, and thanks for catching and fixing it.
>>
>> I see now that I could have reproduced the failure using the readmore
>> functionality.
>>
>> I often wonder about adding both read1 and readmore to make-check-all.sh
>> (and once we go down that road, maybe clang as well), which would make
>> it easier to not forget to run those.
>>
>> The current behaviour is "Run make check with all boards from
>> gdb/testsuite/boards", and read1 and readmore are not target boards, so
>> sofar I've refrained from adding those.  Perhaps you have an opinion on
>> this?
> 
> This would certainly get a +100 from me.  The more testing that can be
> wrapped up in one place the less I forget to check with.

That's how I feel about it as well.

I've submitted a patch here ( 
https://sourceware.org/pipermail/gdb-patches/2024-October/212714.html ).

Thanks,
- Tom

> I'll be
> honest, I wasn't even aware of the 'readmore' testing until you
> mentioned it here.
  

Patch

diff --git a/gdb/testsuite/gdb.base/sect-cmd.exp b/gdb/testsuite/gdb.base/sect-cmd.exp
index 7d52e317167..2a5758a94c6 100644
--- a/gdb/testsuite/gdb.base/sect-cmd.exp
+++ b/gdb/testsuite/gdb.base/sect-cmd.exp
@@ -35,12 +35,19 @@  set address1 ""
 set address2 ""
 set section_name ""
 
-gdb_test_multiple "info files" "" {
-    -re -wrap "\\s+($hex) - ($hex) is (\\\$CODE\\\$|\\.text\\S*) in .*" {
-	set address1 $expect_out(1,string)
-	set address2 $expect_out(2,string)
-	set section_name $expect_out(3,string)
-	pass $gdb_test_name
+set ok 0
+gdb_test_multiple "info files" "" -lbl {
+    -re "\\s+($hex) - ($hex) is (\\\$CODE\\\$|\\.text\\S*) in .*" {
+	if { ! $ok } {
+	    set address1 $expect_out(1,string)
+	    set address2 $expect_out(2,string)
+	    set section_name $expect_out(3,string)
+	    set ok 1
+	}
+	exp_continue
+    }
+    -re -wrap "" {
+	gdb_assert { $ok } $gdb_test_name
     }
 }
 
@@ -56,7 +63,7 @@  if { $address1 == "" || $address2 == "" || $section_name == "" } {
 #
 set saw_section_address_line false
 gdb_test_multiple "section $section_name $address1" \
-    "set section $section_name to original address" {
+    "set section $section_name to original address" -lbl {
 	-re ".*$address1 \- $address2 is $section_name in \[^\r\n\]*" {
 	    set saw_section_address_line true
 	    exp_continue