[pushed] [gdb/testsuite] Require syscall time in gdb.reverse/time-reverse.exp

[pushed] [gdb/testsuite] Require syscall time in gdb.reverse/time-reverse.exp

[Tom de Vries]

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.
---
 gdb/testsuite/gdb.reverse/time-reverse.exp |  2 ++
 gdb/testsuite/lib/gdb.exp                  | 11 +++++++++++
 2 files changed, 13 insertions(+)

diff --git a/gdb/testsuite/gdb.reverse/time-reverse.exp b/gdb/testsuite/gdb.reverse/time-reverse.exp
index 07d55b348e8..befda65d836 100644
--- a/gdb/testsuite/gdb.reverse/time-reverse.exp
+++ b/gdb/testsuite/gdb.reverse/time-reverse.exp
@@ -23,6 +23,8 @@ require supports_reverse
 
 standard_testfile
 
+require {expr [have_syscall time]}
+
 if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
     return -1
 }
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index e48228ed4f6..12839a54710 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -9397,5 +9397,16 @@ gdb_caching_proc linux_kernel_version {
     return [list $v1 $v2 $v3]
 }
 
+# Return 1 if syscall NAME is supported.
+
+proc have_syscall { name } {
+    set src \
+	[list \
+	     "#include <sys/syscall.h>" \
+	     "int var = SYS_$name;"]
+    set src [join $src "\n"]
+    return [gdb_can_simple_compile have_syscall_$name $src object]
+}
+
 # Always load compatibility stuff.
 load_lib future.exp

base-commit: d720176596dd6495c258fa73c4029f009616ebd2
-- 
2.35.3

Re: [pushed] [gdb/testsuite] Require syscall time in gdb.reverse/time-reverse.exp

[Tom Tromey]

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> +require {expr [have_syscall time]}

I think simply "require {have_syscall time}" should work.

Tom

Re: [pushed] [gdb/testsuite] Require syscall time in gdb.reverse/time-reverse.exp

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 372 bytes --]

On 2/21/23 20:44, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> +require {expr [have_syscall time]}
> 
> I think simply "require {have_syscall time}" should work.

Thanks for the review.

Pedro already cleaned this one up, but I found it in another test-case 
as well.

Fixed by commit below.

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Cleanup-unnecessary-expr-from-require-.patch --]
[-- Type: text/x-patch, Size: 1009 bytes --]

From 4f688a5dd7d5710e29f0606bf1c7aede8a6d115e Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Wed, 22 Feb 2023 09:35:07 +0100
Subject: [PATCH] [gdb/testsuite] Cleanup unnecessary expr from require line

In a recent commit I've added:
...
require {expr [have_compile_flag -fsplit-stack]}
...
but actually the expr bit is unnecessary, and we can just use:
...
require {have_compile_flag -fsplit-stack}
...

Reported-By: Tom Tromey <tom@tromey.com>
---
 gdb/testsuite/gdb.base/morestack.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/morestack.exp b/gdb/testsuite/gdb.base/morestack.exp
index b4d22187390..cc81522a51e 100644
--- a/gdb/testsuite/gdb.base/morestack.exp
+++ b/gdb/testsuite/gdb.base/morestack.exp
@@ -15,7 +15,7 @@
 
 require is_c_compiler_gcc
 
-require {expr [have_compile_flag -fsplit-stack]}
+require {have_compile_flag -fsplit-stack}
 
 standard_testfile
 

base-commit: 0676ec3c22f096bdf98b145b720feeef9493b5bb
-- 
2.35.3

[PATCH] 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)

[Pedro Alves]

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(-)

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
-- 
2.36.0

Re: [PATCH] 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)

[Tom de Vries]

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

Re: [PATCH] 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)

[Pedro Alves]

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
>> +