From: Jan-Simon Möller <[email protected]>
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 <[email protected]>
CC: [email protected]
CC: [email protected]
CC: [email protected]
---
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
On 08/29/2013 10:13 AM, [email protected] wrote:
> From: Jan-Simon Möller <[email protected]>
>
> 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
On Thursday 29 August 2013 10:28:52 H. Peter Anvin wrote:
> On 08/29/2013 10:13 AM, [email protected] wrote:
> > From: Jan-Simon M?ller <[email protected]>
> >
> > 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
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
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
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
> 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 ?
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
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
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
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: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
From: Jan-Simon Möller <[email protected]>
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 <[email protected]>
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
---
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
From: Jan-Simon Möller <[email protected]>
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 <[email protected]>
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
---
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
Commit-ID: 3e9b2327b59801e677a7581fe4d2541ca749dcab
Gitweb: http://git.kernel.org/tip/3e9b2327b59801e677a7581fe4d2541ca749dcab
Author: Jan-Simon Möller <[email protected]>
AuthorDate: Thu, 29 Aug 2013 21:13:04 +0200
Committer: H. Peter Anvin <[email protected]>
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 <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: H. Peter Anvin <[email protected]>
---
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)
Commit-ID: bdfc017eead9bc17cd23317ff42eb7297cb9468a
Gitweb: http://git.kernel.org/tip/bdfc017eead9bc17cd23317ff42eb7297cb9468a
Author: Jan-Simon Möller <[email protected]>
AuthorDate: Thu, 29 Aug 2013 21:13:05 +0200
Committer: H. Peter Anvin <[email protected]>
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 <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: H. Peter Anvin <[email protected]>
---
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" \
Commit-ID: f69fa9a91f60fff6f2d8b658b7d84d235d9d89b7
Gitweb: http://git.kernel.org/tip/f69fa9a91f60fff6f2d8b658b7d84d235d9d89b7
Author: H. Peter Anvin <[email protected]>
AuthorDate: Thu, 29 Aug 2013 13:34:50 -0700
Committer: H. Peter Anvin <[email protected]>
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/[email protected]
Signed-off-by: H. Peter Anvin <[email protected]>
Cc: Jan-Simon Möller <[email protected]>
---
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) \
({ \