Re: [PATCH] Fix i386 memmove issue [BZ #22644]

["H.J. Lu" <hjl.tools@gmail.com>]

On Mon, Mar 19, 2018 at 1:32 PM, Andrew Senkevich
<andrew.n.senkevich@gmail.com> wrote:
> 2018-03-19 20:38 GMT+01:00 H.J. Lu <hjl.tools@gmail.com>:
>> On Mon, Mar 19, 2018 at 12:29 PM, Andrew Senkevich
>> <andrew.n.senkevich@gmail.com> wrote:
>>> 2018-03-19 18:57 GMT+01:00 H.J. Lu <hjl.tools@gmail.com>:
>>>> On Mon, Mar 19, 2018 at 10:52 AM, Andrew Senkevich
>>>> <andrew.n.senkevich@gmail.com> wrote:
>>>>> 2018-03-19 16:33 GMT+01:00 Florian Weimer <fweimer@redhat.com>:
>>>>>> On 03/19/2018 03:25 PM, Szabolcs Nagy wrote:
>>>>>>>
>>>>>>> i thought not using MAP_FIXED is the 'non-overriding MAP_FIXED variant'
>>>>>
>>>>> Hmm, I tried so and had got much bigger address. But now I see it
>>>>> works (with size just 0x20000000 and 0x70000000 as a hint) in both 64
>>>>> and 32 bit cases, so proposing the following:
>>>>>
>>>>> diff --git a/string/test-memmove.c b/string/test-memmove.c
>>>>> index edc7a4c..fa4037d 100644
>>>>> --- a/string/test-memmove.c
>>>>> +++ b/string/test-memmove.c
>>>>> @@ -24,6 +24,7 @@
>>>>>  # define TEST_NAME "memmove"
>>>>>  #endif
>>>>>  #include "test-string.h"
>>>>> +#include <support/test-driver.h>
>>>>>
>>>>>  char *simple_memmove (char *, const char *, size_t);
>>>>>
>>>>> @@ -245,6 +246,59 @@ do_random_tests (void)
>>>>>      }
>>>>>  }
>>>>>
>>>>> +static void
>>>>> +do_test2 (void)
>>>>> +{
>>>>> +  size_t size = 0x20000000;
>>>>> +  uint32_t * large_buf;
>>>>> +
>>>>> +  large_buf = mmap ((void*)0x70000000, size, PROT_READ | PROT_WRITE,
>>>>> +     MAP_PRIVATE | MAP_ANON, -1, 0);
>>>>> +
>>>>> +  if (large_buf == MAP_FAILED)
>>>>> +    error (EXIT_UNSUPPORTED, errno, "Large mmap failed");
>>>>> +
>>>>> +  if ((uintptr_t)large_buf > 0x80000000 - 128)
>>>>
>>>> Don't you need to test if address is too low such that
>>>> 0x80000000 - (uintptr_t)large_buf > 0x20000000?
>>>
>>> Indeed, thanks. Updated below.
>>>
>>> diff --git a/string/test-memmove.c b/string/test-memmove.c
>>> index edc7a4c..9f1437d 100644
>>> --- a/string/test-memmove.c
>>> +++ b/string/test-memmove.c
>>> @@ -24,6 +24,7 @@
>>>  # define TEST_NAME "memmove"
>>>  #endif
>>>  #include "test-string.h"
>>> +#include <support/test-driver.h>
>>>
>>>  char *simple_memmove (char *, const char *, size_t);
>>>
>>> @@ -245,6 +246,60 @@ do_random_tests (void)
>>>      }
>>>  }
>>>
>>> +static void
>>> +do_test2 (void)
>>> +{
>>> +  size_t size = 0x20000000;
>>> +  uint32_t * large_buf;
>>> +
>>> +  large_buf = mmap ((void*)0x70000000, size, PROT_READ | PROT_WRITE,
>>> +     MAP_PRIVATE | MAP_ANON, -1, 0);
>>> +
>>> +  if (large_buf == MAP_FAILED)
>>> +    error (EXIT_UNSUPPORTED, errno, "Large mmap failed");
>>> +
>>> +  if ((uintptr_t)large_buf > 0x80000000 - 128
>>> +      || 0x80000000 - (uintptr_t)large_buf > 0x20000000)
>>> +    {
>>> +      error (0, 0,"Large mmap allocated improperly");
>>> +      ret = EXIT_UNSUPPORTED;
>>> +      munmap((void *)large_buf, size);
>>> +      return;
>>> +    }
>>> +
>>> +  size_t bytes_move = 0x80000000 - (uintptr_t)large_buf;
>>> +  size_t arr_size = bytes_move / sizeof(uint32_t);
>>> +  size_t i;
>>> +
>>> +  FOR_EACH_IMPL (impl, 0)
>>> +    {
>>> +      for (i = 0; i < arr_size; i++)
>>> +        large_buf[i] = i;
>>> +
>>> +      uint32_t * dst = &large_buf[33];
>>> +
>>> +#ifdef TEST_BCOPY
>>> +      CALL (impl, (char *)large_buf, (char *)dst, bytes_move);
>>> +#else
>>> +      CALL (impl, (char *)dst, (char *)large_buf, bytes_move);
>>> +#endif
>>> +
>>> +      for (i = 0; i < arr_size; i++)
>>> +        {
>>> +          if (dst[i] != i)
>>> +     {
>>> +       error (0, 0,
>>> +      "Wrong result in function %s dst \"%p\" src \"%p\" offset \"%d\"",
>>> +      impl->name, dst, large_buf, i);
>>> +       ret = 1;
>>> +       break;
>>> +     }
>>> + }
>>> +    }
>>> +
>>> +  munmap((void *)large_buf, size);
>>> +}
>>> +
>>>  int
>>>  test_main (void)
>>>  {
>>> @@ -284,6 +339,9 @@ test_main (void)
>>>      }
>>>
>>>    do_random_tests ();
>>> +
>>> +  do_test2 ();
>>> +
>>>    return ret;
>>>  }
>>>
>>>
>>
>> Look good.
>>
>> Please submit the updated patch with the proper commit message.
>
> [BZ #22644] Fix i386 memmove issue.
>
>
>         * sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S: Fixed
>         branch conditions.
>         * string/test-memmove.c (do_test2): New testcase.
>

OK.

Thanks.

-- 
H.J.