From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x32a.google.com (mail-wm1-x32a.google.com [IPv6:2a00:1450:4864:20::32a]) by sourceware.org (Postfix) with ESMTPS id 932543858D1E for ; Sun, 6 Mar 2022 12:42:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 932543858D1E Received: by mail-wm1-x32a.google.com with SMTP id q7-20020a7bce87000000b00382255f4ca9so9262976wmj.2 for ; Sun, 06 Mar 2022 04:42:59 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=IWib0lJVrwlPMR0TscF312Oz5JmuAmJDNgX0CQ9Pz7c=; b=Xb2vniNhU9tz+6m49DPks2emUSjiN4cKF6QK/42EIK+z+DwGicSEcdXuHGbGh+rG81 zNteiLxSEN5nHdyUvRY5nRgh3n4VzTjbLJxi3Hb8U0yoXRAfdYcDF2otF+FP/fNv640e 3B5J4XmJzvgiVddfmMsvQpY9Y7VM/ogpQEow8GIYQ36xq+m7whtTfxAdljmoOaR7EM0S E1bhVToDNCY1KC3CVsoQ6mnjCarnskY+dC7xwQqSiTUg8gXea1Nub9ADGFIhJUSmAXvJ zPtC2Gb73vgmgJhtMx7L+8etDRO7y+nkKdR9vVWjCIXAECRC191njtEKQbCj2xteIYwM 26wg== X-Gm-Message-State: AOAM532Mvp7viijhiJiqkL5IpjG+vD26g7Bcsrrp1JCNmn26dYMfuWE3 jIMWLnmi8ZFetODDg1K35y0aK+r3k7Yp X-Google-Smtp-Source: ABdhPJz0cYwygoaxKBJe1hA+As7I20hW1snieGY8VgrCCSODTKlgnAcqoRRco/VL6Jkr9R8WKnY8yQ== X-Received: by 2002:a05:600c:34c4:b0:389:a4c3:c4b1 with SMTP id d4-20020a05600c34c400b00389a4c3c4b1mr572548wmq.65.1646570578622; Sun, 06 Mar 2022 04:42:58 -0800 (PST) Received: from takamaka.home (lfbn-reu-1-503-119.w92-130.abo.wanadoo.fr. [92.130.90.119]) by smtp.gmail.com with ESMTPSA id g6-20020a5d5406000000b001f049726044sm8879465wrv.79.2022.03.06.04.42.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 06 Mar 2022 04:42:58 -0800 (PST) Received: by takamaka.home (Postfix, from userid 1000) id B04ECA4A87; Sun, 6 Mar 2022 16:42:55 +0400 (+04) Date: Sun, 6 Mar 2022 16:42:55 +0400 From: Joel Brobecker To: Carl Love via Gdb-patches Cc: Ulrich Weigand , Rogerio Alves , Tulio Magno , Joel Brobecker Subject: Re: [PATCH 2/2] Add recording support for the ISA 3.1 Powerpc instructions Message-ID: References: <7ff0655bead2a8245018fbf9624f207cd38a5b7f.camel@us.ibm.com> <18e61a00641bcaeab322db4d416c03ddc30c8950.camel@us.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <18e61a00641bcaeab322db4d416c03ddc30c8950.camel@us.ibm.com> X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, 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: Sun, 06 Mar 2022 12:43:01 -0000 > 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