gdb.reverse/time-reverse.exp: test both time syscall and C time function (was: Re: [pushed] [gdb/testsuite] Require syscall time in gdb.reverse/time-reverse.exp)

  On 2023-02-21 1:10 p.m., Tom de Vries via Gdb-patches wrote:
> On aarch64-linux, I run into:
> ...
> Running gdb.reverse/time-reverse.exp ...
> gdb compile failed, gdb.reverse/time-reverse.c: In function 'main':
> gdb.reverse/time-reverse.c:39:12: error: 'SYS_time' undeclared \
>   (first use in this function); did you mean 'SYS_times'?
>    syscall (SYS_time, &time_global);
>             ^~~~~~~~
>             SYS_times
> gdb.reverse/time-reverse.c:39:12: note: each undeclared identifier is \
>   reported only once for each function it appears in
> UNTESTED: gdb.reverse/time-reverse.exp: failed to prepare
> ...
> 
> Fix this by adding a new proc have_syscall, and requiring syscall time, such
> that we have instead:
> ...
> UNSUPPORTED: gdb.reverse/time-reverse.exp: require failed: \
>   expr [have_syscall time]
> ...
> 
> Tested on x86_64-linux and aarch64-linux.

I think the patch below would be even better.  Does it work on aarch64?

From 433d6856be51a9df39e0285af9ae1520af59346a Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Wed, 22 Feb 2023 15:40:58 +0000
Subject: [PATCH] gdb.reverse/time-reverse.exp: test both time syscall and C
 time function

Instead of only testing this on systems that have a SYS_time syscall,
test it everywhere using the time(2) C function, and in addition, run
the tests again using the SYS_time syscall.

The C variant ensures that if some platform uses some syscall we are
not aware of yet, we'll still exercise it, and likely fail, at which
point we should teach GDB about the syscall.

The explicit syscall variant is useful on platforms where the C
function does not call a syscall at all by default, e.g., on some
systems the C time function wraps an implementation provided by the
vDSO.

Change-Id: Id4b755d76577d02c46b8acbfa249d9c31b587633
---
 gdb/testsuite/gdb.reverse/time-reverse.c   |  8 ++-
 gdb/testsuite/gdb.reverse/time-reverse.exp | 71 +++++++++++++++-------
 2 files changed, 57 insertions(+), 22 deletions(-)


base-commit: 5e39600a691e3ba76acf6ab94edb24844c2e82b7
  

On 2/22/23 17:12, Pedro Alves wrote:
> On 2023-02-21 1:10 p.m., Tom de Vries via Gdb-patches wrote:
>> On aarch64-linux, I run into:
>> ...
>> Running gdb.reverse/time-reverse.exp ...
>> gdb compile failed, gdb.reverse/time-reverse.c: In function 'main':
>> gdb.reverse/time-reverse.c:39:12: error: 'SYS_time' undeclared \
>>    (first use in this function); did you mean 'SYS_times'?
>>     syscall (SYS_time, &time_global);
>>              ^~~~~~~~
>>              SYS_times
>> gdb.reverse/time-reverse.c:39:12: note: each undeclared identifier is \
>>    reported only once for each function it appears in
>> UNTESTED: gdb.reverse/time-reverse.exp: failed to prepare
>> ...
>>
>> Fix this by adding a new proc have_syscall, and requiring syscall time, such
>> that we have instead:
>> ...
>> UNSUPPORTED: gdb.reverse/time-reverse.exp: require failed: \
>>    expr [have_syscall time]
>> ...
>>
>> Tested on x86_64-linux and aarch64-linux.
> 
> I think the patch below would be even better.  Does it work on aarch64?
> 

It does for me.

FWIW, doing an strace on the exec, I don't see any syscall related to time.

LGTM.

Thanks,
- Tom

