From patchwork Mon Mar 19 20:32:27 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Senkevich X-Patchwork-Id: 26384 Received: (qmail 109941 invoked by alias); 19 Mar 2018 20:33:02 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 109064 invoked by uid 89); 19 Mar 2018 20:33:02 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.9 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-lf0-f66.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=az1YRiKdKU0RZJtkRTbgQbiDzVVzCdRHqGMCwMaTbAE=; b=FWdr1aEdpogm3iY5cdrcbecW9Vm1g65Pz5Jb78e8fLKf7G/KBxDxwkjqrgPVqLD3v7 CAeMKn09c2SLWcL3WQX2mmu4PcFIjWjWd72sNHddZgSKUM4Crp+DuxMxVyL3PZ0lKfah Q7/azLNMgMtWSUnh9VVpgrAoEXo1IVYx0aB5o2bkDDZcnsL2vE31dI8V4UQOEznsA8Az 72mjANm83r+u4bNINSu1GyXpkGySUhRQNJcMnlWhp2x2UKpBcZtVfXaWjs7nOWYeXewW jgCTQxO5J9nonZjQmou6gvq/a0ipXWkGESBo/VwvpuYxJGBat2MYqvy4wbzWbKms42E7 q2QQ== X-Gm-Message-State: AElRT7EmXpB57YhlUS56vZFRuVy3qAko2xzSJKQJu4ytQ8/rCWbbDXYV bkf4jFM4/2YPYKO8RmiZYA+UgUepRJP6F8GLOgQ= X-Google-Smtp-Source: AG47ELtE4MKMvS2oOczWQSMcPreNCbrLFhDkGi3EKkiOiG+YJXq7HR/6QtAOGROY7rQTnVJGYnOdbG9e9De1mbIHc/8= X-Received: by 2002:a19:e347:: with SMTP id c7-v6mr9522810lfk.31.1521491577496; Mon, 19 Mar 2018 13:32:57 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <597954a1-85da-e524-1454-27c46af57413@redhat.com> <8c8ab285-bddc-90e6-a50e-7be89703367a@redhat.com> From: Andrew Senkevich Date: Mon, 19 Mar 2018 21:32:27 +0100 Message-ID: Subject: Re: [PATCH] Fix i386 memmove issue [BZ #22644] To: "H.J. Lu" Cc: Florian Weimer , Szabolcs Nagy , Andreas Schwab , nd , libc-alpha , Max Horn , thomas@grindinggear.com 2018-03-19 20:38 GMT+01:00 H.J. Lu : > On Mon, Mar 19, 2018 at 12:29 PM, Andrew Senkevich > wrote: >> 2018-03-19 18:57 GMT+01:00 H.J. Lu : >>> On Mon, Mar 19, 2018 at 10:52 AM, Andrew Senkevich >>> wrote: >>>> 2018-03-19 16:33 GMT+01:00 Florian Weimer : >>>>> 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 >>>> >>>> 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 >> >> 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. --- WBR, Andrew diff --git a/ChangeLog b/ChangeLog index 4eb0632..5a0e335 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2018-03-19 Andrew Senkevich + Max Horn + + [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 * 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 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. */