When combined with 'MIPS: Changed current_thread_info() to an equivalent ...'
(http://www.linux-mips.org/archives/linux-mips/2015-01/msg00070.html), this
patch series makes it possible to compile a working kernel for MIPS using Clang.
The patches aren't inter-dependent so I'm happy to split this up into individual
submissions if that's preferred.
Daniel Sanders (1):
LLVMLinux: Correct size_index table before replacing the bootstrap
kmem_cache_node.
Toma Tabacu (4):
MIPS: LLVMLinux: Fix a 'cast to type not present in union' error.
MIPS: LLVMLinux: Fix an 'inline asm input/output type mismatch' error.
MIPS: LLVMLinux: Silence variable self-assignment warnings.
MIPS: LLVMLinux: Silence unicode warnings when preprocessing assembly.
arch/mips/include/asm/asmmacro.h | 8 ++++----
arch/mips/include/asm/checksum.h | 2 +-
arch/mips/kernel/branch.c | 6 ++++--
arch/mips/math-emu/dp_add.c | 5 -----
arch/mips/math-emu/dp_sub.c | 5 -----
arch/mips/math-emu/sp_add.c | 5 -----
arch/mips/math-emu/sp_sub.c | 5 -----
mm/slab.c | 1 +
mm/slab.h | 1 +
mm/slab_common.c | 36 +++++++++++++++++++++---------------
mm/slub.c | 1 +
11 files changed, 33 insertions(+), 42 deletions(-)
Signed-off-by: Toma Tabacu <[email protected]>
Signed-off-by: Daniel Sanders <[email protected]>
Cc: "Steven J. Hill" <[email protected]>
Cc: Andreas Herrmann <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Christoph Lameter <[email protected]>
Cc: David Daney <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: Jim Quinlan <[email protected]>
Cc: Joonsoo Kim <[email protected]>
Cc: Leonid Yegoshin <[email protected]>
Cc: Manuel Lauss <[email protected]>
Cc: Markos Chandras <[email protected]>
Cc: Paul Bolle <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: Pekka Enberg <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
--
2.1.4
This patch moves the initialization of the size_index table slightly
earlier so that the first few kmem_cache_node's can be safely allocated
when KMALLOC_MIN_SIZE is large.
There are currently two ways to generate indices into kmalloc_caches
(via kmalloc_index() and via the size_index table in slab_common.c)
and on some arches (possibly only MIPS) they potentially disagree with
each other until create_kmalloc_caches() has been called. It seems
that the intention is that the size_index table is a fast equivalent
to kmalloc_index() and that create_kmalloc_caches() patches the table
to return the correct value for the cases where kmalloc_index()'s
if-statements apply.
The failing sequence was:
* kmalloc_caches contains NULL elements
* kmem_cache_init initialises the element that 'struct
kmem_cache_node' will be allocated to. For 32-bit Mips, this is a
56-byte struct and kmalloc_index returns KMALLOC_SHIFT_LOW (7).
* init_list is called which calls kmalloc_node to allocate a 'struct
kmem_cache_node'.
* kmalloc_slab selects the kmem_caches element using
size_index[size_index_elem(size)]. For MIPS, size is 56, and the
expression returns 6.
* This element of kmalloc_caches is NULL and allocation fails.
* If it had not already failed, it would have called
create_kmalloc_caches() at this point which would have changed
size_index[size_index_elem(size)] to 7.
Signed-off-by: Daniel Sanders <[email protected]>
Cc: Christoph Lameter <[email protected]>
Cc: Pekka Enberg <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: Joonsoo Kim <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
mm/slab.c | 1 +
mm/slab.h | 1 +
mm/slab_common.c | 36 +++++++++++++++++++++---------------
mm/slub.c | 1 +
4 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/mm/slab.c b/mm/slab.c
index 65b5dcb..6c93f28 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1440,6 +1440,7 @@ void __init kmem_cache_init(void)
kmalloc_caches[INDEX_NODE] = create_kmalloc_cache("kmalloc-node",
kmalloc_size(INDEX_NODE), ARCH_KMALLOC_FLAGS);
slab_state = PARTIAL_NODE;
+ correct_kmalloc_cache_index_table();
slab_early_init = 0;
diff --git a/mm/slab.h b/mm/slab.h
index 1cf40054..036c08d 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -71,6 +71,7 @@ unsigned long calculate_alignment(unsigned long flags,
#ifndef CONFIG_SLOB
/* Kmalloc array related functions */
+void correct_kmalloc_cache_index_table(void);
void create_kmalloc_caches(unsigned long);
/* Find the kmalloc slab corresponding for a certain size */
diff --git a/mm/slab_common.c b/mm/slab_common.c
index e03dd6f..d2f7379 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -675,25 +675,20 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
}
/*
- * Create the kmalloc array. Some of the regular kmalloc arrays
- * may already have been created because they were needed to
- * enable allocations for slab creation.
+ * Patch up the size_index table if we have strange large alignment
+ * requirements for the kmalloc array. This is only the case for
+ * MIPS it seems. The standard arches will not generate any code here.
+ *
+ * Largest permitted alignment is 256 bytes due to the way we
+ * handle the index determination for the smaller caches.
+ *
+ * Make sure that nothing crazy happens if someone starts tinkering
+ * around with ARCH_KMALLOC_MINALIGN
*/
-void __init create_kmalloc_caches(unsigned long flags)
+void __init correct_kmalloc_cache_index_table(void)
{
int i;
- /*
- * Patch up the size_index table if we have strange large alignment
- * requirements for the kmalloc array. This is only the case for
- * MIPS it seems. The standard arches will not generate any code here.
- *
- * Largest permitted alignment is 256 bytes due to the way we
- * handle the index determination for the smaller caches.
- *
- * Make sure that nothing crazy happens if someone starts tinkering
- * around with ARCH_KMALLOC_MINALIGN
- */
BUILD_BUG_ON(KMALLOC_MIN_SIZE > 256 ||
(KMALLOC_MIN_SIZE & (KMALLOC_MIN_SIZE - 1)));
@@ -724,6 +719,17 @@ void __init create_kmalloc_caches(unsigned long flags)
for (i = 128 + 8; i <= 192; i += 8)
size_index[size_index_elem(i)] = 8;
}
+}
+
+/*
+ * Create the kmalloc array. Some of the regular kmalloc arrays
+ * may already have been created because they were needed to
+ * enable allocations for slab creation.
+ */
+void __init create_kmalloc_caches(unsigned long flags)
+{
+ int i;
+
for (i = KMALLOC_SHIFT_LOW; i <= KMALLOC_SHIFT_HIGH; i++) {
if (!kmalloc_caches[i]) {
kmalloc_caches[i] = create_kmalloc_cache(NULL,
diff --git a/mm/slub.c b/mm/slub.c
index fe376fe..2217761 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3604,6 +3604,7 @@ void __init kmem_cache_init(void)
kmem_cache_node = bootstrap(&boot_kmem_cache_node);
/* Now we can use the kmem_cache to allocate kmalloc slabs */
+ correct_kmalloc_cache_index_table();
create_kmalloc_caches(0);
#ifdef CONFIG_SMP
--
2.1.4
From: Toma Tabacu <[email protected]>
Remove a cast to the 'mips16e_instruction' union inside an if
condition and instead do an assignment to a local
'union mips16e_instruction' variable's 'full' member before the if
statement and use this variable in the if condition.
This is the error message reported by clang:
arch/mips/kernel/branch.c:38:8: error: cast to union type from type 'unsigned short' not present in union
if (((union mips16e_instruction)inst).ri.opcode
^ ~~~~
The changed code can be compiled successfully by both gcc and clang.
Signed-off-by: Toma Tabacu <[email protected]>
Signed-off-by: Daniel Sanders <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Andreas Herrmann <[email protected]>
Cc: David Daney <[email protected]>
Cc: Manuel Lauss <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
arch/mips/kernel/branch.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/mips/kernel/branch.c b/arch/mips/kernel/branch.c
index 4d7d99d..7110963 100644
--- a/arch/mips/kernel/branch.c
+++ b/arch/mips/kernel/branch.c
@@ -35,8 +35,10 @@ int __isa_exception_epc(struct pt_regs *regs)
return epc;
}
if (cpu_has_mips16) {
- if (((union mips16e_instruction)inst).ri.opcode
- == MIPS16e_jal_op)
+ union mips16e_instruction inst_mips16e;
+
+ inst_mips16e.full = inst;
+ if (inst_mips16e.ri.opcode == MIPS16e_jal_op)
epc += 4;
else
epc += 2;
--
2.1.4
From: Toma Tabacu <[email protected]>
Change the type of csum_ipv6_magic's 'proto' argument from unsigned
short to __u32.
This fixes a type mismatch between the 'htonl(proto)' inline asm
input, which is __u32, and the 'proto' output, which is unsigned
short.
This is the error message reported by clang:
arch/mips/include/asm/checksum.h:285:27: error: unsupported inline asm: input with type '__be32' (aka 'unsigned int') matching output with type 'unsigned short'
"0" (htonl(len)), "1" (htonl(proto)), "r" (sum));
^~~~~~~~~~~~
The changed code can be compiled successfully by both gcc and clang.
Signed-off-by: Toma Tabacu <[email protected]>
Signed-off-by: Daniel Sanders <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Markos Chandras <[email protected]>
Cc: Leonid Yegoshin <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
arch/mips/include/asm/checksum.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/include/asm/checksum.h b/arch/mips/include/asm/checksum.h
index 3418c51..683b9e7 100644
--- a/arch/mips/include/asm/checksum.h
+++ b/arch/mips/include/asm/checksum.h
@@ -225,7 +225,7 @@ static inline __sum16 ip_compute_csum(const void *buff, int len)
#define _HAVE_ARCH_IPV6_CSUM
static __inline__ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
const struct in6_addr *daddr,
- __u32 len, unsigned short proto,
+ __u32 len, __u32 proto,
__wsum sum)
{
__asm__(
--
2.1.4
From: Toma Tabacu <[email protected]>
Remove variable self-assignments.
This silences a bunch of -Wself-assign warnings reported by clang.
The changed code can be compiled without warnings by both gcc and clang.
Signed-off-by: Toma Tabacu <[email protected]>
Signed-off-by: Daniel Sanders <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
arch/mips/math-emu/dp_add.c | 5 -----
arch/mips/math-emu/dp_sub.c | 5 -----
arch/mips/math-emu/sp_add.c | 5 -----
arch/mips/math-emu/sp_sub.c | 5 -----
4 files changed, 20 deletions(-)
diff --git a/arch/mips/math-emu/dp_add.c b/arch/mips/math-emu/dp_add.c
index 7f64577..58b2795 100644
--- a/arch/mips/math-emu/dp_add.c
+++ b/arch/mips/math-emu/dp_add.c
@@ -150,8 +150,6 @@ union ieee754dp ieee754dp_add(union ieee754dp x, union ieee754dp y)
* leaving result in xm, xs and xe.
*/
xm = xm + ym;
- xe = xe;
- xs = xs;
if (xm >> (DP_FBITS + 1 + 3)) { /* carry out */
xm = XDPSRS1(xm);
@@ -160,11 +158,8 @@ union ieee754dp ieee754dp_add(union ieee754dp x, union ieee754dp y)
} else {
if (xm >= ym) {
xm = xm - ym;
- xe = xe;
- xs = xs;
} else {
xm = ym - xm;
- xe = xe;
xs = ys;
}
if (xm == 0)
diff --git a/arch/mips/math-emu/dp_sub.c b/arch/mips/math-emu/dp_sub.c
index 7a17402..2eb87cd2 100644
--- a/arch/mips/math-emu/dp_sub.c
+++ b/arch/mips/math-emu/dp_sub.c
@@ -153,8 +153,6 @@ union ieee754dp ieee754dp_sub(union ieee754dp x, union ieee754dp y)
/* generate 28 bit result of adding two 27 bit numbers
*/
xm = xm + ym;
- xe = xe;
- xs = xs;
if (xm >> (DP_FBITS + 1 + 3)) { /* carry out */
xm = XDPSRS1(xm); /* shift preserving sticky */
@@ -163,11 +161,8 @@ union ieee754dp ieee754dp_sub(union ieee754dp x, union ieee754dp y)
} else {
if (xm >= ym) {
xm = xm - ym;
- xe = xe;
- xs = xs;
} else {
xm = ym - xm;
- xe = xe;
xs = ys;
}
if (xm == 0) {
diff --git a/arch/mips/math-emu/sp_add.c b/arch/mips/math-emu/sp_add.c
index 2d84d46..7a33af4 100644
--- a/arch/mips/math-emu/sp_add.c
+++ b/arch/mips/math-emu/sp_add.c
@@ -148,8 +148,6 @@ union ieee754sp ieee754sp_add(union ieee754sp x, union ieee754sp y)
* leaving result in xm, xs and xe.
*/
xm = xm + ym;
- xe = xe;
- xs = xs;
if (xm >> (SP_FBITS + 1 + 3)) { /* carry out */
SPXSRSX1();
@@ -157,11 +155,8 @@ union ieee754sp ieee754sp_add(union ieee754sp x, union ieee754sp y)
} else {
if (xm >= ym) {
xm = xm - ym;
- xe = xe;
- xs = xs;
} else {
xm = ym - xm;
- xe = xe;
xs = ys;
}
if (xm == 0)
diff --git a/arch/mips/math-emu/sp_sub.c b/arch/mips/math-emu/sp_sub.c
index 8592e49..1189bc5 100644
--- a/arch/mips/math-emu/sp_sub.c
+++ b/arch/mips/math-emu/sp_sub.c
@@ -148,8 +148,6 @@ union ieee754sp ieee754sp_sub(union ieee754sp x, union ieee754sp y)
/* generate 28 bit result of adding two 27 bit numbers
*/
xm = xm + ym;
- xe = xe;
- xs = xs;
if (xm >> (SP_FBITS + 1 + 3)) { /* carry out */
SPXSRSX1(); /* shift preserving sticky */
@@ -157,11 +155,8 @@ union ieee754sp ieee754sp_sub(union ieee754sp x, union ieee754sp y)
} else {
if (xm >= ym) {
xm = xm - ym;
- xe = xe;
- xs = xs;
} else {
xm = ym - xm;
- xe = xe;
xs = ys;
}
if (xm == 0) {
--
2.1.4
From: Toma Tabacu <[email protected]>
Differentiate the 'u' GAS .macro argument from the '\u' C preprocessor unicode
escape sequence by renaming it to '_u'.
When the 'u' argument is evaluated, we end up writing '\u', which causes
clang to emit -Wunicode warnings.
This silences a couple of -Wunicode warnings reported by clang.
The changed code can be preprocessed without warnings by both gcc and clang.
Signed-off-by: Toma Tabacu <[email protected]>
Signed-off-by: Daniel Sanders <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: Paul Bolle <[email protected]>
Cc: "Steven J. Hill" <[email protected]>
Cc: Manuel Lauss <[email protected]>
Cc: Jim Quinlan <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
arch/mips/include/asm/asmmacro.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/mips/include/asm/asmmacro.h b/arch/mips/include/asm/asmmacro.h
index 6caf876..c657932 100644
--- a/arch/mips/include/asm/asmmacro.h
+++ b/arch/mips/include/asm/asmmacro.h
@@ -200,12 +200,12 @@
.word 0x41600021 | (\reg << 16)
.endm
- .macro MFTR rt=0, rd=0, u=0, sel=0
- .word 0x41000000 | (\rt << 16) | (\rd << 11) | (\u << 5) | (\sel)
+ .macro MFTR rt=0, rd=0, _u=0, sel=0
+ .word 0x41000000 | (\rt << 16) | (\rd << 11) | (\_u << 5) | (\sel)
.endm
- .macro MTTR rt=0, rd=0, u=0, sel=0
- .word 0x41800000 | (\rt << 16) | (\rd << 11) | (\u << 5) | (\sel)
+ .macro MTTR rt=0, rd=0, _u=0, sel=0
+ .word 0x41800000 | (\rt << 16) | (\rd << 11) | (\_u << 5) | (\sel)
.endm
#ifdef TOOLCHAIN_SUPPORTS_MSA
--
2.1.4
On Tue, 3 Feb 2015, Daniel Sanders wrote:
> +++ b/mm/slab.c
> @@ -1440,6 +1440,7 @@ void __init kmem_cache_init(void)
> kmalloc_caches[INDEX_NODE] = create_kmalloc_cache("kmalloc-node",
> kmalloc_size(INDEX_NODE), ARCH_KMALLOC_FLAGS);
> slab_state = PARTIAL_NODE;
> + correct_kmalloc_cache_index_table();
Lets call this
setup_kmalloc_cache_index_table
Please?
> -----Original Message-----
> From: Christoph Lameter [mailto:[email protected]]
> Sent: 03 February 2015 15:15
> To: Daniel Sanders
> Cc: Pekka Enberg; David Rientjes; Joonsoo Kim; Andrew Morton; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH 1/5] LLVMLinux: Correct size_index table before
> replacing the bootstrap kmem_cache_node.
>
> On Tue, 3 Feb 2015, Daniel Sanders wrote:
>
> > +++ b/mm/slab.c
> > @@ -1440,6 +1440,7 @@ void __init kmem_cache_init(void)
> > kmalloc_caches[INDEX_NODE] = create_kmalloc_cache("kmalloc-
> node",
> > kmalloc_size(INDEX_NODE),
> ARCH_KMALLOC_FLAGS);
> > slab_state = PARTIAL_NODE;
> > + correct_kmalloc_cache_index_table();
>
> Lets call this
>
> setup_kmalloc_cache_index_table
>
> Please?
Sure, I've made the change in my repo. I'll wait a bit before re-sending the patch in case others have feedback too.
On Tue, 3 Feb 2015, Daniel Sanders wrote:
> From: Toma Tabacu <[email protected]>
>
> Differentiate the 'u' GAS .macro argument from the '\u' C preprocessor unicode
> escape sequence by renaming it to '_u'.
>
> When the 'u' argument is evaluated, we end up writing '\u', which causes
> clang to emit -Wunicode warnings.
>
> This silences a couple of -Wunicode warnings reported by clang.
> The changed code can be preprocessed without warnings by both gcc and clang.
I suspect it is a clang preprocessor bug that:
1. It interprets these character pairs as unicode escapes for assembly
sources, I think it should be up to the language translator rather
than the preprocessor, i.e. the assembler in this case (the notion of
unicode escapes for the preprocessor appears to be limited to
stringification, and then it is implementation-defined).
2. It considers these character pairs to be unicode escapes in the first
place given that they do not follow the syntax required for such
escapes, that is `\unnnn', where `n' are hex digits.
Of course it may be reasonable for us to work this bug around as we've
been doing for years with GCC, but has the issue been reported back to
clang maintainers? What was their response?
Maciej
On Tue, 3 Feb 2015, Daniel Sanders wrote:
> From: Toma Tabacu <[email protected]>
>
> Change the type of csum_ipv6_magic's 'proto' argument from unsigned
> short to __u32.
>
> This fixes a type mismatch between the 'htonl(proto)' inline asm
> input, which is __u32, and the 'proto' output, which is unsigned
> short.
>
> This is the error message reported by clang:
> arch/mips/include/asm/checksum.h:285:27: error: unsupported inline asm: input with type '__be32' (aka 'unsigned int') matching output with type 'unsigned short'
> "0" (htonl(len)), "1" (htonl(proto)), "r" (sum));
> ^~~~~~~~~~~~
>
> The changed code can be compiled successfully by both gcc and clang.
This definitely looks like a bug in clang to me. What this construct
means is both input #5 and output #1 live in the same register, and that
an `__u32' value is taken on input (from the result of the `htonl(proto)'
calculation) and an `unsigned short' value produced in the same register
on output, that'll be the value of the `proto' variable from there on. A
perfectly valid arrangement. This would be the right arrangement to use
with the MIPS16 SEH instruction for example. Has this bug been reported
to clang maintainers?
And I'd prefer to leave the declaration of `proto' alone as IPv6 network
protocol numbers are 16-bit quantities.
That said this code is indeed weird if not wrong, which is probably why
this arrangement resulted, in an attempt to prevent GCC from messing up
the registers used.
First and foremost both outputs, and especially #1, lack an earlyclobber.
This I imagine may have prompted GCC to overwrite one of the inputs, which
in turn is why whoever poked at this code decided to alias input #5 to
output #1. But as you can see in the asm there's no real aliasing between
input #5 and output #1. Input #5 is consumed early on (and even referred
to with `%5' rather than `%1', which would be the norm in the case of
actual aliasing), and the containing register reused for something else.
So the two operands can be separated. This is unlike input #4 vs output
#0, that is both read and written right away (and just as one'd expect
there's no reference to `%4' anywhere).
Output #0 can do without an earlyclobber as it is aliased to input #4 and
therefore cannot be assigned by GCC to another input. But it won't hurt
to have one too and it will set a good practice and serve a documentation
purpose.
I suggest a fix like this then:
static __inline__ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
const struct in6_addr *daddr,
__u32 len, unsigned short proto,
__wsum sum)
{
__wsum tmp;
__asm__(
[...]
: "=&r" (sum), "=&r" (tmp)
: "r" (saddr), "r" (daddr),
"0" (htonl(len)), "r" (htonl(proto)), "r" (sum));
return csum_fold(sum);
}
Try and see if it works for you.
I wonder why this is an asm in the first place though. There's no rocket
science here that GCC couldn't handle. I guess it must have been very bad
at optimising a C equivalent then.
Maciej
On 2/3/15 3:37 PM, Daniel Sanders wrote:
> This patch moves the initialization of the size_index table slightly
> earlier so that the first few kmem_cache_node's can be safely allocated
> when KMALLOC_MIN_SIZE is large.
The patch looks OK to me but how is this related to LLVM?
- Pekka
> -----Original Message-----
> From: Pekka Enberg [mailto:[email protected]]
> Sent: 04 February 2015 19:33
> To: Daniel Sanders
> Cc: Christoph Lameter; Pekka Enberg; David Rientjes; Joonsoo Kim; Andrew
> Morton; [email protected]; [email protected]
> Subject: Re: [PATCH 1/5] LLVMLinux: Correct size_index table before
> replacing the bootstrap kmem_cache_node.
>
> On 2/3/15 3:37 PM, Daniel Sanders wrote:
> > This patch moves the initialization of the size_index table slightly
> > earlier so that the first few kmem_cache_node's can be safely allocated
> > when KMALLOC_MIN_SIZE is large.
>
> The patch looks OK to me but how is this related to LLVM?
>
> - Pekka
I don't believe the bug to be LLVM specific but GCC doesn't normally encounter the problem. I haven't been able to identify exactly what GCC is doing better (probably inlining) but it seems that GCC is managing to optimize to the point that it eliminates the problematic allocations. This theory is supported by the fact that GCC can be made to fail in the same way by changing inline, __inline, __inline__, and __always_inline in include/linux/compiler-gcc.h such that they don't actually inline things.
On Wed, Feb 4, 2015 at 10:38 PM, Daniel Sanders
<[email protected]> wrote:
> I don't believe the bug to be LLVM specific but GCC doesn't normally encounter the problem. I haven't been able to identify exactly what GCC is doing better (probably inlining) but it seems that GCC is managing to optimize to the point that it eliminates the problematic allocations. This theory is supported by the fact that GCC can be made to fail in the same way by changing inline, __inline, __inline__, and __always_inline in include/linux/compiler-gcc.h such that they don't actually inline things.
OK, makes sense. Please include that explanation in the changelog and
drop use proper "slab" prefix instead of the confusing "LLVMLinux"
prefix in the subject line.
- Pekka
This patch moves the initialization of the size_index table slightly
earlier so that the first few kmem_cache_node's can be safely allocated
when KMALLOC_MIN_SIZE is large.
There are currently two ways to generate indices into kmalloc_caches
(via kmalloc_index() and via the size_index table in slab_common.c)
and on some arches (possibly only MIPS) they potentially disagree with
each other until create_kmalloc_caches() has been called. It seems
that the intention is that the size_index table is a fast equivalent
to kmalloc_index() and that create_kmalloc_caches() patches the table
to return the correct value for the cases where kmalloc_index()'s
if-statements apply.
The failing sequence was:
* kmalloc_caches contains NULL elements
* kmem_cache_init initialises the element that 'struct
kmem_cache_node' will be allocated to. For 32-bit Mips, this is a
56-byte struct and kmalloc_index returns KMALLOC_SHIFT_LOW (7).
* init_list is called which calls kmalloc_node to allocate a 'struct
kmem_cache_node'.
* kmalloc_slab selects the kmem_caches element using
size_index[size_index_elem(size)]. For MIPS, size is 56, and the
expression returns 6.
* This element of kmalloc_caches is NULL and allocation fails.
* If it had not already failed, it would have called
create_kmalloc_caches() at this point which would have changed
size_index[size_index_elem(size)] to 7.
Signed-off-by: Daniel Sanders <[email protected]>
Cc: Christoph Lameter <[email protected]>
Cc: Pekka Enberg <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: Joonsoo Kim <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
Renamed correct_kmalloc_cache_index_table() to setup_kmalloc_cache_index_table()
as requested.
mm/slab.c | 1 +
mm/slab.h | 1 +
mm/slab_common.c | 36 +++++++++++++++++++++---------------
mm/slub.c | 1 +
4 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/mm/slab.c b/mm/slab.c
index 65b5dcb..d476181 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1440,6 +1440,7 @@ void __init kmem_cache_init(void)
kmalloc_caches[INDEX_NODE] = create_kmalloc_cache("kmalloc-node",
kmalloc_size(INDEX_NODE), ARCH_KMALLOC_FLAGS);
slab_state = PARTIAL_NODE;
+ setup_kmalloc_cache_index_table();
slab_early_init = 0;
diff --git a/mm/slab.h b/mm/slab.h
index 1cf40054..6121dcc 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -71,6 +71,7 @@ unsigned long calculate_alignment(unsigned long flags,
#ifndef CONFIG_SLOB
/* Kmalloc array related functions */
+void setup_kmalloc_cache_index_table(void);
void create_kmalloc_caches(unsigned long);
/* Find the kmalloc slab corresponding for a certain size */
diff --git a/mm/slab_common.c b/mm/slab_common.c
index e03dd6f..fb45b5a 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -675,25 +675,20 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
}
/*
- * Create the kmalloc array. Some of the regular kmalloc arrays
- * may already have been created because they were needed to
- * enable allocations for slab creation.
+ * Patch up the size_index table if we have strange large alignment
+ * requirements for the kmalloc array. This is only the case for
+ * MIPS it seems. The standard arches will not generate any code here.
+ *
+ * Largest permitted alignment is 256 bytes due to the way we
+ * handle the index determination for the smaller caches.
+ *
+ * Make sure that nothing crazy happens if someone starts tinkering
+ * around with ARCH_KMALLOC_MINALIGN
*/
-void __init create_kmalloc_caches(unsigned long flags)
+void __init setup_kmalloc_cache_index_table(void)
{
int i;
- /*
- * Patch up the size_index table if we have strange large alignment
- * requirements for the kmalloc array. This is only the case for
- * MIPS it seems. The standard arches will not generate any code here.
- *
- * Largest permitted alignment is 256 bytes due to the way we
- * handle the index determination for the smaller caches.
- *
- * Make sure that nothing crazy happens if someone starts tinkering
- * around with ARCH_KMALLOC_MINALIGN
- */
BUILD_BUG_ON(KMALLOC_MIN_SIZE > 256 ||
(KMALLOC_MIN_SIZE & (KMALLOC_MIN_SIZE - 1)));
@@ -724,6 +719,17 @@ void __init create_kmalloc_caches(unsigned long flags)
for (i = 128 + 8; i <= 192; i += 8)
size_index[size_index_elem(i)] = 8;
}
+}
+
+/*
+ * Create the kmalloc array. Some of the regular kmalloc arrays
+ * may already have been created because they were needed to
+ * enable allocations for slab creation.
+ */
+void __init create_kmalloc_caches(unsigned long flags)
+{
+ int i;
+
for (i = KMALLOC_SHIFT_LOW; i <= KMALLOC_SHIFT_HIGH; i++) {
if (!kmalloc_caches[i]) {
kmalloc_caches[i] = create_kmalloc_cache(NULL,
diff --git a/mm/slub.c b/mm/slub.c
index fe376fe..11abd57 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3604,6 +3604,7 @@ void __init kmem_cache_init(void)
kmem_cache_node = bootstrap(&boot_kmem_cache_node);
/* Now we can use the kmem_cache to allocate kmalloc slabs */
+ setup_kmalloc_cache_index_table();
create_kmalloc_caches(0);
#ifdef CONFIG_SMP
--
2.1.4
This patch moves the initialization of the size_index table slightly
earlier so that the first few kmem_cache_node's can be safely allocated
when KMALLOC_MIN_SIZE is large.
There are currently two ways to generate indices into kmalloc_caches
(via kmalloc_index() and via the size_index table in slab_common.c)
and on some arches (possibly only MIPS) they potentially disagree with
each other until create_kmalloc_caches() has been called. It seems
that the intention is that the size_index table is a fast equivalent
to kmalloc_index() and that create_kmalloc_caches() patches the table
to return the correct value for the cases where kmalloc_index()'s
if-statements apply.
The failing sequence was:
* kmalloc_caches contains NULL elements
* kmem_cache_init initialises the element that 'struct
kmem_cache_node' will be allocated to. For 32-bit Mips, this is a
56-byte struct and kmalloc_index returns KMALLOC_SHIFT_LOW (7).
* init_list is called which calls kmalloc_node to allocate a 'struct
kmem_cache_node'.
* kmalloc_slab selects the kmem_caches element using
size_index[size_index_elem(size)]. For MIPS, size is 56, and the
expression returns 6.
* This element of kmalloc_caches is NULL and allocation fails.
* If it had not already failed, it would have called
create_kmalloc_caches() at this point which would have changed
size_index[size_index_elem(size)] to 7.
Signed-off-by: Daniel Sanders <[email protected]>
Cc: Christoph Lameter <[email protected]>
Cc: Pekka Enberg <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: Joonsoo Kim <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
Renamed correct_kmalloc_cache_index_table() to setup_kmalloc_cache_index_table()
as requested.
I don't believe the bug to be LLVM specific but GCC doesn't normally encounter
the problem. I haven't been able to identify exactly what GCC is doing better
(probably inlining) but it seems that GCC is managing to optimize to the point
that it eliminates the problematic allocations. This theory is supported by the
fact that GCC can be made to fail in the same way by changing inline, __inline,
__inline__, and __always_inline in include/linux/compiler-gcc.h such that they
don't actually inline things.
mm/slab.c | 1 +
mm/slab.h | 1 +
mm/slab_common.c | 36 +++++++++++++++++++++---------------
mm/slub.c | 1 +
4 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/mm/slab.c b/mm/slab.c
index 65b5dcb..d476181 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1440,6 +1440,7 @@ void __init kmem_cache_init(void)
kmalloc_caches[INDEX_NODE] = create_kmalloc_cache("kmalloc-node",
kmalloc_size(INDEX_NODE), ARCH_KMALLOC_FLAGS);
slab_state = PARTIAL_NODE;
+ setup_kmalloc_cache_index_table();
slab_early_init = 0;
diff --git a/mm/slab.h b/mm/slab.h
index 1cf40054..6121dcc 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -71,6 +71,7 @@ unsigned long calculate_alignment(unsigned long flags,
#ifndef CONFIG_SLOB
/* Kmalloc array related functions */
+void setup_kmalloc_cache_index_table(void);
void create_kmalloc_caches(unsigned long);
/* Find the kmalloc slab corresponding for a certain size */
diff --git a/mm/slab_common.c b/mm/slab_common.c
index e03dd6f..fb45b5a 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -675,25 +675,20 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
}
/*
- * Create the kmalloc array. Some of the regular kmalloc arrays
- * may already have been created because they were needed to
- * enable allocations for slab creation.
+ * Patch up the size_index table if we have strange large alignment
+ * requirements for the kmalloc array. This is only the case for
+ * MIPS it seems. The standard arches will not generate any code here.
+ *
+ * Largest permitted alignment is 256 bytes due to the way we
+ * handle the index determination for the smaller caches.
+ *
+ * Make sure that nothing crazy happens if someone starts tinkering
+ * around with ARCH_KMALLOC_MINALIGN
*/
-void __init create_kmalloc_caches(unsigned long flags)
+void __init setup_kmalloc_cache_index_table(void)
{
int i;
- /*
- * Patch up the size_index table if we have strange large alignment
- * requirements for the kmalloc array. This is only the case for
- * MIPS it seems. The standard arches will not generate any code here.
- *
- * Largest permitted alignment is 256 bytes due to the way we
- * handle the index determination for the smaller caches.
- *
- * Make sure that nothing crazy happens if someone starts tinkering
- * around with ARCH_KMALLOC_MINALIGN
- */
BUILD_BUG_ON(KMALLOC_MIN_SIZE > 256 ||
(KMALLOC_MIN_SIZE & (KMALLOC_MIN_SIZE - 1)));
@@ -724,6 +719,17 @@ void __init create_kmalloc_caches(unsigned long flags)
for (i = 128 + 8; i <= 192; i += 8)
size_index[size_index_elem(i)] = 8;
}
+}
+
+/*
+ * Create the kmalloc array. Some of the regular kmalloc arrays
+ * may already have been created because they were needed to
+ * enable allocations for slab creation.
+ */
+void __init create_kmalloc_caches(unsigned long flags)
+{
+ int i;
+
for (i = KMALLOC_SHIFT_LOW; i <= KMALLOC_SHIFT_HIGH; i++) {
if (!kmalloc_caches[i]) {
kmalloc_caches[i] = create_kmalloc_cache(NULL,
diff --git a/mm/slub.c b/mm/slub.c
index fe376fe..11abd57 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3604,6 +3604,7 @@ void __init kmem_cache_init(void)
kmem_cache_node = bootstrap(&boot_kmem_cache_node);
/* Now we can use the kmem_cache to allocate kmalloc slabs */
+ setup_kmalloc_cache_index_table();
create_kmalloc_caches(0);
#ifdef CONFIG_SMP
--
2.1.4
> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On Behalf Of
> Pekka Enberg
> Sent: 04 February 2015 20:42
> To: Daniel Sanders
> Cc: Christoph Lameter; David Rientjes; Joonsoo Kim; Andrew Morton; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH 1/5] LLVMLinux: Correct size_index table before
> replacing the bootstrap kmem_cache_node.
>
> On Wed, Feb 4, 2015 at 10:38 PM, Daniel Sanders
> <[email protected]> wrote:
> > I don't believe the bug to be LLVM specific but GCC doesn't normally
> encounter the problem. I haven't been able to identify exactly what GCC is
> doing better (probably inlining) but it seems that GCC is managing to
> optimize to the point that it eliminates the problematic allocations. This
> theory is supported by the fact that GCC can be made to fail in the same way
> by changing inline, __inline, __inline__, and __always_inline in
> include/linux/compiler-gcc.h such that they don't actually inline things.
>
> OK, makes sense. Please include that explanation in the changelog and
> drop use proper "slab" prefix instead of the confusing "LLVMLinux"
> prefix in the subject line.
>
> - Pekka
Sure. I've just updated the patch with those changes.
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 02/04/2015 11:06 PM, Daniel Sanders wrote:
> This patch moves the initialization of the size_index table slightly
> earlier so that the first few kmem_cache_node's can be safely allocated
> when KMALLOC_MIN_SIZE is large.
>
> There are currently two ways to generate indices into kmalloc_caches
> (via kmalloc_index() and via the size_index table in slab_common.c)
> and on some arches (possibly only MIPS) they potentially disagree with
> each other until create_kmalloc_caches() has been called. It seems
> that the intention is that the size_index table is a fast equivalent
> to kmalloc_index() and that create_kmalloc_caches() patches the table
> to return the correct value for the cases where kmalloc_index()'s
> if-statements apply.
>
> The failing sequence was:
> * kmalloc_caches contains NULL elements
> * kmem_cache_init initialises the element that 'struct
> kmem_cache_node' will be allocated to. For 32-bit Mips, this is a
> 56-byte struct and kmalloc_index returns KMALLOC_SHIFT_LOW (7).
> * init_list is called which calls kmalloc_node to allocate a 'struct
> kmem_cache_node'.
> * kmalloc_slab selects the kmem_caches element using
> size_index[size_index_elem(size)]. For MIPS, size is 56, and the
> expression returns 6.
> * This element of kmalloc_caches is NULL and allocation fails.
> * If it had not already failed, it would have called
> create_kmalloc_caches() at this point which would have changed
> size_index[size_index_elem(size)] to 7.
>
> Signed-off-by: Daniel Sanders <[email protected]>
> Cc: Christoph Lameter <[email protected]>
> Cc: Pekka Enberg <[email protected]>
> Cc: David Rientjes <[email protected]>
> Cc: Joonsoo Kim <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
Acked-by: Pekka Enberg <[email protected]>
On Wed, 4 Feb 2015, Maciej W. Rozycki wrote:
> 2. It considers these character pairs to be unicode escapes in the first
> place given that they do not follow the syntax required for such
> escapes, that is `\unnnn', where `n' are hex digits.
>
It doesn't actually treat them as unicode escapes, but it still warns the user,
in case they were meant to be unicode escapes. Here's the warning message:
arch/mips/include/asm/asmmacro.h:197:51: warning: \u used with no following hex digits; treating as '\' followed by identifier [-Wunicode]
.word 0x41000000 | (\rt << 16) | (\rd << 11) | (\u << 5) | (\sel)
^
I'll add it to the summary in v2.
> Of course it may be reasonable for us to work this bug around as we've
> been doing for years with GCC, but has the issue been reported back to
> clang maintainers? What was their response?
>
It hasn't been reported, but I don't think they would agree with removing
unicode escape sequences from the assembler-with-cpp mode because it is
currently being used for other languages as well, not just assembly.
One such language is Haskell (ghc, to be more specific), for which the clang
developers had to actually stop the preprocessor from enforcing the C universal
character name restrictions in assembler-with-cpp mode, which suggests that ghc
wants the preprocessor to check for unicode escape sequences.
At the moment, we can either disable -Wunicode for asmmacro.h or refrain from
using '\u' as an identifier.
On Thu, 5 Feb 2015, Toma Tabacu wrote:
> > 2. It considers these character pairs to be unicode escapes in the first
> > place given that they do not follow the syntax required for such
> > escapes, that is `\unnnn', where `n' are hex digits.
> >
>
> It doesn't actually treat them as unicode escapes, but it still warns the user,
> in case they were meant to be unicode escapes. Here's the warning message:
>
> arch/mips/include/asm/asmmacro.h:197:51: warning: \u used with no following hex digits; treating as '\' followed by identifier [-Wunicode]
> .word 0x41000000 | (\rt << 16) | (\rd << 11) | (\u << 5) | (\sel)
> ^
> I'll add it to the summary in v2.
Thanks, that makes things clearer. It always makes sense to include the
exact error message produced where applicable or otherwise people do not
necessarily know what the matter is.
> > Of course it may be reasonable for us to work this bug around as we've
> > been doing for years with GCC, but has the issue been reported back to
> > clang maintainers? What was their response?
> >
>
> It hasn't been reported, but I don't think they would agree with removing
> unicode escape sequences from the assembler-with-cpp mode because it is
> currently being used for other languages as well, not just assembly.
First, preprocessing rules surely have to be language specific. The C
language standard does not specify what the preprocessor is meant to do
(if anything) for other languages. GCC or clang -- that's no different.
The assembly language has a different syntax and `\u' has a different
meaning in the context of assembly macro expansion than it would have in a
name of a symbol, where such a Unicode escape sequence might indeed be
interpreted as such and character encoded propagated to the symbol
produced. But that's up to the assembler -- GAS for example does not
AFAIK support Unicode escape sequences in symbol names right now, but I
suppose such a feature could be added if desired.
Which prompts another question of course: how does the clang C compiler
represent Unicode characters in identifiers in its assembly output?
I have looked into the C language standard and it appears to me like the
translation phase to interpret universal character names at has not been
defined. This is probably why the standard does specify the result of
pasting preprocessor tokens together as undefined if a universal character
name is produced this way.
Consequently I think an important question in this context is: does
clang's preprocessor actually convert these sequences anyhow before
passing them down to the compiler? How for example does C output from a
trivial example that contains such Unicode escape sequences look like
then?
> One such language is Haskell (ghc, to be more specific), for which the clang
> developers had to actually stop the preprocessor from enforcing the C universal
> character name restrictions in assembler-with-cpp mode, which suggests that ghc
> wants the preprocessor to check for unicode escape sequences.
>
> At the moment, we can either disable -Wunicode for asmmacro.h or refrain from
> using '\u' as an identifier.
To be clear: it's `u' here that is the identifier, the leading `\' is
merely how assembly syntax has been specified for references to macro
arguments. And TBH I find banning any macro arguments starting with `u'
rather silly. I'm leaning towards considering having -Wunicode disabled
for all assembly sources, or maybe even for the whole Linux compilation,
the right solution. It's not like we have a need for Unicode identifiers.
What's the exact semantics of -Wunicode for clang?
Maciej
"Maciej W. Rozycki" <[email protected]> writes:
> On Thu, 5 Feb 2015, Toma Tabacu wrote:
>
>> > 2. It considers these character pairs to be unicode escapes in the first
>> > place given that they do not follow the syntax required for such
>> > escapes, that is `\unnnn', where `n' are hex digits.
>> >
>>
>> It doesn't actually treat them as unicode escapes, but it still warns
>> the user, in case they were meant to be unicode escapes. Here's the
>> warning message:
>>
>> arch/mips/include/asm/asmmacro.h:197:51: warning: \u used with no following hex digits; treating as '\' followed by identifier [-Wunicode]
>> .word 0x41000000 | (\rt << 16) | (\rd << 11) | (\u << 5) | (\sel)
>> ^
>> I'll add it to the summary in v2.
>
> Thanks, that makes things clearer. It always makes sense to include the
> exact error message produced where applicable or otherwise people do not
> necessarily know what the matter is.
>
>> > Of course it may be reasonable for us to work this bug around as we've
>> > been doing for years with GCC, but has the issue been reported back to
>> > clang maintainers? What was their response?
>> >
>>
>> It hasn't been reported, but I don't think they would agree with removing
>> unicode escape sequences from the assembler-with-cpp mode because it is
>> currently being used for other languages as well, not just assembly.
>
> First, preprocessing rules surely have to be language specific. The C
> language standard does not specify what the preprocessor is meant to do
> (if anything) for other languages. GCC or clang -- that's no different.
>
> The assembly language has a different syntax and `\u' has a different
> meaning in the context of assembly macro expansion than it would have in a
> name of a symbol, where such a Unicode escape sequence might indeed be
> interpreted as such and character encoded propagated to the symbol
> produced. But that's up to the assembler -- GAS for example does not
> AFAIK support Unicode escape sequences in symbol names right now, but I
> suppose such a feature could be added if desired.
>
> Which prompts another question of course: how does the clang C compiler
> represent Unicode characters in identifiers in its assembly output?
>
> I have looked into the C language standard and it appears to me like the
> translation phase to interpret universal character names at has not been
> defined. This is probably why the standard does specify the result of
> pasting preprocessor tokens together as undefined if a universal character
> name is produced this way.
That is my interpretation as well.
> Consequently I think an important question in this context is: does
> clang's preprocessor actually convert these sequences anyhow before
> passing them down to the compiler? How for example does C output from a
> trivial example that contains such Unicode escape sequences look like
> then?
>
>> One such language is Haskell (ghc, to be more specific), for which
>> the clang developers had to actually stop the preprocessor from
>> enforcing the C universal character name restrictions in
>> assembler-with-cpp mode, which suggests that ghc wants the
>> preprocessor to check for unicode escape sequences.
>>
>> At the moment, we can either disable -Wunicode for asmmacro.h or
>> refrain from using '\u' as an identifier.
>
> To be clear: it's `u' here that is the identifier, the leading `\' is
> merely how assembly syntax has been specified for references to macro
> arguments. And TBH I find banning any macro arguments starting with `u'
> rather silly.
Agreed.
> I'm leaning towards considering having -Wunicode disabled for all
> assembly sources, or maybe even for the whole Linux compilation, the
> right solution. It's not like we have a need for Unicode identifiers.
It might be an idea to disable -Wunicode and have checkpatch warn about
Unicode escapes instead if people are worried about this. Personally, I
doubt there's much cause for concern here.
--
M?ns Rullg?rd
[email protected]
Apologies for the slow response. I've had an excessive amount of meetings in the last couple days.
> -----Original Message-----
> From: Maciej W. Rozycki [mailto:[email protected]]
> Sent: 04 February 2015 12:58
> To: Daniel Sanders
> Cc: Toma Tabacu; Ralf Baechle; Markos Chandras; Leonid Yegoshin; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH 3/5] MIPS: LLVMLinux: Fix an 'inline asm input/output
> type mismatch' error.
>
> On Tue, 3 Feb 2015, Daniel Sanders wrote:
>
> > From: Toma Tabacu <[email protected]>
> >
> > Change the type of csum_ipv6_magic's 'proto' argument from unsigned
> > short to __u32.
> >
> > This fixes a type mismatch between the 'htonl(proto)' inline asm
> > input, which is __u32, and the 'proto' output, which is unsigned
> > short.
> >
> > This is the error message reported by clang:
> > arch/mips/include/asm/checksum.h:285:27: error: unsupported inline asm:
> input with type '__be32' (aka 'unsigned int') matching output with type
> 'unsigned short'
> > "0" (htonl(len)), "1" (htonl(proto)), "r" (sum));
> > ^~~~~~~~~~~~
> >
> > The changed code can be compiled successfully by both gcc and clang.
>
> This definitely looks like a bug in clang to me. What this construct
> means is both input #5 and output #1 live in the same register, and that
> an `__u32' value is taken on input (from the result of the `htonl(proto)'
> calculation) and an `unsigned short' value produced in the same register
> on output, that'll be the value of the `proto' variable from there on. A
> perfectly valid arrangement. This would be the right arrangement to use
> with the MIPS16 SEH instruction for example. Has this bug been reported
> to clang maintainers?
I'm not convinced it's a bug, but I do at least agree that the use case sounds
sensible. It makes sense to me that the focus should be on register allocations
rather than on types. However, the relevant clang source is being very specific
about the cases it is/isn't allowing which suggests it's deliberate. I've started a
thread on the clang mailing list to try to find out more about why we currently
reject it.
> And I'd prefer to leave the declaration of `proto' alone as IPv6 network
> protocol numbers are 16-bit quantities.
>
> That said this code is indeed weird if not wrong, which is probably why
> this arrangement resulted, in an attempt to prevent GCC from messing up
> the registers used.
>
> First and foremost both outputs, and especially #1, lack an earlyclobber.
> This I imagine may have prompted GCC to overwrite one of the inputs, which
> in turn is why whoever poked at this code decided to alias input #5 to
> output #1. But as you can see in the asm there's no real aliasing between
> input #5 and output #1. Input #5 is consumed early on (and even referred
> to with `%5' rather than `%1', which would be the norm in the case of
> actual aliasing), and the containing register reused for something else.
> So the two operands can be separated. This is unlike input #4 vs output
> #0, that is both read and written right away (and just as one'd expect
> there's no reference to `%4' anywhere).
>
> Output #0 can do without an earlyclobber as it is aliased to input #4 and
> therefore cannot be assigned by GCC to another input. But it won't hurt
> to have one too and it will set a good practice and serve a documentation
> purpose.
>
> I suggest a fix like this then:
>
> static __inline__ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
> const struct in6_addr *daddr,
> __u32 len, unsigned short proto,
> __wsum sum)
> {
> __wsum tmp;
>
> __asm__(
> [...]
> : "=&r" (sum), "=&r" (tmp)
> : "r" (saddr), "r" (daddr),
> "0" (htonl(len)), "r" (htonl(proto)), "r" (sum));
>
> return csum_fold(sum);
> }
>
> Try and see if it works for you.
>
> I wonder why this is an asm in the first place though. There's no rocket
> science here that GCC couldn't handle. I guess it must have been very bad
> at optimising a C equivalent then.
>
> Maciej
Yes, that works for me on both GCC and Clang. I'll change the patch to this.
Would you like a 'Suggested-By' in the patch description?
On Thu, 5 Feb 2015, Daniel Sanders wrote:
> Apologies for the slow response. I've had an excessive amount of
> meetings in the last couple days.
No worries, if anyone, it's not me in a hurry here. ;)
> > This definitely looks like a bug in clang to me. What this construct
> > means is both input #5 and output #1 live in the same register, and that
> > an `__u32' value is taken on input (from the result of the `htonl(proto)'
> > calculation) and an `unsigned short' value produced in the same register
> > on output, that'll be the value of the `proto' variable from there on. A
> > perfectly valid arrangement. This would be the right arrangement to use
> > with the MIPS16 SEH instruction for example. Has this bug been reported
> > to clang maintainers?
>
> I'm not convinced it's a bug, but I do at least agree that the use case sounds
> sensible. It makes sense to me that the focus should be on register allocations
> rather than on types. However, the relevant clang source is being very specific
> about the cases it is/isn't allowing which suggests it's deliberate. I've started a
> thread on the clang mailing list to try to find out more about why we currently
> reject it.
I think it boils down to the register model implemented by a given
architecture. MIPS processors do not have subregisters for integer data
quantities narrower than the size of the machine word. The same GPR will
hold a `char', a `short' or an `int', and therefore it is perfectly valid
to arrange that it holds say an `int' on input and say a `short' on
output. So given an artificial example like this:
short foo(int i)
{
short v;
asm("frob %0" : "=r" (v) : "0" (i));
return v;
}
the compiler knows it does not have to truncate the result of the
calculation made in the asm before returning it to the caller, which it
would unnecessarily have with this code:
short foo(int i)
{
asm("frob %0" : "+r" (i));
return i;
}
This is unlike some other architectures. On x86 for example you do have
subregisters, so for example an `int' will be stored in %eax, a short will
be stored in %ax, and a `char' will be stored in %al, or worse yet, %ah.
Consequently you may not be able to alias an input operand and an output
operand of a different width each to each other.
Also I think it is important to note that in the (first) example above,
`i' and `v' are separate data entities as far as C code is concerned.
And that the operands of the asm merely tell the C compiler that the value
of `i' must appear in a register (that need not be the variable's original
storage location) on entry to the asm and the value to set `v' to will
appear in a register (that again need not be the place where the actual
variable lives) on exit from the asm, and that the two registers must be
physically the same (presumably because of a machine limitation, such as
one observed with the MIPS16 SEH instruction mentioned), but that does not
mean the input and the output otherwise have anything in common.
And last but not least, the extended asm feature is a GCC extension, so
if clang developers chose to implement it for GCC compatibility, then I am
afraid they need to follow the rules set by GCC. So if GCC accepts it,
they need too.
> Yes, that works for me on both GCC and Clang. I'll change the patch to this.
> Would you like a 'Suggested-By' in the patch description?
Sure.
Maciej
Replace incorrect matching constraint that caused the error with an alternative
that still has the required constraints on the inline assembly.
This is the error message reported by clang:
arch/mips/include/asm/checksum.h:285:27: error: unsupported inline asm: input with type '__be32' (aka 'unsigned int') matching output with type 'unsigned short'
"0" (htonl(len)), "1" (htonl(proto)), "r" (sum));
^~~~~~~~~~~~
The changed code can be compiled successfully by both gcc and clang.
Signed-off-by: Daniel Sanders <[email protected]>
Signed-off-by: Toma Tabacu <[email protected]>
Suggested-by: Maciej W. Rozycki <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Markos Chandras <[email protected]>
Cc: Leonid Yegoshin <[email protected]>
Cc: Maciej W. Rozycki <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
Rewrote the patch following Maciej's suggestion where he observed that
the assembly was somewhat strange and suggested correcting the
constraints and using a local of matching type.
arch/mips/include/asm/checksum.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/mips/include/asm/checksum.h b/arch/mips/include/asm/checksum.h
index 3418c51..48bfcaf 100644
--- a/arch/mips/include/asm/checksum.h
+++ b/arch/mips/include/asm/checksum.h
@@ -228,6 +228,8 @@ static __inline__ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
__u32 len, unsigned short proto,
__wsum sum)
{
+ __wsum tmp;
+
__asm__(
" .set push # csum_ipv6_magic\n"
" .set noreorder \n"
@@ -280,9 +282,9 @@ static __inline__ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
" addu %0, $1 # Add final carry\n"
" .set pop"
- : "=r" (sum), "=r" (proto)
+ : "=&r" (sum), "=&r" (tmp)
: "r" (saddr), "r" (daddr),
- "0" (htonl(len)), "1" (htonl(proto)), "r" (sum));
+ "0" (htonl(len)), "r" (htonl(proto)), "r" (sum));
return csum_fold(sum);
}
--
2.1.4
On Mon, 9 Feb 2015, Daniel Sanders wrote:
> --- a/arch/mips/include/asm/checksum.h
> +++ b/arch/mips/include/asm/checksum.h
> @@ -228,6 +228,8 @@ static __inline__ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
> __u32 len, unsigned short proto,
> __wsum sum)
> {
> + __wsum tmp;
Formatting issue here, use a tab.
Maciej
Replace incorrect matching constraint that caused the error with an alternative
that still has the required constraints on the inline assembly.
This is the error message reported by clang:
arch/mips/include/asm/checksum.h:285:27: error: unsupported inline asm: input with type '__be32' (aka 'unsigned int') matching output with type 'unsigned short'
"0" (htonl(len)), "1" (htonl(proto)), "r" (sum));
^~~~~~~~~~~~
The changed code can be compiled successfully by both gcc and clang.
Signed-off-by: Daniel Sanders <[email protected]>
Signed-off-by: Toma Tabacu <[email protected]>
Suggested-by: Maciej W. Rozycki <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Markos Chandras <[email protected]>
Cc: Leonid Yegoshin <[email protected]>
Cc: Maciej W. Rozycki <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
Rewrote the patch following Maciej's suggestion where he observed that
the assembly was somewhat strange and suggested correcting the
constraints and using a local of matching type.
Fixed a silly whitespace mistake that was introduced in v2.
arch/mips/include/asm/checksum.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/mips/include/asm/checksum.h b/arch/mips/include/asm/checksum.h
index 3418c51..ebad4f4 100644
--- a/arch/mips/include/asm/checksum.h
+++ b/arch/mips/include/asm/checksum.h
@@ -228,6 +228,8 @@ static __inline__ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
__u32 len, unsigned short proto,
__wsum sum)
{
+ __wsum tmp;
+
__asm__(
" .set push # csum_ipv6_magic\n"
" .set noreorder \n"
@@ -280,9 +282,9 @@ static __inline__ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
" addu %0, $1 # Add final carry\n"
" .set pop"
- : "=r" (sum), "=r" (proto)
+ : "=&r" (sum), "=&r" (tmp)
: "r" (saddr), "r" (daddr),
- "0" (htonl(len)), "1" (htonl(proto)), "r" (sum));
+ "0" (htonl(len)), "r" (htonl(proto)), "r" (sum));
return csum_fold(sum);
}
--
2.1.4
Apologies for the slow reply.
> -----Original Message-----
> From: M?ns Rullg?rd [mailto:[email protected]]
> Sent: 05 February 2015 12:56
> To: Maciej W. Rozycki
> Cc: Toma Tabacu; Daniel Sanders; Ralf Baechle; Paul Burton; Paul Bolle;
> Steven J. Hill; Manuel Lauss; Jim Quinlan; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH 5/5] MIPS: LLVMLinux: Silence unicode warnings when
> preprocessing assembly.
>
> "Maciej W. Rozycki" <[email protected]> writes:
>
> > On Thu, 5 Feb 2015, Toma Tabacu wrote:
> >
> >> > 2. It considers these character pairs to be unicode escapes in the first
> >> > place given that they do not follow the syntax required for such
> >> > escapes, that is `\unnnn', where `n' are hex digits.
> >> >
> >>
> >> It doesn't actually treat them as unicode escapes, but it still warns
> >> the user, in case they were meant to be unicode escapes. Here's the
> >> warning message:
> >>
> >> arch/mips/include/asm/asmmacro.h:197:51: warning: \u used with no
> following hex digits; treating as '\' followed by identifier [-Wunicode]
> >> .word 0x41000000 | (\rt << 16) | (\rd << 11) | (\u << 5) | (\sel)
> >> ^
> >> I'll add it to the summary in v2.
> >
> > Thanks, that makes things clearer. It always makes sense to include the
> > exact error message produced where applicable or otherwise people do not
> > necessarily know what the matter is.
> >
> >> > Of course it may be reasonable for us to work this bug around as we've
> >> > been doing for years with GCC, but has the issue been reported back to
> >> > clang maintainers? What was their response?
> >> >
> >>
> >> It hasn't been reported, but I don't think they would agree with removing
> >> unicode escape sequences from the assembler-with-cpp mode because it is
> >> currently being used for other languages as well, not just assembly.
> >
> > First, preprocessing rules surely have to be language specific. The C
> > language standard does not specify what the preprocessor is meant to do
> > (if anything) for other languages. GCC or clang -- that's no different.
> >
> > The assembly language has a different syntax and `\u' has a different
> > meaning in the context of assembly macro expansion than it would have in a
> > name of a symbol, where such a Unicode escape sequence might indeed be
> > interpreted as such and character encoded propagated to the symbol
> > produced. But that's up to the assembler -- GAS for example does not
> > AFAIK support Unicode escape sequences in symbol names right now, but I
> > suppose such a feature could be added if desired.
Pre-processed assembly is somewhat unusual in that it has traditionally been
pre-processed with a pre-processor designed for the C language. It's certainly
possible to have assembly specific tweaks (GCC has a couple) but it is still a C
pre-processor at heart. It doesn't know anything about the assembly language,
it just happens to be similar enough to be usable.
>From the pre-processors point of view, '\u' is two pre-processor tokens '\' and
the identifier 'u'. However, with following hex digits it would have been an identifier
starting with a universal character name. Clang's warning is effectively saying that
the former is more likely to be the intention. That's probably not as true for
pre-processed assembly as it is for C/C++.
> > Which prompts another question of course: how does the clang C compiler
> > represent Unicode characters in identifiers in its assembly output?
They're emitted as multi-byte characters.
> > I have looked into the C language standard and it appears to me like the
> > translation phase to interpret universal character names at has not been
> > defined. This is probably why the standard does specify the result of
> > pasting preprocessor tokens together as undefined if a universal character
> > name is produced this way.
>
> That is my interpretation as well.
It's my understanding that they should be interpreted when pre-processing tokens
are formed. This is based on the fact that the universal character names are included in
the grammar for identifiers and are not discussed in a separate translation phase.
I agree that it doesn't explicitly state that though.
> > Consequently I think an important question in this context is: does
> > clang's preprocessor actually convert these sequences anyhow before
> > passing them down to the compiler? How for example does C output from a
> > trivial example that contains such Unicode escape sequences look like
> > then?
Clang is converting them to multibyte characters during pre-processing.
> >> One such language is Haskell (ghc, to be more specific), for which
> >> the clang developers had to actually stop the preprocessor from
> >> enforcing the C universal character name restrictions in
> >> assembler-with-cpp mode, which suggests that ghc wants the
> >> preprocessor to check for unicode escape sequences.
> >>
> >> At the moment, we can either disable -Wunicode for asmmacro.h or
> >> refrain from using '\u' as an identifier.
> >
> > To be clear: it's `u' here that is the identifier, the leading `\' is
> > merely how assembly syntax has been specified for references to macro
> > arguments. And TBH I find banning any macro arguments starting with `u'
> > rather silly.
>
> Agreed.
That's the crux of the issue. Had it been followed by some hex-digits,
it would be an identifier '\u1234' and not a '\' followed by the identifier 'u'.
Clang currently thinks the former is more likely and warns.
I do agree that warning about all macro arguments beginning with 'u' is silly though.
Perhaps for assembler-with-cpp mode the warning should be suppressed when
it's the first character of an identifier.
> > I'm leaning towards considering having -Wunicode disabled for all
> > assembly sources, or maybe even for the whole Linux compilation, the
> > right solution. It's not like we have a need for Unicode identifiers.
>
> It might be an idea to disable -Wunicode and have checkpatch warn about
> Unicode escapes instead if people are worried about this. Personally, I
> doubt there's much cause for concern here.
>
> --
> M?ns Rullg?rd
> [email protected]
I'm fine with disabling -Wunicode if that's our preferred solution.