[PATCH] [RFC] [X86] Fix a compilation issue with clang.

[PATCH] [RFC] [X86] Fix a compilation issue with clang.

[dl9pf]

From: Jan-Simon Möller <dl9pf@gmx.de>

Clang does not support the "shortcut" we're taking here for gcc (see below).
The patch extends and uses the macro _ASM_DX to do the job.

>From arch/x86/include/asm/uaccess.h:
/*
 * Careful: we have to cast the result to the type of the pointer
 * for sign reasons.
 *
 * The use of %edx as the register specifier is a bit of a
 * simplification, as gcc only cares about it as the starting point
 * and not size: for a 64-bit value it will use %ecx:%edx on 32 bits
 * (%ecx being the next register in gcc's x86 register sequence), and
 * %rdx on 64 bits.
 */

Signed-off-by: Jan-Simon Möller <dl9pf@gmx.de>
CC: pageexec@freemail.hu
CC: llvmlinux@lists.linuxfoundation.org
CC: behanw@converseincode.com
---
 arch/x86/include/asm/asm.h     | 6 +++++-
 arch/x86/include/asm/uaccess.h | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index 1c2d247..4582e8e 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -3,21 +3,25 @@
 
 #ifdef __ASSEMBLY__
 # define __ASM_FORM(x)	x
+# define __ASM_FORM_RAW(x)     x
 # define __ASM_FORM_COMMA(x) x,
 #else
 # define __ASM_FORM(x)	" " #x " "
+# define __ASM_FORM_RAW(x)     #x
 # define __ASM_FORM_COMMA(x) " " #x ","
 #endif
 
 #ifdef CONFIG_X86_32
 # define __ASM_SEL(a,b) __ASM_FORM(a)
+# define __ASM_SEL_RAW(a,b) __ASM_FORM_RAW(a)
 #else
 # define __ASM_SEL(a,b) __ASM_FORM(b)
+# define __ASM_SEL_RAW(a,b) __ASM_FORM_RAW(b)
 #endif
 
 #define __ASM_SIZE(inst, ...)	__ASM_SEL(inst##l##__VA_ARGS__, \
 					  inst##q##__VA_ARGS__)
