64bit unaligned access on 32bit kernel

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line · Ralf Baechle

I've rewriten Atushi's fix for the 64-bit put_unaligned on 32-bit systems bug to generate more efficient code.

This case has buzilla URL http://bugzilla.kernel.org/show_bug.cgi?id=5138.

Signed-off-by Ralf Baechle <[email protected]>

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines · Ralf Baechle

I've rewriten Atushi's fix for the 64-bit put_unaligned on 32-bit systems bug to generate more efficient code.

This case has buzilla URL http://bugzilla.kernel.org/show_bug.cgi?id=5138.

Signed-off-by Ralf Baechle <[email protected]>

...

include/asm-generic/unaligned.h static inline void __ustw(__u16 val, __u · line 84

Quoted 7 lines

#define __get_unaligned(ptr, size) ({ \

include/asm-generic/unaligned.h +1 -1

 	const void *__gu_p = ptr;		\
-	unsigned long val;			\
+	__typeof__(*(ptr)) val;			\
 	switch (size) {				\
 	case 1:					\
 		val = *(const __u8 *)__gu_p;	\

It looks gcc 4.x strike back. If the 'ptr' is a const, this code cause "assignment of read-only variable" error on gcc 4.x. Let's step a back, or do you have any other good idea?

Use __u64 instead of __typeof__(*(ptr)) for temporary variable to get rid of errors on gcc 4.x.

Signed-off-by Atsushi Nemoto <[email protected]>

include/asm-generic/unaligned.h +1 -1

index 4dc8ddb..09ec447 100644
@@ -78,7 +78,7 @@ static inline void __ustw(__u16 val, __u
 
 #define __get_unaligned(ptr, size) ({		\
 	const void *__gu_p = ptr;		\
-	__typeof__(*(ptr)) val;			\
+	__u64 val;				\
 	switch (size) {				\
 	case 1:					\
 		val = *(const __u8 *)__gu_p;	\

Quoted 3 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line · Andrew Morton

Use __u64 instead of __typeof__(*(ptr)) for temporary variable to get rid of errors on gcc 4.x.

I worry about what impact that change might have on code generation. Hopefully none, if gcc is good enough.

But I cannot think of a better fix.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line · Ralf Baechle

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines · Ralf Baechle

ralf> Below's fix results in exactly the same code size on all ralf> compilers and configurations I've tested it.

ralf> I also have another more elegant fix which as a side effect ralf> makes get_unaligned work for arbitrary data types but it that ralf> one results in a slight code bloat:

I tested the patch attached on several MIPS kernel (big/little, 32bit/64bit) with gcc 3.4.5. In most (but not all) case, Ralf's fix resulted better than the previous fix.

Quoted 19 lines

include/asm-generic/unaligned.h +2 -2

@@ -78,7 +78,7 @@ static inline void __ustw(__u16 val, __u
 
 #define __get_unaligned(ptr, size) ({		\
 	const void *__gu_p = ptr;		\
-	__typeof__(*(ptr)) val;			\
+	__u64 val;				\
 	switch (size) {				\
 	case 1:					\
 		val = *(const __u8 *)__gu_p;	\

@@ -95,7 +95,7 @@ static inline void __ustw(__u16 val, __u
 	default:				\
 		bad_unaligned_access_length();	\
 	};					\
-	val;					\
+	(__typeof__(*(ptr)))val;		\
 })
 

include/asm-generic/unaligned.h static inline void __ustw(__u16 val, __u · line 101

Quoted 1 line

include/asm-generic/unaligned.h +0 -0

 #define __put_unaligned(val, ptr, size)		\

I worry about what impact that change might have on code generation. Hopefully none, if gcc is good enough.

But I cannot think of a better fix.

Time for a little bit of dead horse flogging.

Quoted 19 lines · Andrew Morton

Quoted 19 lines

include/asm-generic/unaligned.h +2 -2

@@ -78,7 +78,7 @@ static inline void __ustw(__u16 val, __u
 
 #define __get_unaligned(ptr, size) ({		\
 	const void *__gu_p = ptr;		\
-	__typeof__(*(ptr)) val;			\
+	__u64 val;				\
 	switch (size) {				\
 	case 1:					\
 		val = *(const __u8 *)__gu_p;	\

@@ -95,7 +95,7 @@ static inline void __ustw(__u16 val, __u
 	default:				\
 		bad_unaligned_access_length();	\
 	};					\
-	val;					\
+	(__typeof__(*(ptr)))val;		\
 })
 

Quoted 6 lines

include/asm-generic/unaligned.h static inline void __ustw(__u16 val, __u · line 101

Quoted 1 line

include/asm-generic/unaligned.h +0 -0

 #define __put_unaligned(val, ptr, size)		\

I worry about what impact that change might have on code generation. Hopefully none, if gcc is good enough.

But I cannot think of a better fix.

It does inflate the code but back then we agreed to go for Atsushi's patch because it was fairly obviously correct. This patch obviously is less obvious but generates fairly decent, works for arbitrary data types and cuts down the size of unaligned.h from 122 lines to 44 so it must be good.

...

include/asm-generic/unaligned.h +1 -0

+#define get_unaligned(ptr)						\

include/asm-generic/unaligned.h line 30

Quoted 11 lines

include/asm-generic/unaligned.h +10 -0

+#define get_unaligned(ptr)						\
+({									\
+	const struct {							\
+		union {							\
+			const int __un_foo[0];				\
+			const __typeof__(*(ptr)) __un;			\
+		} __un __attribute__ ((packed));			\
+	} * const __gu_p = (void *) (ptr);				\
+									\
+	__gu_p->__un.__un;						\
 })

Can someone please tell us how this magic works? (And it does appear to work).

It seems to assuming that the compiler will assume that members of packed structures can have arbitrary alignment, even if that alignment is obvious.

Which makes sense, but I'd like to see chapter-and-verse from the spec or from the gcc docs so we can rely upon it working on all architectures and compilers from now until ever more.

Can someone please tell us how this magic works? (And it does appear to work).

It seems to assuming that the compiler will assume that members of packed structures can have arbitrary alignment, even if that alignment is obvious.

Which makes sense, but I'd like to see chapter-and-verse from the spec or from the gcc docs so we can rely upon it working on all architectures and compilers from now until ever more.

IOW: your changlogging sucks ;)

Quoted 10 lines · Andrew Morton

Can someone please tell us how this magic works? (And it does appear to work).

It seems to assuming that the compiler will assume that members of packed structures can have arbitrary alignment, even if that alignment is obvious.

Which makes sense, but I'd like to see chapter-and-verse from the spec or from the gcc docs so we can rely upon it working on all architectures and compilers from now until ever more.

Quoted 1 line

IOW: your changlogging sucks ;)

It was my entry for the next edition of the C Puzzle Book ;-)

The whole union thing was only needed to get rid of a warning but Marcel's solution does the same thing by attaching the packed keyword to the entire structure instead, so this patch is now using his macros but using __packed instead.

Quoted 4 lines

The whole union thing was only needed to get rid of a warning but Marcel's solution does the same thing by attaching the packed keyword to the entire structure instead, so this patch is now using his macros but using __packed instead.

How do we know this trick will work as-designed across all versions of gcc and icc (at least) and for all architectures and for all sets of compiler options?

Basically, it has to be guaranteed by a C standard. Is it?

Gcc info page says:

[...]
`packed'
The `packed' attribute specifies that a variable or structure field should have the smallest possible alignment--one byte for a variable, and one bit for a field, unless you specify a larger value with the `aligned' attribute.
[...]

Qed?

Quoted 10 lines · Andrew Morton

Quoted 4 lines

The whole union thing was only needed to get rid of a warning but Marcel's solution does the same thing by attaching the packed keyword to the entire structure instead, so this patch is now using his macros but using __packed instead.

How do we know this trick will work as-designed across all versions of gcc and icc (at least) and for all architectures and for all sets of compiler options?

Basically, it has to be guaranteed by a C standard. Is it?

Gcc info page says:

[...]
`packed'
The `packed' attribute specifies that a variable or structure field should have the smallest possible alignment--one byte for a variable, and one bit for a field, unless you specify a larger value with the `aligned' attribute.
[...]

Qed?

hm. So if I have

	struct bar {
		unsigned long b;
	} __attribute__((packed));

	struct foo {
		unsigned long u;
		struct bar b;
	};

then the compiler can see that foo.b.b is well-aligned, regardless of the packedness.

hm. So if I have

	struct bar {
		unsigned long b;
	} __attribute__((packed));

	struct foo {
		unsigned long u;
		struct bar b;
	};

then the compiler can see that foo.b.b is well-aligned, regardless of the packedness.

Plus some crazy people compile the kernel with icc (or at least they used to). What happens there?

Quoted 11 lines · Andrew Morton

hm. So if I have

	struct bar {
		unsigned long b;
	} __attribute__((packed));

	struct foo {
		unsigned long u;
		struct bar b;
	};

Quoted 5 lines

then the compiler can see that foo.b.b is well-aligned, regardless of the packedness.

Plus some crazy people compile the kernel with icc (or at least they used to). What happens there?

A quick grep for __attribute__((packed)) and __packed find around 900 hits, I'd probably find more if I'd look for syntactical variations. Some hits are in arch/{i386,x86_64,ia64}. At a glance it seems hard to configure a useful x86 kernel that doesn't involve any packed attribute. I take that as statistical proof that icc either has doesn't really work for building the kernel or groks packing. Any compiler not implementing gcc extensions is lost at building the kernel but that's old news.

No, icc surely supports attribute(packed). My point is that we shouldn't rely upon the gcc info file for this, because other compilers can (or could) be used to build the kernel.

So it would be safer if the C spec said (or could be interpreted to say) "members of packed structures are always copied bytewise". So then we can be reasonably confident that this change won't break the use of those compilers.

But then, I don't even know if any C standard says anything about packing.

Ho hum. Why are we talking about this, anyway? Does the patch make the code faster? Or just nicer?

Quoted 17 lines

Quoted 11 lines

hm. So if I have

	struct bar {
		unsigned long b;
	} __attribute__((packed));

	struct foo {
		unsigned long u;
		struct bar b;
	};

Quoted 5 lines

then the compiler can see that foo.b.b is well-aligned, regardless of the packedness.

Plus some crazy people compile the kernel with icc (or at least they used to). What happens there?

Quoted 8 lines

A quick grep for __attribute__((packed)) and __packed find around 900 hits, I'd probably find more if I'd look for syntactical variations. Some hits are in arch/{i386,x86_64,ia64}. At a glance it seems hard to configure a useful x86 kernel that doesn't involve any packed attribute. I take that as statistical proof that icc either has doesn't really work for building the kernel or groks packing. Any compiler not implementing gcc extensions is lost at building the kernel but that's old news.

No, icc surely supports attribute(packed). My point is that we shouldn't rely upon the gcc info file for this, because other compilers can (or could) be used to build the kernel.