Re: [PATCH 2/2] Add recording support for the ISA 3.1 Powerpc instructions

[Joel Brobecker <brobecker@adacore.com>]

> The patch adds the gdb recording test cases for the Powerpc ISA 2.06
> and ISA 3.1 instruction sets.
> 
> The patch has been run on both Power 10 and Power 9 to verify the ISA
> 2.06 test case runs on both platforms without errors.  The ISA 3.1 test
> runs without errors on Power 10 and is skipped as expected on Power 9.
> 
> Please let me know if this patch is acceptable for gdb mainline. 
> Thanks.

Thanks for this patch.

I am not a specialist of writing testcases in the GDB testsuite,
anymore, so others may have more comments. In the meantime, here
are the things I saw.

FWIW, the description you provide at the start of this email would
also be useful to have in the commit message as well, to give people
a record of what you did without having to read your email version.

One general note: The formatting seems to have been affected by
the sending, which surprises me a little, as this was sent by
"git send-email", is that right? What makes me say this is because
we see a number of lines where it looks like the text was moved
to the next line. E.g.

> +###### Test 1:  Test an ISA 2.06 load (lxvd2x) and square root
> instructiion
> +###### (xvsqrtsp).  The load instruction will load vs1.  The sqrt
> instruction

I'm going to assume this is correct in your patch, but it would
be nice to figure out where this is coming from.


> +++ b/gdb/testsuite/gdb.reverse/ppc_record_test_isa_3_1.c

Can you please add a copyright header to this file, and more
generally, to all new files?

> +/* globals used for vector tests */
> +static vector unsigned long vec_xa, vec_xb, vec_xt;
> +static unsigned long ra, rb, rs;
> +
> +int main () {

We ask that we try to make a reasonable attempt that our test
programs also follow the GNU Coding Standard. So, can you
move the opening curly brace to the next line, please?

I'll let you scan this commit for other formatting issues as
per my comments to your other commit enhancing GDB itself.

> +# The basic flow of the record tests are:
> +#    1) Stop before executing the instructions of interest. Record

Missing second space after period.

> +standard_testfile ppc_record_test_isa_2_06.c

Just thinking out loud: Wondering if ppc_record_test_isa_2_06.c
is actually needed, here...

> +if ![runto_main] then {
> +    perror "couldn't run to breakpoint"
> +    continue
> +}

For this block, let's follow what our testcase template
recommends:

https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/testsuite/gdb.base/template.exp;h=007a1472fee6866e76c3c6d991251502f278a615;hb=HEAD

    | if { ![runto_main] } {
    |     untested "could not run to main"
    |     return
    | }


> +#check initial and new of vs0 are different

Here and below, can you make sure there is a space between
the "#" and the start of the text. Also, can you start them
with an upper-case letter in this case, and end the sentence
with a period? This is mandated by the GNU Coding Style.

> +# Change execution direction to forward for next test
> +gdb_test "set exec-direction forward" "" "Start forward test2"
> +#gdb_test_no_output "set exec-direction forward"

Can you removed this commented-out line? I don't think we need
to keep it.

> +gdb_test "record stop" ".*Process record is stopped.*" "Stopped
> recording"
> +set test_del_bkpts "delete breakpoints, answer prompt"
> +gdb_test_multiple "delete breakpoints" $test_del_bkpts {
> +    -re "Delete all breakpoints.*y or n.*$" {
> +	send_gdb "y\n"
> +    }
> +}
> +
> +gdb_test "record" "" "Start recording test2"

Is this part needed? There doesn't seem anything done after,
so I'm wondering what the purpose of this test is.

(note: if needed, we should use gdb_test_no_output instead)

> +if ![runto_main] then {
> +    perror "couldn't run to breakpoint"
> +    continue
> +}

Same as above.

> +#check initial and new of r0 are different

Here and below, same as above.
> +# Change execution direction to forward for next test
> +gdb_test "set exec-direction forward" "" "Start forward test3"
> +#gdb_test_no_output "set exec-direction forward"

Same as above, can you remove this commented-out line, please?

> +gdb_test "record stop" ".*Process record is stopped.*" "Stopped
> recording 2"
> +set test_del_bkpts "delete breakpoints, answer prompt 2"
> +gdb_test_multiple "delete breakpoints" $test_del_bkpts {
> +    -re "Delete all breakpoints.*y or n.*$" {
> +	send_gdb "y\n"
> +    }
> +}

I think you can simplify the above using something like:

   with confirm off -- delete breakpoints

-- 
Joel