gdb: use gdb_test_multiple in gdb_breakpoint

  When running the testsuite in a non-optimized build on a slow machine, I
sometimes get:

    UNTESTED: gdb.gdb/selftest.exp: Cannot set breakpoint at captured_main, skipping testcase.

do_self_tests, in lib/selftest-support.exp, uses `with_timeout_factor
10`, to account for the fact that reading the debug info of the gdb
binary (especially in a non-optimized GDB) can take time.  But then it
ends up calling gdb_breakpoint, which uses gdb_expect with a hard-coded
timeout of 30 seconds.

Fix this by making gdb_breakpoint use gdb_test_multiple, which is a
desired change anyway for this kind of simple command / expected
output case.

Change-Id: I9b06ce991cc584810d8cc231b2b4893980b8be75
---
 gdb/testsuite/lib/gdb.exp | 32 +-------------------------------
 1 file changed, 1 insertion(+), 31 deletions(-)


base-commit: a7d5fcaf8e15820f997ba7774a8eef7ab7e2f2e3
  

Hi Simon,

On Tue, Jan 03, 2023 at 02:22:16PM -0500, Simon Marchi via Gdb-patches wrote:
> When running the testsuite in a non-optimized build on a slow machine, I
> sometimes get:
> 
>     UNTESTED: gdb.gdb/selftest.exp: Cannot set breakpoint at captured_main, skipping testcase.
> 
> do_self_tests, in lib/selftest-support.exp, uses `with_timeout_factor
> 10`, to account for the fact that reading the debug info of the gdb
> binary (especially in a non-optimized GDB) can take time.  But then it
> ends up calling gdb_breakpoint, which uses gdb_expect with a hard-coded
> timeout of 30 seconds.
> 
> Fix this by making gdb_breakpoint use gdb_test_multiple, which is a
> desired change anyway for this kind of simple command / expected
> output case.
> 
> Change-Id: I9b06ce991cc584810d8cc231b2b4893980b8be75
> ---
>  gdb/testsuite/lib/gdb.exp | 32 +-------------------------------
>  1 file changed, 1 insertion(+), 31 deletions(-)
> 
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 135ace68d5ed..5a0cd46d8998 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -641,9 +641,8 @@ proc gdb_breakpoint { linespec args } {
>  
>      set test_name "gdb_breakpoint: set breakpoint at $linespec"

Should you use $test_name as second arg of the call to gdb_test_multiple
you introduce?  This way the error cases handled by gdb_test_multiple
will the desired test name.

Also, I guess that to make is slightly more consistent with other usages
of gdb_test_multiple, $gdb_test_name should be used instead of
$test_name in the untouched actions.

Best,
Lancelot.

>  
> -    send_gdb "$break_command $linespec\n"
>      # The first two regexps are what we get with -g, the third is without -g.
> -    gdb_expect 30 {
> +    gdb_test_multiple "$break_command $linespec" "" {
>  	-re "$break_message \[0-9\]* at .*: file .*, line $decimal.\r\n$gdb_prompt $" {}
>  	-re "$break_message \[0-9\]*: file .*, line $decimal.\r\n$gdb_prompt $" {}
>  	-re "$break_message \[0-9\]* at .*$gdb_prompt $" {}
> @@ -659,35 +658,6 @@ proc gdb_breakpoint { linespec args } {
>  		send_gdb "$pending_response\n"
>  		exp_continue
>  	}
> -	-re "A problem internal to GDB has been detected" {
> -		if { $print_fail } {
> -		    fail "$test_name (GDB internal error)"
> -		}
> -		gdb_internal_error_resync
> -		return 0
> -	}
> -	-re "$gdb_prompt $" {
> -		if { $print_fail } {
> -			fail $test_name
> -		}
> -		return 0
> -	}
> -	eof {
> -		perror "GDB process no longer exists"
> -		global gdb_spawn_id
> -		set wait_status [wait -i $gdb_spawn_id]
> -		verbose -log "GDB process exited with wait status $wait_status"
> -		if { $print_fail } {
> -			fail "$test_name (eof)"
> -		}
> -		return 0
> -	}
> -	timeout {
> -		if { $print_fail } {
> -			fail "$test_name (timeout)"
> -		}
> -		return 0
> -	}
>      }
>      if { $print_pass } {
>  	pass $test_name
> 
> base-commit: a7d5fcaf8e15820f997ba7774a8eef7ab7e2f2e3
> -- 
> 2.39.0
>
  

On 1/4/23 04:15, Lancelot SIX wrote:
> Hi Simon,
> 
> On Tue, Jan 03, 2023 at 02:22:16PM -0500, Simon Marchi via Gdb-patches wrote:
>> When running the testsuite in a non-optimized build on a slow machine, I
>> sometimes get:
>>
>>     UNTESTED: gdb.gdb/selftest.exp: Cannot set breakpoint at captured_main, skipping testcase.
>>
>> do_self_tests, in lib/selftest-support.exp, uses `with_timeout_factor
>> 10`, to account for the fact that reading the debug info of the gdb
>> binary (especially in a non-optimized GDB) can take time.  But then it
>> ends up calling gdb_breakpoint, which uses gdb_expect with a hard-coded
>> timeout of 30 seconds.
>>
>> Fix this by making gdb_breakpoint use gdb_test_multiple, which is a
>> desired change anyway for this kind of simple command / expected
>> output case.
>>
>> Change-Id: I9b06ce991cc584810d8cc231b2b4893980b8be75
>> ---
>>  gdb/testsuite/lib/gdb.exp | 32 +-------------------------------
>>  1 file changed, 1 insertion(+), 31 deletions(-)
>>
>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>> index 135ace68d5ed..5a0cd46d8998 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -641,9 +641,8 @@ proc gdb_breakpoint { linespec args } {
>>  
>>      set test_name "gdb_breakpoint: set breakpoint at $linespec"
> 
> Should you use $test_name as second arg of the call to gdb_test_multiple
> you introduce?  This way the error cases handled by gdb_test_multiple
> will the desired test name.
> 
> Also, I guess that to make is slightly more consistent with other usages
> of gdb_test_multiple, $gdb_test_name should be used instead of
> $test_name in the untouched actions.

Oh, right, I missed that.  I folded the test name in the
gdb_test_multiple call (removed the test_name var) and used
$gdb_test_name in the fail call.  I updated the patch locally.  Should I
add your Reviewed-By after those changes?

Simon
  

> Oh, right, I missed that.  I folded the test name in the
> gdb_test_multiple call (removed the test_name var) and used
> $gdb_test_name in the fail call.  I updated the patch locally.  Should I
> add your Reviewed-By after those changes?
> 
> Simon
> 

Yes, with those change, feel free to add

Reviewed-By: Lancelot Six <lancelot.six@amd.com>

Best,
Lancelot.
  

On 1/4/23 11:18, Lancelot SIX wrote:
>> Oh, right, I missed that.  I folded the test name in the
>> gdb_test_multiple call (removed the test_name var) and used
>> $gdb_test_name in the fail call.  I updated the patch locally.  Should I
>> add your Reviewed-By after those changes?
>>
>> Simon
>>
> 
> Yes, with those change, feel free to add
> 
> Reviewed-By: Lancelot Six <lancelot.six@amd.com>
> 
> Best,
> Lancelot.

Thanks, pushed!

Simon
  

On Wed, Jan 04, 2023 at 11:22:55AM -0500, Simon Marchi via Gdb-patches wrote:
> 
> 
> On 1/4/23 11:18, Lancelot SIX wrote:
> >> Oh, right, I missed that.  I folded the test name in the
> >> gdb_test_multiple call (removed the test_name var) and used
> >> $gdb_test_name in the fail call.  I updated the patch locally.  Should I
> >> add your Reviewed-By after those changes?
> >>
> >> Simon
> >>
> > 
> > Yes, with those change, feel free to add
> > 
> > Reviewed-By: Lancelot Six <lancelot.six@amd.com>
> > 
> > Best,
> > Lancelot.
> 
> Thanks, pushed!
> 
> Simon

Hi,

I just saw the resulting patch, and is seems that $test_name is
(conditionally) used after the gdb_test_multiple:

    diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
    index 135ace68d5e..ba16b2ab315 100644
    --- a/gdb/testsuite/lib/gdb.exp
    +++ b/gdb/testsuite/lib/gdb.exp
    @@ -639,18 +639,15 @@ proc gdb_breakpoint { linespec args } {
            set print_pass 1
         }
    
    -    set test_name "gdb_breakpoint: set breakpoint at $linespec"
    -
    -    send_gdb "$break_command $linespec\n"
         # The first two regexps are what we get with -g, the third is without -g.
    -    gdb_expect 30 {
    +    gdb_test_multiple "$break_command $linespec" "gdb_breakpoint: set breakpoint at $linespec" {
            -re "$break_message \[0-9\]* at .*: file .*, line $decimal.\r\n$gdb_prompt $" {}
            -re "$break_message \[0-9\]*: file .*, line $decimal.\r\n$gdb_prompt $" {}
            -re "$break_message \[0-9\]* at .*$gdb_prompt $" {}
            -re "$break_message \[0-9\]* \\(.*\\) pending.*$gdb_prompt $" {
                    if {$pending_response == "n"} {
                            if { $print_fail } {
    -                               fail $test_name
    +                               fail $gdb_name_name
                            }
                            return 0
                    }
    @@ -659,35 +656,6 @@ proc gdb_breakpoint { linespec args } {
                    send_gdb "$pending_response\n"
                    exp_continue
            }
    -       -re "A problem internal to GDB has been detected" {
    -               if { $print_fail } {
    -                   fail "$test_name (GDB internal error)"
    -               }
    -               gdb_internal_error_resync
    -               return 0
    -       }
    -       -re "$gdb_prompt $" {
    -               if { $print_fail } {
    -                       fail $test_name
    -               }
    -               return 0
    -       }
    -       eof {
    -               perror "GDB process no longer exists"
    -               global gdb_spawn_id
    -               set wait_status [wait -i $gdb_spawn_id]
    -               verbose -log "GDB process exited with wait status $wait_status"
    -               if { $print_fail } {
    -                       fail "$test_name (eof)"
    -               }
    -               return 0
    -       }
    -       timeout {
    -               if { $print_fail } {
    -                       fail "$test_name (timeout)"
    -               }
    -               return 0
    -       }
         }
         if { $print_pass } {
            pass $test_name

Just here   ^ $test_name is used, but the patch does not set it anymore.
Looks like that it should become something like:

    set test_name "gdb_breakpoint: set breakpoint at $linespec"
    gdb_test_multiple "$break_command $linespec" $test_name {
      # unchanged
    }
    if { $print_pass } {
      pass $test_name
    }

Sorry I missed that earlier.

Lancelot.
  

Something like this should fix the issue:.

    From 25585cc98812c90f819d78742243299a933ac879 Mon Sep 17 00:00:00 2001
    From: Lancelot SIX <lancelot.six@amd.com>
    Date: Wed, 4 Jan 2023 17:58:08 +0000
    Subject: [PATCH] gdb: ensure test_name is initialized in gdb_breakpoint
    
    A refactoring in 4b9728bec15 (gdb: use gdb_test_multiple in
    gdb_breakpoint) left the $test_name variable initialized.
    
    This patch fixes this.
    ---
     gdb/testsuite/lib/gdb.exp | 3 ++-
     1 file changed, 2 insertions(+), 1 deletion(-)
    
    diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
    index ba16b2ab315..e17eace4cb1 100644
    --- a/gdb/testsuite/lib/gdb.exp
    +++ b/gdb/testsuite/lib/gdb.exp
    @@ -639,8 +639,9 @@ proc gdb_breakpoint { linespec args } {
     	set print_pass 1
         }
     
    +    set test_name "gdb_breakpoint: set breakpoint at $linespec"
         # The first two regexps are what we get with -g, the third is without -g.
    -    gdb_test_multiple "$break_command $linespec" "gdb_breakpoint: set breakpoint at $linespec" {
    +    gdb_test_multiple "$break_command $linespec" $test_name {
     	-re "$break_message \[0-9\]* at .*: file .*, line $decimal.\r\n$gdb_prompt $" {}
     	-re "$break_message \[0-9\]*: file .*, line $decimal.\r\n$gdb_prompt $" {}
     	-re "$break_message \[0-9\]* at .*$gdb_prompt $" {}
    -- 
    2.34.1

Best,
Lancelot.

On Wed, Jan 04, 2023 at 05:40:38PM +0000, Lancelot SIX via Gdb-patches wrote:
> On Wed, Jan 04, 2023 at 11:22:55AM -0500, Simon Marchi via Gdb-patches wrote:
> > 
> > 
> > On 1/4/23 11:18, Lancelot SIX wrote:
> > >> Oh, right, I missed that.  I folded the test name in the
> > >> gdb_test_multiple call (removed the test_name var) and used
> > >> $gdb_test_name in the fail call.  I updated the patch locally.  Should I
> > >> add your Reviewed-By after those changes?
> > >>
> > >> Simon
> > >>
> > > 
> > > Yes, with those change, feel free to add
> > > 
> > > Reviewed-By: Lancelot Six <lancelot.six@amd.com>
> > > 
> > > Best,
> > > Lancelot.
> > 
> > Thanks, pushed!
> > 
> > Simon
> 
> Hi,
> 
> I just saw the resulting patch, and is seems that $test_name is
> (conditionally) used after the gdb_test_multiple:
> 
>     diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>     index 135ace68d5e..ba16b2ab315 100644
>     --- a/gdb/testsuite/lib/gdb.exp
>     +++ b/gdb/testsuite/lib/gdb.exp
>     @@ -639,18 +639,15 @@ proc gdb_breakpoint { linespec args } {
>             set print_pass 1
>          }
>     
>     -    set test_name "gdb_breakpoint: set breakpoint at $linespec"
>     -
>     -    send_gdb "$break_command $linespec\n"
>          # The first two regexps are what we get with -g, the third is without -g.
>     -    gdb_expect 30 {
>     +    gdb_test_multiple "$break_command $linespec" "gdb_breakpoint: set breakpoint at $linespec" {
>             -re "$break_message \[0-9\]* at .*: file .*, line $decimal.\r\n$gdb_prompt $" {}
>             -re "$break_message \[0-9\]*: file .*, line $decimal.\r\n$gdb_prompt $" {}
>             -re "$break_message \[0-9\]* at .*$gdb_prompt $" {}
>             -re "$break_message \[0-9\]* \\(.*\\) pending.*$gdb_prompt $" {
>                     if {$pending_response == "n"} {
>                             if { $print_fail } {
>     -                               fail $test_name
>     +                               fail $gdb_name_name
>                             }
>                             return 0
>                     }
>     @@ -659,35 +656,6 @@ proc gdb_breakpoint { linespec args } {
>                     send_gdb "$pending_response\n"
>                     exp_continue
>             }
>     -       -re "A problem internal to GDB has been detected" {
>     -               if { $print_fail } {
>     -                   fail "$test_name (GDB internal error)"
>     -               }
>     -               gdb_internal_error_resync
>     -               return 0
>     -       }
>     -       -re "$gdb_prompt $" {
>     -               if { $print_fail } {
>     -                       fail $test_name
>     -               }
>     -               return 0
>     -       }
>     -       eof {
>     -               perror "GDB process no longer exists"
>     -               global gdb_spawn_id
>     -               set wait_status [wait -i $gdb_spawn_id]
>     -               verbose -log "GDB process exited with wait status $wait_status"
>     -               if { $print_fail } {
>     -                       fail "$test_name (eof)"
>     -               }
>     -               return 0
>     -       }
>     -       timeout {
>     -               if { $print_fail } {
>     -                       fail "$test_name (timeout)"
>     -               }
>     -               return 0
>     -       }
>          }
>          if { $print_pass } {
>             pass $test_name
> 
> Just here   ^ $test_name is used, but the patch does not set it anymore.
> Looks like that it should become something like:
> 
>     set test_name "gdb_breakpoint: set breakpoint at $linespec"
>     gdb_test_multiple "$break_command $linespec" $test_name {
>       # unchanged
>     }
>     if { $print_pass } {
>       pass $test_name
>     }
> 
> Sorry I missed that earlier.
> 
> Lancelot.
  

On 1/4/23 13:02, Lancelot SIX wrote:
> Something like this should fix the issue:.
> 
>     From 25585cc98812c90f819d78742243299a933ac879 Mon Sep 17 00:00:00 2001
>     From: Lancelot SIX <lancelot.six@amd.com>
>     Date: Wed, 4 Jan 2023 17:58:08 +0000
>     Subject: [PATCH] gdb: ensure test_name is initialized in gdb_breakpoint
>     
>     A refactoring in 4b9728bec15 (gdb: use gdb_test_multiple in
>     gdb_breakpoint) left the $test_name variable initialized.

Woops, thanks for noticing and for the patch.

initialized -> uninitialized (or undefined)?

>     
>     This patch fixes this.
>     ---
>      gdb/testsuite/lib/gdb.exp | 3 ++-
>      1 file changed, 2 insertions(+), 1 deletion(-)
>     
>     diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>     index ba16b2ab315..e17eace4cb1 100644
>     --- a/gdb/testsuite/lib/gdb.exp
>     +++ b/gdb/testsuite/lib/gdb.exp
>     @@ -639,8 +639,9 @@ proc gdb_breakpoint { linespec args } {
>      	set print_pass 1
>          }
>      
>     +    set test_name "gdb_breakpoint: set breakpoint at $linespec"
>          # The first two regexps are what we get with -g, the third is without -g.
>     -    gdb_test_multiple "$break_command $linespec" "gdb_breakpoint: set breakpoint at $linespec" {
>     +    gdb_test_multiple "$break_command $linespec" $test_name {
>      	-re "$break_message \[0-9\]* at .*: file .*, line $decimal.\r\n$gdb_prompt $" {}
>      	-re "$break_message \[0-9\]*: file .*, line $decimal.\r\n$gdb_prompt $" {}
>      	-re "$break_message \[0-9\]* at .*$gdb_prompt $" {}
>     -- 
>     2.34.1

This LGTM, thanks:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon
  

On Wed, Jan 04, 2023 at 02:05:23PM -0500, Simon Marchi wrote:
> 
> 
> On 1/4/23 13:02, Lancelot SIX wrote:
> > Something like this should fix the issue:.
> > 
> >     From 25585cc98812c90f819d78742243299a933ac879 Mon Sep 17 00:00:00 2001
> >     From: Lancelot SIX <lancelot.six@amd.com>
> >     Date: Wed, 4 Jan 2023 17:58:08 +0000
> >     Subject: [PATCH] gdb: ensure test_name is initialized in gdb_breakpoint
> >     
> >     A refactoring in 4b9728bec15 (gdb: use gdb_test_multiple in
> >     gdb_breakpoint) left the $test_name variable initialized.
> 
> Woops, thanks for noticing and for the patch.
> 
> initialized -> uninitialized (or undefined)?

I went for undefined which is more appropriate.

Pushed with this modification.

Best,
Lancelot.

> 
> >     
> >     This patch fixes this.
> >     ---
> >      gdb/testsuite/lib/gdb.exp | 3 ++-
> >      1 file changed, 2 insertions(+), 1 deletion(-)
> >     
> >     diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> >     index ba16b2ab315..e17eace4cb1 100644
> >     --- a/gdb/testsuite/lib/gdb.exp
> >     +++ b/gdb/testsuite/lib/gdb.exp
> >     @@ -639,8 +639,9 @@ proc gdb_breakpoint { linespec args } {
> >      	set print_pass 1
> >          }
> >      
> >     +    set test_name "gdb_breakpoint: set breakpoint at $linespec"
> >          # The first two regexps are what we get with -g, the third is without -g.
> >     -    gdb_test_multiple "$break_command $linespec" "gdb_breakpoint: set breakpoint at $linespec" {
> >     +    gdb_test_multiple "$break_command $linespec" $test_name {
> >      	-re "$break_message \[0-9\]* at .*: file .*, line $decimal.\r\n$gdb_prompt $" {}
> >      	-re "$break_message \[0-9\]*: file .*, line $decimal.\r\n$gdb_prompt $" {}
> >      	-re "$break_message \[0-9\]* at .*$gdb_prompt $" {}
> >     -- 
> >     2.34.1
> 
> This LGTM, thanks:
> 
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
> 
> Simon
  

On 1/3/23 20:22, Simon Marchi via Gdb-patches wrote:
> -	-re "$gdb_prompt $" {
> -		if { $print_fail } {
> -			fail $test_name
> -		}
> -		return 0
> -	}

This caused:
...
FAIL: gdb.dwarf2/dw2-main-no-line-number.exp: gdb_breakpoint: set 
breakpoint at 1
FAIL: gdb.dwarf2/dw2-main-no-line-number.exp: 
!$breakpoint_at_missing_lineno_set
FAIL: gdb.go/methods.exp: going to first breakpoint (the program exited)
FAIL: gdb.go/methods.exp: going to second breakpoint (the program is no 
longer running)
...

Re-inserting this piece of code fixes it.

Thanks,
- Tom
  

On 1/5/23 04:04, Tom de Vries wrote:
> On 1/3/23 20:22, Simon Marchi via Gdb-patches wrote:
>> -    -re "$gdb_prompt $" {
>> -        if { $print_fail } {
>> -            fail $test_name
>> -        }
>> -        return 0
>> -    }
> 
> This caused:
> ...
> FAIL: gdb.dwarf2/dw2-main-no-line-number.exp: gdb_breakpoint: set breakpoint at 1
> FAIL: gdb.dwarf2/dw2-main-no-line-number.exp: !$breakpoint_at_missing_lineno_set
> FAIL: gdb.go/methods.exp: going to first breakpoint (the program exited)
> FAIL: gdb.go/methods.exp: going to second breakpoint (the program is no longer running)
> ...
> 
> Re-inserting this piece of code fixes it.

Ah, sorry for this, and thanks for reporting.  The CI test job I usually
use is a bit broken right now, so I don't test as well as I should.
Does the patch below look good?


From db4ea2e9710bfe460d5f99ebf8d3fd670a81dfa2 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Thu, 5 Jan 2023 11:23:45 -0500
Subject: [PATCH] gdb/testsuite: add back needed -re clause in gdb_breakpoint

Commit 4b9728be ("gdb: use gdb_test_multiple in gdb_breakpoint") caused,
amongst others:

   (gdb) break 1^M
   No line 1 in the current file.^M
   Make breakpoint pending on future shared library load? (y or [n]) n^M
   (gdb) FAIL: gdb.dwarf2/dw2-main-no-line-number.exp: gdb_breakpoint: set breakpoint at 1
   FAIL: gdb.dwarf2/dw2-main-no-line-number.exp: !$breakpoint_at_missing_lineno_set

This is because it removed one empty -re clause (matching just the
prompt) that is necessary after replying "n" to the pending breakpoint
question.  Add this clause back.

Change-Id: Ibfaa059d58bbea660bc29f0547e2f75c323fcbc6
---
 gdb/testsuite/lib/gdb.exp | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index e17eace4cb13..af538e5c8fbd 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -657,6 +657,12 @@ proc gdb_breakpoint { linespec args } {
                send_gdb "$pending_response\n"
                exp_continue
        }
+       -re "$gdb_prompt $" {
+           if { $print_fail } {
+               fail $test_name
+           }
+           return 0
+       }
     }
     if { $print_pass } {
        pass $test_name

base-commit: d66641b604182246b648f662d3c32200ac921365
  

On 1/5/23 17:28, Simon Marchi wrote:
> On 1/5/23 04:04, Tom de Vries wrote:
>> On 1/3/23 20:22, Simon Marchi via Gdb-patches wrote:
>>> -    -re "$gdb_prompt $" {
>>> -        if { $print_fail } {
>>> -            fail $test_name
>>> -        }
>>> -        return 0
>>> -    }
>>
>> This caused:
>> ...
>> FAIL: gdb.dwarf2/dw2-main-no-line-number.exp: gdb_breakpoint: set breakpoint at 1
>> FAIL: gdb.dwarf2/dw2-main-no-line-number.exp: !$breakpoint_at_missing_lineno_set
>> FAIL: gdb.go/methods.exp: going to first breakpoint (the program exited)
>> FAIL: gdb.go/methods.exp: going to second breakpoint (the program is no longer running)
>> ...
>>
>> Re-inserting this piece of code fixes it.
> 
> Ah, sorry for this, and thanks for reporting.  The CI test job I usually
> use is a bit broken right now, so I don't test as well as I should.

Ah, that's too bad, I hope you get it running properly again.

> Does the patch below look good?
> 

LGTM, thanks.

- Tom

> 
>  From db4ea2e9710bfe460d5f99ebf8d3fd670a81dfa2 Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Thu, 5 Jan 2023 11:23:45 -0500
> Subject: [PATCH] gdb/testsuite: add back needed -re clause in gdb_breakpoint
> 
> Commit 4b9728be ("gdb: use gdb_test_multiple in gdb_breakpoint") caused,
> amongst others:
> 
>     (gdb) break 1^M
>     No line 1 in the current file.^M
>     Make breakpoint pending on future shared library load? (y or [n]) n^M
>     (gdb) FAIL: gdb.dwarf2/dw2-main-no-line-number.exp: gdb_breakpoint: set breakpoint at 1
>     FAIL: gdb.dwarf2/dw2-main-no-line-number.exp: !$breakpoint_at_missing_lineno_set
> 
> This is because it removed one empty -re clause (matching just the
> prompt) that is necessary after replying "n" to the pending breakpoint
> question.  Add this clause back.
> 
> Change-Id: Ibfaa059d58bbea660bc29f0547e2f75c323fcbc6
> ---
>   gdb/testsuite/lib/gdb.exp | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index e17eace4cb13..af538e5c8fbd 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -657,6 +657,12 @@ proc gdb_breakpoint { linespec args } {
>                  send_gdb "$pending_response\n"
>                  exp_continue
>          }
> +       -re "$gdb_prompt $" {
> +           if { $print_fail } {
> +               fail $test_name
> +           }
> +           return 0
> +       }
>       }
>       if { $print_pass } {
>          pass $test_name
> 
> base-commit: d66641b604182246b648f662d3c32200ac921365
  

On 1/5/23 11:31, Tom de Vries wrote:
> On 1/5/23 17:28, Simon Marchi wrote:
>> On 1/5/23 04:04, Tom de Vries wrote:
>>> On 1/3/23 20:22, Simon Marchi via Gdb-patches wrote:
>>>> -    -re "$gdb_prompt $" {
>>>> -        if { $print_fail } {
>>>> -            fail $test_name
>>>> -        }
>>>> -        return 0
>>>> -    }
>>>
>>> This caused:
>>> ...
>>> FAIL: gdb.dwarf2/dw2-main-no-line-number.exp: gdb_breakpoint: set breakpoint at 1
>>> FAIL: gdb.dwarf2/dw2-main-no-line-number.exp: !$breakpoint_at_missing_lineno_set
>>> FAIL: gdb.go/methods.exp: going to first breakpoint (the program exited)
>>> FAIL: gdb.go/methods.exp: going to second breakpoint (the program is no longer running)
>>> ...
>>>
>>> Re-inserting this piece of code fixes it.
>>
>> Ah, sorry for this, and thanks for reporting.  The CI test job I usually
>> use is a bit broken right now, so I don't test as well as I should.
> 
> Ah, that's too bad, I hope you get it running properly again.
> 
>> Does the patch below look good?
>>
> 
> LGTM, thanks.
> 
> - Tom

Thanks, pushed.

Simon
  

On 2023-01-05 4:28 p.m., Simon Marchi via Gdb-patches wrote:

> ---
>  gdb/testsuite/lib/gdb.exp | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index e17eace4cb13..af538e5c8fbd 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -657,6 +657,12 @@ proc gdb_breakpoint { linespec args } {
>                 send_gdb "$pending_response\n"
>                 exp_continue
>         }
> +       -re "$gdb_prompt $" {
> +           if { $print_fail } {
> +               fail $test_name
> +           }
> +           return 0
> +       }
>      }

The other removed "-re" cases also considered $print_fail, so if their replacement
inside gdb_test_multiple is hit, they'll produce a FAIL.  Was that intended?
Should we instead add a "-nofail" option to gdb_test / gdb_test_multiple ?
  

On 1/10/23 10:33, Pedro Alves wrote:
> On 2023-01-05 4:28 p.m., Simon Marchi via Gdb-patches wrote:
> 
>> ---
>>  gdb/testsuite/lib/gdb.exp | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>> index e17eace4cb13..af538e5c8fbd 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -657,6 +657,12 @@ proc gdb_breakpoint { linespec args } {
>>                 send_gdb "$pending_response\n"
>>                 exp_continue
>>         }
>> +       -re "$gdb_prompt $" {
>> +           if { $print_fail } {
>> +               fail $test_name
>> +           }
>> +           return 0
>> +       }
>>      }
> 
> The other removed "-re" cases also considered $print_fail, so if their replacement
> inside gdb_test_multiple is hit, they'll produce a FAIL.  Was that intended?
> Should we instead add a "-nofail" option to gdb_test / gdb_test_multiple ?

Good point, this is a change in behavior.  Does this change cause you an
unexpected FAIL in practice?  I was actually planning on removing that
message / no-message option to gdb_breakpoint [1] to simplify it.  I
don't see any use for the current behavior, I'd rather have it log a
test result all the time.  I can kind of see when that would maybe be
useful: if you wanted to set a breakpoint, and it could legitimately not
work (you could do something with gdb_breakpoint's return value).  But
you could also use the break command directly, like many places do
already, so it's not really needed.  Anyway, all of this to say that I
could fix what you pointed out by pruning / simplifying code rather than
adding more.

Simon

[1] https://review.lttng.org/c/binutils-gdb/+/7158/6
  

On 2023-01-10 3:50 p.m., Simon Marchi wrote:
> 
> 
> On 1/10/23 10:33, Pedro Alves wrote:
>> On 2023-01-05 4:28 p.m., Simon Marchi via Gdb-patches wrote:
>>
>>> ---
>>>  gdb/testsuite/lib/gdb.exp | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>>> index e17eace4cb13..af538e5c8fbd 100644
>>> --- a/gdb/testsuite/lib/gdb.exp
>>> +++ b/gdb/testsuite/lib/gdb.exp
>>> @@ -657,6 +657,12 @@ proc gdb_breakpoint { linespec args } {
>>>                 send_gdb "$pending_response\n"
>>>                 exp_continue
>>>         }
>>> +       -re "$gdb_prompt $" {
>>> +           if { $print_fail } {
>>> +               fail $test_name
>>> +           }
>>> +           return 0
>>> +       }
>>>      }
>>
>> The other removed "-re" cases also considered $print_fail, so if their replacement
>> inside gdb_test_multiple is hit, they'll produce a FAIL.  Was that intended?
>> Should we instead add a "-nofail" option to gdb_test / gdb_test_multiple ?
> 
> Good point, this is a change in behavior.  Does this change cause you an
> unexpected FAIL in practice?  

No, I was just trying to catch up on patches a bit, and read your patch and noticed
that issue.  I was going to reply to the patch directly, but then saw the
follow up discussion and replied there (or rather, here).

> I was actually planning on removing that
> message / no-message option to gdb_breakpoint [1] to simplify it.  I
> don't see any use for the current behavior, I'd rather have it log a
> test result all the time.

I've always found the message/no-message API confusing.  It exists on
runto too, and maybe other procs.  

I think the intention of not issuing PASS by default, was that you can
use gdb_breakpoint to implement other procedures inside lib/gdb.exp.
If gdb_breakpoint starts issuing a PASS, then an implementation detail
of such procedures starts being visible, by ending up with two PASSes for
each call of the procedure that happens to use gdb_breakpoint, one for 
gdb_breakpoint, and one for the caller procedure proper.

I think it's the same reason many procedures in lib/gdb.exp were kept using
gdb_expect directly instead of being converted to gdb_test/gdb_test_multiple -- to
avoid the internal PASS/FAIL.

Pedro Alves

> I can kind of see when that would maybe be
> useful: if you wanted to set a breakpoint, and it could legitimately not
> work (you could do something with gdb_breakpoint's return value).  But
> you could also use the break command directly, like many places do
> already, so it's not really needed.  Anyway, all of this to say that I
> could fix what you pointed out by pruning / simplifying code rather than
> adding more.
> 
> Simon
> 
> [1] https://review.lttng.org/c/binutils-gdb/+/7158/6
>
  

On 1/10/23 14:56, Pedro Alves wrote:
> On 2023-01-10 3:50 p.m., Simon Marchi wrote:
>>
>>
>> On 1/10/23 10:33, Pedro Alves wrote:
>>> On 2023-01-05 4:28 p.m., Simon Marchi via Gdb-patches wrote:
>>>
>>>> ---
>>>>  gdb/testsuite/lib/gdb.exp | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>>>> index e17eace4cb13..af538e5c8fbd 100644
>>>> --- a/gdb/testsuite/lib/gdb.exp
>>>> +++ b/gdb/testsuite/lib/gdb.exp
>>>> @@ -657,6 +657,12 @@ proc gdb_breakpoint { linespec args } {
>>>>                 send_gdb "$pending_response\n"
>>>>                 exp_continue
>>>>         }
>>>> +       -re "$gdb_prompt $" {
>>>> +           if { $print_fail } {
>>>> +               fail $test_name
>>>> +           }
>>>> +           return 0
>>>> +       }
>>>>      }
>>>
>>> The other removed "-re" cases also considered $print_fail, so if their replacement
>>> inside gdb_test_multiple is hit, they'll produce a FAIL.  Was that intended?
>>> Should we instead add a "-nofail" option to gdb_test / gdb_test_multiple ?
>>
>> Good point, this is a change in behavior.  Does this change cause you an
>> unexpected FAIL in practice?  
> 
> No, I was just trying to catch up on patches a bit, and read your patch and noticed
> that issue.  I was going to reply to the patch directly, but then saw the
> follow up discussion and replied there (or rather, here).
> 
>> I was actually planning on removing that
>> message / no-message option to gdb_breakpoint [1] to simplify it.  I
>> don't see any use for the current behavior, I'd rather have it log a
>> test result all the time.
> 
> I've always found the message/no-message API confusing.  It exists on
> runto too, and maybe other procs.

I was planning to remove it from runto too.

> 
> I think the intention of not issuing PASS by default, was that you can
> use gdb_breakpoint to implement other procedures inside lib/gdb.exp.
> If gdb_breakpoint starts issuing a PASS, then an implementation detail
> of such procedures starts being visible, by ending up with two PASSes for
> each call of the procedure that happens to use gdb_breakpoint, one for 
> gdb_breakpoint, and one for the caller procedure proper.

And do you think this is important?  Personally, I don't think the
multiple PASSes is a problem.

Simon
  

On 2023-01-10 8:42 p.m., Simon Marchi wrote:
> On 1/10/23 14:56, Pedro Alves wrote:

>>
>> I think the intention of not issuing PASS by default, was that you can
>> use gdb_breakpoint to implement other procedures inside lib/gdb.exp.
>> If gdb_breakpoint starts issuing a PASS, then an implementation detail
>> of such procedures starts being visible, by ending up with two PASSes for
>> each call of the procedure that happens to use gdb_breakpoint, one for 
>> gdb_breakpoint, and one for the caller procedure proper.
> 
> And do you think this is important?  Personally, I don't think the
> multiple PASSes is a problem.

As mentioned, this is more of a generic concern of all procedures exposed
by lib/gdb.exp.  gdb_breakpoint was just one among many.  Seems better to look
at the overall design/direction rather than just one case in isolation.

Let's take runto.  It calls gdb_breakpoint (which used to use gdb_expect before your change),
and then gdb_run_cmd, and then gdb_expect directly.  gdb_run_cmd itself uses gdb_expect.
gdb_run_cmd may use gdb_reload, which calls gdb_load, which uses gdb_load_cmd, which uses
gdb_expect.

So changing any of these gdb_expect to gdb_test_multiple would result in intermediate PASSes
starting to be emitted.  Depending on refactoring, etc, you'd get different internal PASSes.
Depending on different target, you'd get wildly different internal PASSes, etc.  You'd get
a lot more mismatching PASS/FAIL cases when one of the internal gdb_test_multiple's failed.
Etc.  That doesn't seem like the ideal approach to me.
  

> As mentioned, this is more of a generic concern of all procedures exposed
> by lib/gdb.exp.  gdb_breakpoint was just one among many.  Seems better to look
> at the overall design/direction rather than just one case in isolation.
> 
> Let's take runto.  It calls gdb_breakpoint (which used to use gdb_expect before your change),
> and then gdb_run_cmd, and then gdb_expect directly.  gdb_run_cmd itself uses gdb_expect.
> gdb_run_cmd may use gdb_reload, which calls gdb_load, which uses gdb_load_cmd, which uses
> gdb_expect.
> 
> So changing any of these gdb_expect to gdb_test_multiple would result in intermediate PASSes
> starting to be emitted.  Depending on refactoring, etc, you'd get different internal PASSes.
> Depending on different target, you'd get wildly different internal PASSes, etc.  You'd get
> a lot more mismatching PASS/FAIL cases when one of the internal gdb_test_multiple's failed.
> Etc.  That doesn't seem like the ideal approach to me.
I don't see that as a big problem.  In fact I like having fine-grained
PASSes or FAILs for the various intermediate steps of the test.  I think
it's fine to get different PASSes on different targets, it represents
what's actually being done.  When debugging a test, I look at the first
FAIL/UNRESOLVED. In the example you gave, if the gdb_load_cmd fails, I'd
appreciate having a FAIL for that, close to the source of the problem,
rather than starting from a later point and having to backtrack.

Plus, the fact that I encountered a few times cases like:

  # Return early if do_something fails
  if { ![do_something] } {
    return
  }

... where do_something would not log anything.  So I broke do_something,
but didn't notice because nothing logged a failure.

However, I can see that having different PASSes in each test can be
annoying if you are diffing test results from two target boards.  That
will add noise in the diff.  When mixing these two concerns, I guess
that we arrive to the "don't tell me when it works, but tell me when it
fails" that gdb_breakpoint and others have.  Maybe it's good after all.

Simon