Re: [PATCH] x86: fix inline assembly constraints

Re: [PATCH] x86: fix inline assembly constraints

[Robert Hancock]

Jike Song wrote:
> On Thu, Oct 30, 2008 at 12:34 AM, Ingo Molnar <mingo@elte.hu> wrote:
>> * Jike Song <albcamus@gmail.com> wrote:
>>> --
>> that was an easy patch to act upon ;-)
>>
>>        Ingo
>>
> Sorry Ingo, I missed your point. Do you mean this patch is trivial or
> unnecessary?

Your first message was empty.

Re: [PATCH] x86: fix inline assembly constraints

[Jike Song]

Ingo Molnar wrote:
>
>  nah, it was a joke - you sent two mails, one of them was empty ;-)
>
>  Ingo

Yeah, I had problems with git-send-email, it always send two emails(the
first being empty) if I specify --compose ;-) Will take care next time;)

Jike

Re: [PATCH] x86: fix inline assembly constraints

[Ingo Molnar]

* Jike Song <albcamus@gmail.com> wrote:

> On Thu, Oct 30, 2008 at 12:34 AM, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Jike Song <albcamus@gmail.com> wrote:
> >>
> >> --
> >
> > that was an easy patch to act upon ;-)
> >
> >        Ingo
> >
> Sorry Ingo, I missed your point. Do you mean this patch is trivial or
> unnecessary?

nah, it was a joke - you sent two mails, one of them was empty ;-)

	Ingo

Re: [PATCH] x86: fix inline assembly constraints

[H. Peter Anvin]

Jike Song wrote:
> 
> Yes, sometimes gcc did have bugs with its obscure inline asm
> conventions. But I think the change of x86-64 atomic operations should
> be OK. Anyway, the "+" constraint is more clear than a "=m" output and
>  a "m" input.
> 
> The 32-bit atomic ops were already changed to "+m".(commit
> b862f3b099f3ea672c7438c0b282ce8201d39dfc)
> 

You *THINK*.  It's very easy to *THINK* that gcc won't do something
utterly moronic, and you'd be wrong.

Just changing it for the sake of churn is pointless... if there is a
bug, then we have to take the risk anyway, but if it is already correct,
then there is no point in provoking a bug.  Not *your* bug, because your
code is correct, but gcc's bug.

FWIW, the reason that code doesn't use "+m" is because a version of gcc
which we no longer support didn't handle it.  That by itself isn't a
reason to keep it, but there is also no reason to just "tidy" it, IMNSHO.

	-hpa

Re: [PATCH] x86: fix inline assembly constraints

[Jike Song]

On Thu, Oct 30, 2008 at 11:54 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> Jike Song wrote:
>>
>> Besides, by looking at the inline assembly in kernel,  I found lots of
>> codes like this:
>>
>>         static inline void atomic_add(int i, atomic_t *v)
>>         {
>>               asm volatile(LOCK_PREFIX "addl %1,%0"
>>                            : "=m" (v->counter)
>>                            : "ir" (i), "m" (v->counter));
>>         }
>>
>>
>> Yes, it works. But a little ugly..  Should this be cleaned-up with the
>> following?
>>
>>         : "+m" (v->counter)
>>         : "ir" (i)
>>
>> If you agrees, I'll send out the patch; otherwise I won't wasting your time ;-)
>>
>
> Please don't change them just to change them, if there is no actual
> error.  You never know when you're going to trigger a new bug in some
> weird version of gcc.
>
>        -hpa

Yes, sometimes gcc did have bugs with its obscure inline asm
conventions. But I think the change of x86-64 atomic operations should
be OK. Anyway, the "+" constraint is more clear than a "=m" output and
 a "m" input.

The 32-bit atomic ops were already changed to "+m".(commit
b862f3b099f3ea672c7438c0b282ce8201d39dfc)

-- 
Thanks,
Jike

Re: [PATCH] x86: fix inline assembly constraints

[H. Peter Anvin]

Jike Song wrote:
> 
> Besides, by looking at the inline assembly in kernel,  I found lots of
> codes like this:
> 
>         static inline void atomic_add(int i, atomic_t *v)
>         {
> 	        asm volatile(LOCK_PREFIX "addl %1,%0"
> 		             : "=m" (v->counter)
> 		             : "ir" (i), "m" (v->counter));
>         }
> 
> 
> Yes, it works. But a little ugly..  Should this be cleaned-up with the
> following?
> 
>         : "+m" (v->counter)
>         : "ir" (i)
> 
> If you agrees, I'll send out the patch; otherwise I won't wasting your time ;-)
> 