>  From 433d6856be51a9df39e0285af9ae1520af59346a Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Wed, 22 Feb 2023 15:40:58 +0000
> Subject: [PATCH] gdb.reverse/time-reverse.exp: test both time syscall and C
>   time function
> 
> Instead of only testing this on systems that have a SYS_time syscall,
> test it everywhere using the time(2) C function, and in addition, run
> the tests again using the SYS_time syscall.
> 
> The C variant ensures that if some platform uses some syscall we are
> not aware of yet, we'll still exercise it, and likely fail, at which
> point we should teach GDB about the syscall.
> 
> The explicit syscall variant is useful on platforms where the C
> function does not call a syscall at all by default, e.g., on some
> systems the C time function wraps an implementation provided by the
> vDSO.
> 
> Change-Id: Id4b755d76577d02c46b8acbfa249d9c31b587633
> ---
>   gdb/testsuite/gdb.reverse/time-reverse.c   |  8 ++-
>   gdb/testsuite/gdb.reverse/time-reverse.exp | 71 +++++++++++++++-------
>   2 files changed, 57 insertions(+), 22 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.reverse/time-reverse.c b/gdb/testsuite/gdb.reverse/time-reverse.c
> index 668fb102ad2..43f5762d447 100644
> --- a/gdb/testsuite/gdb.reverse/time-reverse.c
> +++ b/gdb/testsuite/gdb.reverse/time-reverse.c
> @@ -20,6 +20,12 @@
>   #include <sys/syscall.h>
>   #include <unistd.h>
>   
> +#ifdef USE_SYSCALL
> +# define my_time(TLOC) syscall (SYS_time, &time_global)
> +#else
> +# define my_time(TLOC) time (TLOC)
> +#endif
> +
>   void
>   marker1 (void)
>   {
> @@ -36,7 +42,7 @@ int
>   main (void)
>   {
>     marker1 ();
> -  syscall (SYS_time, &time_global);
> +  my_time (&time_global);
>     marker2 ();
>     return 0;
>   }
> diff --git a/gdb/testsuite/gdb.reverse/time-reverse.exp b/gdb/testsuite/gdb.reverse/time-reverse.exp
> index befda65d836..91f9911c33a 100644
> --- a/gdb/testsuite/gdb.reverse/time-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/time-reverse.exp
> @@ -23,33 +23,62 @@ require supports_reverse
>   
>   standard_testfile
>   
> -require {expr [have_syscall time]}
> +# MODE is either "syscall" for testing the time syscall explicitly, or
> +# "c" for testing the C time(2) function.
> +proc test {mode} {
> +    set options {debug}
>   
> -if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
> -    return -1
> -}
> +    if {$mode == "syscall"} {
> +	lappend options additional_flags=-DUSE_SYSCALL
> +    } elseif {$mode != "c"} {
> +	error "unrecognized mode: $mode"
> +    }
>   
> -runto_main
> +    if { [prepare_for_testing "failed to prepare" $::testfile-$mode $::srcfile $options] } {
> +	return
> +    }
>   
> -if [supports_process_record] {
> -    # Activate process record/replay
> -    gdb_test_no_output "record" "turn on process record"
> -}
> +    runto_main
> +
> +    if [supports_process_record] {
> +	# Activate process record/replay
> +	gdb_test_no_output "record" "turn on process record"
> +    }
> +
> +    gdb_test "break marker2" \
> +	"Breakpoint $::decimal at $::hex: file .*$::srcfile, line $::decimal.*" \
> +	"set breakpoint at marker2"
> +
> +    gdb_continue_to_breakpoint "marker2" ".*$::srcfile:.*"
>   
> -gdb_test "break marker2" \
> -    "Breakpoint $decimal at $hex: file .*$srcfile, line $decimal.*" \
> -    "set breakpoint at marker2"
> +    gdb_test "break marker1" \
> +	"Breakpoint $::decimal at $::hex: file .*$::srcfile, line $::decimal.*" \
> +	"set breakpoint at marker1"
>   
> -gdb_continue_to_breakpoint "marker2" ".*$srcfile:.*"
> +    gdb_test "reverse-continue" ".*$::srcfile:$::decimal.*" "reverse to marker1"
>   
> -gdb_test "break marker1" \
> -    "Breakpoint $decimal at $hex: file .*$srcfile, line $decimal.*" \
> -    "set breakpoint at marker1"
> +    # If the variable was recorded properly, the old contents (-1)
> +    # will be remembered.  If not, new contents (current time) will be
> +    # used, and the test will fail.
>   
> -gdb_test "reverse-continue" ".*$srcfile:$decimal.*" "reverse to marker1"
> +    gdb_test "print time_global" ".* = -1" "check time record"
> +}
>   
> -# If the variable was recorded properly on syscall, the old contents (-1)
> -# will be remembered.  If not, new contents (current time) will be used,
> -# and the test will fail.
> +# Test both using the syscall explicitly, and using the time(2) C
> +# function.
> +#
> +# The C variant ensures that if some platform uses some syscall we are
> +# not aware of yet, we'll still exercise it (and likely fail).
> +#
> +# The explicit syscall variant is useful on platforms where the C
> +# function does not call a syscall at all by default, e.g., on some
> +# systems the C time function wraps an implementation provided by the
> +# vDSO.
>   
> -gdb_test "print time_global" ".* = -1" "check time record"
> +foreach_with_prefix mode {syscall c} {
> +    if {$mode == "syscall" && ![have_syscall time]} {
> +	continue
> +    }
> +
> +    test $mode
> +}
> 
> base-commit: 5e39600a691e3ba76acf6ab94edb24844c2e82b7
  