-#define __ASM_REG(reg)		__ASM_SEL(e##reg, r##reg)
+#define __ASM_REG(reg)         __ASM_SEL_RAW(e##reg, r##reg)
 
 #define _ASM_PTR	__ASM_SEL(.long, .quad)
 #define _ASM_ALIGN	__ASM_SEL(.balign 4, .balign 8)
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 8fa3bd6..32432d1 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -163,7 +163,7 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
 #define get_user(x, ptr)						\
 ({									\
 	int __ret_gu;							\
-	register __inttype(*(ptr)) __val_gu asm("%edx");		\
+	register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX);		\
 	__chk_user_ptr(ptr);						\
 	might_fault();							\
 	asm volatile("call __get_user_%P3"				\
-- 
1.8.1.4

Re: [PATCH] [RFC] [X86] Fix a compilation issue with clang.

[H. Peter Anvin]

On 08/29/2013 10:13 AM, dl9pf@gmx.de wrote:
> From: Jan-Simon Möller <dl9pf@gmx.de>
> 
> Clang does not support the "shortcut" we're taking here for gcc (see below).

Read: clang is gratuitously incompatible with gcc.

> The patch extends and uses the macro _ASM_DX to do the job.

You just changed the sematics of the _ASM_* macros... that doesn't seem
like a great idea.  It's probably okay for the registers, but still...

	-hpa

Re: [llvmlinux] [PATCH] [RFC] [X86] Fix a compilation issue with clang.

[Jan-Simon Möller]

On Thursday 29 August 2013 10:28:52 H. Peter Anvin wrote:
> On 08/29/2013 10:13 AM, dl9pf@gmx.de wrote:
> > From: Jan-Simon Möller <dl9pf@gmx.de>
> > 
> > Clang does not support the "shortcut" we're taking here for gcc (see
> > below).
> Read: clang is gratuitously incompatible with gcc.

Let's discuss this during the llvm track @ plumbers . 
 
> > The patch extends and uses the macro _ASM_DX to do the job.
> 
> You just changed the sematics of the _ASM_* macros... that doesn't seem
> like a great idea.  It's probably okay for the registers, but still...
> 

Well, we followed your idea for the _ASM_DX macros as you mentioned them 
earlier.

I tested this change for both clang-3.3 and gcc-4.7.2 . 

Best,
JS

Re: [llvmlinux] [PATCH] [RFC] [X86] Fix a compilation issue with clang.

[H. Peter Anvin]

On 08/29/2013 11:00 AM, Jan-Simon Möller wrote:
>>
>> You just changed the sematics of the _ASM_* macros... that doesn't seem
>> like a great idea.  It's probably okay for the registers, but still...
> 
> Well, we followed your idea for the _ASM_DX macros as you mentioned them 
> earlier.
> 

We can probably do this, because register names are usually not assumed
to have spaces around them... so it's probably okay.  Definitely a lot
less ugly than the rest.  That being said, if we're going to fix these
macros, we should also fix another piece of drain bramage:

ALL instances of these have a % prepended.  The % should be part of the
macro.

	-hpa

Re: [llvmlinux] [PATCH] [RFC] [X86] Fix a compilation issue with clang.

[Jan-Simon Möller]

On Thursday 29 August 2013 11:11:32 H. Peter Anvin wrote:
> On 08/29/2013 11:00 AM, Jan-Simon Möller wrote:
> >> You just changed the sematics of the _ASM_* macros... that doesn't seem
> >> like a great idea.  It's probably okay for the registers, but still...
> > 
> > Well, we followed your idea for the _ASM_DX macros as you mentioned them
> > earlier.
> 
> We can probably do this, because register names are usually not assumed
> to have spaces around them... so it's probably okay.  Definitely a lot
> less ugly than the rest.  That being said, if we're going to fix these
> macros, we should also fix another piece of drain bramage:
> 
> ALL instances of these have a % prepended.  The % should be part of the
> macro.

Ok, I can make a follow-up patch for the above change. Give me a bit to cook 
it up.

Best,
JS

Re: [llvmlinux] [PATCH] [RFC] [X86] Fix a compilation issue with clang.

[H. Peter Anvin]

On 08/29/2013 11:11 AM, Jan-Simon Möller wrote:
> On Thursday 29 August 2013 11:11:32 H. Peter Anvin wrote:
>> On 08/29/2013 11:00 AM, Jan-Simon Möller wrote:
>>>> You just changed the sematics of the _ASM_* macros... that doesn't seem
>>>> like a great idea.  It's probably okay for the registers, but still...
>>>
>>> Well, we followed your idea for the _ASM_DX macros as you mentioned them
>>> earlier.
>>
>> We can probably do this, because register names are usually not assumed
>> to have spaces around them... so it's probably okay.  Definitely a lot
>> less ugly than the rest.  That being said, if we're going to fix these
>> macros, we should also fix another piece of drain bramage:
>>
>> ALL instances of these have a % prepended.  The % should be part of the
>> macro.
> 
> Ok, I can make a follow-up patch for the above change. Give me a bit to cook 
> it up.
> 

Actually, it might be a bad idea.  In C embedded assembly we sometimes
need %% and sometimes %.

	-hpa

Re: [llvmlinux] [PATCH] [RFC] [X86] Fix a compilation issue with clang.

[Jan-Simon Möller]

> Actually, it might be a bad idea.  In C embedded assembly we sometimes
> need %% and sometimes %.

You speak of arch/x86/kvm/vmx.c, right ?

Re: [llvmlinux] [PATCH] [RFC] [X86] Fix a compilation issue with clang.

[H. Peter Anvin]

On 08/29/2013 11:23 AM, Jan-Simon Möller wrote:
> 
>> Actually, it might be a bad idea.  In C embedded assembly we sometimes
>> need %% and sometimes %.
> 
> You speak of arch/x86/kvm/vmx.c, right ?
> 

Well, in general.

	-hpa

Re: [llvmlinux] [PATCH] [RFC] [X86] Fix a compilation issue with clang.

[Jan-Simon Möller]

On Thursday 29 August 2013 11:31:44 H. Peter Anvin wrote:
> On 08/29/2013 11:23 AM, Jan-Simon Möller wrote:
> >> Actually, it might be a bad idea.  In C embedded assembly we sometimes
> >> need %% and sometimes %.
> > 
> > You speak of arch/x86/kvm/vmx.c, right ?
> 
> Well, in general.

Ok, I see.  So back to the patch - can we merge it then ?

Best,
JS

Re: [llvmlinux] [PATCH] [RFC] [X86] Fix a compilation issue with clang.

[H. Peter Anvin]

On 08/29/2013 11:36 AM, Jan-Simon Möller wrote:
> On Thursday 29 August 2013 11:31:44 H. Peter Anvin wrote:
>> On 08/29/2013 11:23 AM, Jan-Simon Möller wrote:
>>>> Actually, it might be a bad idea.  In C embedded assembly we sometimes
>>>> need %% and sometimes %.
>>>
>>> You speak of arch/x86/kvm/vmx.c, right ?
>>
>> Well, in general.
> 
> Ok, I see.  So back to the patch - can we merge it then ?
> 

Please split it in two: one for the changes to the _ASM_* macros (and
explain why it matters) and one for the actual change.

	-hpa

Re: [llvmlinux] [PATCH] [RFC] [X86] Fix a compilation issue with clang.

[dl9pf]

These to patches fix a compilation issues with clang.

[PATCH 1/2] Extend definitions of __ASM_* with a raw format.

Extends the helper macros __ASM_*  (e.g. __ASM_DX) with a format that 
does not add whitespace. Both gcc and clang choke in that case.

[PATCH 2/2] [X86] Fix a compilation issue with clang.

This is the actual issue here. In the 64bit case, clang just sees
edx (a 32bit register name) and does not use more than 32bit.
As pointed out in the comment in uaccess.h, gcc is happy with edx as the
starting point (and does not care about the size).
This fix pleases both worlds as we just use edx on 32 and rdx on 64.

Please apply.

CC: pageexec@freemail.hu
CC: llvmlinux@lists.linuxfoundation.org
CC: behanw@converseincode.com
CC: hpa@zytor.com
CC: linux-kernel@vger.kernel.org

[PATCH 1/2] Extend definitions of _ASM_* with a raw format.

[dl9pf]

From: Jan-Simon Möller <dl9pf@gmx.de>

The __ASM_* macros (e.g. __ASM_DX) are used to return the proper
register name (e.g. edx for 32bit / rdx for 64bit). We want to use this
also in arch/x86/include/asm/uaccess.h / get_user() .
For this to work, we need a raw form as both gcc and clang choke on the
whitespace introduced by __ASM_FORM. Thus __ASM_FORM_RAW was added.
We change __ASM_REG to use the new RAW form.

Signed-off-by: Jan-Simon Möller <dl9pf@gmx.de>
CC: pageexec@freemail.hu
CC: llvmlinux@lists.linuxfoundation.org
CC: behanw@converseincode.com
CC: hpa@zytor.com
CC: linux-kernel@vger.kernel.org
---
 arch/x86/include/asm/asm.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index 1c2d247..4582e8e 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -3,21 +3,25 @@
 
 #ifdef __ASSEMBLY__
 # define __ASM_FORM(x)	x
+# define __ASM_FORM_RAW(x)     x
 # define __ASM_FORM_COMMA(x) x,
 #else
 # define __ASM_FORM(x)	" " #x " "
+# define __ASM_FORM_RAW(x)     #x
 # define __ASM_FORM_COMMA(x) " " #x ","
 #endif
 
 #ifdef CONFIG_X86_32
 # define __ASM_SEL(a,b) __ASM_FORM(a)
+# define __ASM_SEL_RAW(a,b) __ASM_FORM_RAW(a)
 #else
 # define __ASM_SEL(a,b) __ASM_FORM(b)
+# define __ASM_SEL_RAW(a,b) __ASM_FORM_RAW(b)
 #endif
 
 #define __ASM_SIZE(inst, ...)	__ASM_SEL(inst##l##__VA_ARGS__, \
 					  inst##q##__VA_ARGS__)
-#define __ASM_REG(reg)		__ASM_SEL(e##reg, r##reg)
+#define __ASM_REG(reg)         __ASM_SEL_RAW(e##reg, r##reg)
 
 #define _ASM_PTR	__ASM_SEL(.long, .quad)
 #define _ASM_ALIGN	__ASM_SEL(.balign 4, .balign 8)
-- 
1.8.1.4

[tip:x86/asm] x86, asm: Extend definitions of _ASM_* with a raw format

[tip-bot for Jan-Simon Möller]

Commit-ID:  3e9b2327b59801e677a7581fe4d2541ca749dcab
Gitweb:     http://git.kernel.org/tip/3e9b2327b59801e677a7581fe4d2541ca749dcab
Author:     Jan-Simon Möller <dl9pf@gmx.de>
AuthorDate: Thu, 29 Aug 2013 21:13:04 +0200
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Thu, 29 Aug 2013 13:26:32 -0700

x86, asm: Extend definitions of _ASM_* with a raw format

The __ASM_* macros (e.g. __ASM_DX) are used to return the proper
register name (e.g. edx for 32bit / rdx for 64bit). We want to use
this also in arch/x86/include/asm/uaccess.h / get_user() .  For this
to work, we need a raw form as both gcc and clang choke on the
whitespace in a register asm() statement, and the __ASM_FORM macro
surrounds the argument with blanks.  A new macro, __ASM_FORM_RAW was
added and we change __ASM_REG to use the new RAW form.

Signed-off-by: Jan-Simon Möller <dl9pf@gmx.de>
Link: http://lkml.kernel.org/r/1377803585-5913-2-git-send-email-dl9pf@gmx.de
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/include/asm/asm.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index 1c2d247..4582e8e 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -3,21 +3,25 @@
 
 #ifdef __ASSEMBLY__
 # define __ASM_FORM(x)	x
+# define __ASM_FORM_RAW(x)     x
 # define __ASM_FORM_COMMA(x) x,
 #else
 # define __ASM_FORM(x)	" " #x " "
+# define __ASM_FORM_RAW(x)     #x
 # define __ASM_FORM_COMMA(x) " " #x ","
 #endif
 
 #ifdef CONFIG_X86_32
 # define __ASM_SEL(a,b) __ASM_FORM(a)
+# define __ASM_SEL_RAW(a,b) __ASM_FORM_RAW(a)
 #else
 # define __ASM_SEL(a,b) __ASM_FORM(b)
+# define __ASM_SEL_RAW(a,b) __ASM_FORM_RAW(b)
 #endif
 
 #define __ASM_SIZE(inst, ...)	__ASM_SEL(inst##l##__VA_ARGS__, \
 					  inst##q##__VA_ARGS__)
-#define __ASM_REG(reg)		__ASM_SEL(e##reg, r##reg)
+#define __ASM_REG(reg)         __ASM_SEL_RAW(e##reg, r##reg)
 
 #define _ASM_PTR	__ASM_SEL(.long, .quad)
 #define _ASM_ALIGN	__ASM_SEL(.balign 4, .balign 8)

[PATCH 2/2] [X86] Fix a compilation issue with clang.

[dl9pf]

From: Jan-Simon Möller <dl9pf@gmx.de>

Clang does not support the "shortcut" we're taking here for gcc (see below).
The patch uses the macro _ASM_DX to do the job.

>From arch/x86/include/asm/uaccess.h:
/*
 * Careful: we have to cast the result to the type of the pointer
 * for sign reasons.
 *
 * The use of %edx as the register specifier is a bit of a
 * simplification, as gcc only cares about it as the starting point
 * and not size: for a 64-bit value it will use %ecx:%edx on 32 bits
 * (%ecx being the next register in gcc's x86 register sequence), and
 * %rdx on 64 bits.
 */

Signed-off-by: Jan-Simon Möller <dl9pf@gmx.de>
CC: pageexec@freemail.hu
CC: llvmlinux@lists.linuxfoundation.org
CC: behanw@converseincode.com
CC: hpa@zytor.com
CC: linux-kernel@vger.kernel.org
---
 arch/x86/include/asm/uaccess.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 8fa3bd6..32432d1 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -163,7 +163,7 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
 #define get_user(x, ptr)						\
 ({									\
 	int __ret_gu;							\
-	register __inttype(*(ptr)) __val_gu asm("%edx");		\
+	register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX);		\
 	__chk_user_ptr(ptr);						\
 	might_fault();							\
 	asm volatile("call __get_user_%P3"				\
-- 
1.8.1.4

[tip:x86/asm] x86, asm: Fix a compilation issue with clang

[tip-bot for Jan-Simon Möller]

Commit-ID:  bdfc017eead9bc17cd23317ff42eb7297cb9468a
Gitweb:     http://git.kernel.org/tip/bdfc017eead9bc17cd23317ff42eb7297cb9468a
Author:     Jan-Simon Möller <dl9pf@gmx.de>
AuthorDate: Thu, 29 Aug 2013 21:13:05 +0200
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Thu, 29 Aug 2013 13:26:33 -0700

x86, asm: Fix a compilation issue with clang

Clang does not support the "shortcut" we're taking here for gcc (see below).
The patch uses the macro _ASM_DX to do the job.

>From arch/x86/include/asm/uaccess.h:
/*
 * Careful: we have to cast the result to the type of the pointer
 * for sign reasons.
 *
 * The use of %edx as the register specifier is a bit of a
 * simplification, as gcc only cares about it as the starting point
 * and not size: for a 64-bit value it will use %ecx:%edx on 32 bits
 * (%ecx being the next register in gcc's x86 register sequence), and
 * %rdx on 64 bits.
 */

[ hpa: I consider this a compatibility bug in clang as this reflects a
  bit of a misunderstanding about how register strings are used by
  gcc, but the workaround is straightforward and there is no
  particular reason to not do it. ]

Signed-off-by: Jan-Simon Möller <dl9pf@gmx.de>
Link: http://lkml.kernel.org/r/1377803585-5913-3-git-send-email-dl9pf@gmx.de
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/include/asm/uaccess.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 5ee2687..f715fee 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -162,7 +162,7 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
 #define get_user(x, ptr)						\
 ({									\
 	int __ret_gu;							\
-	register __inttype(*(ptr)) __val_gu asm("%edx");		\
+	register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX);		\
 	__chk_user_ptr(ptr);						\
 	might_fault();							\
 	asm volatile("call __get_user_%P3"				\

[tip:x86/asm] x86, doc: Update uaccess.h comment to reflect clang changes

[tip-bot for H. Peter Anvin]

Commit-ID:  f69fa9a91f60fff6f2d8b658b7d84d235d9d89b7
Gitweb:     http://git.kernel.org/tip/f69fa9a91f60fff6f2d8b658b7d84d235d9d89b7
Author:     H. Peter Anvin <hpa@linux.intel.com>
AuthorDate: Thu, 29 Aug 2013 13:34:50 -0700
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Thu, 29 Aug 2013 13:34:50 -0700

x86, doc: Update uaccess.h comment to reflect clang changes

Update comment in uaccess.h to reflect the changes for clang support:
gcc only cares about the base register (most architectures don't
encode the size of the operation in the operands like x86 does, and so
it is treated effectively like a register number), whereas clang tries
to enforce the size -- but not for register pairs.

Link: http://lkml.kernel.org/r/1377803585-5913-3-git-send-email-dl9pf@gmx.de
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
Cc: Jan-Simon Möller <dl9pf@gmx.de>
---
 arch/x86/include/asm/uaccess.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index f715fee..5838fa9 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -153,11 +153,14 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
  * Careful: we have to cast the result to the type of the pointer
  * for sign reasons.
  *
- * The use of %edx as the register specifier is a bit of a
+ * The use of _ASM_DX as the register specifier is a bit of a
  * simplification, as gcc only cares about it as the starting point
  * and not size: for a 64-bit value it will use %ecx:%edx on 32 bits
  * (%ecx being the next register in gcc's x86 register sequence), and
  * %rdx on 64 bits.
+ *
+ * Clang/LLVM cares about the size of the register, but still wants
+ * the base register for something that ends up being a pair.
  */
 #define get_user(x, ptr)						\
 ({									\