[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