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

[Tom de Vries]

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