parisc: fix out-of-register compiler error in ldcw inline assembler function

PATCH In review Helge Deller 2 participants 6 messages Oct 21, 2014 – Dec 15, 2014 2 messages not in the archive PA-RISC architecture

lore

Share & tools

lore

Helge Deller

author maintainer

2014-10-21 19:46 UTC #0 lore raw

PATCH 1/1 parisc: fix out-of-register compiler error in ldcw inline assembler function

Sometimes we face this compiler error:

arch/parisc/include/asm/ldcw.h:39:2: error: can't find a register in class 'R1_REGS' while reloading 'asm'
__asm__ __volatile__(__LDCW " 0(%2),%0"...
note: in expansion of macro '__ldcw'
error: 'asm' operand has impossible constraints

Dave suggested:
Likely the problem can be fixed by making __ldcw a static inline function and forcing the argument 'a' to a specific register before using in ldcw.

Since it's not easy to reproduce this bug, this patch now tries to still let the compiler decide on which register should be used. If it doesn't work, we'll have to assign a specific register as suggested by Dave.

Signed-off-by Helge Deller <[email protected]>

Cc John David Anglin <[email protected]>

arch/parisc/include/asm/ldcw.h +8 -6

index d2d11b7..b951e01 100644
@@ -34,12 +34,14 @@
 #endif /*!CONFIG_PA20*/
 
 /* LDCW, the only atomic read-write operation PA-RISC has. *sigh*.  */
-#define __ldcw(a) ({						\
-	unsigned __ret;						\
-	__asm__ __volatile__(__LDCW " 0(%2),%0"			\
-		: "=r" (__ret), "+m" (*(a)) : "r" (a));		\
-	__ret;							\
-})
+static inline unsigned int __ldcw(volatile unsigned int *address)
+{
+	unsigned int ret;
+	register volatile unsigned int *a = address;
+	__asm__ __volatile__(__LDCW " 0(%2),%0"
+		: "=r" (ret), "+m" (*(a)) : "r" (a));
+	return ret;
+}
 
 #ifdef CONFIG_SMP
 # define __lock_aligned __attribute__((__section__(".data..lock_aligned")))

John David Anglin

2014-10-21 20:01 UTC #1 lore raw

Quoted 5 lines · Helge Deller

/* LDCW, the only atomic read-write operation PA-RISC has. *sigh*. */
arch/parisc/include/asm/ldcw.h +0 -4
-#define __ldcw(a) ({						\
-	unsigned __ret;						\
-	__asm__ __volatile__(__LDCW " 0(%2),%0"			\
-		: "=r" (__ret), "+m" (*(a)) : "r" (a));		\

arch/parisc/include/asm/ldcw.h line 44

Quoted 10 lines

arch/parisc/include/asm/ldcw.h +8 -2

-	__ret;							\
-})
+static inline unsigned int __ldcw(volatile unsigned int *address)
+{
+	unsigned int ret;
+	register volatile unsigned int *a = address;
+	__asm__ __volatile__(__LDCW " 0(%2),%0"
+		: "=r" (ret), "+m" (*(a)) : "r" (a));
+	return ret;
+}

You could keep the old macro version for 32-bit builds as the problem shouldn't occur there.

Dave

Signature · 1 line

John David Anglin    [email protected]

Helge Deller

author maintainer

2014-12-10 23:08 UTC #2 lore raw

Quoted 17 lines · John David Anglin

Quoted 5 lines · Helge Deller
/* LDCW, the only atomic read-write operation PA-RISC has. *sigh*. */
(quoted diff) +0 -4
-#define __ldcw(a) ({                        \
-    unsigned __ret;                        \
-    __asm__ __volatile__(__LDCW " 0(%2),%0"            \
-        : "=r" (__ret), "+m" (*(a)) : "r" (a));        \
Review
Quoted 10 lines
(quoted diff) +8 -2
-    __ret;                            \
-})
+static inline unsigned int __ldcw(volatile unsigned int *address)
+{
+    unsigned int ret;
+    register volatile unsigned int *a = address;
+    __asm__ __volatile__(__LDCW " 0(%2),%0"
+        : "=r" (ret), "+m" (*(a)) : "r" (a));
+    return ret;
+}
You could keep the old macro version for 32-bit builds as the problem shouldn't occur there.

Attached is an updated patch/work-around which just reuses the existing register. I don't like this patch much, but it's better than nothing for now.

Helge

arch/parisc/include/asm/ldcw.h +18 -6

index d2d11b7..989595d 100644
@@ -1,6 +1,8 @@
 #ifndef __PARISC_LDCW_H
 #define __PARISC_LDCW_H
 
+#include <linux/compiler.h>
+
 #ifndef CONFIG_PA20
 /* Because kmalloc only guarantees 8-byte alignment for kmalloc'd data,
    and GCC only guarantees 8-byte alignment for stack locals, we can't

@@ -34,12 +36,22 @@
 #endif /*!CONFIG_PA20*/
 
 /* LDCW, the only atomic read-write operation PA-RISC has. *sigh*.  */
-#define __ldcw(a) ({						\
-	unsigned __ret;						\
-	__asm__ __volatile__(__LDCW " 0(%2),%0"			\
-		: "=r" (__ret), "+m" (*(a)) : "r" (a));		\
-	__ret;							\
-})
+static inline unsigned int __ldcw(volatile unsigned int *address)
+{
+#if GCC_VERSION >= 40900
+	/* work around gcc-4.9 bug:
+	 * error: can't find a register in class 'R1_REGS' while reloading 'asm'
+	 */
+	__asm__ __volatile__(__LDCW " 0(%0),%0"
+		: "=r" (address), "+m" (*(address)) : "0" (address));
+	return (unsigned long) address;
+#else
+	unsigned int val;
+	__asm__ __volatile__(__LDCW " 0(%2),%0"
+		: "=r" (val), "+m" (*(address)) : "r" (address));
+	return val;
+#endif
+}
 
 #ifdef CONFIG_SMP
 # define __lock_aligned __attribute__((__section__(".data..lock_aligned")))

