Re: [PATCH v3] gdb: add 'maintenance print record-instruction' command

[Bruno Larsen <blarsen@redhat.com>]

On 02/01/2023 17:26, Lancelot SIX wrote:
> Hi Bruno
>
>> diff --git a/gdb/testsuite/gdb.reverse/maint-print-instruction.c b/gdb/testsuite/gdb.reverse/maint-print-instruction.c
>> new file mode 100644
>> index 00000000000..f06ed530620
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.reverse/maint-print-instruction.c
>> @@ -0,0 +1,25 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright 2022 Free Software Foundation, Inc.
> Just mentioning so you do not forget, this will need to change to
> 2022-2023 before this gets pushed.
nice catch, thanks!
>
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +int
>> +main ()
>> +{
>> +  int x = 0;
>> +  x ++;
>> +  x --;
>> +  return x;
>> +}
>> diff --git a/gdb/testsuite/gdb.reverse/maint-print-instruction.exp b/gdb/testsuite/gdb.reverse/maint-print-instruction.exp
>> new file mode 100644
>> index 00000000000..b78b696add6
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.reverse/maint-print-instruction.exp
>> @@ -0,0 +1,66 @@
>> +#   Copyright 2008-2022 Free Software Foundation, Inc.
> Should be 2022-2023.
>
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +
>> +# This file is part of the GDB testsuite.  it tests the functionality of
>                                                ^
> Missing an uppercase here.
>
>> +# the maintenance print record-instruction command, but does not check the
>> +# syntax, only if the command finds or fails to find recorded history.
>> +
>> +if ![supports_reverse] {
>> +    return
>> +}
>> +
>> +standard_testfile
>> +
>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
>> +    return -1
>> +}
>> +
>> +proc test_print { has_history level test_name } {
>> +    gdb_test_multiple "maint print record-instruction $level" $test_name {
>> +	-re -wrap ".*Not enough recorded history.*" {
>> +	    gdb_assert !$has_history $test_name
>> +	}
>> +
>> +    -re -wrap ".*changed.*" {
>> +	    gdb_assert $has_history $test_name
>> +	}
>> +    }
>> +}
>> +
>> +if { ![runto_main] } {
>> +    return 0
>> +}
>> +
>> +test_print false "" "print before starting to record"
>> +
>> +if [supports_process_record] {
>> +    # Activate process record/replay
>> +    gdb_test_no_output "record" "turn on process record"
>> +}
>> +
> I do not think the rest of the test makes much sense if
> [supports_process_record] returns false, does it?  I guess that
> everything below this point should be be in the if block.
>
> Another approach might to have the initial test (at the top of the file)
> check for both supports_reverse and supports_process_record and ignore
> the test if either feature is not supported.  WDYT?
Good call, but since I do think it makes sense to test that GDB doesn't 
go wild even if recording is not supported, I think I'll make the test 
exit early here instead.
>
> For what it is worth, the rest of the patch looks OK to me.
Awesome, may I add your R-b tag, or do you want to see my change to the 
test first?
>
> Best,
> Lancelot.
>
>> +test_print false "" "print before any instruction"
>> +
>> +gdb_test "stepi 3" ".*" "collecting history"
>> +test_print true "" "print current after executing a bit"
>> +test_print true "-1" "print previous after executing a bit"
>> +test_print false "1" "print following after executing a bit"
>> +
>> +gdb_test "reverse-stepi" ".*" "moving back"
>> +test_print true "" "print current after reversing"
>> +test_print true "-1" "print previous after reversing"
>> +test_print true "1" "print following after reversing"
>> +
>> +test_print false "-10" "trying to print too far back"
>> +test_print false "10" "trying to print too far forward"
>> -- 
>> 2.38.1
>>

-- 
Cheers,
Bruno