Please don't change them just to change them, if there is no actual
error.  You never know when you're going to trigger a new bug in some
weird version of gcc.

	-hpa

Re: [PATCH] x86: fix inline assembly constraints

[Jike Song]

On Thu, Oct 30, 2008 at 12:34 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Jike Song <albcamus@gmail.com> wrote:
>>
>> --
>
> that was an easy patch to act upon ;-)
>
>        Ingo
>
Sorry Ingo, I missed your point. Do you mean this patch is trivial or
unnecessary?

Besides, by looking at the inline assembly in kernel,  I found lots of
codes like this:

        static inline void atomic_add(int i, atomic_t *v)
        {
	        asm volatile(LOCK_PREFIX "addl %1,%0"
		             : "=m" (v->counter)
		             : "ir" (i), "m" (v->counter));
        }


Yes, it works. But a little ugly..  Should this be cleaned-up with the
following?

        : "+m" (v->counter)
        : "ir" (i)

If you agrees, I'll send out the patch; otherwise I won't wasting your time ;-)


-- 
Thanks,
Jike

Re: [PATCH] x86: fix inline assembly constraints

[Ingo Molnar]

* Jike Song <albcamus@gmail.com> wrote:

> 
> --

that was an easy patch to act upon ;-)

	Ingo

[PATCH] x86: fix inline assembly constraints

[Jike Song]

In atomic_set_mask, *addr should be both read and written.

Signed-off-by: Jike Song <albcamus@gmail.com>
---
 arch/x86/include/asm/atomic_32.h |    8 ++++----
 arch/x86/include/asm/atomic_64.h |    7 ++++---
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/atomic_32.h b/arch/x86/include/asm/atomic_32.h
index ad5b9f6..23a7c7f 100644
--- a/arch/x86/include/asm/atomic_32.h
+++ b/arch/x86/include/asm/atomic_32.h
@@ -242,12 +242,12 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u)
 
 /* These are x86-specific, used by some header files */
 #define atomic_clear_mask(mask, addr)				\
-	asm volatile(LOCK_PREFIX "andl %0,%1"			\
-		     : : "r" (~(mask)), "m" (*(addr)) : "memory")
+	asm volatile(LOCK_PREFIX "andl %1, %0"			\
+		     : "+m" (*(addr)) : "r" (~(mask)) : "memory")
 
 #define atomic_set_mask(mask, addr)				\
-	asm volatile(LOCK_PREFIX "orl %0,%1"				\
-		     : : "r" (mask), "m" (*(addr)) : "memory")
+	asm volatile(LOCK_PREFIX "orl %1, %0"			\
+		     : "+m" (*(addr)) : "r" (mask) : "memory")
 
 /* Atomic operations are already serializing on x86 */
 #define smp_mb__before_atomic_dec()	barrier()
diff --git a/arch/x86/include/asm/atomic_64.h b/arch/x86/include/asm/atomic_64.h
index fa59212..31b34f3 100644
--- a/arch/x86/include/asm/atomic_64.h
+++ b/arch/x86/include/asm/atomic_64.h
@@ -451,12 +451,13 @@ static inline void atomic_or_long(unsigned long *v1, unsigned long v2)
 
 /* These are x86-specific, used by some header files */
 #define atomic_clear_mask(mask, addr)					\
-	asm volatile(LOCK_PREFIX "andl %0,%1"				\
-		     : : "r" (~(mask)), "m" (*(addr)) : "memory")
+	asm volatile(LOCK_PREFIX "andl %1, %0"				\
+		     : "+m" (*(addr)) : "r" (~(mask)) : "memory")
 
 #define atomic_set_mask(mask, addr)					\
-	asm volatile(LOCK_PREFIX "orl %0,%1"				\
+	asm volatile(LOCK_PREFIX "orl %1, %0"				\
 		     : : "r" ((unsigned)(mask)), "m" (*(addr))		\
+		     : "+m" (*(addr)) : "r" ((unsigned)(mask))		\
 		     : "memory")
 
 /* Atomic operations are already serializing on x86 */
-- 
1.6.0.1

[PATCH] x86: fix inline assembly constraints

[Jike Song]