Subject: Register allocation in copy_to_user

From: [email-masked]
Date: Tue, 25 Sep 2001 20:51:03 +0200

For some time, I have seen occasional corruption of tty-output (pty's and
serial). This turned out to be caused by a register collision in read_chan
()
in n_tty.c. In the expansion of copy_to_user, the compiler chose register
"a0" to hold the value of local variable __cu_from. Since this register is
modified in the asm statement, before __cu_from is used, the corruption
occured.

I am not sure, whether this is a compiler-bug (egcs-2.91.66) or the code
should prevent this from happening. Have the semantics about side-effects
of asm statements changed?

Anyway, the attached patch solves this by explicitly building the arguments
to __copy_user in the argument registers ;-) instead of moving them around.
So it actually saves some instructions as well. And the compiler can
generate
better code since it now has more registers for temporary variables ...

Is this OK? It works just fine for me with a 2.4.9 kernel (VR5000).

-Tommy
--- include/asm-mips/uaccess.h.orig      Mon Aug  6 10:35:52 2001
+++ include/asm-mips/uaccess.h     Tue Sep 25 18:51:00 2001
@@ -270,97 +270,81 @@
 extern size_t __copy_user(void *__to, const void *__from, size_t __n);

 #define __copy_to_user(to,from,n) ({ \
-    void *__cu_to; \
-    const void *__cu_from; \
-    long __cu_len; \
+    register void *__cu_to __asm__ ("$4"); \
+    register const void *__cu_from __asm__ ("$5"); \
+    register long __cu_len __asm__ ("$6"); \
     \
     __cu_to = (to); \
     __cu_from = (from); \
     __cu_len = (n); \
     __asm__ __volatile__( \
-         "move\t$4, %1\n\t" \
-         "move\t$5, %2\n\t" \
-         "move\t$6, %3\n\t" \
          __MODULE_JAL(__copy_user) \
-         "move\t%0, $6" \
-         : "=r" (__cu_len) \
-         : "r" (__cu_to), "r" (__cu_from), "r" (__cu_len) \
-         : "$4", "$5", "$6", "$8", "$9", "$10", "$11", "$12", "$15", \
+         : "+r" (__cu_to), "+r" (__cu_from), "+r" (__cu_len) \
+         : \
+         : "$8", "$9", "$10", "$11", "$12", "$15", \
            "$24", "$31","memory"); \
     __cu_len; \
 })

 #define __copy_from_user(to,from,n) ({ \
-    void *__cu_to; \
-    const void *__cu_from; \
-    long __cu_len; \
+    register void *__cu_to __asm__ ("$4"); \
+    register const void *__cu_from __asm__ ("$5"); \
+    register long __cu_len __asm__ ("$6"); \
     \
     __cu_to = (to); \
     __cu_from = (from); \
     __cu_len = (n); \
     __asm__ __volatile__( \
-         "move\t$4, %1\n\t" \
-         "move\t$5, %2\n\t" \
-         "move\t$6, %3\n\t" \
          ".set\tnoreorder\n\t" \
          __MODULE_JAL(__copy_user) \
          ".set\tnoat\n\t" \
-         "addu\t$1, %2, %3\n\t" \
+         "addu\t$1, %1, %2\n\t" \
          ".set\tat\n\t" \
          ".set\treorder\n\t" \
-         "move\t%0, $6" \
-         : "=r" (__cu_len) \
-         : "r" (__cu_to), "r" (__cu_from), "r" (__cu_len) \
-         : "$4", "$5", "$6", "$8", "$9", "$10", "$11", "$12", "$15", \
+         : "+r" (__cu_to), "+r" (__cu_from), "+r" (__cu_len) \
+         : \
+         : "$8", "$9", "$10", "$11", "$12", "$15", \
            "$24", "$31","memory"); \
     __cu_len; \
 })

 #define copy_to_user(to,from,n) ({ \
-    void *__cu_to; \
-    const void *__cu_from; \
-    long __cu_len; \
+    register void *__cu_to __asm__ ("$4"); \
+    register const void *__cu_from __asm__ ("$5"); \
+    register long __cu_len __asm__ ("$6"); \
     \
     __cu_to = (to); \
     __cu_from = (from); \
     __cu_len = (n); \
     if (access_ok(VERIFY_WRITE, __cu_to, __cu_len)) \
          __asm__ __volatile__( \
-              "move\t$4, %1\n\t" \
-              "move\t$5, %2\n\t" \
-              "move\t$6, %3\n\t" \
               __MODULE_JAL(__copy_user) \
-              "move\t%0, $6" \
-              : "=r" (__cu_len) \
-              : "r" (__cu_to), "r" (__cu_from), "r" (__cu_len) \
-              : "$4", "$5", "$6", "$8", "$9", "$10", "$11", "$12", \
+              : "+r" (__cu_to), "+r" (__cu_from), "+r" (__cu_len) \
+              : \
+              : "$8", "$9", "$10", "$11", "$12", \
                 "$15", "$24", "$31","memory"); \
     __cu_len; \
 })

 #define copy_from_user(to,from,n) ({ \
-    void *__cu_to; \
-    const void *__cu_from; \
-    long __cu_len; \
+    register void *__cu_to __asm__ ("$4"); \
+    register const void *__cu_from __asm__ ("$5"); \
+    register long __cu_len __asm__ ("$6"); \
     \
     __cu_to = (to); \
     __cu_from = (from); \
     __cu_len = (n); \
     if (access_ok(VERIFY_READ, __cu_from, __cu_len)) \
          __asm__ __volatile__( \
-              "move\t$4, %1\n\t" \
-              "move\t$5, %2\n\t" \
-              "move\t$6, %3\n\t" \
               ".set\tnoreorder\n\t" \
               __MODULE_JAL(__copy_user) \
               ".set\tnoat\n\t" \
-              "addu\t$1, %2, %3\n\t" \
+              "addu\t$1, %1, %2\n\t" \
               ".set\tat\n\t" \
               ".set\treorder\n\t" \
-              "move\t%0, $6" \
-              : "=r" (__cu_len) \
-              : "r" (__cu_to), "r" (__cu_from), "r" (__cu_len) \
-              : "$4", "$5", "$6", "$8", "$9", "$10", "$11", "$12", \
+              : "+r" (__cu_to), "+r" (__cu_from), "+r" (__cu_len) \
+              : \
+              : "$8", "$9", "$10", "$11", "$12", \
                 "$15", "$24", "$31","memory"); \
     __cu_len; \
 })

---

From: Ralf Baechle <[email-masked]>
Date: Wed, 26 Sep 2001 00:11:36 +0200

On Tue, Sep 25, 2001 at 08:51:03PM +0200, [email-masked] wrote:

> generate better code since it now has more registers for temporary
> variables ...
> 

Unfortunately I had to find that your bugreport is correct.   To make
things worse at the time when I implemented this code I used your approach
(which definately is the cleaner approach) and I ran into the same problem.

  Ralf