John David Anglin

2014-12-14 15:49 UTC #3 lore raw

PATCH 1/1 parisc: fix out-of-register compiler error in ldcw inline assembler function

In reply to Helge Deller

Quoted 34 lines · Helge Deller

Sometimes we face this compiler error:
arch/parisc/include/asm/ldcw.h:39:2: error: can't find a register in class 'R1_REGS' while reloading 'asm'
__asm__ __volatile__(__LDCW " 0(%2),%0"...
note: in expansion of macro '__ldcw'
error: 'asm' operand has impossible constraints
Dave suggested:
Likely the problem can be fixed by making __ldcw a static inline function and forcing the argument 'a' to a specific register before using in ldcw.
Since it's not easy to reproduce this bug, this patch now tries to still let the compiler decide on which register should be used. If it doesn't work, we'll have to assign a specific register as suggested by Dave.
Signed-off-by Helge Deller <[email protected]>
Cc John David Anglin <[email protected]>
arch/parisc/include/asm/ldcw.h +1 -6
index d2d11b7..b951e01 100644
@@ -34,12 +34,14 @@
 #endif /*!CONFIG_PA20*/
 
 /* LDCW, the only atomic read-write operation PA-RISC has. *sigh*.  */
-#define __ldcw(a) ({						\
-	unsigned __ret;						\
-	__asm__ __volatile__(__LDCW " 0(%2),%0"			\
-		: "=r" (__ret), "+m" (*(a)) : "r" (a));		\
-	__ret;							\
-})
+static inline unsigned int __ldcw(volatile unsigned int *address)

arch/parisc/include/asm/ldcw.h line 47

Quoted 10 lines

arch/parisc/include/asm/ldcw.h +7 -0

+{
+	unsigned int ret;
+	register volatile unsigned int *a = address;
+	__asm__ __volatile__(__LDCW " 0(%2),%0"
+		: "=r" (ret), "+m" (*(a)) : "r" (a));
+	return ret;
+}
 
 #ifdef CONFIG_SMP
 # define __lock_aligned __attribute__((__section__(".data..lock_aligned")))

Helge, could you test the attached patch. Updated comment in change explains the "memory" clobber. It's been lightly tested.

Signed-off-by John David Anglin <[email protected]>

---

arch/parisc/include/asm/ldcw.h +10 -3

index d2d11b7..8121aa6 100644
@@ -33,11 +33,18 @@
 
 #endif /*!CONFIG_PA20*/
 
-/* LDCW, the only atomic read-write operation PA-RISC has. *sigh*.  */
+/* LDCW, the only atomic read-write operation PA-RISC has. *sigh*.
+   We don't explicitly expose that "*a" may be written as reload
+   fails to find a register in class R1_REGS when "a" needs to be
+   reloaded when generating 64-bit PIC code.  Instead, we clobber
+   memory to indicate to the compiler that the assembly code reads
+   or writes to items other than those listed in the input and output
+   operands.  This may pessimize the code somewhat but __ldcw is
+   usually used within code blocks surrounded by memory barriors.  */
 #define __ldcw(a) ({						\
 	unsigned __ret;						\
-	__asm__ __volatile__(__LDCW " 0(%2),%0"			\
-		: "=r" (__ret), "+m" (*(a)) : "r" (a));		\
+	__asm__ __volatile__(__LDCW " 0(%1),%0"			\
+		: "=r" (__ret) : "r" (a) : "memory");		\
 	__ret;							\
 })

--
John David Anglin [email protected]

Helge Deller

author maintainer

2014-12-15 20:58 UTC #4 lore raw

Hi Dave,

Quoted 10 lines · John David Anglin

Quoted 7 lines · Helge Deller
Sometimes we face this compiler error:
arch/parisc/include/asm/ldcw.h:39:2: error: can't find a register in class 'R1_REGS' while reloading 'asm'
__asm__ __volatile__(__LDCW " 0(%2),%0"...
note: in expansion of macro '__ldcw'
error: 'asm' operand has impossible constraints

Quoted 2 lines

Helge, could you test the attached patch. Updated comment in change explains the "memory" clobber. It's been lightly tested.

Your patch seems to do the right thing, and it's nicer.
I didn't faced any compiler issues with it, but still want to test it some more, but I think that's the best approach. Do you mind sending a full patch to the list ?

Thanks,
Helge

A message that is not in the archive (sent off-list or not yet received): [email protected]

John David Anglin

2014-12-15 21:45 UTC #7 lore raw

Quoted 10 lines · Helge Deller

Hi Dave,
Quoted 3 lines · John David Anglin
Quoted 1 line · Helge Deller
Do you mind sending a full patch to the list ?
Thought I did. Was something missing?
A nice changelog and title for the patch...
I could copy over your comments from that patch, but maybe you want to rephrase it, so that it actually comes from you. You can send me the wording here in mail as well, then I'll put it together.

I think the email subject is fine for the title.

For the comment:

The __ldcw macro has a problem when its argument needs to be reloaded from memory. The output memory operand and the input register operand both need to be reloaded using a register in class R1_REGS when generating 64-bit code. This fails because there's only a single register in the class. Instead, use a memory clobber. This also makes the __ldcw macro a compiler memory barrier.

Dave

Signature · 1 line

John David Anglin    [email protected]