[systemd-devel] PATCH: fix sparse warnings

[Josh Triplett]

On Mon, Mar 05, 2012 at 03:21:12PM +0100, Frederic Crozat wrote:
> Le lundi 05 mars 2012 ? 15:10 +0100, Lennart Poettering a ?crit :
> > I have little experience with sparse, but iirc it knows decorators for
> > variables for le/be, right? Is this something we might want to use in
> > the journal to avoid LE/BE issues like those you tracked down?
> 
> Well, I spend almost a day trying to get sparse to spot endian-ness
> errors but couldn't get it to work properly :(
> 
> The idea would be to replace any uint64_t type with __le64
> (from /usr/include/linux/types.h) in data structures written on disk and
> make sure only function returning __le64 are used to modify those
> variables. Unfortunately, I wasn't able to teach sparse about htole64 /
> le64toh. 
> 
> Help welcome ;)

Sparse's endianness support works via the attribute "bitwise", which
applies to an integral type and creates a new incompatible type every
time you use it.  Thus, if you write:

typedef uint64_t __attribute__((bitwise)) le64;
typedef uint64_t __attribute__((bitwise)) be64;

then you have types "le64" and "be64" which Sparse considers
incompatible with uint64_t.  (The odd name "bitwise" refers to what you
can do with such a type: you can do bitwise operations with two values
of the same bitwise type, but you can't do arithmetic or similar.)

The corresponding __attribute__((force)) should appear on a type used in
a cast, to make that cast suppress warnings (including those about
bitwise).  The conversion functions should use these casts to convert
the type, in addition to actually doing any necessary byteswapping.  In
order to do that, I'd suggest actually using static inline functions
with real types, rather than macros that have no types.  (You can use
hacks to do typechecking in macros, but you'll end up with something
much worse than just defining a function, and more importantly your
warning messages won't look as clear.)  One of these days, I'd love to
see glibc add built-in support for Sparse-style endian-specific types,
but for now, you'll have to write your own functions.  Assuming that you
don't want to change the names of all the byteswapping functions, I'd
suggest defining your own set of static inlines with the same names;
unfortunately that means #undef-ing all the ones defined by endian.h.

Sparse defines the preprocessor symbol __CHECKER__, which you can use to
detect Sparse and use Sparse-specific attributes.
include/linux/compiler.h in the kernel uses this to define macros like
__bitwise and __force which wrap the corresponding Sparse attributes,
and which become no-ops when compiling with GCC.  I'd recommend
following that approach.

I've attached a header file which should provide all the endianness
checking you need.  Just include it in place of endian.h everywhere you
currently include endian.h.  I stuck an all-permissive MIT license on
it, for maximum possible reuse.

- Josh Triplett
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sparse-endian.h
Type: text/x-chdr
Size: 3412 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20120307/8b5719f9/attachment.h>