[gdb/testsuite] Record less in gdb.reverse/time-reverse.exp

Message ID 20250124125054.1597-1-tdevries@suse.de
State Committed
Headers
Series [gdb/testsuite] Record less in gdb.reverse/time-reverse.exp |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 fail Patch failed to apply

Commit Message

Tom de Vries Jan. 24, 2025, 12:50 p.m. UTC
  While stepping through gdb.reverse/time-reverse.exp I realized that we're
recording the instructions for resolving the PLT entries for functions time
and syscall, while that's not really the focus of the test-case.

Limit the scope of the test, by calling the functions once before starting
to record.

Also call "info record" after recording to make it clear how many
instructions where recorded.

On x86_64-linux, before this patch (but with info record added), we have:
...
$ grep "Log contains" gdb.log
Log contains 750 instructions.
Log contains 1218 instructions.
...
and with this patch we have:
...
$ grep "Log contains" gdb.log
Log contains 24 instructions.
Log contains 19 instructions.
...

Tested on x86_64-linux.
---
 gdb/testsuite/gdb.reverse/time-reverse.c   | 12 ++++++++++++
 gdb/testsuite/gdb.reverse/time-reverse.exp |  5 ++++-
 2 files changed, 16 insertions(+), 1 deletion(-)


base-commit: 4998f9ea9d351d546f317af80e054a5d615edaae
  

Comments

Guinevere Larsen Jan. 24, 2025, 2:53 p.m. UTC | #1
On 1/24/25 9:50 AM, Tom de Vries wrote:
> While stepping through gdb.reverse/time-reverse.exp I realized that we're
> recording the instructions for resolving the PLT entries for functions time
> and syscall, while that's not really the focus of the test-case.
>
> Limit the scope of the test, by calling the functions once before starting
> to record.
>
> Also call "info record" after recording to make it clear how many
> instructions where recorded.
>
> On x86_64-linux, before this patch (but with info record added), we have:
> ...
> $ grep "Log contains" gdb.log
> Log contains 750 instructions.
> Log contains 1218 instructions.
> ...
> and with this patch we have:
> ...
> $ grep "Log contains" gdb.log
> Log contains 24 instructions.
> Log contains 19 instructions.
> ...
>
> Tested on x86_64-linux.
> ---

Hi Tom, thanks for catching this!

I have a minor comment inlined, but with that fixed, Approved-By: 
Guinevere Larsen <guinevere@redhat.com>

