This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: i686/memmove.S always copies backwards when dst > src


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]