Re: [PATCH v2] aarch64: fix off-by-one in reading cpuinfo

[Philipp Tomsich <philipp.tomsich@vrull.eu>]

[-- Attachment #1: Type: text/plain, Size: 3906 bytes --]

On Thu, 6 Oct 2022 at 12:06, Richard Sandiford <richard.sandiford@arm.com>
wrote:

> Philipp Tomsich <philipp.tomsich@vrull.eu> 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 <philipp.tomsich@vrull.eu>
> >
> > ---
> >
> > 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.
>

Applied to master (with that change). Thanks!

I'll backport around the end of this month, if no new issues are caused by
this change.

Philipp.


>
> 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). */
>