This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: i686/memmove.S always copies backwards when dst > src
- From: Yuriy Kaminskiy <yumkam at gmail dot com>
- To: libc-alpha at sourceware dot org
- Date: Mon, 23 Dec 2013 13:26:20 +0400
- Subject: Re: i686/memmove.S always copies backwards when dst > src
- Authentication-results: sourceware.org; auth=none
- References: <l97qbr$m1$1 at ger dot gmane dot org> <956CAFFD-2EA0-4AAB-BC41-94EA0D5531D4 at kugelworks dot com>
Maxim Kuvyrkov wrote:
> On 23/12/2013, at 11:50 am, Yuriy Kaminskiy <yumkam@gmail.com> wrote:
>
>> Compare:
>> === cut string/memmove.c ===
>> rettype
>> MEMMOVE (a1, a2, len)
>> a1const void *a1;
>> a2const void *a2;
>> size_t len;
>> {
>> unsigned long int dstp = (long int) dest;
>> unsigned long int srcp = (long int) src;
>>
>> /* This test makes the forward copying code be used whenever possible.
>> Reduces the working set. */
>> if (dstp - srcp >= len) /* *Unsigned* compare! */
>> {
>> /* Copy from the beginning to the end. */
>> === cut sysdeps/i386/i686/memmove.S ===
>> ....
>> movl LEN(%esp), %ecx
>> movl DEST(%esp), %edi
>> ...
>> movl SRC(%esp), %esi
>> ...
>> movl %edi, %eax
>> subl %esi, %eax
>> cmpl %eax, %edi
>> jae 3f
>> [...copy forward ...]
>> ret
>> 3:
>> [...copy backward...]
>> === cut ===
>>
>> Obviously, the assembler code checks 'dstp - srcp >= dstp' (an awkward way to
>> check for dstp > srcp?) instead of 'dstp - srcp > len', as was in the C code;
>> apparently this was /supposed/ to replicate the same logic as in the C code, but
>> registers names was mixed up, and as "it works", nobody noticed. Fortunately, it
>> seems only result in choosing suboptimal backward copy in non-overlapping case
>> when dst > src. Git blame says this mistaken check was already present when this
>> code was first committed.
>> Patch attached.
> Good job on catching this problem and even providing solution!
>
> Libc-alpha@ (CC'ed) is the right list to post patches to; libc-help@ is for
> user questions about glibc. Please post your patch to libc-alpha@, e.g., by
> reply-all to this email.
[including patch this time]
--- glibc/sysdeps/i386/i686/memmove.S.orig
+++ glibc/sysdeps/i386/i686/memmove.S
@@ -63,8 +63,8 @@
movl %edi, %eax
subl %esi, %eax
- cmpl %eax, %edi
- jae 3f
+ cmpl %eax, %ecx
+ ja 3f
cld
shrl $1, %ecx