* [PATCH] Fix i386 memmove issue [BZ #22644] @ 2018-02-19 10:13 Andrew Senkevich 2018-02-19 10:31 ` Andreas Schwab 0 siblings, 1 reply; 19+ messages in thread From: Andrew Senkevich @ 2018-02-19 10:13 UTC (permalink / raw) To: libc-alpha; +Cc: max Hi, in BZ #22644 was localized bug in i386 memmove-sse2-unaligned. This patch adds fix and testcase. Any comments? * sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S: Fixed branch conditions. * string/test-memmove.c (do_test2): New testcase. diff --git a/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S b/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S index 9c3bbe7..9aa17de 100644 --- a/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S +++ b/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S @@ -72,7 +72,7 @@ ENTRY (MEMCPY) cmp %edx, %eax # ifdef USE_AS_MEMMOVE - jg L(check_forward) + ja L(check_forward) L(mm_len_0_or_more_backward): /* Now do checks for lengths. We do [0..16], [16..32], [32..64], [64..128] @@ -81,7 +81,7 @@ L(mm_len_0_or_more_backward): jbe L(mm_len_0_16_bytes_backward) cmpl $32, %ecx - jg L(mm_len_32_or_more_backward) + ja L(mm_len_32_or_more_backward) /* Copy [0..32] and return. */ movdqu (%eax), %xmm0 @@ -92,7 +92,7 @@ L(mm_len_0_or_more_backward): L(mm_len_32_or_more_backward): cmpl $64, %ecx - jg L(mm_len_64_or_more_backward) + ja L(mm_len_64_or_more_backward) /* Copy [0..64] and return. */ movdqu (%eax), %xmm0 @@ -107,7 +107,7 @@ L(mm_len_32_or_more_backward): L(mm_len_64_or_more_backward): cmpl $128, %ecx - jg L(mm_len_128_or_more_backward) + ja L(mm_len_128_or_more_backward) /* Copy [0..128] and return. */ movdqu (%eax), %xmm0 @@ -132,7 +132,7 @@ L(mm_len_128_or_more_backward): add %ecx, %eax cmp %edx, %eax movl SRC(%esp), %eax - jle L(forward) + jbe L(forward) PUSH (%esi) PUSH (%edi) PUSH (%ebx) @@ -269,7 +269,7 @@ L(check_forward): add %edx, %ecx cmp %eax, %ecx movl LEN(%esp), %ecx - jle L(forward) + jbe L(forward) /* Now do checks for lengths. We do [0..16], [0..32], [0..64], [0..128] separately. */ diff --git a/string/test-memmove.c b/string/test-memmove.c index edc7a4c..8dc152b --- a/string/test-memmove.c +++ b/string/test-memmove.c @@ -245,6 +245,49 @@ do_random_tests (void) } } +#if __SIZEOF_POINTER__ == 4 +static void +do_test2 (void) +{ + uint32_t i; + uint32_t num = 0x20000000; + uint32_t * large_buf = mmap (0, sizeof(uint32_t) * num, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANON, -1, 0); + if (large_buf == MAP_FAILED) + error (EXIT_FAILURE, errno, "large mmap failed"); + + if (!((uint32_t)(large_buf) < (0x80000000 - 128) && (0x80000000 + 128) < (uint32_t)(&large_buf[num]))) + { + error (0, 0,"allocated large memory doesn't cross 0x80000000 boundary"); + ret = 1; + return; + } + + int bytes_move = 0x80000000 - (uint32_t)large_buf; + int arr_size = bytes_move / sizeof(uint32_t); + + FOR_EACH_IMPL (impl, 0) + { + for (i = 0; i < arr_size; i++) + large_buf[i] = i; + + uint32_t * dst = &large_buf[33]; + + CALL (impl, (char *)dst, (char *)large_buf, bytes_move); + + 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; + return; + } + } +} +#endif + int test_main (void) { @@ -284,6 +327,11 @@ test_main (void) } do_random_tests (); + +#if __SIZEOF_POINTER__ == 4 + do_test2 (); +#endif + return ret; } -- WBR, Andrew ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Fix i386 memmove issue [BZ #22644] 2018-02-19 10:13 [PATCH] Fix i386 memmove issue [BZ #22644] Andrew Senkevich @ 2018-02-19 10:31 ` Andreas Schwab 2018-03-14 14:43 ` Andrew Senkevich 0 siblings, 1 reply; 19+ messages in thread From: Andreas Schwab @ 2018-02-19 10:31 UTC (permalink / raw) To: Andrew Senkevich; +Cc: libc-alpha, max On Feb 19 2018, Andrew Senkevich <andrew.n.senkevich@gmail.com> wrote: > diff --git a/string/test-memmove.c b/string/test-memmove.c > index edc7a4c..8dc152b > --- a/string/test-memmove.c > +++ b/string/test-memmove.c > @@ -245,6 +245,49 @@ do_random_tests (void) > } > } > > +#if __SIZEOF_POINTER__ == 4 > +static void > +do_test2 (void) > +{ > + uint32_t i; > + uint32_t num = 0x20000000; > + uint32_t * large_buf = mmap (0, sizeof(uint32_t) * num, PROT_READ | > PROT_WRITE, > + MAP_PRIVATE | MAP_ANON, -1, 0); > + if (large_buf == MAP_FAILED) > + error (EXIT_FAILURE, errno, "large mmap failed"); > + > + if (!((uint32_t)(large_buf) < (0x80000000 - 128) && (0x80000000 + > 128) < (uint32_t)(&large_buf[num]))) > + { > + error (0, 0,"allocated large memory doesn't cross 0x80000000 boundary"); > + ret = 1; > + return; Please properly fold long lines, and remove the redundant parens. Also, there is no guarantee that the address range is unallocated. Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Fix i386 memmove issue [BZ #22644] 2018-02-19 10:31 ` Andreas Schwab @ 2018-03-14 14:43 ` Andrew Senkevich 2018-03-14 14:59 ` H.J. Lu 0 siblings, 1 reply; 19+ messages in thread From: Andrew Senkevich @ 2018-03-14 14:43 UTC (permalink / raw) To: Andreas Schwab; +Cc: libc-alpha, Max Horn 2018-02-19 11:13 GMT+01:00 Andreas Schwab <schwab@suse.de>: > On Feb 19 2018, Andrew Senkevich <andrew.n.senkevich@gmail.com> wrote: > >> diff --git a/string/test-memmove.c b/string/test-memmove.c >> index edc7a4c..8dc152b >> --- a/string/test-memmove.c >> +++ b/string/test-memmove.c >> @@ -245,6 +245,49 @@ do_random_tests (void) >> } >> } >> >> +#if __SIZEOF_POINTER__ == 4 >> +static void >> +do_test2 (void) >> +{ >> + uint32_t i; >> + uint32_t num = 0x20000000; >> + uint32_t * large_buf = mmap (0, sizeof(uint32_t) * num, PROT_READ | >> PROT_WRITE, >> + MAP_PRIVATE | MAP_ANON, -1, 0); >> + if (large_buf == MAP_FAILED) >> + error (EXIT_FAILURE, errno, "large mmap failed"); >> + >> + if (!((uint32_t)(large_buf) < (0x80000000 - 128) && (0x80000000 + >> 128) < (uint32_t)(&large_buf[num]))) >> + { >> + error (0, 0,"allocated large memory doesn't cross 0x80000000 boundary"); >> + ret = 1; >> + return; > > Please properly fold long lines, and remove the redundant parens. Also, > there is no guarantee that the address range is unallocated. Thanks, updated patch below. Any comment or it is Ok for trunk? diff --git a/string/test-memmove.c b/string/test-memmove.c index edc7a4c..a8c4af4 100644 --- a/string/test-memmove.c +++ b/string/test-memmove.c @@ -245,6 +245,57 @@ do_random_tests (void) } } +#if __SIZEOF_POINTER__ == 4 +static void +do_test2 (void) +{ + uint32_t i; + uint32_t num = 0x20000000; + uint32_t * large_buf = mmap (0, sizeof(uint32_t) * num, + PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANON, -1, 0); + if (large_buf == MAP_FAILED) + error (EXIT_FAILURE, errno, "Large mmap failed"); + + if ((uint32_t)large_buf > 0x80000000 - 128 + || (uint32_t)&large_buf[num] < 0x80000000 + 128) + { + error (0, 0,"Large mmap doesn't cross 0x80000000 boundary"); + ret = EXIT_UNSUPPORTED; + munmap((void *)large_buf, sizeof(uint32_t) * num); + return; + } + + int bytes_move = 0x80000000 - (uint32_t)large_buf; + int arr_size = bytes_move / sizeof(uint32_t); + + FOR_EACH_IMPL (impl, 0) + { + for (i = 0; i < arr_size; i++) + large_buf[i] = i; + + uint32_t * dst = &large_buf[33]; + + CALL (impl, (char *)dst, (char *)large_buf, bytes_move); + + 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; + munmap((void *)large_buf, sizeof(uint32_t) * num); + return; + } + } + } + + munmap((void *)large_buf, sizeof(uint32_t) * num); +} +#endif + int test_main (void) { @@ -284,6 +335,11 @@ test_main (void) } do_random_tests (); + +#if __SIZEOF_POINTER__ == 4 + do_test2 (); +#endif + return ret; } diff --git a/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S b/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S index 9c3bbe7..9aa17de 100644 --- a/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S +++ b/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S @@ -72,7 +72,7 @@ ENTRY (MEMCPY) cmp %edx, %eax # ifdef USE_AS_MEMMOVE - jg L(check_forward) + ja L(check_forward) L(mm_len_0_or_more_backward): /* Now do checks for lengths. We do [0..16], [16..32], [32..64], [64..128] @@ -81,7 +81,7 @@ L(mm_len_0_or_more_backward): jbe L(mm_len_0_16_bytes_backward) cmpl $32, %ecx - jg L(mm_len_32_or_more_backward) + ja L(mm_len_32_or_more_backward) /* Copy [0..32] and return. */ movdqu (%eax), %xmm0 @@ -92,7 +92,7 @@ L(mm_len_0_or_more_backward): L(mm_len_32_or_more_backward): cmpl $64, %ecx - jg L(mm_len_64_or_more_backward) + ja L(mm_len_64_or_more_backward) /* Copy [0..64] and return. */ movdqu (%eax), %xmm0 @@ -107,7 +107,7 @@ L(mm_len_32_or_more_backward): L(mm_len_64_or_more_backward): cmpl $128, %ecx - jg L(mm_len_128_or_more_backward) + ja L(mm_len_128_or_more_backward) /* Copy [0..128] and return. */ movdqu (%eax), %xmm0 @@ -132,7 +132,7 @@ L(mm_len_128_or_more_backward): add %ecx, %eax cmp %edx, %eax movl SRC(%esp), %eax - jle L(forward) + jbe L(forward) PUSH (%esi) PUSH (%edi) PUSH (%ebx) @@ -269,7 +269,7 @@ L(check_forward): add %edx, %ecx cmp %eax, %ecx movl LEN(%esp), %ecx - jle L(forward) + jbe L(forward) /* Now do checks for lengths. We do [0..16], [0..32], [0..64], [0..128] separately. */ -- WBR, Andrew ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Fix i386 memmove issue [BZ #22644] 2018-03-14 14:43 ` Andrew Senkevich @ 2018-03-14 14:59 ` H.J. Lu 2018-03-19 12:46 ` Andrew Senkevich 0 siblings, 1 reply; 19+ messages in thread From: H.J. Lu @ 2018-03-14 14:59 UTC (permalink / raw) To: Andrew Senkevich; +Cc: Andreas Schwab, libc-alpha, Max Horn On Wed, Mar 14, 2018 at 7:43 AM, Andrew Senkevich <andrew.n.senkevich@gmail.com> wrote: > 2018-02-19 11:13 GMT+01:00 Andreas Schwab <schwab@suse.de>: >> On Feb 19 2018, Andrew Senkevich <andrew.n.senkevich@gmail.com> wrote: >> >>> diff --git a/string/test-memmove.c b/string/test-memmove.c >>> index edc7a4c..8dc152b >>> --- a/string/test-memmove.c >>> +++ b/string/test-memmove.c >>> @@ -245,6 +245,49 @@ do_random_tests (void) >>> } >>> } >>> >>> +#if __SIZEOF_POINTER__ == 4 >>> +static void >>> +do_test2 (void) >>> +{ >>> + uint32_t i; >>> + uint32_t num = 0x20000000; >>> + uint32_t * large_buf = mmap (0, sizeof(uint32_t) * num, PROT_READ | >>> PROT_WRITE, >>> + MAP_PRIVATE | MAP_ANON, -1, 0); >>> + if (large_buf == MAP_FAILED) >>> + error (EXIT_FAILURE, errno, "large mmap failed"); >>> + >>> + if (!((uint32_t)(large_buf) < (0x80000000 - 128) && (0x80000000 + >>> 128) < (uint32_t)(&large_buf[num]))) >>> + { >>> + error (0, 0,"allocated large memory doesn't cross 0x80000000 boundary"); >>> + ret = 1; >>> + return; >> >> Please properly fold long lines, and remove the redundant parens. Also, >> there is no guarantee that the address range is unallocated. > > Thanks, updated patch below. Any comment or it is Ok for trunk? Please also test crossing 0x80000000 boundary on 64-bit systems. > diff --git a/string/test-memmove.c b/string/test-memmove.c > index edc7a4c..a8c4af4 100644 > --- a/string/test-memmove.c > +++ b/string/test-memmove.c > @@ -245,6 +245,57 @@ do_random_tests (void) > } > } > > +#if __SIZEOF_POINTER__ == 4 > +static void > +do_test2 (void) > +{ > + uint32_t i; > + uint32_t num = 0x20000000; > + uint32_t * large_buf = mmap (0, sizeof(uint32_t) * num, > + PROT_READ | PROT_WRITE, > + MAP_PRIVATE | MAP_ANON, -1, 0); > + if (large_buf == MAP_FAILED) > + error (EXIT_FAILURE, errno, "Large mmap failed"); > + > + if ((uint32_t)large_buf > 0x80000000 - 128 > + || (uint32_t)&large_buf[num] < 0x80000000 + 128) > + { > + error (0, 0,"Large mmap doesn't cross 0x80000000 boundary"); > + ret = EXIT_UNSUPPORTED; > + munmap((void *)large_buf, sizeof(uint32_t) * num); > + return; > + } > + > + int bytes_move = 0x80000000 - (uint32_t)large_buf; > + int arr_size = bytes_move / sizeof(uint32_t); > + > + FOR_EACH_IMPL (impl, 0) > + { > + for (i = 0; i < arr_size; i++) > + large_buf[i] = i; > + > + uint32_t * dst = &large_buf[33]; > + > + CALL (impl, (char *)dst, (char *)large_buf, bytes_move); > + > + 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; > + munmap((void *)large_buf, sizeof(uint32_t) * num); > + return; > + } > + } > + } > + > + munmap((void *)large_buf, sizeof(uint32_t) * num); > +} > +#endif > + > int > test_main (void) > { > @@ -284,6 +335,11 @@ test_main (void) > } > > do_random_tests (); > + > +#if __SIZEOF_POINTER__ == 4 > + do_test2 (); > +#endif > + > return ret; > } > > diff --git a/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S > b/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S > index 9c3bbe7..9aa17de 100644 > --- a/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S > +++ b/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S > @@ -72,7 +72,7 @@ ENTRY (MEMCPY) > cmp %edx, %eax > > # ifdef USE_AS_MEMMOVE > - jg L(check_forward) > + ja L(check_forward) > > L(mm_len_0_or_more_backward): > /* Now do checks for lengths. We do [0..16], [16..32], [32..64], [64..128] > @@ -81,7 +81,7 @@ L(mm_len_0_or_more_backward): > jbe L(mm_len_0_16_bytes_backward) > > cmpl $32, %ecx > - jg L(mm_len_32_or_more_backward) > + ja L(mm_len_32_or_more_backward) > > /* Copy [0..32] and return. */ > movdqu (%eax), %xmm0 > @@ -92,7 +92,7 @@ L(mm_len_0_or_more_backward): > > L(mm_len_32_or_more_backward): > cmpl $64, %ecx > - jg L(mm_len_64_or_more_backward) > + ja L(mm_len_64_or_more_backward) > > /* Copy [0..64] and return. */ > movdqu (%eax), %xmm0 > @@ -107,7 +107,7 @@ L(mm_len_32_or_more_backward): > > L(mm_len_64_or_more_backward): > cmpl $128, %ecx > - jg L(mm_len_128_or_more_backward) > + ja L(mm_len_128_or_more_backward) > > /* Copy [0..128] and return. */ > movdqu (%eax), %xmm0 > @@ -132,7 +132,7 @@ L(mm_len_128_or_more_backward): > add %ecx, %eax > cmp %edx, %eax > movl SRC(%esp), %eax > - jle L(forward) > + jbe L(forward) > PUSH (%esi) > PUSH (%edi) > PUSH (%ebx) > @@ -269,7 +269,7 @@ L(check_forward): > add %edx, %ecx > cmp %eax, %ecx > movl LEN(%esp), %ecx > - jle L(forward) > + jbe L(forward) > > /* Now do checks for lengths. We do [0..16], [0..32], [0..64], [0..128] > separately. */ > > > -- > WBR, > Andrew -- H.J. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Fix i386 memmove issue [BZ #22644] 2018-03-14 14:59 ` H.J. Lu @ 2018-03-19 12:46 ` Andrew Senkevich 2018-03-19 12:55 ` H.J. Lu 2018-03-19 13:11 ` Andreas Schwab 0 siblings, 2 replies; 19+ messages in thread From: Andrew Senkevich @ 2018-03-19 12:46 UTC (permalink / raw) To: H.J. Lu; +Cc: Andreas Schwab, libc-alpha, Max Horn, thomas 2018-03-14 15:59 GMT+01:00 H.J. Lu <hjl.tools@gmail.com>: > On Wed, Mar 14, 2018 at 7:43 AM, Andrew Senkevich > <andrew.n.senkevich@gmail.com> wrote: >> 2018-02-19 11:13 GMT+01:00 Andreas Schwab <schwab@suse.de>: >>> On Feb 19 2018, Andrew Senkevich <andrew.n.senkevich@gmail.com> wrote: >>> >>>> diff --git a/string/test-memmove.c b/string/test-memmove.c >>>> index edc7a4c..8dc152b >>>> --- a/string/test-memmove.c >>>> +++ b/string/test-memmove.c >>>> @@ -245,6 +245,49 @@ do_random_tests (void) >>>> } >>>> } >>>> >>>> +#if __SIZEOF_POINTER__ == 4 >>>> +static void >>>> +do_test2 (void) >>>> +{ >>>> + uint32_t i; >>>> + uint32_t num = 0x20000000; >>>> + uint32_t * large_buf = mmap (0, sizeof(uint32_t) * num, PROT_READ | >>>> PROT_WRITE, >>>> + MAP_PRIVATE | MAP_ANON, -1, 0); >>>> + if (large_buf == MAP_FAILED) >>>> + error (EXIT_FAILURE, errno, "large mmap failed"); >>>> + >>>> + if (!((uint32_t)(large_buf) < (0x80000000 - 128) && (0x80000000 + >>>> 128) < (uint32_t)(&large_buf[num]))) >>>> + { >>>> + error (0, 0,"allocated large memory doesn't cross 0x80000000 boundary"); >>>> + ret = 1; >>>> + return; >>> >>> Please properly fold long lines, and remove the redundant parens. Also, >>> there is no guarantee that the address range is unallocated. >> >> Thanks, updated patch below. Any comment or it is Ok for trunk? > > Please also test crossing 0x80000000 boundary on 64-bit systems. Hi, I extended test for 64-bit using MAP_FIXED which lets to hardcode allocated address. Manual says it is less portable, but without MAP_FIXED I was unable to get proper address for 64bits. I checked what test fails without fix of memcpy-sse2-unaligned implementation. Is it Ok for trunk? diff --git a/string/test-memmove.c b/string/test-memmove.c index edc7a4c..5920652 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,57 @@ do_random_tests (void) } } +#if __SIZEOF_POINTER__ == 4 +# define ptr_type uint32_t +#else +# define ptr_type uint64_t +#endif + +static void +do_test2 (void) +{ + uint32_t num = 0x20000000; + uint32_t * large_buf; + + large_buf = mmap ((void*)0x70000000, num, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANON | MAP_FIXED, -1, 0); + + if (large_buf == MAP_FAILED) + error (EXIT_FAILURE, errno, "Large mmap failed"); + + uint32_t bytes_move = 0x80000000 - (ptr_type)large_buf; + uint32_t arr_size = bytes_move / sizeof(uint32_t); + uint32_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, sizeof(uint32_t) * num); +} + int test_main (void) { @@ -284,6 +336,9 @@ test_main (void) } do_random_tests (); + + do_test2 (); + return ret; } diff --git a/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S b/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S index 9c3bbe7..9aa17de 100644 --- a/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S +++ b/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S @@ -72,7 +72,7 @@ ENTRY (MEMCPY) cmp %edx, %eax # ifdef USE_AS_MEMMOVE - jg L(check_forward) + ja L(check_forward) L(mm_len_0_or_more_backward): /* Now do checks for lengths. We do [0..16], [16..32], [32..64], [64..128] @@ -81,7 +81,7 @@ L(mm_len_0_or_more_backward): jbe L(mm_len_0_16_bytes_backward) cmpl $32, %ecx - jg L(mm_len_32_or_more_backward) + ja L(mm_len_32_or_more_backward) /* Copy [0..32] and return. */ movdqu (%eax), %xmm0 @@ -92,7 +92,7 @@ L(mm_len_0_or_more_backward): L(mm_len_32_or_more_backward): cmpl $64, %ecx - jg L(mm_len_64_or_more_backward) + ja L(mm_len_64_or_more_backward) /* Copy [0..64] and return. */ movdqu (%eax), %xmm0 @@ -107,7 +107,7 @@ L(mm_len_32_or_more_backward): L(mm_len_64_or_more_backward): cmpl $128, %ecx - jg L(mm_len_128_or_more_backward) + ja L(mm_len_128_or_more_backward) /* Copy [0..128] and return. */ movdqu (%eax), %xmm0 @@ -132,7 +132,7 @@ L(mm_len_128_or_more_backward): add %ecx, %eax cmp %edx, %eax movl SRC(%esp), %eax - jle L(forward) + jbe L(forward) PUSH (%esi) PUSH (%edi) PUSH (%ebx) @@ -269,7 +269,7 @@ L(check_forward): add %edx, %ecx cmp %eax, %ecx movl LEN(%esp), %ecx - jle L(forward) + jbe L(forward) /* Now do checks for lengths. We do [0..16], [0..32], [0..64], [0..128] separately. */ -- WBR, Andrew ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Fix i386 memmove issue [BZ #22644] 2018-03-19 12:46 ` Andrew Senkevich @ 2018-03-19 12:55 ` H.J. Lu 2018-03-19 13:11 ` Andreas Schwab 1 sibling, 0 replies; 19+ messages in thread From: H.J. Lu @ 2018-03-19 12:55 UTC (permalink / raw) To: Andrew Senkevich; +Cc: Andreas Schwab, libc-alpha, Max Horn, thomas On Mon, Mar 19, 2018 at 5:45 AM, Andrew Senkevich <andrew.n.senkevich@gmail.com> wrote: > 2018-03-14 15:59 GMT+01:00 H.J. Lu <hjl.tools@gmail.com>: >> On Wed, Mar 14, 2018 at 7:43 AM, Andrew Senkevich >> <andrew.n.senkevich@gmail.com> wrote: >>> 2018-02-19 11:13 GMT+01:00 Andreas Schwab <schwab@suse.de>: >>>> On Feb 19 2018, Andrew Senkevich <andrew.n.senkevich@gmail.com> wrote: >>>> >>>>> diff --git a/string/test-memmove.c b/string/test-memmove.c >>>>> index edc7a4c..8dc152b >>>>> --- a/string/test-memmove.c >>>>> +++ b/string/test-memmove.c >>>>> @@ -245,6 +245,49 @@ do_random_tests (void) >>>>> } >>>>> } >>>>> >>>>> +#if __SIZEOF_POINTER__ == 4 >>>>> +static void >>>>> +do_test2 (void) >>>>> +{ >>>>> + uint32_t i; >>>>> + uint32_t num = 0x20000000; >>>>> + uint32_t * large_buf = mmap (0, sizeof(uint32_t) * num, PROT_READ | >>>>> PROT_WRITE, >>>>> + MAP_PRIVATE | MAP_ANON, -1, 0); >>>>> + if (large_buf == MAP_FAILED) >>>>> + error (EXIT_FAILURE, errno, "large mmap failed"); >>>>> + >>>>> + if (!((uint32_t)(large_buf) < (0x80000000 - 128) && (0x80000000 + >>>>> 128) < (uint32_t)(&large_buf[num]))) >>>>> + { >>>>> + error (0, 0,"allocated large memory doesn't cross 0x80000000 boundary"); >>>>> + ret = 1; >>>>> + return; >>>> >>>> Please properly fold long lines, and remove the redundant parens. Also, >>>> there is no guarantee that the address range is unallocated. >>> >>> Thanks, updated patch below. Any comment or it is Ok for trunk? >> >> Please also test crossing 0x80000000 boundary on 64-bit systems. > > Hi, > > I extended test for 64-bit using MAP_FIXED which lets to hardcode > allocated address. Manual says it is less portable, but without > MAP_FIXED I was unable to get proper address for 64bits. > I checked what test fails without fix of memcpy-sse2-unaligned > implementation. Is it Ok for trunk? > > diff --git a/string/test-memmove.c b/string/test-memmove.c > index edc7a4c..5920652 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,57 @@ do_random_tests (void) > } > } > > +#if __SIZEOF_POINTER__ == 4 > +# define ptr_type uint32_t > +#else > +# define ptr_type uint64_t > +#endif Please use uintptr_t instead. > +static void > +do_test2 (void) > +{ > + uint32_t num = 0x20000000; > + uint32_t * large_buf; > + > + large_buf = mmap ((void*)0x70000000, num, PROT_READ | PROT_WRITE, > + MAP_PRIVATE | MAP_ANON | MAP_FIXED, -1, 0); > + > + if (large_buf == MAP_FAILED) > + error (EXIT_FAILURE, errno, "Large mmap failed"); Please EXIT_UNSUPPORTED. > + uint32_t bytes_move = 0x80000000 - (ptr_type)large_buf; > + uint32_t arr_size = bytes_move / sizeof(uint32_t); > + uint32_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, sizeof(uint32_t) * num); > +} > + > int > test_main (void) > { > @@ -284,6 +336,9 @@ test_main (void) > } > > do_random_tests (); > + > + do_test2 (); > + > return ret; > } > > diff --git a/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S > b/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S > index 9c3bbe7..9aa17de 100644 > --- a/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S > +++ b/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S > @@ -72,7 +72,7 @@ ENTRY (MEMCPY) > cmp %edx, %eax > > # ifdef USE_AS_MEMMOVE > - jg L(check_forward) > + ja L(check_forward) > > L(mm_len_0_or_more_backward): > /* Now do checks for lengths. We do [0..16], [16..32], [32..64], [64..128] > @@ -81,7 +81,7 @@ L(mm_len_0_or_more_backward): > jbe L(mm_len_0_16_bytes_backward) > > cmpl $32, %ecx > - jg L(mm_len_32_or_more_backward) > + ja L(mm_len_32_or_more_backward) > > /* Copy [0..32] and return. */ > movdqu (%eax), %xmm0 > @@ -92,7 +92,7 @@ L(mm_len_0_or_more_backward): > > L(mm_len_32_or_more_backward): > cmpl $64, %ecx > - jg L(mm_len_64_or_more_backward) > + ja L(mm_len_64_or_more_backward) > > /* Copy [0..64] and return. */ > movdqu (%eax), %xmm0 > @@ -107,7 +107,7 @@ L(mm_len_32_or_more_backward): > > L(mm_len_64_or_more_backward): > cmpl $128, %ecx > - jg L(mm_len_128_or_more_backward) > + ja L(mm_len_128_or_more_backward) > > /* Copy [0..128] and return. */ > movdqu (%eax), %xmm0 > @@ -132,7 +132,7 @@ L(mm_len_128_or_more_backward): > add %ecx, %eax > cmp %edx, %eax > movl SRC(%esp), %eax > - jle L(forward) > + jbe L(forward) > PUSH (%esi) > PUSH (%edi) > PUSH (%ebx) > @@ -269,7 +269,7 @@ L(check_forward): > add %edx, %ecx > cmp %eax, %ecx > movl LEN(%esp), %ecx > - jle L(forward) > + jbe L(forward) > > /* Now do checks for lengths. We do [0..16], [0..32], [0..64], [0..128] > separately. */ > > > -- > WBR, > Andrew -- H.J. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Fix i386 memmove issue [BZ #22644] 2018-03-19 12:46 ` Andrew Senkevich 2018-03-19 12:55 ` H.J. Lu @ 2018-03-19 13:11 ` Andreas Schwab 2018-03-19 13:17 ` Florian Weimer 1 sibling, 1 reply; 19+ messages in thread From: Andreas Schwab @ 2018-03-19 13:11 UTC (permalink / raw) To: Andrew Senkevich; +Cc: H.J. Lu, libc-alpha, Max Horn, thomas On Mär 19 2018, Andrew Senkevich <andrew.n.senkevich@gmail.com> wrote: > +static void > +do_test2 (void) > +{ > + uint32_t num = 0x20000000; > + uint32_t * large_buf; > + > + large_buf = mmap ((void*)0x70000000, num, PROT_READ | PROT_WRITE, > + MAP_PRIVATE | MAP_ANON | MAP_FIXED, -1, 0); Since you are using MAP_FIXED this may overwrite an existing mapping. Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Fix i386 memmove issue [BZ #22644] 2018-03-19 13:11 ` Andreas Schwab @ 2018-03-19 13:17 ` Florian Weimer 2018-03-19 14:01 ` Andrew Senkevich 2018-03-19 14:25 ` Szabolcs Nagy 0 siblings, 2 replies; 19+ messages in thread From: Florian Weimer @ 2018-03-19 13:17 UTC (permalink / raw) To: Andreas Schwab, Andrew Senkevich; +Cc: H.J. Lu, libc-alpha, Max Horn, thomas On 03/19/2018 02:11 PM, Andreas Schwab wrote: > On Mär 19 2018, Andrew Senkevich<andrew.n.senkevich@gmail.com> wrote: > >> +static void >> +do_test2 (void) >> +{ >> + uint32_t num = 0x20000000; >> + uint32_t * large_buf; >> + >> + large_buf = mmap ((void*)0x70000000, num, PROT_READ | PROT_WRITE, >> + MAP_PRIVATE | MAP_ANON | MAP_FIXED, -1, 0); > Since you are using MAP_FIXED this may overwrite an existing mapping. Leading to a hard-to-debug crash, maybe sporadically due to ASLR. Yes, I have this concern as well. There was a long, long Linux thread about a non-overriding MAP_FIXED variant, but as far as I can see, this has not been merged. Maybe it would have helped here. Is it very difficult to split out this test into a separate test file? Then link the whole thing statically, as non-PIE, and keep using MAP_FIXED. This should make it quite likely that you don't override anything valuable. Or you could parse /proc/self/maps to make sure that you don't override an existing mapping. Yuck. Thanks, Florian ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Fix i386 memmove issue [BZ #22644] 2018-03-19 13:17 ` Florian Weimer @ 2018-03-19 14:01 ` Andrew Senkevich 2018-03-19 14:25 ` Szabolcs Nagy 1 sibling, 0 replies; 19+ messages in thread From: Andrew Senkevich @ 2018-03-19 14:01 UTC (permalink / raw) To: Florian Weimer; +Cc: Andreas Schwab, H.J. Lu, libc-alpha, Max Horn, thomas 2018-03-19 14:17 GMT+01:00 Florian Weimer <fweimer@redhat.com>: > On 03/19/2018 02:11 PM, Andreas Schwab wrote: >> >> On Mär 19 2018, Andrew Senkevich<andrew.n.senkevich@gmail.com> wrote: >> >>> +static void >>> +do_test2 (void) >>> +{ >>> + uint32_t num = 0x20000000; >>> + uint32_t * large_buf; >>> + >>> + large_buf = mmap ((void*)0x70000000, num, PROT_READ | PROT_WRITE, >>> + MAP_PRIVATE | MAP_ANON | MAP_FIXED, -1, 0); >> >> Since you are using MAP_FIXED this may overwrite an existing mapping. > > > Leading to a hard-to-debug crash, maybe sporadically due to ASLR. Yes, I > have this concern as well. > > There was a long, long Linux thread about a non-overriding MAP_FIXED > variant, but as far as I can see, this has not been merged. Maybe it would > have helped here. > > Is it very difficult to split out this test into a separate test file? Then > link the whole thing statically, as non-PIE, and keep using MAP_FIXED. This > should make it quite likely that you don't override anything valuable. I think not very difficult, I will try this way. -- WBR, Andrew ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Fix i386 memmove issue [BZ #22644] 2018-03-19 13:17 ` Florian Weimer 2018-03-19 14:01 ` Andrew Senkevich @ 2018-03-19 14:25 ` Szabolcs Nagy 2018-03-19 15:33 ` Florian Weimer 1 sibling, 1 reply; 19+ messages in thread From: Szabolcs Nagy @ 2018-03-19 14:25 UTC (permalink / raw) To: Florian Weimer, Andreas Schwab, Andrew Senkevich Cc: nd, H.J. Lu, libc-alpha, Max Horn, thomas On 19/03/18 13:17, Florian Weimer wrote: > On 03/19/2018 02:11 PM, Andreas Schwab wrote: >> On Mär 19 2018, Andrew Senkevich<andrew.n.senkevich@gmail.com> wrote: >> >>> +static void >>> +do_test2 (void) >>> +{ >>> + uint32_t num = 0x20000000; >>> + uint32_t * large_buf; >>> + >>> + large_buf = mmap ((void*)0x70000000, num, PROT_READ | PROT_WRITE, >>> +    MAP_PRIVATE | MAP_ANON | MAP_FIXED, -1, 0); >> Since you are using MAP_FIXED this may overwrite an existing mapping. > > Leading to a hard-to-debug crash, maybe sporadically due to ASLR. Yes, I have this concern as well. > > There was a long, long Linux thread about a non-overriding MAP_FIXED variant, but as far as I can see, this has not been merged. Maybe it would > have helped here. > i thought not using MAP_FIXED is the 'non-overriding MAP_FIXED variant' if you use an address hint then the kernel will use that unless it's not available and you can check the result. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Fix i386 memmove issue [BZ #22644] 2018-03-19 14:25 ` Szabolcs Nagy @ 2018-03-19 15:33 ` Florian Weimer 2018-03-19 17:52 ` Andrew Senkevich 0 siblings, 1 reply; 19+ messages in thread From: Florian Weimer @ 2018-03-19 15:33 UTC (permalink / raw) To: Szabolcs Nagy, Andreas Schwab, Andrew Senkevich Cc: nd, H.J. Lu, libc-alpha, Max Horn, thomas On 03/19/2018 03:25 PM, Szabolcs Nagy wrote: > i thought not using MAP_FIXED is the 'non-overriding MAP_FIXED variant' In general, yes. But I think there cases where the hint is ignored even if there isn't a pre-existing mapping. Maybe mapping things at the middle of the (32-bit) address space is one such case? Thanks, Florian ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Fix i386 memmove issue [BZ #22644] 2018-03-19 15:33 ` Florian Weimer @ 2018-03-19 17:52 ` Andrew Senkevich 2018-03-19 17:57 ` H.J. Lu 0 siblings, 1 reply; 19+ messages in thread From: Andrew Senkevich @ 2018-03-19 17:52 UTC (permalink / raw) To: Florian Weimer Cc: Szabolcs Nagy, Andreas Schwab, nd, H.J. Lu, libc-alpha, Max Horn, thomas 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) + { + error (0, 0,"Large mmap doesn't cross 0x80000000 boundary"); + ret = EXIT_UNSUPPORTED; + munmap((void *)large_buf, size); + return; + } + + uint32_t bytes_move = 0x80000000 - (uintptr_t)large_buf; + uint32_t arr_size = bytes_move / sizeof(uint32_t); + uint32_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 +338,9 @@ test_main (void) } do_random_tests (); + + do_test2 (); + return ret; } Ok? -- WBR, Andrew ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Fix i386 memmove issue [BZ #22644] 2018-03-19 17:52 ` Andrew Senkevich @ 2018-03-19 17:57 ` H.J. Lu 2018-03-19 19:30 ` Andrew Senkevich 0 siblings, 1 reply; 19+ messages in thread From: H.J. Lu @ 2018-03-19 17:57 UTC (permalink / raw) To: Andrew Senkevich Cc: Florian Weimer, Szabolcs Nagy, Andreas Schwab, nd, libc-alpha, Max Horn, thomas 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? > + { > + error (0, 0,"Large mmap doesn't cross 0x80000000 boundary"); > + ret = EXIT_UNSUPPORTED; > + munmap((void *)large_buf, size); > + return; > + } > + > + uint32_t bytes_move = 0x80000000 - (uintptr_t)large_buf; > + uint32_t arr_size = bytes_move / sizeof(uint32_t); > + uint32_t i; Please use size_t instead of uint32_t since 64-bit test is added now. > + 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 +338,9 @@ test_main (void) > } > > do_random_tests (); > + > + do_test2 (); > + > return ret; > } > > Ok? > > > -- > WBR, > Andrew -- H.J. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Fix i386 memmove issue [BZ #22644] 2018-03-19 17:57 ` H.J. Lu @ 2018-03-19 19:30 ` Andrew Senkevich 2018-03-19 19:38 ` H.J. Lu 0 siblings, 1 reply; 19+ messages in thread From: Andrew Senkevich @ 2018-03-19 19:30 UTC (permalink / raw) To: H.J. Lu Cc: Florian Weimer, Szabolcs Nagy, Andreas Schwab, nd, libc-alpha, Max Horn, thomas 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; } -- WBR, Andrew ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Fix i386 memmove issue [BZ #22644] 2018-03-19 19:30 ` Andrew Senkevich @ 2018-03-19 19:38 ` H.J. Lu 2018-03-19 20:33 ` Andrew Senkevich 0 siblings, 1 reply; 19+ messages in thread From: H.J. Lu @ 2018-03-19 19:38 UTC (permalink / raw) To: Andrew Senkevich Cc: Florian Weimer, Szabolcs Nagy, Andreas Schwab, nd, libc-alpha, Max Horn, thomas 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. -- H.J. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Fix i386 memmove issue [BZ #22644] 2018-03-19 19:38 ` H.J. Lu @ 2018-03-19 20:33 ` Andrew Senkevich 2018-03-19 20:50 ` H.J. Lu 2018-03-20 8:58 ` Andreas Schwab 0 siblings, 2 replies; 19+ messages in thread From: Andrew Senkevich @ 2018-03-19 20:33 UTC (permalink / raw) To: H.J. Lu Cc: Florian Weimer, Szabolcs Nagy, Andreas Schwab, nd, libc-alpha, Max Horn, thomas 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. diff --git a/ChangeLog b/ChangeLog index 4eb0632..5a0e335 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2018-03-19 Andrew Senkevich <andrew.senkevich@intel.com> + Max Horn <max@quendi.de> + + [BZ #22644] + * sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S: Fixed + branch conditions. + * string/test-memmove.c (do_test2): New testcase. + 2018-03-19 Wilco Dijkstra <wdijkstr@arm.com> * benchtests/bench-timing.h (attribute_hidden): Undefine. 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; } diff --git a/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S b/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S index 9c3bbe7..9aa17de 100644 --- a/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S +++ b/sysdeps/i386/i686/multiarch/memcpy-sse2-unaligned.S @@ -72,7 +72,7 @@ ENTRY (MEMCPY) cmp %edx, %eax # ifdef USE_AS_MEMMOVE - jg L(check_forward) + ja L(check_forward) L(mm_len_0_or_more_backward): /* Now do checks for lengths. We do [0..16], [16..32], [32..64], [64..128] @@ -81,7 +81,7 @@ L(mm_len_0_or_more_backward): jbe L(mm_len_0_16_bytes_backward) cmpl $32, %ecx - jg L(mm_len_32_or_more_backward) + ja L(mm_len_32_or_more_backward) /* Copy [0..32] and return. */ movdqu (%eax), %xmm0 @@ -92,7 +92,7 @@ L(mm_len_0_or_more_backward): L(mm_len_32_or_more_backward): cmpl $64, %ecx - jg L(mm_len_64_or_more_backward) + ja L(mm_len_64_or_more_backward) /* Copy [0..64] and return. */ movdqu (%eax), %xmm0 @@ -107,7 +107,7 @@ L(mm_len_32_or_more_backward): L(mm_len_64_or_more_backward): cmpl $128, %ecx - jg L(mm_len_128_or_more_backward) + ja L(mm_len_128_or_more_backward) /* Copy [0..128] and return. */ movdqu (%eax), %xmm0 @@ -132,7 +132,7 @@ L(mm_len_128_or_more_backward): add %ecx, %eax cmp %edx, %eax movl SRC(%esp), %eax - jle L(forward) + jbe L(forward) PUSH (%esi) PUSH (%edi) PUSH (%ebx) @@ -269,7 +269,7 @@ L(check_forward): add %edx, %ecx cmp %eax, %ecx movl LEN(%esp), %ecx - jle L(forward) + jbe L(forward) /* Now do checks for lengths. We do [0..16], [0..32], [0..64], [0..128] separately. */ -- WBR, Andrew ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Fix i386 memmove issue [BZ #22644] 2018-03-19 20:33 ` Andrew Senkevich @ 2018-03-19 20:50 ` H.J. Lu 2018-03-20 8:58 ` Andreas Schwab 1 sibling, 0 replies; 19+ messages in thread From: H.J. Lu @ 2018-03-19 20:50 UTC (permalink / raw) To: Andrew Senkevich Cc: Florian Weimer, Szabolcs Nagy, Andreas Schwab, nd, libc-alpha, Max Horn, thomas 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Fix i386 memmove issue [BZ #22644] 2018-03-19 20:33 ` Andrew Senkevich 2018-03-19 20:50 ` H.J. Lu @ 2018-03-20 8:58 ` Andreas Schwab 2018-03-23 17:15 ` Andrew Senkevich 1 sibling, 1 reply; 19+ messages in thread From: Andreas Schwab @ 2018-03-20 8:58 UTC (permalink / raw) To: Andrew Senkevich Cc: H.J. Lu, Florian Weimer, Szabolcs Nagy, nd, libc-alpha, Max Horn, thomas On Mär 19 2018, Andrew Senkevich <andrew.n.senkevich@gmail.com> wrote: > 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); Style: line up indentation with paren. > + > + 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) Style: space after cast. > + { > + error (0, 0,"Large mmap allocated improperly"); Style: space after comma. > + ret = EXIT_UNSUPPORTED; > + munmap((void *)large_buf, size); Style: space before paren and after cast. > + return; > + } > + > + size_t bytes_move = 0x80000000 - (uintptr_t)large_buf; Style: space after cast. > + size_t arr_size = bytes_move / sizeof(uint32_t); Style: space before paren. > + 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 Styles: space after cast. > + > + for (i = 0; i < arr_size; i++) > + { > + if (dst[i] != i) > + { Style: wrong indentation. > + error (0, 0, > + "Wrong result in function %s dst \"%p\" src \"%p\" offset \"%d\"", > + impl->name, dst, large_buf, i); Style: line up indentation with paren. > + ret = 1; > + break; > + } > + } > + } > + > + munmap((void *)large_buf, size); Style: space before paren and after cast. Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Fix i386 memmove issue [BZ #22644] 2018-03-20 8:58 ` Andreas Schwab @ 2018-03-23 17:15 ` Andrew Senkevich 0 siblings, 0 replies; 19+ messages in thread From: Andrew Senkevich @ 2018-03-23 17:15 UTC (permalink / raw) To: Andreas Schwab Cc: H.J. Lu, Florian Weimer, Szabolcs Nagy, nd, libc-alpha, Max Horn, thomas 2018-03-20 9:58 GMT+01:00 Andreas Schwab <schwab@suse.de>: > On Mär 19 2018, Andrew Senkevich <andrew.n.senkevich@gmail.com> wrote: > >> 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); > > Style: line up indentation with paren. > >> + >> + 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) > > Style: space after cast. > >> + { >> + error (0, 0,"Large mmap allocated improperly"); > > Style: space after comma. > >> + ret = EXIT_UNSUPPORTED; >> + munmap((void *)large_buf, size); > > Style: space before paren and after cast. > >> + return; >> + } >> + >> + size_t bytes_move = 0x80000000 - (uintptr_t)large_buf; > > Style: space after cast. > >> + size_t arr_size = bytes_move / sizeof(uint32_t); > > Style: space before paren. > >> + 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 > > Styles: space after cast. > >> + >> + for (i = 0; i < arr_size; i++) >> + { >> + if (dst[i] != i) >> + { > > Style: wrong indentation. > >> + error (0, 0, >> + "Wrong result in function %s dst \"%p\" src \"%p\" offset \"%d\"", >> + impl->name, dst, large_buf, i); > > Style: line up indentation with paren. > >> + ret = 1; >> + break; >> + } >> + } >> + } >> + >> + munmap((void *)large_buf, size); > > Style: space before paren and after cast. Thanks, committed with fixed style. I will also backport it to affected branches. -- WBR, Andrew ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-03-23 17:15 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-19 10:13 [PATCH] Fix i386 memmove issue [BZ #22644] Andrew Senkevich 2018-02-19 10:31 ` Andreas Schwab 2018-03-14 14:43 ` Andrew Senkevich 2018-03-14 14:59 ` H.J. Lu 2018-03-19 12:46 ` Andrew Senkevich 2018-03-19 12:55 ` H.J. Lu 2018-03-19 13:11 ` Andreas Schwab 2018-03-19 13:17 ` Florian Weimer 2018-03-19 14:01 ` Andrew Senkevich 2018-03-19 14:25 ` Szabolcs Nagy 2018-03-19 15:33 ` Florian Weimer 2018-03-19 17:52 ` Andrew Senkevich 2018-03-19 17:57 ` H.J. Lu 2018-03-19 19:30 ` Andrew Senkevich 2018-03-19 19:38 ` H.J. Lu 2018-03-19 20:33 ` Andrew Senkevich 2018-03-19 20:50 ` H.J. Lu 2018-03-20 8:58 ` Andreas Schwab 2018-03-23 17:15 ` Andrew Senkevich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).