Re: [PATCH v4 1/5] string: Make tests birdirectional test-memcpy.c

[Noah Goldstein <goldstein.w.n@gmail.com>]

On Tue, Dec 7, 2021 at 3:10 PM Stafford Horne <shorne@gmail.com> wrote:
>
> On Sat, Nov 06, 2021 at 01:33:18PM -0500, Noah Goldstein via Libc-alpha wrote:
> > This commit updates the memcpy tests to test both dst > src and dst <
> > src. This is because there is logic in the code based on the
> > Reviewed-by: H.J. Lu <hjl.tools@gmail.com>
> > ---
> >  string/test-memcpy.c  | 167 +++++++++++++++++++++++++++++++++++-------
> >  string/test-memmove.c |  75 ++++++++++++++++++-
> >  2 files changed, 214 insertions(+), 28 deletions(-)
> >
> > diff --git a/string/test-memcpy.c b/string/test-memcpy.c
> [..]
> >  static void
> > -do_test1 (size_t size)
> > +do_test1 (size_t align1, size_t align2, size_t size)
> >  {
> >    void *large_buf;
> > -  large_buf = mmap (NULL, size * 2 + page_size, PROT_READ | PROT_WRITE,
> > -                 MAP_PRIVATE | MAP_ANON, -1, 0);
> > +  size_t mmap_size, region_size;
> > +
> > +  align1 &= (page_size - 1);
> > +  if (align1 == 0)
> > +    align1 = page_size;
> > +
> > +  align2 &= (page_size - 1);
> > +  if (align2 == 0)
> > +    align2 = page_size;
> > +
> > +  region_size = (size + page_size - 1) & (~(page_size - 1));
> > +
> > +  mmap_size = region_size * 2 + 3 * page_size;
> > +  large_buf = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE,
> > +                   MAP_PRIVATE | MAP_ANON, -1, 0);
> >    if (large_buf == MAP_FAILED)
> >      {
> > -      puts ("Failed to allocat large_buf, skipping do_test1");
> > +      puts ("Failed to allocate large_buf, skipping do_test1");
> >        return;
> >      }
> > -
> > -  if (mprotect (large_buf + size, page_size, PROT_NONE))
> > +  if (mprotect (large_buf + region_size + page_size, page_size, PROT_NONE))
> >      error (EXIT_FAILURE, errno, "mprotect failed");
> >
> > -  size_t arrary_size = size / sizeof (uint32_t);
> > -  uint32_t *dest = large_buf;
> > -  uint32_t *src = large_buf + size + page_size;
> > +  size_t array_size = size / sizeof (uint32_t);
> > +  uint32_t *dest = large_buf + align1;
> > +  uint32_t *src = large_buf + region_size + 2 * page_size + align2;
>
> Hello, this causes Bus errors on the new OpenRISC port I am working on.

Bugzilla for this issue is here:
https://sourceware.org/bugzilla/show_bug.cgi?id=28572

There is a patch attached to the bugzilla and a patch proposed
by Adhemerval Zanella that both fix the issue.

>
> >    size_t i;
> >    size_t repeats;
> >    for(repeats = 0; repeats < 2; repeats++)
> >      {
> > -      for (i = 0; i < arrary_size; i++)
> > +      for (i = 0; i < array_size; i++)
> >          src[i] = (uint32_t) i;
>
> The bus errors happen here caused when align2 is 1 or 2.  OpenRISC (and maybe
> other architectures?) do not support unaligned copies of words.
>
> I fixed this by limiting the align1/align2 to 4 but I am not sure if that is
> what you are trying to copy here.
>
> Maybe we need to change how we setup the src array.
>
> > -
> >        FOR_EACH_IMPL (impl, 0)
> >          {
> > -            printf ("\t\tRunning: %s\n", impl->name);
> >            memset (dest, -1, size);
> >            CALL (impl, (char *) dest, (char *) src, size);
> > -          for (i = 0; i < arrary_size; i++)
> > +          for (i = 0; i < array_size; i++)
> >          if (dest[i] != src[i])
> >            {
> >              error (0, 0,
> >                 "Wrong result in function %s dst \"%p\" src \"%p\" offset \"%zd\"",
> >                 impl->name, dest, src, i);
> >              ret = 1;
> > -            munmap ((void *) large_buf, size * 2 + page_size);
> > +            munmap ((void *) large_buf, mmap_size);
> >              return;
> >            }
> >          }
> > -      dest = src;
> > -      src = large_buf;
> > +      dest = large_buf + region_size + 2 * page_size + align1;
> > +      src = large_buf + align2;
> > +    }
> > +  munmap ((void *) large_buf, mmap_size);
> > +}
> > +
> [..]
> > @@ -306,8 +341,88 @@ test_main (void)
> >
> >    do_random_tests ();
> >
> > -  do_test1 (0x100000);
> > -  do_test1 (0x2000000);
> > +  do_test1 (0, 0, 0x100000);
> > +  do_test1 (0, 0, 0x2000000);
> > +
> > +  for (i = 4096; i < 32768; i += 4096)
> > +    {
> > +      for (j = 1; j <= 1024; j <<= 1)
> > +        {
> > +          do_test1 (0, j, i);
> > +          do_test1 (4095, j, i);
> > +          do_test1 (4096 - j, 0, i);
> > +
> > +          do_test1 (0, j - 1, i);
> > +          do_test1 (4095, j - 1, i);
> > +          do_test1 (4096 - j - 1, 0, i);
> > +
> > +          do_test1 (0, j + 1, i);
> > +          do_test1 (4095, j + 1, i);
> > +          do_test1 (4096 - j, 1, i);
>
> These +1, -1's cause non-aligned word access.
>
> > +        }
> > +    }
> > +
>
> -Stafford