[v2] Require async support for DAP tests [PR34548]

  Hi Tom,

>>>>>> "Rainer" == Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:
>
> Rainer> Right: some of them already do their own checks for async support:
>
> Rainer>       gdb.base/async.exp
> Rainer>       gdb.base/bg-exec-sigint-bp-cond.exp
> Rainer>       gdb.base/interrupt-noterm.exp
>
> Rainer> but in those cases, async commands are a crucial part of the tests and I
> Rainer> couldn't easily use the new supports_target_async there.
>
> I think it would be fine to stick
>
>     require supports_target_async
>
> At the top of each of these files, and simplify some of the tests to
> just assumes async execution will work.

in some cases, this seems to be the right way forward.  However, I've a
hard time telling which ones should have this treatment.  Others clearly
run tests both in sync and async modes (like
gdb.base/infcall-timeout.exp,
gdb.threads/infcall-from-bp-cond-timeout.exp).  At least in those cases,
we can just skip the async subtests.

> However that can also be comfortably done in a separate patch as well.

I'd rather do it this way, initially concentrating on the cases where
skipping target_async tests can avoid large numbers of timeouts.

> Rainer> +# Return 1 if target supports asynchronous execution, otherwise return 0.
> Rainer> +gdb_caching_proc supports_target_async {} {
> Rainer> +    global srcdir subdir gdb_prompt
> Rainer> +
> Rainer> +    set me "supports_target_async"
> Rainer> +
> Rainer> +    set src { int main() { return 0; } }
> Rainer> +    if {![gdb_simple_compile $me $src executable ""]} {
> Rainer> +	return 0
> Rainer> +    }
> Rainer> +
> Rainer> +    gdb_exit
> Rainer> +    gdb_start
> Rainer> +    gdb_reinitialize_dir $srcdir/$subdir
>
> I think these three lines can be replaced with "clean_restart".

Nice.  So far, I've simply followed existing current practice, although
I've wondered how many tests seem extremely repetetive and
consequentially hard to read.

> Rainer> +    if { ![runto_main] } {
> Rainer> +	return 0
>
> This branch doesn't gdb_exit but falling through does.
> Likewise this return doesn't delete the file.
>
> So maybe reworking this to fall through to the cleanup & final return
> would be better.

Done in the attached revision.

Thanks.
	Rainer
  

>>>>> "Rainer" == Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:

>> I think these three lines can be replaced with "clean_restart".

Rainer> Nice.  So far, I've simply followed existing current practice, although
Rainer> I've wondered how many tests seem extremely repetetive and
Rainer> consequentially hard to read.

Yeah.  Not everything gets updated in a timely way, sometimes things are
missed, etc.

Rainer> Done in the attached revision.

Looks good.
Approved-By: Tom Tromey <tom@tromey.com>

Tom
  

Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:

> Hi Tom,
>
>>>>>>> "Rainer" == Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:
>>
>> Rainer> Right: some of them already do their own checks for async support:
>>
>> Rainer>       gdb.base/async.exp
>> Rainer>       gdb.base/bg-exec-sigint-bp-cond.exp
>> Rainer>       gdb.base/interrupt-noterm.exp
>>
>> Rainer> but in those cases, async commands are a crucial part of the tests and I
>> Rainer> couldn't easily use the new supports_target_async there.
>>
>> I think it would be fine to stick
>>
>>     require supports_target_async
>>
>> At the top of each of these files, and simplify some of the tests to
>> just assumes async execution will work.
>
> in some cases, this seems to be the right way forward.  However, I've a
> hard time telling which ones should have this treatment.  Others clearly
> run tests both in sync and async modes (like
> gdb.base/infcall-timeout.exp,
> gdb.threads/infcall-from-bp-cond-timeout.exp).  At least in those cases,
> we can just skip the async subtests.
>
>> However that can also be comfortably done in a separate patch as well.
>
> I'd rather do it this way, initially concentrating on the cases where
> skipping target_async tests can avoid large numbers of timeouts.
>
>> Rainer> +# Return 1 if target supports asynchronous execution, otherwise return 0.
>> Rainer> +gdb_caching_proc supports_target_async {} {
>> Rainer> +    global srcdir subdir gdb_prompt
>> Rainer> +
>> Rainer> +    set me "supports_target_async"
>> Rainer> +
>> Rainer> +    set src { int main() { return 0; } }
>> Rainer> +    if {![gdb_simple_compile $me $src executable ""]} {
>> Rainer> +	return 0
>> Rainer> +    }
>> Rainer> +
>> Rainer> +    gdb_exit
>> Rainer> +    gdb_start
>> Rainer> +    gdb_reinitialize_dir $srcdir/$subdir
>>
>> I think these three lines can be replaced with "clean_restart".
>
> Nice.  So far, I've simply followed existing current practice, although
> I've wondered how many tests seem extremely repetetive and
> consequentially hard to read.
>
>> Rainer> +    if { ![runto_main] } {
>> Rainer> +	return 0
>>
>> This branch doesn't gdb_exit but falling through does.
>> Likewise this return doesn't delete the file.
>>
>> So maybe reworking this to fall through to the cleanup & final return
>> would be better.
>
> Done in the attached revision.
>
> Thanks.
> 	Rainer
>
> -- 
> -----------------------------------------------------------------------------
> Rainer Orth, Center for Biotechnology, Bielefeld University
>
>
> # HG changeset patch
> # Parent  3fcec728f85b5d0cfef637d13368c88dad2e7bbc
> Require async support for DAP tests [PR34548]
>
> diff --git a/gdb/testsuite/gdb.dap/pause.exp b/gdb/testsuite/gdb.dap/pause.exp
> --- a/gdb/testsuite/gdb.dap/pause.exp
> +++ b/gdb/testsuite/gdb.dap/pause.exp
> @@ -15,9 +15,6 @@
>  
>  # Test "pause" in DAP.
>  
> -# PR dap/34548
> -require {!istarget "*-*-solaris2*"}
> -
>  require allow_dap_tests
>  
>  load_lib dap-support.exp
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -3070,6 +3070,10 @@ gdb_caching_proc with_system_readline {}
>  }
>  
>  gdb_caching_proc allow_dap_tests {} {
> +    if { ![supports_target_async] } {
> +	return 0
> +    }
> +
>      if { ![allow_python_tests] } {
>  	return 0
>      }
> @@ -3908,6 +3912,37 @@ gdb_caching_proc supports_memtag {} {
>      return 0
>  }
>  
> +# Return 1 if target supports asynchronous execution, otherwise return 0.
> +gdb_caching_proc supports_target_async {} {
> +    global srcdir subdir gdb_prompt

These are not needed and should be removed.

> +
> +    set me "supports_target_async"
> +
> +    set src { int main() { return 0; } }
> +    if {![gdb_simple_compile $me $src executable ""]} {
> +	return 0
> +    }
> +
> +    clean_restart
> +    gdb_load $obj
> +
> +    if { [runto_main] } {
> +	set res 0

If 'runto_main' fails then RES is left unset.  Please can you initialise
it outside the `if`.

> +	gdb_test_multiple "continue &" "" {
> +	    -re "Continuing..*" {
> +		set res 1
> +	    }
> +	    -re ".*Asynchronous execution not supported on this target..*" {
> +		set res 0
> +	    }

Both of these would be better written as '-re -wrap "....etc..."' adding
the -wrap will cause the $gdb_prompt to be matched.  I don't think it
really matters in this case as you do call gdb_exit immediately after,
so I think any pending output from GDB is discarded, but it does make
the gdb.log a little neater, and so easier to grok.

With these 3 fixes:

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

thanks,
Andrew

> +	}
> +    }
> +
> +    gdb_exit
> +    remote_file build delete $obj
> +    return $res
> +}
> +
>  # Return 1 if catch syscall is supported, otherwise return 0.
>  
>  gdb_caching_proc supports_catch_syscall {} {
  

Hi Andrew,

>> +# Return 1 if target supports asynchronous execution, otherwise return 0.
>> +gdb_caching_proc supports_target_async {} {
>> +    global srcdir subdir gdb_prompt
>
> These are not needed and should be removed.

right: they are a remnant of the explicit gdb_reinitialize_dir
$srcdir/$subdir in the first version.

>> +	gdb_test_multiple "continue &" "" {
>> +	    -re "Continuing..*" {
>> +		set res 1
>> +	    }
>> +	    -re ".*Asynchronous execution not supported on this target..*" {
>> +		set res 0
>> +	    }
>
> Both of these would be better written as '-re -wrap "....etc..."' adding
> the -wrap will cause the $gdb_prompt to be matched.  I don't think it
> really matters in this case as you do call gdb_exit immediately after,
> so I think any pending output from GDB is discarded, but it does make
> the gdb.log a little neater, and so easier to grok.

Unfortunately this doesn't work for the same reason I'd removed the
explicit $gdb_prompt anchor

      -re "Continuing\\.\r\n$gdb_prompt $" {

that I'd tried initially: in some cases, you get output like

(gdb) continue &^M
Continuing.^M
(gdb) [Inferior 1 (process 3669382) exited normally]^M
FAIL: gdb.dap/ada-scopes.exp: continue & (timeout)

instead of the expected

(gdb) continue &^M
Continuing.^M
(gdb) gdb_caching_proc supports_target_async caused gdb_exit to be called

causing supports_target_async to incorrectly return 0.

So that's what I'd like to check in.

	Rainer
  

Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:

> Hi Andrew,
>
>>> +# Return 1 if target supports asynchronous execution, otherwise return 0.
>>> +gdb_caching_proc supports_target_async {} {
>>> +    global srcdir subdir gdb_prompt
>>
>> These are not needed and should be removed.
>
> right: they are a remnant of the explicit gdb_reinitialize_dir
> $srcdir/$subdir in the first version.
>
>>> +	gdb_test_multiple "continue &" "" {
>>> +	    -re "Continuing..*" {
>>> +		set res 1
>>> +	    }
>>> +	    -re ".*Asynchronous execution not supported on this target..*" {
>>> +		set res 0
>>> +	    }
>>
>> Both of these would be better written as '-re -wrap "....etc..."' adding
>> the -wrap will cause the $gdb_prompt to be matched.  I don't think it
>> really matters in this case as you do call gdb_exit immediately after,
>> so I think any pending output from GDB is discarded, but it does make
>> the gdb.log a little neater, and so easier to grok.
>
> Unfortunately this doesn't work for the same reason I'd removed the
> explicit $gdb_prompt anchor
>
>       -re "Continuing\\.\r\n$gdb_prompt $" {
>
> that I'd tried initially: in some cases, you get output like
>
> (gdb) continue &^M
> Continuing.^M
> (gdb) [Inferior 1 (process 3669382) exited normally]^M
> FAIL: gdb.dap/ada-scopes.exp: continue & (timeout)
>
> instead of the expected
>
> (gdb) continue &^M
> Continuing.^M
> (gdb) gdb_caching_proc supports_target_async caused gdb_exit to be called
>
> causing supports_target_async to incorrectly return 0.
>
> So that's what I'd like to check in.

OK, thanks for clarifying.

The usual way to handle this is to run a synchronous command afterwars,
and use that to discard any pending output,

  gdb_test "p 123" " = 123" "clear expect buffer"

but in this case, as you immediately exit, I don't think that's really
needed.  I'm happy if you just fix the other issues which I think are
still valid.

Thanks,
Andrew



>
> 	Rainer
>
> -- 
> -----------------------------------------------------------------------------
> Rainer Orth, Center for Biotechnology, Bielefeld University
>
>
> # HG changeset patch
> # Parent  069fc8428271fcfc95508f3097833f35443e371f
> Require async support for DAP tests [PR34548]
>
> diff --git a/gdb/testsuite/gdb.dap/pause.exp b/gdb/testsuite/gdb.dap/pause.exp
> --- a/gdb/testsuite/gdb.dap/pause.exp
> +++ b/gdb/testsuite/gdb.dap/pause.exp
> @@ -15,9 +15,6 @@
>  
>  # Test "pause" in DAP.
>  
> -# PR dap/34548
> -require {!istarget "*-*-solaris2*"}
> -
>  require allow_dap_tests
>  
>  load_lib dap-support.exp
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -3070,6 +3070,10 @@ gdb_caching_proc with_system_readline {}
>  }
>  
>  gdb_caching_proc allow_dap_tests {} {
> +    if { ![supports_target_async] } {
> +	return 0
> +    }
> +
>      if { ![allow_python_tests] } {
>  	return 0
>      }
> @@ -3908,6 +3912,35 @@ gdb_caching_proc supports_memtag {} {
>      return 0
>  }
>  
> +# Return 1 if target supports asynchronous execution, otherwise return 0.
> +gdb_caching_proc supports_target_async {} {
> +    set me "supports_target_async"
> +
> +    set src { int main() { return 0; } }
> +    if {![gdb_simple_compile $me $src executable ""]} {
> +	return 0
> +    }
> +
> +    clean_restart
> +    gdb_load $obj
> +
> +    set res 0
> +    if { [runto_main] } {
> +	gdb_test_multiple "continue &" "" {
> +	    -re "Continuing..*" {
> +		set res 1
> +	    }
> +	    -re ".*Asynchronous execution not supported on this target..*" {
> +		set res 0
> +	    }
> +	}
> +    }
> +
> +    gdb_exit
> +    remote_file build delete $obj
> +    return $res
> +}
> +
>  # Return 1 if catch syscall is supported, otherwise return 0.
>  
>  gdb_caching_proc supports_catch_syscall {} {
  

Hi Andrew,

>>>> +	gdb_test_multiple "continue &" "" {
>>>> +	    -re "Continuing..*" {
>>>> +		set res 1
>>>> +	    }
>>>> +	    -re ".*Asynchronous execution not supported on this target..*" {
>>>> +		set res 0
>>>> +	    }
>>>
>>> Both of these would be better written as '-re -wrap "....etc..."' adding
>>> the -wrap will cause the $gdb_prompt to be matched.  I don't think it
>>> really matters in this case as you do call gdb_exit immediately after,
>>> so I think any pending output from GDB is discarded, but it does make
>>> the gdb.log a little neater, and so easier to grok.
>>
>> Unfortunately this doesn't work for the same reason I'd removed the
>> explicit $gdb_prompt anchor
>>
>>       -re "Continuing\\.\r\n$gdb_prompt $" {
>>
>> that I'd tried initially: in some cases, you get output like
>>
>> (gdb) continue &^M
>> Continuing.^M
>> (gdb) [Inferior 1 (process 3669382) exited normally]^M
>> FAIL: gdb.dap/ada-scopes.exp: continue & (timeout)
>>
>> instead of the expected
>>
>> (gdb) continue &^M
>> Continuing.^M
>> (gdb) gdb_caching_proc supports_target_async caused gdb_exit to be called
>>
>> causing supports_target_async to incorrectly return 0.
>>
>> So that's what I'd like to check in.
>
> OK, thanks for clarifying.
>
> The usual way to handle this is to run a synchronous command afterwars,
> and use that to discard any pending output,
>
>   gdb_test "p 123" " = 123" "clear expect buffer"
>
> but in this case, as you immediately exit, I don't think that's really
> needed.  I'm happy if you just fix the other issues which I think are
> still valid.

that's what I'd done in the patch version attached to my last message.

I've now committed the patch.  Now there's only the issue of backporting
it to the gdb-18 branch.

Thanks.
        Rainer