On 2023-02-22 5:06 p.m., Tom de Vries wrote:
> On 2/22/23 17:12, Pedro Alves wrote:
>> On 2023-02-21 1:10 p.m., Tom de Vries via Gdb-patches wrote:
>>> On aarch64-linux, I run into:
>>> ...
>>> Running gdb.reverse/time-reverse.exp ...
>>> gdb compile failed, gdb.reverse/time-reverse.c: In function 'main':
>>> gdb.reverse/time-reverse.c:39:12: error: 'SYS_time' undeclared \
>>>    (first use in this function); did you mean 'SYS_times'?
>>>     syscall (SYS_time, &time_global);
>>>              ^~~~~~~~
>>>              SYS_times
>>> gdb.reverse/time-reverse.c:39:12: note: each undeclared identifier is \
>>>    reported only once for each function it appears in
>>> UNTESTED: gdb.reverse/time-reverse.exp: failed to prepare
>>> ...
>>>
>>> Fix this by adding a new proc have_syscall, and requiring syscall time, such
>>> that we have instead:
>>> ...
>>> UNSUPPORTED: gdb.reverse/time-reverse.exp: require failed: \
>>>    expr [have_syscall time]
>>> ...
>>>
>>> Tested on x86_64-linux and aarch64-linux.
>>
>> I think the patch below would be even better.  Does it work on aarch64?
>>
> 
> It does for me.

Thanks.

> 
> FWIW, doing an strace on the exec, I don't see any syscall related to time.

Googling around, it seems like Aarch64 uses the vDSO for time.

> 
> LGTM.
> 

Thanks, I've merged it, with one small tweak:

>> --- a/gdb/testsuite/gdb.reverse/time-reverse.c
>> +++ b/gdb/testsuite/gdb.reverse/time-reverse.c
>> @@ -20,6 +20,12 @@
>>   #include <sys/syscall.h>
>>   #include <unistd.h>
>>   +#ifdef USE_SYSCALL
>> +# define my_time(TLOC) syscall (SYS_time, &time_global)

I did not mean to hardcode the global here.  I meant to write instead:

 # define my_time(TLOC) syscall (SYS_time, TLOC)

Fixed in the pushed version.

>> +#else
>> +# define my_time(TLOC) time (TLOC)
>> +#endif
>> +