On Thu, Oct 13, 2011 at 05:13:54PM -0700, Jesse Gross wrote: > On Thu, Oct 13, 2011 at 9:53 AM, Ben Pfaff <[email protected]> wrote: > > Unix 64-bit ABIs have two 64-bit types: "long" and "long long". ??Either of > > these is a reasonable choice for uint64_t (the userspace type) and for > > __u64 (the kernel type). ??Unfortunately, kernel and userspace don't > > necessarily agree on the choice, and in fact the choice varies across > > kernel versions and architectures. > > > > Now that OVS is actually using kernel types in its kernel header, this > > can make a difference: when __u64 and uint64_t differ, passing a pointer > > to __u64 to OVS function get_unaligned_u64() yields a compiler warning > > or error. > > > > This commit fixes up the problems of this type found in OVS, by avoiding > > calls to get_unaligned_u64() on these types. > > > > I suspect that this problem will recur in the future. ??I'm open to > > suggestions on how we can making "uint64_t *" and "__u64 *" always > > incompatible from the viewpoint of sparse. > > > > Reported-by: Pravin Shelar <[email protected]> > > --- > > Another solution would be to make get_unaligned_u64() accept both > > types with some kind of macro, like this: > > > > #define get_unaligned_u64(p) \ > > ?? ?? ?? ??(BUILD_ASSERT(sizeof *(p) == 8), \ > > ?? ?? ?? ?? sizeof (*(p) % 1), \ > > ?? ?? ?? ?? get_unaligned_u64__((const uint64_t *) (p))) > > I think you probably could convince sparse to complain by declaring a > type with __attribute__((address_space(XXX))) but it seems like more > mess than it is worth. > > I think this can only occur with things that should be using > get_unaligned_u64(), right? In that case, the macro magic that you > have above seems like a more comprehensive and self contained > solution. To me, that makes it a pretty significant improvement.
OK, here's v2: --8<--------------------------cut here-------------------------->8-- From: Ben Pfaff <[email protected]> Date: Fri, 14 Oct 2011 09:39:48 -0700 Subject: [PATCH] dpif-linux: Fix build with certain 64-bit kernel/userspace combinations. Unix 64-bit ABIs have two 64-bit types: "long" and "long long". Either of these is a reasonable choice for uint64_t (the userspace type) and for __u64 (the kernel type). Unfortunately, kernel and userspace don't necessarily agree on the choice, and in fact the choice varies across kernel versions and architectures. Now that OVS is actually using kernel types in its kernel header, this can make a difference: when __u64 and uint64_t differ, passing a pointer to __u64 to OVS function get_unaligned_u64() yields a compiler warning or error. This commit fixes up the problems of this type found in OVS, by making get_unaligned_u64() accept all 64-bit unsigned integer types, not just whichever one happens to be uint64_t. I didn't do the same thing for put_unaligned_u64() because it is less likely to be a problem in practice: usually, when userspace writes to kernel data structures it does so with copies that it knows to be aligned, so that it's not necessary to use put_unaligned_u64(). This problem won't occur for uint8_t, uint16_t, or uint32_t, since there is only one reasonable choice of type for each. It won't occur for ovs_be<N> because OVS always defines those as aliases for the kernel's __be<N> types when those are available. This compiled cleanly for me in Scientific Linux 6.0 x86-64. Reported-by: Pravin Shelar <[email protected]> --- lib/unaligned.h | 41 +++++++++++++++++++++++++++++++++++++---- lib/util.h | 8 ++++++++ 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/lib/unaligned.h b/lib/unaligned.h index f1aab23..a5ae4be 100644 --- a/lib/unaligned.h +++ b/lib/unaligned.h @@ -20,11 +20,12 @@ #include <stdint.h> #include "byte-order.h" #include "openvswitch/types.h" +#include "type-props.h" +#include "util.h" /* Public API. */ static inline uint16_t get_unaligned_u16(const uint16_t *); static inline uint32_t get_unaligned_u32(const uint32_t *); -static inline uint64_t get_unaligned_u64(const uint64_t *); static inline void put_unaligned_u16(uint16_t *, uint16_t); static inline void put_unaligned_u32(uint32_t *, uint32_t); static inline void put_unaligned_u64(uint64_t *, uint64_t); @@ -62,7 +63,7 @@ put_unaligned_##ABBREV(TYPE *p, TYPE x) \ GCC_UNALIGNED_ACCESSORS(uint16_t, u16); GCC_UNALIGNED_ACCESSORS(uint32_t, u32); -GCC_UNALIGNED_ACCESSORS(uint64_t, u64); +GCC_UNALIGNED_ACCESSORS(uint64_t, u64__); /* Special case: see below. */ GCC_UNALIGNED_ACCESSORS(ovs_be16, be16); GCC_UNALIGNED_ACCESSORS(ovs_be32, be32); @@ -102,7 +103,7 @@ static inline void put_unaligned_u32(uint32_t *p_, uint32_t x_) p[3] = x; } -static inline uint64_t get_unaligned_u64(const uint64_t *p_) +static inline uint64_t get_unaligned_u64__(const uint64_t *p_) { const uint8_t *p = (const uint8_t *) p_; return ntohll(((uint64_t) p[0] << 56) @@ -115,7 +116,7 @@ static inline uint64_t get_unaligned_u64(const uint64_t *p_) | p[7]); } -static inline void put_unaligned_u64(uint64_t *p_, uint64_t x_) +static inline void put_unaligned_u64__(uint64_t *p_, uint64_t x_) { uint8_t *p = (uint8_t *) p_; uint64_t x = ntohll(x_); @@ -140,6 +141,38 @@ static inline void put_unaligned_u64(uint64_t *p_, uint64_t x_) #define put_unaligned_be32 put_unaligned_u32 #define put_unaligned_be64 put_unaligned_u64 #endif + +/* uint64_t get_unaligned_u64(uint64_t *p); + * + * Returns the value of the possibly misaligned uint64_t at 'p'. 'p' may + * actually be any type that points to a 64-bit integer. That is, on Unix-like + * 32-bit ABIs, it may point to an "unsigned long long int", and on Unix-like + * 64-bit ABIs, it may point to an "unsigned long int" or an "unsigned long + * long int". + * + * This is special-cased because on some Linux targets, the kernel __u64 is + * unsigned long long int and the userspace uint64_t is unsigned long int, so + * that any single function prototype would fail to accept one or the other. + * + * Below, "sizeof (*(P) % 1)" verifies that *P has an integer type, since + * operands to % must be integers. + */ +#define get_unaligned_u64(P) \ + (BUILD_ASSERT(sizeof *(P) == 8), \ + BUILD_ASSERT_GCCONLY(!TYPE_IS_SIGNED(typeof(*(P)))), \ + (void) sizeof (*(P) % 1), \ + get_unaligned_u64__((const uint64_t *) (P))) + +/* Stores 'x' at possibly misaligned address 'p'. + * + * put_unaligned_u64() could be overloaded in the same way as + * get_unaligned_u64(), but so far it has not proven necessary. + */ +static inline void +put_unaligned_u64(uint64_t *p, uint64_t x) +{ + put_unaligned_u64__(p, x); +} /* Returns the value in 'x'. */ static inline uint64_t diff --git a/lib/util.h b/lib/util.h index 5ae0775..61039be 100644 --- a/lib/util.h +++ b/lib/util.h @@ -53,6 +53,14 @@ #define BUILD_ASSERT_DECL BOOST_STATIC_ASSERT #endif /* __cplusplus */ +#ifdef __GNUC__ +#define BUILD_ASSERT_GCCONLY(EXPR) BUILD_ASSERT(EXPR) +#define BUILD_ASSERT_DECL_GCCONLY(EXPR) BUILD_ASSERT_DECL(EXPR) +#else +#define BUILD_ASSERT_GCCONLY(EXPR) ((void) 0) +#define BUILD_ASSERT_DECL_GCCONLY(EXPR) ((void) 0) +#endif + extern const char *program_name; /* Returns the number of elements in ARRAY. */ -- 1.7.4.4 _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