>   gdb/testsuite/gdb.reverse/time-reverse.c   | 12 ++++++++++++
>   gdb/testsuite/gdb.reverse/time-reverse.exp |  5 ++++-
>   2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/testsuite/gdb.reverse/time-reverse.c b/gdb/testsuite/gdb.reverse/time-reverse.c
> index 8a552f820ad..c4ce1282620 100644
> --- a/gdb/testsuite/gdb.reverse/time-reverse.c
> +++ b/gdb/testsuite/gdb.reverse/time-reverse.c
> @@ -41,8 +41,20 @@ time_t time_global = -1;
>   int
>   main (void)
>   {
> +  /* Call once before recording to resolve the PLT, if any.  This reduces the
> +     amount of instructions that is recorded.  */
> +  my_time (&time_global);
> +
> +  /* Reset back to initial value.  */
> +  time_global = -1;
> +
> +  /* Start recording here.  */
>     marker1 ();
> +
>     my_time (&time_global);
> +
> +  /* Stop recording here.  */
>     marker2 ();
> +
>     return 0;
>   }
> diff --git a/gdb/testsuite/gdb.reverse/time-reverse.exp b/gdb/testsuite/gdb.reverse/time-reverse.exp
> index e83da319899..1343bf1f23d 100644
> --- a/gdb/testsuite/gdb.reverse/time-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/time-reverse.exp
> @@ -38,7 +38,7 @@ proc test {mode} {
>   	return
>       }
>   
> -    runto_main
> +    runto marker1
>   
>       if [supports_process_record] {
>   	# Activate process record/replay
> @@ -51,6 +51,9 @@ proc test {mode} {
>   
>       gdb_continue_to_breakpoint "marker2" ".*$::srcfile:.*"
>   
> +    # Show how many instructions we've recorded.
> +    gdb_test "info record" "Active record target: .*"
> +
>       gdb_test "break marker1" \
>   	"Breakpoint $::decimal at $::hex: file .*$::srcfile, line $::decimal.*" \
>   	"set breakpoint at marker1"

Now that you're running to marker1, we get the following:

break marker1
Note: breakpoint 1 also set at pc 0x40112a.
Breakpoint 3 at 0x40112a: file (...)

So it's safe to remove this test "break".

As a side-note, a bit earlier there is another gdb_test "break ...". 
Since you're already touching this file, it'd be nice to convert it to 
"gdb_breakpoint", but that's just a suggestion, take it or leave it as 
you wish.
  
Tom de Vries Jan. 24, 2025, 3:40 p.m. UTC | #2
On 1/24/25 15:53, Guinevere Larsen wrote:
> On 1/24/25 9:50 AM, Tom de Vries wrote:
>> While stepping through gdb.reverse/time-reverse.exp I realized that we're
>> recording the instructions for resolving the PLT entries for functions 
>> time
>> and syscall, while that's not really the focus of the test-case.
>>
>> Limit the scope of the test, by calling the functions once before 
>> starting
>> to record.
>>
>> Also call "info record" after recording to make it clear how many
>> instructions where recorded.
>>
>> On x86_64-linux, before this patch (but with info record added), we have:
>> ...
>> $ grep "Log contains" gdb.log
>> Log contains 750 instructions.
>> Log contains 1218 instructions.
>> ...
>> and with this patch we have:
>> ...
>> $ grep "Log contains" gdb.log
>> Log contains 24 instructions.
>> Log contains 19 instructions.
>> ...
>>
>> Tested on x86_64-linux.
>> ---
> 
> Hi Tom, thanks for catching this!
> 
> I have a minor comment inlined, but with that fixed, Approved-By: 
> Guinevere Larsen <guinevere@redhat.com>
> 
>>   gdb/testsuite/gdb.reverse/time-reverse.c   | 12 ++++++++++++
>>   gdb/testsuite/gdb.reverse/time-reverse.exp |  5 ++++-
>>   2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/testsuite/gdb.reverse/time-reverse.c b/gdb/testsuite/ 
>> gdb.reverse/time-reverse.c
>> index 8a552f820ad..c4ce1282620 100644
>> --- a/gdb/testsuite/gdb.reverse/time-reverse.c
>> +++ b/gdb/testsuite/gdb.reverse/time-reverse.c
>> @@ -41,8 +41,20 @@ time_t time_global = -1;
>>   int
>>   main (void)
>>   {
>> +  /* Call once before recording to resolve the PLT, if any.  This 
>> reduces the
>> +     amount of instructions that is recorded.  */
>> +  my_time (&time_global);
>> +
>> +  /* Reset back to initial value.  */
>> +  time_global = -1;
>> +
>> +  /* Start recording here.  */
>>     marker1 ();
>> +
>>     my_time (&time_global);
>> +
>> +  /* Stop recording here.  */
>>     marker2 ();
>> +
>>     return 0;
>>   }
>> diff --git a/gdb/testsuite/gdb.reverse/time-reverse.exp b/gdb/ 
>> testsuite/gdb.reverse/time-reverse.exp
>> index e83da319899..1343bf1f23d 100644
>> --- a/gdb/testsuite/gdb.reverse/time-reverse.exp
>> +++ b/gdb/testsuite/gdb.reverse/time-reverse.exp
>> @@ -38,7 +38,7 @@ proc test {mode} {
>>       return
>>       }
>> -    runto_main
>> +    runto marker1
>>       if [supports_process_record] {
>>       # Activate process record/replay
>> @@ -51,6 +51,9 @@ proc test {mode} {
>>       gdb_continue_to_breakpoint "marker2" ".*$::srcfile:.*"
>> +    # Show how many instructions we've recorded.
>> +    gdb_test "info record" "Active record target: .*"
>> +
>>       gdb_test "break marker1" \
>>       "Breakpoint $::decimal at $::hex: file .*$::srcfile, line 
>> $::decimal.*" \
>>       "set breakpoint at marker1"
> 
> Now that you're running to marker1, we get the following:
> 
> break marker1
> Note: breakpoint 1 also set at pc 0x40112a.
> Breakpoint 3 at 0x40112a: file (...)
> 
> So it's safe to remove this test "break".
> 

Hi Gwen,

thanks for the review.

I've removed the "break marker1".

 > As a side-note, a bit earlier there is another gdb_test "break ...". 
 > Since you're already touching this file, it'd be nice to convert it to
> "gdb_breakpoint", but that's just a suggestion, take it or leave it as 
> you wish.
> 

I see, true, but I've left it as is.

I've pushed this.

Thanks,
- Tom
  

Patch

diff --git a/gdb/testsuite/gdb.reverse/time-reverse.c b/gdb/testsuite/gdb.reverse/time-reverse.c
index 8a552f820ad..c4ce1282620 100644
--- a/gdb/testsuite/gdb.reverse/time-reverse.c
+++ b/gdb/testsuite/gdb.reverse/time-reverse.c
@@ -41,8 +41,20 @@  time_t time_global = -1;
 int
 main (void)
 {
+  /* Call once before recording to resolve the PLT, if any.  This reduces the
+     amount of instructions that is recorded.  */
+  my_time (&time_global);
+
+  /* Reset back to initial value.  */
+  time_global = -1;
+
+  /* Start recording here.  */
   marker1 ();
+
   my_time (&time_global);
+
+  /* Stop recording here.  */
   marker2 ();
+
   return 0;
 }
diff --git a/gdb/testsuite/gdb.reverse/time-reverse.exp b/gdb/testsuite/gdb.reverse/time-reverse.exp
index e83da319899..1343bf1f23d 100644
--- a/gdb/testsuite/gdb.reverse/time-reverse.exp
+++ b/gdb/testsuite/gdb.reverse/time-reverse.exp
@@ -38,7 +38,7 @@  proc test {mode} {
 	return
     }
 
-    runto_main
+    runto marker1
 
     if [supports_process_record] {
 	# Activate process record/replay
@@ -51,6 +51,9 @@  proc test {mode} {
 
     gdb_continue_to_breakpoint "marker2" ".*$::srcfile:.*"
 
+    # Show how many instructions we've recorded.
+    gdb_test "info record" "Active record target: .*"
+
     gdb_test "break marker1" \
 	"Breakpoint $::decimal at $::hex: file .*$::srcfile, line $::decimal.*" \
 	"set breakpoint at marker1"