[systemd-devel] PATCH: fix sparse warnings

[Frederic Crozat]

Le mercredi 07 mars 2012 à 06:34 -0800, Josh Triplett a écrit :
> 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.

Thanks Josh, I tried to do something similar to your header and failed
(but I'm not sparse expert), so I'll test it and report the results.

-- 
Frederic Crozat <fcrozat at suse.com>
SUSE