* endianness and sparse warnings
@ 2008-08-29 14:54 Geert Uytterhoeven
2008-08-29 16:24 ` Harvey Harrison
0 siblings, 1 reply; 3+ messages in thread
From: Geert Uytterhoeven @ 2008-08-29 14:54 UTC (permalink / raw)
To: Linux Kernel Development; +Cc: Harvey Harrison
[-- Attachment #1: Type: TEXT/PLAIN, Size: 12653 bytes --]
With `make C=1 CF="-D__CHECK_ENDIAN__"', you can let sparse check for bad
handling of endian-annotated data.
Unfortunately several of the accessors for endian-annotated data do not cause
sparse warnings.
Summarized:
- [bl]e{16,32,64}_to_cpu() is OK
- [bl]e{16,32,64}_to_cpup() (aka get_aligned_[bl]e{16,32,64}() ;-) is OK
- get_unaligned_[bl]e{16,32,64} is not OK
- __get_unaligned_[bl]e() is partially OK, as long as you don't use it on
non-annotated data, but
o it's meant for internal use only
o it incorrectly causes sparse warnings when assigning the resulting
value to a non-annotated variable
To check it for yourself, plug in the code below into any kernel source file.
#include <asm/unaligned.h>
__u8 d8;
__u16 d16;
__u32 d32;
__u64 d64;
__be16 be16;
__be32 be32;
__be64 be64;
__le16 le16;
__le32 le32;
__le64 le64;
char dr[16];
#define get_aligned_be16 be16_to_cpup
#define get_aligned_be32 be32_to_cpup
#define get_aligned_be64 be64_to_cpup
#define get_aligned_le16 le16_to_cpup
#define get_aligned_le32 le32_to_cpup
#define get_aligned_le64 le64_to_cpup
int test(void)
{
u64 x = 0;
x ^= d8;
// ----------------------------------------------------------------------------
x ^= d16; /* OK */
x ^= d32; /* OK */
x ^= d64; /* OK */
x ^= be16; /* sparse warning */
x ^= be32; /* sparse warning */
x ^= be64; /* sparse warning */
x ^= le16; /* sparse warning */
x ^= le32; /* sparse warning */
x ^= le64; /* sparse warning */
// ----------------------------------------------------------------------------
x ^= be16_to_cpu(d16); /* sparse warning */
x ^= be16_to_cpu(d32); /* sparse warning */
x ^= be16_to_cpu(d64); /* sparse warning */
x ^= be16_to_cpu(be16); /* OK */
x ^= be16_to_cpu(be32); /* sparse warning */
x ^= be16_to_cpu(be64); /* sparse warning */
x ^= be16_to_cpu(le16); /* sparse warning */
x ^= be16_to_cpu(le32); /* sparse warning */
x ^= be16_to_cpu(le64); /* sparse warning */
x ^= be32_to_cpu(d16); /* sparse warning */
x ^= be32_to_cpu(d32); /* sparse warning */
x ^= be32_to_cpu(d64); /* sparse warning */
x ^= be32_to_cpu(be16); /* sparse warning */
x ^= be32_to_cpu(be32); /* OK */
x ^= be32_to_cpu(be64); /* sparse warning */
x ^= be32_to_cpu(le16); /* sparse warning */
x ^= be32_to_cpu(le32); /* sparse warning */
x ^= be32_to_cpu(le64); /* sparse warning */
x ^= be64_to_cpu(d16); /* sparse warning */
x ^= be64_to_cpu(d32); /* sparse warning */
x ^= be64_to_cpu(d64); /* sparse warning */
x ^= be64_to_cpu(be16); /* sparse warning */
x ^= be64_to_cpu(be32); /* sparse warning */
x ^= be64_to_cpu(be64); /* OK */
x ^= be64_to_cpu(le16); /* sparse warning */
x ^= be64_to_cpu(le32); /* sparse warning */
x ^= be64_to_cpu(le64); /* sparse warning */
x ^= le16_to_cpu(d16); /* sparse warning */
x ^= le16_to_cpu(d32); /* sparse warning */
x ^= le16_to_cpu(d64); /* sparse warning */
x ^= le16_to_cpu(be16); /* sparse warning */
x ^= le16_to_cpu(be32); /* sparse warning */
x ^= le16_to_cpu(be64); /* sparse warning */
x ^= le16_to_cpu(le16); /* OK */
x ^= le16_to_cpu(le32); /* sparse warning */
x ^= le16_to_cpu(le64); /* sparse warning */
x ^= le32_to_cpu(d16); /* sparse warning */
x ^= le32_to_cpu(d32); /* sparse warning */
x ^= le32_to_cpu(d64); /* sparse warning */
x ^= le32_to_cpu(be16); /* sparse warning */
x ^= le32_to_cpu(be32); /* sparse warning */
x ^= le32_to_cpu(be64); /* sparse warning */
x ^= le32_to_cpu(le16); /* sparse warning */
x ^= le32_to_cpu(le32); /* OK */
x ^= le32_to_cpu(le64); /* sparse warning */
x ^= le64_to_cpu(d16); /* sparse warning */
x ^= le64_to_cpu(d32); /* sparse warning */
x ^= le64_to_cpu(d64); /* sparse warning */
x ^= le64_to_cpu(be16); /* sparse warning */
x ^= le64_to_cpu(be32); /* sparse warning */
x ^= le64_to_cpu(be64); /* sparse warning */
x ^= le64_to_cpu(le16); /* sparse warning */
x ^= le64_to_cpu(le32); /* sparse warning */
x ^= le64_to_cpu(le64); /* OK */
// ----------------------------------------------------------------------------
x ^= get_unaligned_be16(&d16); /* NO WARNING!! */
x ^= get_unaligned_be16(&d32); /* NO WARNING!! */
x ^= get_unaligned_be16(&d64); /* NO WARNING!! */
x ^= get_unaligned_be16(&be16); /* OK */
x ^= get_unaligned_be16(&be32); /* NO WARNING!! */
x ^= get_unaligned_be16(&be64); /* NO WARNING!! */
x ^= get_unaligned_be16(&le16); /* NO WARNING!! */
x ^= get_unaligned_be16(&le32); /* NO WARNING!! */
x ^= get_unaligned_be16(&le64); /* NO WARNING!! */
x ^= get_unaligned_be16(&dr[1]); /* OK */
x ^= get_unaligned_be32(&d16); /* NO WARNING!! */
x ^= get_unaligned_be32(&d32); /* NO WARNING!! */
x ^= get_unaligned_be32(&d64); /* NO WARNING!! */
x ^= get_unaligned_be32(&be16); /* NO WARNING!! */
x ^= get_unaligned_be32(&be32); /* OK */
x ^= get_unaligned_be32(&be64); /* NO WARNING!! */
x ^= get_unaligned_be32(&le16); /* NO WARNING!! */
x ^= get_unaligned_be32(&le32); /* NO WARNING!! */
x ^= get_unaligned_be32(&le64); /* NO WARNING!! */
x ^= get_unaligned_be32(&dr[1]); /* OK */
x ^= get_unaligned_be64(&d16); /* NO WARNING!! */
x ^= get_unaligned_be64(&d32); /* NO WARNING!! */
x ^= get_unaligned_be64(&d64); /* NO WARNING!! */
x ^= get_unaligned_be64(&be16); /* NO WARNING!! */
x ^= get_unaligned_be64(&be32); /* NO WARNING!! */
x ^= get_unaligned_be64(&be64); /* OK */
x ^= get_unaligned_be64(&le16); /* NO WARNING!! */
x ^= get_unaligned_be64(&le32); /* NO WARNING!! */
x ^= get_unaligned_be64(&le64); /* NO WARNING!! */
x ^= get_unaligned_be64(&dr[1]); /* OK */
x ^= get_unaligned_le16(&d16); /* NO WARNING!! */
x ^= get_unaligned_le16(&d32); /* NO WARNING!! */
x ^= get_unaligned_le16(&d64); /* NO WARNING!! */
x ^= get_unaligned_le16(&be16); /* NO WARNING!! */
x ^= get_unaligned_le16(&be32); /* NO WARNING!! */
x ^= get_unaligned_le16(&be64); /* NO WARNING!! */
x ^= get_unaligned_le16(&le16); /* OK */
x ^= get_unaligned_le16(&le32); /* NO WARNING!! */
x ^= get_unaligned_le16(&le64); /* NO WARNING!! */
x ^= get_unaligned_le16(&dr[1]); /* OK */
x ^= get_unaligned_le32(&d16); /* NO WARNING!! */
x ^= get_unaligned_le32(&d32); /* NO WARNING!! */
x ^= get_unaligned_le32(&d64); /* NO WARNING!! */
x ^= get_unaligned_le32(&be16); /* NO WARNING!! */
x ^= get_unaligned_le32(&be32); /* NO WARNING!! */
x ^= get_unaligned_le32(&be64); /* NO WARNING!! */
x ^= get_unaligned_le32(&le16); /* NO WARNING!! */
x ^= get_unaligned_le32(&le32); /* OK */
x ^= get_unaligned_le32(&le64); /* NO WARNING!! */
x ^= get_unaligned_le16(&dr[1]); /* OK */
x ^= get_unaligned_le64(&d16); /* NO WARNING!! */
x ^= get_unaligned_le64(&d32); /* NO WARNING!! */
x ^= get_unaligned_le64(&d64); /* NO WARNING!! */
x ^= get_unaligned_le64(&be16); /* NO WARNING!! */
x ^= get_unaligned_le64(&be32); /* NO WARNING!! */
x ^= get_unaligned_le64(&be64); /* NO WARNING!! */
x ^= get_unaligned_le64(&le16); /* NO WARNING!! */
x ^= get_unaligned_le64(&le32); /* NO WARNING!! */
x ^= get_unaligned_le64(&le64); /* OK */
x ^= get_unaligned_le16(&dr[1]); /* OK */
// ----------------------------------------------------------------------------
x ^= get_unaligned(&d16); /* OK */
x ^= get_unaligned(&d32); /* OK */
x ^= get_unaligned(&d64); /* OK */
x ^= get_unaligned(&be16); /* sparse warning */
x ^= get_unaligned(&be32); /* sparse warning */
x ^= get_unaligned(&be64); /* sparse warning */
x ^= get_unaligned(&le16); /* sparse warning */
x ^= get_unaligned(&le32); /* sparse warning */
x ^= get_unaligned(&le64); /* sparse warning */
x ^= get_unaligned(&dr[1]); /* OK */
x ^= __get_unaligned_be(&d16); /* NO WARNING!! */
x ^= __get_unaligned_be(&d32); /* NO WARNING!! */
x ^= __get_unaligned_be(&d64); /* NO WARNING!! */
x ^= __get_unaligned_be(&be16); /* sparse warning */
x ^= __get_unaligned_be(&be32); /* sparse warning */
x ^= __get_unaligned_be(&be64); /* sparse warning */
x ^= __get_unaligned_be(&le16); /* sparse warning */
x ^= __get_unaligned_be(&le32); /* sparse warning */
x ^= __get_unaligned_be(&le64); /* sparse warning */
x ^= __get_unaligned_be(&dr[1]); /* NO WARNING!! */
x ^= __get_unaligned_le(&d16); /* NO WARNING!! */
x ^= __get_unaligned_le(&d32); /* NO WARNING!! */
x ^= __get_unaligned_le(&d64); /* NO WARNING!! */
x ^= __get_unaligned_le(&be16); /* sparse warning */
x ^= __get_unaligned_le(&be32); /* sparse warning */
x ^= __get_unaligned_le(&be64); /* sparse warning */
x ^= __get_unaligned_le(&le16); /* sparse warning */
x ^= __get_unaligned_le(&le32); /* sparse warning */
x ^= __get_unaligned_le(&le64); /* sparse warning */
x ^= __get_unaligned_le(&dr[1]); /* NO WARNING!! */
// ----------------------------------------------------------------------------
x ^= get_aligned_be16(&d16); /* sparse warning */
x ^= get_aligned_be16(&d32); /* gcc/sparse warning */
x ^= get_aligned_be16(&d64); /* gcc/sparse warning */
x ^= get_aligned_be16(&be16); /* OK */
x ^= get_aligned_be16(&be32); /* gcc/sparse warning */
x ^= get_aligned_be16(&be64); /* gcc/sparse warning */
x ^= get_aligned_be16(&le16); /* sparse warning */
x ^= get_aligned_be16(&le32); /* gcc/sparse warning */
x ^= get_aligned_be16(&le64); /* gcc/sparse warning */
x ^= get_aligned_be16(&dr[1]); /* gcc/sparse warning */
x ^= get_aligned_be32(&d16); /* gcc/sparse warning */
x ^= get_aligned_be32(&d32); /* sparse warning */
x ^= get_aligned_be32(&d64); /* gcc/sparse warning */
x ^= get_aligned_be32(&be16); /* gcc/sparse warning */
x ^= get_aligned_be32(&be32); /* OK */
x ^= get_aligned_be32(&be64); /* gcc/sparse warning */
x ^= get_aligned_be32(&le16); /* gcc/sparse warning */
x ^= get_aligned_be32(&le32); /* sparse warning */
x ^= get_aligned_be32(&le64); /* gcc/sparse warning */
x ^= get_aligned_be32(&dr[1]); /* gcc/sparse warning */
x ^= get_aligned_be64(&d16); /* gcc/sparse warning */
x ^= get_aligned_be64(&d32); /* gcc/sparse warning */
x ^= get_aligned_be64(&d64); /* sparse warning */
x ^= get_aligned_be64(&be16); /* gcc/sparse warning */
x ^= get_aligned_be64(&be32); /* gcc/sparse warning */
x ^= get_aligned_be64(&be64); /* OK */
x ^= get_aligned_be64(&le16); /* gcc/sparse warning */
x ^= get_aligned_be64(&le32); /* gcc/sparse warning */
x ^= get_aligned_be64(&le64); /* sparse warning */
x ^= get_aligned_be64(&dr[1]); /* gcc/sparse warning */
x ^= get_aligned_le16(&d16); /* sparse warning */
x ^= get_aligned_le16(&d32); /* gcc/sparse warning */
x ^= get_aligned_le16(&d64); /* gcc/sparse warning */
x ^= get_aligned_le16(&be16); /* sparse warning */
x ^= get_aligned_le16(&be32); /* gcc/sparse warning */
x ^= get_aligned_le16(&be64); /* gcc/sparse warning */
x ^= get_aligned_le16(&le16); /* OK */
x ^= get_aligned_le16(&le32); /* gcc/sparse warning */
x ^= get_aligned_le16(&le64); /* gcc/sparse warning */
x ^= get_aligned_le16(&dr[1]); /* gcc/sparse warning */
x ^= get_aligned_le32(&d16); /* gcc/sparse warning */
x ^= get_aligned_le32(&d32); /* sparse warning */
x ^= get_aligned_le32(&d64); /* gcc/sparse warning */
x ^= get_aligned_le32(&be16); /* gcc/sparse warning */
x ^= get_aligned_le32(&be32); /* sparse warning */
x ^= get_aligned_le32(&be64); /* gcc/sparse warning */
x ^= get_aligned_le32(&le16); /* gcc/sparse warning */
x ^= get_aligned_le32(&le32); /* OK */
x ^= get_aligned_le32(&le64); /* gcc/sparse warning */
x ^= get_aligned_le16(&dr[1]); /* gcc/sparse warning */
x ^= get_aligned_le64(&d16); /* gcc/sparse warning */
x ^= get_aligned_le64(&d32); /* gcc/sparse warning */
x ^= get_aligned_le64(&d64); /* sparse warning */
x ^= get_aligned_le64(&be16); /* gcc/sparse warning */
x ^= get_aligned_le64(&be32); /* gcc/sparse warning */
x ^= get_aligned_le64(&be64); /* sparse warning */
x ^= get_aligned_le64(&le16); /* gcc/sparse warning */
x ^= get_aligned_le64(&le32); /* gcc/sparse warning */
x ^= get_aligned_le64(&le64); /* OK */
x ^= get_aligned_le16(&dr[1]); /* gcc/sparse warning */
// ----------------------------------------------------------------------------
return x;
}
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: endianness and sparse warnings
2008-08-29 14:54 endianness and sparse warnings Geert Uytterhoeven
@ 2008-08-29 16:24 ` Harvey Harrison
2008-09-01 9:23 ` Geert Uytterhoeven
0 siblings, 1 reply; 3+ messages in thread
From: Harvey Harrison @ 2008-08-29 16:24 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Linux Kernel Development
On Fri, 2008-08-29 at 16:54 +0200, Geert Uytterhoeven wrote:
> With `make C=1 CF="-D__CHECK_ENDIAN__"', you can let sparse check for bad
> handling of endian-annotated data.
>
> Unfortunately several of the accessors for endian-annotated data do not cause
> sparse warnings.
I'll try and give some background on why the unaligned versions are implemented
this way.
The get_unaligned helpers were meant to replace two kinds of use (using le16 as
an example)
char *ptr;
1 - le16_to_cpu(get_unaligned((__le16 *)ptr))
2 - u16 val = ptr[0] | (ptr[1] << 8)
The places where 1 was replaced with the unaligned helpers would have been fine
with an annotated version as it already had the cast to a proper type.
The places where 2 was replaced would have required a new cast to __le16 *.
Lots of places that were using 2 are drivers that have some data area pointed
to by a char * and they are grabbing values from there at known offsets,
for these users, the need for extra casting was quite ugly and it was known
exactly how many bytes and in what endianness you are reading as it is
right in the function name so I thought it would be ok to omit the annotation
on the parameter.
u16 foo, bar;
char *my_data;
foo = get_unaligned_le16((__le16 *)my_data); /* if unaligned helpers were annotated */
bar = get_unaligned_le16(my_data); /* current version */
>
> Summarized:
> - [bl]e{16,32,64}_to_cpu() is OK
> - [bl]e{16,32,64}_to_cpup() (aka get_aligned_[bl]e{16,32,64}() ;-) is OK
> - get_unaligned_[bl]e{16,32,64} is not OK
> - __get_unaligned_[bl]e() is partially OK, as long as you don't use it on
> non-annotated data, but
> o it's meant for internal use only
> o it incorrectly causes sparse warnings when assigning the resulting
> value to a non-annotated variable
Almost... __get_unaligned_le16 etc are _never_ to be used...as some arches
choose to use memmove-based implementations, and on arches where unaligned
access is OK, they don't exist _at_all_.
Cheers,
Harvey
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: endianness and sparse warnings
2008-08-29 16:24 ` Harvey Harrison
@ 2008-09-01 9:23 ` Geert Uytterhoeven
0 siblings, 0 replies; 3+ messages in thread
From: Geert Uytterhoeven @ 2008-09-01 9:23 UTC (permalink / raw)
To: Harvey Harrison; +Cc: Linux Kernel Development
[-- Attachment #1: Type: TEXT/PLAIN, Size: 3009 bytes --]
On Fri, 29 Aug 2008, Harvey Harrison wrote:
> On Fri, 2008-08-29 at 16:54 +0200, Geert Uytterhoeven wrote:
> > With `make C=1 CF="-D__CHECK_ENDIAN__"', you can let sparse check for bad
> > handling of endian-annotated data.
> >
> > Unfortunately several of the accessors for endian-annotated data do not cause
> > sparse warnings.
>
> I'll try and give some background on why the unaligned versions are implemented
> this way.
>
> The get_unaligned helpers were meant to replace two kinds of use (using le16 as
> an example)
>
> char *ptr;
>
> 1 - le16_to_cpu(get_unaligned((__le16 *)ptr))
> 2 - u16 val = ptr[0] | (ptr[1] << 8)
>
> The places where 1 was replaced with the unaligned helpers would have been fine
> with an annotated version as it already had the cast to a proper type.
>
> The places where 2 was replaced would have required a new cast to __le16 *.
>
> Lots of places that were using 2 are drivers that have some data area pointed
> to by a char * and they are grabbing values from there at known offsets,
> for these users, the need for extra casting was quite ugly and it was known
> exactly how many bytes and in what endianness you are reading as it is
> right in the function name so I thought it would be ok to omit the annotation
> on the parameter.
>
> u16 foo, bar;
> char *my_data;
>
> foo = get_unaligned_le16((__le16 *)my_data); /* if unaligned helpers were annotated */
> bar = get_unaligned_le16(my_data); /* current version */
Indeed, if you have a void/char *, this works, and there's not much annotation
you can do here.
But please consider this case:
struct xxx {
u8 a;
__le16 b;
} __attribute__ ((packed));
struct xxx *p;
u16 foo = get_unaligned_le16(&p->b);
Here you could have typechecking.
Note that a `get_unaligned_le()' that handles different sizes automatically
would work.
> > Summarized:
> > - [bl]e{16,32,64}_to_cpu() is OK
> > - [bl]e{16,32,64}_to_cpup() (aka get_aligned_[bl]e{16,32,64}() ;-) is OK
> > - get_unaligned_[bl]e{16,32,64} is not OK
> > - __get_unaligned_[bl]e() is partially OK, as long as you don't use it on
> > non-annotated data, but
> > o it's meant for internal use only
> > o it incorrectly causes sparse warnings when assigning the resulting
> > value to a non-annotated variable
>
> Almost... __get_unaligned_le16 etc are _never_ to be used...as some arches
> choose to use memmove-based implementations, and on arches where unaligned
> access is OK, they don't exist _at_all_.
So perhaps we want a public get_unaligned_[bl]e() for the case above?
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2008-09-01 9:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-08-29 14:54 endianness and sparse warnings Geert Uytterhoeven
2008-08-29 16:24 ` Harvey Harrison
2008-09-01 9:23 ` Geert Uytterhoeven
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox