Re: [PATCH] Skip gdb.dap/pauseR.exp on Solaris [PR34548]

[Andrew Burgess <aburgess@redhat.com>]

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

> Hi Andrew,
>
>>>> So maybe DAP testing should be entirely disabled for Solaris?
>>>
>>> Like just returning 0 from allow_dap_tests on Solaris?  Or doing so for
>>> all targets lacking async support?
>>
>> I'd suggest adding:
>>
>>   # Return true for targets that support target async,
>>   # otherwise return false.
>>   proc supports_target_async {} {
>>     # ...
>>   }
>>
>> then return 0 from allow_dap_tests for any target that returns false
>> from the above.  That would seem better than just having a selective fix
>> in the pause.exp file.
>
> that certainly makes the most sense.
>
>>> Here's a breakdown of gdb.dap results on Solaris:
>>>
>>>       8 ERROR
>>>      11 FAIL
>>>     713 PASS
>>>       8 PATH
>>>       8 UNRESOLVED
>>>       2 UNSUPPORTED
>>>
>>> I can't tell if it's still useful this way.
>>
>> I haven't checked by a lot of these passes are going to be general
>> boiler plate stuff.  If DAP support is known to require target async
>
> ... which wasn't completely certain as far as I understood Tom...
>
>> then my personal feeling is that we'd be better just skipping those
>> tests on Solaris.  There's plenty of testing done on other targets where
>> target async is supported.
>
> Right: some of them already do their own checks for async support:
>
>       gdb.base/async.exp
>       gdb.base/bg-exec-sigint-bp-cond.exp
>       gdb.base/interrupt-noterm.exp
>
> but in those cases, async commands are a crucial part of the tests and I
> couldn't easily use the new supports_target_async there.
>
> There are certainly others that aren't handled yet.  Besides, there are
> ca. 400 tests that currently time out on Solaris, massively increasing
> make check time ;-(  I guess it's a major effort to reduce that number.
>
>>>> Rainer> This might also be a candidate for the gdb-18 branch.
>>>>
>>>> It's fine by me.
>>>
>>> Thanks.  I'll way for approval from a release manager then.
>>
>> It might be worth getting the above changes made first.  If you don't
>> have time then let me know and I'll take care of it.
>
> I've now managed to get this working, although it took me some time to
> wrap my head around gdb testing procedures.
>
> Here's what I came up with.  The only change on x86-64-pc-linux-gnu was
>
> +PASS: gdb.testsuite/gdb-caching-proc-consistency.exp: supports_target_async: consistency
>
> while on Solaris it fully disabled the gdb.dap tests as expected.
>
> ---------------------------------------------------------------------------
>
> As detailed in PR PR dap/34548, the gdb.dap/pause.exp test runs
> indefinitely on Solaris.  To allow make check to finish, it needs to be
> terminated manually.
>
> Since DAP requires asynchronous execution support, this patch checks for
> that and disables all gdb.dap tests if missing.
>
> Tested on x86_64-pc-solaris2.11, sparcv9-sun-solaris2.11, and
> x86_64-pc-linux-gnu.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34548
>
> -- 
> -----------------------------------------------------------------------------
> Rainer Orth, Center for Biotechnology, Bielefeld University
>
>
> # HG changeset patch
> # Parent  3fcec728f85b5d0cfef637d13368c88dad2e7bbc
> Require async support for DAP tests [PR34548]
>
> 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,39 @@ 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
> +
> +    set me "supports_target_async"
> +
> +    set src { int main() { return 0; } }
> +    if {![gdb_simple_compile $me $src executable ""]} {
> +	return 0
> +    }
> +
> +    gdb_exit
> +    gdb_start
> +    gdb_reinitialize_dir $srcdir/$subdir
> +    gdb_load $obj
> +    if { ![runto_main] } {
> +	return 0
> +    }
> +
> +    set res 0
> +    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 {} {

Thanks for working on this.

Other than Tom's comments, I think this looks fine.  I will note that
the issues Tom pointed out are present in lots of the other supports_*
procs -- not that I expect you to fix them, just noting that lots of
this code is a little rough.

I'd also ask that you remove these lines from gdb.dap/pause.exp:

  # PR dap/34548
  require {!istarget "*-*-solaris2*"}

at the same time as you merge this, as these are no longer needed.

Thanks,
Andrew