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

[Maxim Kuvyrkov <maxim at kugelworks dot com>]

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 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.
> 

Hi Yuriy,

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.

Thank you,

--
Maxim Kuvyrkov
www.kugelworks.com