From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 771F1384D1BE for ; Thu, 6 Oct 2022 10:06:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 771F1384D1BE Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 5E2D5169C; Thu, 6 Oct 2022 03:06:07 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.62]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 6B3073F792; Thu, 6 Oct 2022 03:06:00 -0700 (PDT) From: Richard Sandiford To: Philipp Tomsich Mail-Followup-To: Philipp Tomsich ,gcc-patches@gcc.gnu.org, Tamar Christina , Christoph Muellner , richard.sandiford@arm.com Cc: gcc-patches@gcc.gnu.org, Tamar Christina , Christoph Muellner Subject: Re: [PATCH v2] aarch64: fix off-by-one in reading cpuinfo References: <20221006092840.607374-1-philipp.tomsich@vrull.eu> Date: Thu, 06 Oct 2022 11:05:59 +0100 In-Reply-To: <20221006092840.607374-1-philipp.tomsich@vrull.eu> (Philipp Tomsich's message of "Thu, 6 Oct 2022 11:28:39 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-44.8 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Philipp Tomsich writes: > Fixes: 341573406b39 > > Don't subtract one from the result of strnlen() when trying to point > to the first character after the current string. This issue would > cause individual characters (where the 128 byte buffers are stitched > together) to be lost. > > gcc/ChangeLog: > > * config/aarch64/driver-aarch64.cc (readline): Fix off-by-one. > > gcc/testsuite/ChangeLog: > > * gcc.target/aarch64/cpunative/info_18: New test. > * gcc.target/aarch64/cpunative/native_cpu_18.c: New test. > > Signed-off-by: Philipp Tomsich > > --- > > Changes in v2: > - Add a a regression test (as per review comment). > > gcc/config/aarch64/driver-aarch64.cc | 4 ++-- > .../gcc.target/aarch64/cpunative/info_18 | 8 ++++++++ > .../gcc.target/aarch64/cpunative/native_cpu_18.c | 15 +++++++++++++++ > 3 files changed, 25 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/aarch64/cpunative/info_18 > create mode 100644 gcc/testsuite/gcc.target/aarch64/cpunative/native_cpu_18.c > > diff --git a/gcc/config/aarch64/driver-aarch64.cc b/gcc/config/aarch64/driver-aarch64.cc > index 52ff537908e..48250e68034 100644 > --- a/gcc/config/aarch64/driver-aarch64.cc > +++ b/gcc/config/aarch64/driver-aarch64.cc > @@ -203,9 +203,9 @@ readline (FILE *f) > return std::string (); > /* If we're not at the end of the line then override the > \0 added by fgets. */ > - last = strnlen (buf, size) - 1; > + last = strnlen (buf, size); > } > - while (!feof (f) && buf[last] != '\n'); > + while (!feof (f) && (last > 0 && buf[last - 1] != '\n')); Very minor, but: I think the normal GCC style would be to avoid the extra (...). OK with that change, thanks. OK for backports too after a settling period. Richard > > std::string result (buf); > free (buf); > diff --git a/gcc/testsuite/gcc.target/aarch64/cpunative/info_18 b/gcc/testsuite/gcc.target/aarch64/cpunative/info_18 > new file mode 100644 > index 00000000000..25061a4abe8 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/cpunative/info_18 > @@ -0,0 +1,8 @@ > +processor : 0 > +BogoMIPS : 2000.00 > +Features : fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 asimddp sha512 asimdfhm dit uscat ilrcpc flagm ssbs sb dcpodp flagm2 frint i8mm bf16 rng ecv > +CPU implementer : 0xc0 > +CPU architecture: 8 > +CPU variant : 0x0 > +CPU part : 0xac3 > +CPU revision : 0 > diff --git a/gcc/testsuite/gcc.target/aarch64/cpunative/native_cpu_18.c b/gcc/testsuite/gcc.target/aarch64/cpunative/native_cpu_18.c > new file mode 100644 > index 00000000000..b5f0a3005f5 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/cpunative/native_cpu_18.c > @@ -0,0 +1,15 @@ > +/* { dg-do compile { target { { aarch64*-*-linux*} && native } } } */ > +/* { dg-set-compiler-env-var GCC_CPUINFO "$srcdir/gcc.target/aarch64/cpunative/info_18" } */ > +/* { dg-additional-options "-mcpu=native" } */ > + > +int main() > +{ > + return 0; > +} > + > +/* { dg-final { scan-assembler {\.arch armv8.6-a\+crc\+fp16\+aes\+sha3\+rng} } } */ > + > +/* Test one where the boundary of buffer size would overwrite the last > + character read when stitching the fgets-calls together. With the > + test data provided, this would truncate the 'sha512' into 'ha512' > + (dropping the 'sha3' feature). */