From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 2lTvAnG+6mJCBiAAWB0awg (envelope-from ) for ; Wed, 03 Aug 2022 14:29:05 -0400 Received: by simark.ca (Postfix, from userid 112) id EE0161EA05; Wed, 3 Aug 2022 14:29:04 -0400 (EDT) Authentication-Results: simark.ca; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.a=rsa-sha256 header.s=default header.b=NztXRddy; dkim-atps=neutral X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=BAYES_00,DATE_IN_PAST_12_24, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_DYNAMIC, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 Received: from sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id E3E411E9EB for ; Wed, 3 Aug 2022 14:29:03 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 47973385743A for ; Wed, 3 Aug 2022 18:29:02 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 47973385743A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1659551342; bh=Gh2vd07FtCfzKk4bmeU993CQqg2oi89jqu6IkdAL0hU=; h=References:To:Subject:Date:In-reply-to:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=NztXRddykyGpDjbR805kduFLZQ0iDtlraU3cPycC7mAoXtwPIIn0FWS4I1TX9zuYG p0KgQCfWSd38jFYDrrygKfBM3fYMjuHl+o5lgufzA6gQv1eCE72Cva3rWjIc8PqZRf J/GazQG6Hytomp8eWczO3QxVgih5B+MGydy9H15E= Received: from mail-vk1-xa32.google.com (mail-vk1-xa32.google.com [IPv6:2607:f8b0:4864:20::a32]) by sourceware.org (Postfix) with ESMTPS id 401F03858C39 for ; Wed, 3 Aug 2022 18:28:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 401F03858C39 Received: by mail-vk1-xa32.google.com with SMTP id q191so6831111vkb.6 for ; Wed, 03 Aug 2022 11:28:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:in-reply-to:date :subject:cc:to:from:user-agent:references:x-gm-message-state:from:to :cc; bh=Gh2vd07FtCfzKk4bmeU993CQqg2oi89jqu6IkdAL0hU=; b=lnbRHXSFNbTwz1CPwtxlGQQcfnBsCnnvN9u7q8SaimSWqIAXFPI3q3WJE1JcfjRi5g T42FAxCmX/0autEB4+FEHZ25r/2A6Cs08HgMSwLp3a9wzrCbHPnEMrjdxO6sQOpOXtjQ s1RUGi6mIykXQrb07CEo9J2+QHaoZ2EXSej7X0KjtTqwUs83qU5zfhMikCZqY4JR79WQ xvHEeaoK++dobVnu2WtrNp7BFeD610hKqGbM5tnmHWz0PBD6GwA/J4xdAC6LlY+Pzrpt 23Ia/fWgtIbVnEJ9v6XxhFAeGi0tfJ5jd/7akTMEMZ6wonMjJ1mcL5BjfnWtj7HgKPVn UCOQ== X-Gm-Message-State: ACgBeo2nwlBGprJuOvKx+4e/HRfTinEwdUJbOge4mnPMRE9eg1cYupa8 gH8Kz2hJc9xDCNBGvqasnzPf02QELGjfbw== X-Google-Smtp-Source: AA6agR6AcIkXzTamaBrmd9CDMSNzWy6I1q93qQRgEs8omwvpcZlB8gZHfnsnZ8hoRPdg3SLZEKHuWg== X-Received: by 2002:a1f:6142:0:b0:377:4b38:1156 with SMTP id v63-20020a1f6142000000b003774b381156mr7271637vkb.28.1659551319438; Wed, 03 Aug 2022 11:28:39 -0700 (PDT) Received: from localhost ([2804:14d:7e39:8470:b92a:2e2:2963:8b51]) by smtp.gmail.com with ESMTPSA id i22-20020a05610221b600b00384b60a5160sm5933930vsb.12.2022.08.03.11.28.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 03 Aug 2022 11:28:38 -0700 (PDT) References: <20220728012306.157639-1-thiago.bauermann@linaro.org> <20220728012306.157639-3-thiago.bauermann@linaro.org> <6d5a2ff1-6273-3b63-e86b-08e7d334233f@arm.com> User-agent: mu4e 1.6.11; emacs 28.1 To: Luis Machado Subject: Re: [PATCH 2/2] gdb/testsuite: Add test for AArch64 Scalable Vector Extension Date: Tue, 02 Aug 2022 22:59:58 +0000 In-reply-to: <6d5a2ff1-6273-3b63-e86b-08e7d334233f@arm.com> Message-ID: <87zgglnpv1.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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: , From: Thiago Jung Bauermann via Gdb-patches Reply-To: Thiago Jung Bauermann Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" Hello Luis, Luis Machado writes: > Thanks for spotting this. A few comments below, mostly just small adjustm= ents/suggestions. Thanks for your review! > On 7/28/22 02:23, Thiago Jung Bauermann via Gdb-patches wrote: >> It exercises a bug that GDB previously had where it would lose track of >> some registers when the inferior changed its vector length. >> --- >> gdb/testsuite/gdb.arch/aarch64-sve.c | 61 +++++++++++++++++++ >> gdb/testsuite/gdb.arch/aarch64-sve.exp | 81 ++++++++++++++++++++++++++ >> gdb/testsuite/lib/gdb.exp | 4 ++ >> gdb/testsuite/lib/mi-support.exp | 4 -- >> 4 files changed, 146 insertions(+), 4 deletions(-) >> create mode 100644 gdb/testsuite/gdb.arch/aarch64-sve.c >> create mode 100644 gdb/testsuite/gdb.arch/aarch64-sve.exp >> diff --git a/gdb/testsuite/gdb.arch/aarch64-sve.c b/gdb/testsuite/gdb.ar= ch/aarch64-sve.c >> new file mode 100644 >> index 000000000000..f56a5799a522 >> --- /dev/null >> +++ b/gdb/testsuite/gdb.arch/aarch64-sve.c > > Given this exercises the target description re-creation for a particular = thread, should we > name the test > with something that hints at that purpose? > > Unless you have future plans for this test to exercise changing the vecto= r length > mid-execution. The test changes the vector length mid-execution but doesn't check the new vector length. Doing that is a good idea, will do in v2. I haven't implemented anything else yet, but I was considering extending it to set and check the SVE registers. I can submit it as a separate patch later, if it's considered a useful addition. >> +/* This test was based on QEMU's sve-ioctls.c test file, which at the t= ime had >> + the following copyright statement: >> + >> + Copyright (c) 2019 Linaro Ltd >> + >> + SPDX-License-Identifier: GPL-2.0-or-later */ > > I don't see other obvious occurrences of these copyright statements. It m= ight be OK to > just mention > its origin, as it is also GPL. Indeed. In v2 I'll just mention =E2=80=9CThis test was based on QEMU's sve-ioctls.c test file.=E2=80=9D >> +#include >> +#include >> +#include >> + >> +static int do_sve_ioctl_test(void) > > The usual formatting fun, space before ( and other bits across the file. I didn't worry about formatting of the test file because I have a vague memory of someone saying a long time ago that it was good for the testsuite to have a variety of formatting styles so that GDB can be exposed to them during testing. I see now that the Internals wiki says that =E2=80=9Cunless a test requires a particular style (which is rare) you can't go wrong following the GDB coding standards=E2=80=9D=C2=B9. I'll fix the formatting. >> --- /dev/null >> +++ b/gdb/testsuite/gdb.arch/aarch64-sve.exp >> @@ -0,0 +1,81 @@ >> +# Copyright 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 . >> + >> +# Test a binary that uses SVE and exercise SVE-related scenarios. > > Same comment about mentioning the purpose of the test more precisely. Ok, I'll be more specific. >> +if {![is_aarch64_target]} { >> + verbose "Skipping ${gdb_test_file_name}." >> + return >> +} >> + > > In additional to the above guard, we should also guard against aarch64 ta= rgets that don't > support SVE, with skip_aarch64_sve_tests. > > Otherwise we get the following: > > FAIL: gdb.arch/aarch64-sve.exp: runto: run to aarch64-sve.c:44 Ah, I thought the C code checking AT_HWCAP and returning was enough, but I forgot to verify it. I'll use skip_aarch64_sve_tests. >> +standard_testfile >> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] }= { >> + return >> +} >> + >> +set linespec ${srcfile}:[gdb_get_line_number "break here"] >> + >> +if ![runto ${linespec}] { >> + return >> +} >> + >> +# Count number of lines in "info registers" output. >> +proc count_info_registers {} { >> + global gdb_prompt >> + set ret 0 >> + >> + gdb_test_multiple "info registers" "" { > > The fp/vector registers are usually not displayed by "info registers". Do= es it make sense > to use "info all-registers" instead? Good idea, it does allow the test to catch potential bugs affecting registers not displayed by =E2=80=9Cinfo registers=E2=80=9D. The downside i= s that it makes the test slower: using =E2=80=9Cinfo registers=E2=80=9D, the test tak= es about 2.8s on my development machine. Changing it to use =E2=80=9Cinfo all-registers= =E2=80=9D increases the test time to 14.5s. Is this a problem? > Its output is quite lengthy. Alternatively, there is also "maint print xm= l-tdesc". It's an interesting idea and it takes the test time back down to 3.5s. Unfortunately, then the test doesn't catch the bug (tpidr is present in =E2=80=9Cmaint print xml-tdesc=E2=80=9D but not shown in =E2=80=9Cinfo regi= sters=E2=80=9D). >> + -re ".*$gdb_prompt $" { >> + set ret [count_newlines $expect_out(buffer)] >> + } >> + } >> + >> + return ${ret} >> +} >> + >> +# The test executable halves the vector length in a loop, so loop along >> +# to check it. >> +set i 0 >> +while 1 { > > Instead of having an infinite loop, should we iterate over this test only= as many times as > needed? > > For example, if we have 8 different VL values, we'd iterate over this 8 t= imes. > > My worry is that we might run into a failure and keep looping indefinitel= y. I addressed this problem by adding an =E2=80=9Cunexpected output=E2=80=9D c= ase to the gdb_test_multiple call that continues the loop, and verified that it works by changing the C code to segfault within the loop. The test noticed the problem and exited. But I understand that it looks fragile. I'll change the test to calculate how many times the C code is supposed to loop and iterate just that many times. >> + incr i >> + >> + set lines_before [count_info_registers] >> + >> + gdb_test "next" ".*if .res < 0. ." "step over prctl iteration ${i}" >> + >> + set lines_after [count_info_registers] >> + >> + # There was a bug where GDB would lose track of some registers when= the >> + # vector length changed. Make sure they're still all there. >> + if {${lines_before} =3D=3D ${lines_after}} { >> + pass "same number of registers iteration ${i}" >> + } else { >> + fail "same number of registers iteration ${i}" >> + } >> + >> + gdb_test_multiple "continue" "" { >> + -re ".*Breakpoint $decimal, do_sve_ioctl_test .*$gdb_prompt $" { >> + # Next iteration. >> + } >> + -re "Inferior 1 .* exited normally.*$gdb_prompt $" { >> + # We're done. >> + break >> + } >> + -re "$gdb_prompt $" { >> + fail "unexpected output" >> + break; >> + } >> + } >> +} >> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp >> index a8f25b5f0dd5..3a8b880c074d 100644 >> --- a/gdb/testsuite/lib/gdb.exp >> +++ b/gdb/testsuite/lib/gdb.exp >> @@ -7885,6 +7885,10 @@ proc multi_line_input { args } { >> return [join $args "\n"] >> } >> +proc count_newlines { string } { >> + return [regexp -all "\n" $string] >> +} >> + > > Given you're moving this, how about adding some light documentation to it? Ok, will do. --=20 Thiago =C2=B9 https://sourceware.org/gdb/wiki/Internals%20GDB-Testsuite-Coding-Sta= ndards