[Bug libc/16394] New: i686/memmove.S always copies backwards when dst > src

[Bug libc/16394] New: i686/memmove.S always copies backwards when dst > src

[maxim.kuvyrkov at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16394

            Bug ID: 16394
           Summary: i686/memmove.S always copies backwards when dst > src
           Product: glibc
           Version: unspecified
            Status: NEW
          Severity: enhancement
          Priority: P2
         Component: libc
          Assignee: unassigned at sourceware dot org
          Reporter: maxim.kuvyrkov at gmail dot com
                CC: drepper.fsp at gmail dot com

Submitted by Yuriy Kaminskiy on libc-alpha@:

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

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/16394] i686/memmove.S always copies backwards when dst > src

[maxim.kuvyrkov at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16394

Maxim Kuvyrkov <maxim.kuvyrkov at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |maxim.kuvyrkov at gmail dot com

--- Comment #1 from Maxim Kuvyrkov <maxim.kuvyrkov at gmail dot com> ---
Note that
if (dstp - srcp >= len)       /* *Unsigned* compare!  */
is the right way to check for whether to use forward or backward copy.  We
still want to use forward copy when "dstp - srcp == len" as memory regions
don't overlap for any LEN > 0.
Your patch and the use of "ja" is correct though.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/16394] i686/memmove.S always copies backwards when dst > src

[maxim.kuvyrkov at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16394

Maxim Kuvyrkov <maxim.kuvyrkov at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #2 from Maxim Kuvyrkov <maxim.kuvyrkov at gmail dot com> ---
Fixed in 66671c84d58d6ae705bac39dc476c3d3b2b81116

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/16394] i686/memmove.S always copies backwards when dst > src

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16394

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |security-

-- 
You are receiving this mail because:
You are on the CC list for the bug.