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

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

[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 <deller@gmx.de>
Cc: John David Anglin <dave.anglin@bell.net>

diff --git a/arch/parisc/include/asm/ldcw.h b/arch/parisc/include/asm/ldcw.h
index d2d11b7..b951e01 100644
--- a/arch/parisc/include/asm/ldcw.h
+++ b/arch/parisc/include/asm/ldcw.h
@@ -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")))

Re: [PATCH] parisc: fix out-of-register compiler error in ldcw inline assembler function

[John David Anglin]

On 10/21/2014 3:46 PM, Helge Deller wrote:
>   /* 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;
> +}
You could keep the old macro version for 32-bit builds as the problem 
shouldn't occur there.

Dave

-- 
John David Anglin    dave.anglin@bell.net

Re: [PATCH] parisc: fix out-of-register compiler error in ldcw inline assembler function

[Helge Deller]

[-- Attachment #1: Type: text/plain, Size: 978 bytes --]

On 10/21/2014 10:01 PM, John David Anglin wrote:
> On 10/21/2014 3:46 PM, Helge Deller wrote:
>>   /* 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;
>> +}
> 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
  


[-- Attachment #2: ldcw2.patch --]
[-- Type: text/x-patch, Size: 1317 bytes --]

diff --git a/arch/parisc/include/asm/ldcw.h b/arch/parisc/include/asm/ldcw.h
index d2d11b7..989595d 100644
--- a/arch/parisc/include/asm/ldcw.h
+++ b/arch/parisc/include/asm/ldcw.h
@@ -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")))

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

[John David Anglin]

[-- Attachment #1: Type: text/plain, Size: 1891 bytes --]

On 21-Oct-14, at 3:46 PM, Helge Deller wrote:

> 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 <deller@gmx.de>
> Cc: John David Anglin <dave.anglin@bell.net>
>
> diff --git a/arch/parisc/include/asm/ldcw.h b/arch/parisc/include/ 
> asm/ldcw.h
> index d2d11b7..b951e01 100644
> --- a/arch/parisc/include/asm/ldcw.h
> +++ b/arch/parisc/include/asm/ldcw.h
> @@ -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")))
>


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 <dave.anglin@bell.net>
---



[-- Attachment #2: ldcw.h.d.txt --]
[-- Type: text/plain, Size: 1105 bytes --]

diff --git a/arch/parisc/include/asm/ldcw.h b/arch/parisc/include/asm/ldcw.h
index d2d11b7..8121aa6 100644
--- a/arch/parisc/include/asm/ldcw.h
+++ b/arch/parisc/include/asm/ldcw.h
@@ -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;							\
 })
 

[-- Attachment #3: Type: text/plain, Size: 46 bytes --]



--
John David Anglin	dave.anglin@bell.net

Re: [PATCH] parisc: fix out-of-register compiler error in ldcw inline assembler function

[Helge Deller]

Hi Dave,

On 12/14/2014 04:49 PM, John David Anglin wrote:
> On 21-Oct-14, at 3:46 PM, Helge Deller wrote:
>> 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
>>
>
>
> 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

Re: [PATCH] parisc: fix out-of-register compiler error in ldcw inline assembler function

[John David Anglin]

On 12/15/2014 4:27 PM, Helge Deller wrote:
> Hi Dave,
>
> On 12/15/2014 10:25 PM, John David Anglin wrote:
>> On 12/15/2014 3:58 PM, Helge Deller wrote:
>>> 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

-- 
John David Anglin    dave.anglin@bell.net