From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f46.google.com (mail-wm1-f46.google.com [209.85.128.46]) by sourceware.org (Postfix) with ESMTPS id DBBBB3858C2C for ; Thu, 14 Apr 2022 13:05:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DBBBB3858C2C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wm1-f46.google.com with SMTP id u17-20020a05600c211100b0038eaf4cdaaeso5931689wml.1 for ; Thu, 14 Apr 2022 06:05:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=hs7vOs2r6klnj/V4lqLIV8i9pISX4WycMBVKTfiMaVg=; b=bsV3qYf3KRbwle4tbIRcJDe50z70pAToiyJ0s58bW4me7svYu7MoKvwgzHg3Jf8S1T HOI/58bNULufyeLA0uyacjg5iZN6u+2ZiMpKcUw5HvhpvuzsOPa3hq6IQ9Kvs6yAUVvx CYoQgfvtV1CBteYDM81iLTvQ7JEw4ox88n5iiN7bcQvLgubfrMN6QTVOkbsmh12LRyJr yFVOR+44yreuS4KwwM1KTTay5jE4oesPp4QFmH+L3M0o118BHizmmC8qHhw7I6rHNXPU uqJBdnUXqz3onrk43ektsGM9/6vpdP6m5M9WdnXAN0/iZNRudKYtbgXcqU7VFPXKFMCD INMQ== X-Gm-Message-State: AOAM530e73oLCdKDBD0vzklAeZh9rxNmhL7VaktrYz6OCSVSF1CrUsVK oVzv9XPQCCogxTz1aUkT9Fk= X-Google-Smtp-Source: ABdhPJxBO0Oz7Nv0lKLq7Jv08aHafwjAPcAlPije+UBajejRGAf+q9KrNKHK2JYVVjmTyR91xROcWA== X-Received: by 2002:a1c:f20d:0:b0:38e:ab1c:3f70 with SMTP id s13-20020a1cf20d000000b0038eab1c3f70mr3581414wmc.27.1649941536500; Thu, 14 Apr 2022 06:05:36 -0700 (PDT) Received: from ?IPV6:2001:8a0:f924:2600:209d:85e2:409e:8726? ([2001:8a0:f924:2600:209d:85e2:409e:8726]) by smtp.gmail.com with ESMTPSA id s7-20020a5d6a87000000b00207aad420c4sm1722697wru.101.2022.04.14.06.05.34 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 14 Apr 2022 06:05:34 -0700 (PDT) Message-ID: <2cde8652-c16a-bfa8-5f28-2339f70bf58f@palves.net> Date: Thu, 14 Apr 2022 14:05:33 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 Subject: Re: [PATCH 2/2 Version 3] Add recording support for the ISA 3.1 Powerpc instructions Content-Language: en-US To: Carl Love , will schmidt , Joel Brobecker , gdb-patches@sourceware.org Cc: Ulrich Weigand , Tulio Magno , Rogerio Alves References: <7ff0655bead2a8245018fbf9624f207cd38a5b7f.camel@us.ibm.com> <18e61a00641bcaeab322db4d416c03ddc30c8950.camel@us.ibm.com> <09bc969fde2b89abbcc6060e9adc0df3c583fd4c.camel@us.ibm.com> From: Pedro Alves In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Apr 2022 13:05:41 -0000 Hi Carl, Some nits below. On 2022-04-13 22:38, Carl Love via Gdb-patches wrote: > > Will, Joel, GDB maintainers: > > Version 2 updated the test cases per the comments from Joel. > > Version 3 updated the test cases and source code files per the comments > from Will. > > I have retested the testcases with version 3 of the gdb source code > patch on Power 9 and Power 10 to verify the tests still work. > > Please let me know if you see any additional issues with the patch. > Thanks. > > Carl Love > -------------------------------------------------------- > GDB Powerpc record test cases for ISA 2.06 and ISA 3.1 > > This patch adds Powerpc specific tests to verify recording of various ISTR that the right spelling was "PowerPC". > instructions. The first test case checks the ISA 2.06 lxvd2x instruction. > The second test case tests several of the ISA 3.01 instructions. Specifically, > it checks the word and prefixed instructions and some of the Matrix > Multiply Assist (MMA) instructions. > > 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. > --- > .../gdb.reverse/ppc_record_test_isa_2_06.c | 46 ++ > .../gdb.reverse/ppc_record_test_isa_2_06.exp | 122 +++++ > .../gdb.reverse/ppc_record_test_isa_3_1.c | 102 ++++ > .../gdb.reverse/ppc_record_test_isa_3_1.exp | 494 ++++++++++++++++++ > 4 files changed, 764 insertions(+) > create mode 100644 gdb/testsuite/gdb.reverse/ppc_record_test_isa_2_06.c > create mode 100644 gdb/testsuite/gdb.reverse/ppc_record_test_isa_2_06.exp > create mode 100644 gdb/testsuite/gdb.reverse/ppc_record_test_isa_3_1.c > create mode 100644 gdb/testsuite/gdb.reverse/ppc_record_test_isa_3_1.exp > > diff --git a/gdb/testsuite/gdb.reverse/ppc_record_test_isa_2_06.c b/gdb/testsuite/gdb.reverse/ppc_record_test_isa_2_06.c > new file mode 100644 > index 00000000000..e9c3eb1c659 > --- /dev/null > +++ b/gdb/testsuite/gdb.reverse/ppc_record_test_isa_2_06.c > @@ -0,0 +1,46 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2012-2022 Free Software Foundation, Inc. > + > + 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 . */ > + > +#include Is this really needed? The reason I ask is that we try to avoid having tests depend on I/O because that doesn't work on all targets. > +#include > +#include // isspace > +#include > +#include > +#include // getopt > +#include // vector > + The test doesn't seem to use isspace nor vector. Can you trim this to the minimum necessary set, and remove the // comments, please? > +/* globals used for vector tests */ Upperspace, period at end, and double-space: /* Globals used for vector tests. */ > +static vector unsigned long vec_xb; > +static unsigned long ra, rb, rs; > + > +int main () Break before "main". > +{ > + ra = 0xABCDEF012; > + rb = 0; > + rs = 0x012345678; > + > + /* 9.0, 16.0, 25.0, 36.0 */ > + vec_xb = (vector unsigned long){0x4110000041800000, 0x41c8000042100000}; > + > + /* Test 1 ISA 2.06 instructions. Load source into vs1, result of sqrt > + put into vs0. */ > + ra = (unsigned long) & vec_xb; /* stop 1 */ > + __asm__ __volatile__ ("lxvd2x 1, %0, %1" :: "r" (ra ), "r" (rb)); > + __asm__ __volatile__ ("xvsqrtsp 0, 1"); > + ra = 0; /* stop 2 */ > +} > + > diff --git a/gdb/testsuite/gdb.reverse/ppc_record_test_isa_2_06.exp b/gdb/testsuite/gdb.reverse/ppc_record_test_isa_2_06.exp > new file mode 100644 > index 00000000000..adb5a08278e > --- /dev/null > +++ b/gdb/testsuite/gdb.reverse/ppc_record_test_isa_2_06.exp > @@ -0,0 +1,122 @@ > +# Copyright 2008-2022 Free Software Foundation, Inc. > + > +# 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 . > +# This file is part of the GDB testsuite. It tests reverse stepping. > +# Lots of code borrowed from "step-reverse.exp". Is the: "It tests reverse stepping. Lots of code borrowed from "step-reverse.exp". part true, or relevant? You have a description of the test just below, which leads me to think that this might be stale. > +# > +# Test instruction record for Powerpc, ISA 2.06. > +# > + > +# The basic flow of the record tests are: > +# 1) Stop before executing the instructions of interest. Record Double space after period. > +# the initial value of the registers that the instruction will > +# change, i.e. the destination register. > +# 2) Execute the instructions. Record the new value of the > +# registers that changed. > +# 3) Reverse the direction of the execution and execute back to > +# just before the instructions of interest. Record the final > +# value of the registers of interest. > +# 4) Check that the initial and new values of the registers are > +# different, i.e. the instruction changed the registers as expected. > +# 5) Check that the initial and final values of the registers are > +# the same, i.e. gdb record restored the registers to their > +# original values. > + > +standard_testfile > + > +set gen_src record_test_isa_2_06.c > +set executable record_test_isa_2_06 > +set options [list debug] > + > +if {![istarget "powerpc*"]} then { > + verbose "Skipping Powerpc ISA 2.06 instruction record_test_2_06." > + return > +} > + Since the .c file included altivec.c, I wonder whether the .exp file should use skip_altivec_tests. > +if {[build_executable "failed to prepare" $executable $srcfile $options] == -1} then { > + return -1 > +} > + > +clean_restart $executable > + > +if ![runto_main] then { > + untested "could not run to main" > + continue > +} > + > +gdb_test_no_output "record" > + > +###### Test 1: Test an ISA 2.06 load (lxvd2x) and square root instruction > +###### (xvsqrtsp). The load instruction will load vs1. The sqrt instruction > +###### will put its result into vs0. Theres's no "Test 2" in the file, so this "Test 1" looks odd. IOW, why the need to think of these tests as "1" ? BTW, if there's some natural grouping between tests, it's most often better to use with_test_prefix than to use comments to separate sections. > + > +set stop1 [gdb_get_line_number "stop 1"] > +set stop2 [gdb_get_line_number "stop 2"] Spurious double space. > + > +gdb_test "break $stop1" ".*Breakpoint .*" "about to execute test 1" > +gdb_test "continue" ".*Breakpoint .*" "At stop 1" Use lowercase for test messages. > + > +# Record the initial values in vs0, vs1. > +set vs0_initial [capture_command_output "info register vs0" ""] > +set vs1_initial [capture_command_output "info register vs1" ""] > + > +gdb_test "break $stop2" ".*Breakpoint .*" "executed lxvd2x, xvsqrtsp" > +gdb_test "continue" ".*Breakpoint .*" "At stop 2" > + > +# Record the new values of vs0 and vs1. > +set vs0_new [capture_command_output "info register vs0" ""] > +set vs1_new [capture_command_output "info register vs1" ""] > + > +# Reverse the execution direction. > +gdb_test_no_output "set exec-direction reverse" > +gdb_test "break $stop1" ".*Breakpoint .*" "un executed lxvd2x, xvsqrtsp" > + > +# Execute in reverse to before the lxvd2x instruction. > +gdb_test "continue" ".*Breakpoint.*" "At stop 1 in reverse" > + > +# Record the final values of vs0, vs1. > +set vs0_final [capture_command_output "info register vs0" ""] > +set vs1_final [capture_command_output "info register vs1" ""] > + > +# Check initial and new of vs0 are different. > +set test_vs0_init_new "check vs0 initial versus vs0 new" > +if {[string compare $vs0_initial $vs0_new ] == 0} { > + fail $test_vs0_init_new > +} else { > + pass $test_vs0_init_new > +} > + > +# Check initial and new of vs1 are different. > +set test_vs1_init_new "check vs0 initial versus vs1 new" > +if {[string compare $vs1_initial $vs1_new ] == 0} { > + fail $test_vs1_init_new > +} else { > + pass $test_vs1_init_new > +} > + > +# Check initial and final are the same. > +set test_vs0_init_final "check vs0 initial versus vs0 final" > +if {[string compare $vs0_initial $vs0_final ] == 0} { > + pass $test_vs0_init_final > +} else { > + fail $test_vs0_init_final > +} > + > +# Check initial and final are the same. > +set test_vs1_init_final "check vs1 initial versus vs1 final" > +if {[string compare $vs1_initial $vs1_final ] == 0} { spurious double space in all these string compares. You could also use gdb_assert instead of explicit pass/fail. > + pass $test_vs1_init_final > +} else { > + fail $test_vs1_init_final > +} > diff --git a/gdb/testsuite/gdb.reverse/ppc_record_test_isa_3_1.c b/gdb/testsuite/gdb.reverse/ppc_record_test_isa_3_1.c > new file mode 100644 > index 00000000000..e02c3daaed3 > --- /dev/null > +++ b/gdb/testsuite/gdb.reverse/ppc_record_test_isa_3_1.c > @@ -0,0 +1,102 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2012-2022 Free Software Foundation, Inc. > + > + 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 . */ > + > +#include > +#include > +#include // isspace > +#include > +#include > +#include // getopt > +#include // vector > + Same comments as the other file. > +/* globals used for vector tests */ > +static vector unsigned long vec_xa, vec_xb, vec_xt; > +static unsigned long ra, rb, rs; > + > +int main () > +{ > + ra = 0xABCDEF012; > + rb = 0; > + rs = 0x012345678; > + > + /* 9.0, 16.0, 25.0, 36.0 */ > + vec_xb = (vector unsigned long){0x4110000041800000, 0x41c8000042100000}; > + > + vec_xt = (vector unsigned long){0xFF00FF00FF00FF00, 0xAA00AA00AA00AA00}; > + > + /* Test 1, ISA 3.1 word instructions. Load source into r1, result of brh > + put in r0. */ > + ra = 0xABCDEF012; /* stop 1 */ > + __asm__ __volatile__ ("pld 1, %0" :: "r" (ra )); > + __asm__ __volatile__ ("brh 0, 1" ); > + ra = 0; /* stop 2 */ > + > + /* Test 2, ISA 3.1 MMA instructions with results in various ACC entries > + xxsetaccz - ACC[3] > + xvi4ger8 - ACC[4] > + xvf16ger2pn - ACC[5] > + pmxvi8ger4 - ACC[6] > + pmxvf32gerpp - ACC[7] and fpscr */ > + /* Need to initialize the vs registers to a non zero value. */ > + ra = (unsigned long) & vec_xb; > + __asm__ __volatile__ ("lxvd2x 12, %0, %1" :: "r" (ra ), "r" (rb)); > + __asm__ __volatile__ ("lxvd2x 13, %0, %1" :: "r" (ra ), "r" (rb)); > + __asm__ __volatile__ ("lxvd2x 14, %0, %1" :: "r" (ra ), "r" (rb)); > + __asm__ __volatile__ ("lxvd2x 15, %0, %1" :: "r" (ra ), "r" (rb)); > + vec_xa = (vector unsigned long){0x333134343987601, 0x9994bbbc9983307}; > + vec_xb = (vector unsigned long){0x411234041898760, 0x41c833042103400}; > + __asm__ __volatile__ ("lxvd2x 16, %0, %1" :: "r" (ra ), "r" (rb)); > + vec_xb = (vector unsigned long){0x123456789987650, 0x235676546989807}; > + __asm__ __volatile__ ("lxvd2x 17, %0, %1" :: "r" (ra ), "r" (rb)); > + vec_xb = (vector unsigned long){0x878363439823470, 0x413434c99839870}; > + __asm__ __volatile__ ("lxvd2x 18, %0, %1" :: "r" (ra ), "r" (rb)); > + vec_xb = (vector unsigned long){0x043765434398760, 0x419876555558850}; > + __asm__ __volatile__ ("lxvd2x 19, %0, %1" :: "r" (ra ), "r" (rb)); > + vec_xb = (vector unsigned long){0x33313434398760, 0x9994bbbc99899330}; > + __asm__ __volatile__ ("lxvd2x 20, %0, %1" :: "r" (ra ), "r" (rb)); > + __asm__ __volatile__ ("lxvd2x 21, %0, %1" :: "r" (ra ), "r" (rb)); > + __asm__ __volatile__ ("lxvd2x 22, %0, %1" :: "r" (ra ), "r" (rb)); > + __asm__ __volatile__ ("lxvd2x 23, %0, %1" :: "r" (ra ), "r" (rb)); > + __asm__ __volatile__ ("lxvd2x 24, %0, %1" :: "r" (ra ), "r" (rb)); > + __asm__ __volatile__ ("lxvd2x 25, %0, %1" :: "r" (ra ), "r" (rb)); > + __asm__ __volatile__ ("lxvd2x 26, %0, %1" :: "r" (ra ), "r" (rb)); > + __asm__ __volatile__ ("lxvd2x 27, %0, %1" :: "r" (ra ), "r" (rb)); > + vec_xa = (vector unsigned long){0x33313434398760, 0x9994bbbc998330}; > + vec_xb = (vector unsigned long){0x4110000041800000, 0x41c8000042100000}; > + __asm__ __volatile__ ("lxvd2x 28, %0, %1" :: "r" (ra ), "r" (rb)); > + vec_xb = (vector unsigned long){0x4567000046800000, 0x4458000048700000}; > + __asm__ __volatile__ ("lxvd2x 29, %0, %1" :: "r" (ra ), "r" (rb)); > + vec_xb = (vector unsigned long){0x41dd000041e00000, 0x41c8000046544400}; > + __asm__ __volatile__ ("lxvd2x 30, %0, %1" :: "r" (ra ), "r" (rb)); > + > + /* SNAN */ > + vec_xb = (vector unsigned long){0x7F8F00007F8F0000, 0x7F8F00007F8F0000}; > + > + __asm__ __volatile__ ("lxvd2x 31, %0, %1" :: "r" (ra ), "r" (rb)); > + > + ra = 0xAB; /* stop 3 */ > + __asm__ __volatile__ ("xxsetaccz 3"); > + __asm__ __volatile__ ("xvi4ger8 4, %x0, %x1" :: "wa" (vec_xa), \ > + "wa" (vec_xb) ); > + __asm__ __volatile__ ("xvf16ger2pn 5, %x0, %x1" :: "wa" (vec_xa),\ > + "wa" (vec_xb) ); > + __asm__ __volatile__ ("pmxvi8ger4spp 6, %x0, %x1, 11, 13, 5" > + :: "wa" (vec_xa), "wa" (vec_xb) ); > + __asm__ __volatile__ ("pmxvf32gerpp 7, %x0, %x1, 11, 13" > + :: "wa" (vec_xa), "wa" (vec_xb) ); > + ra = 0; /* stop 4 */ > +} > diff --git a/gdb/testsuite/gdb.reverse/ppc_record_test_isa_3_1.exp b/gdb/testsuite/gdb.reverse/ppc_record_test_isa_3_1.exp > new file mode 100644 > index 00000000000..00187de547a > --- /dev/null > +++ b/gdb/testsuite/gdb.reverse/ppc_record_test_isa_3_1.exp > @@ -0,0 +1,494 @@ > +# Copyright 2008-2022 Free Software Foundation, Inc. > + > +# 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 . > +# This file is part of the GDB testsuite. It tests reverse stepping. > +# Lots of code borrowed from "step-reverse.exp". Ditto. > +# > +# Test instruction record for Powerpc, ISA 3.1. > +# > + > +# The basic flow of the record tests are: > +# 1) Stop before executing the instructions of interest. Record > +# the initial value of the registers that the instruction will > +# change, i.e. the destination register. > +# 2) Execute the instructions. Record the new value of the > +# registers that changed. > +# 3) Reverse the direction of the execution and execute back to > +# just before the instructions of interest. Record the final > +# value of the registers of interest. > +# 4) Check that the initial and new values of the registers are > +# different, i.e. the instruction changed the registers as expected. > +# 5) Check that the initial and final values of the registers are > +# the same, i.e. gdb record restored the registers to their > +# original values. > + > + > +standard_testfile > + > +set gen_src record_test_isa_3_1.c > +set executable record_test_isa_3_1 > + > +if {![istarget "powerpc*"] || [skip_power_isa_3_1_tests] } then { > + verbose "Skipping Powerpc ISA 3.1 instruction record_test." > + return > +} > + > +set options [list additional_flags=-mcpu=power10 debug] Spurious double space. > +if {[build_executable "failed to prepare" $executable $srcfile $options] == -1} then { > + return -1 > +} > + > +clean_restart $executable > + > +if ![runto_main] then { > + untested "could not run to main" > + continue > +} > + > +gdb_test_no_output "record" > + Comments to the other file apply to this one as well.