gfp_t is a bitwise type, casting to unsigned long produces a
warning. Suppress it with __force.
Otherwise sparse complains thusly:
include/linux/kmemtrace.h:33:2: warning: cast from restricted gfp_t
Signed-off-by: Harvey Harrison <[email protected]>
---
Eduard, this is a local patch I've had sitting around in my sparse testing
tree. I'm really not sure what the appropriate format specifier is for a
gfp_t, but I don't think the trace infrastructure has support for it
anyway...so if you are going to keep casting to unsigned long you'll need
this...perhaps with a comment why added.
include/linux/kmemtrace.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/linux/kmemtrace.h b/include/linux/kmemtrace.h
index 5bea8ea..9d82085 100644
--- a/include/linux/kmemtrace.h
+++ b/include/linux/kmemtrace.h
@@ -34,7 +34,7 @@ static inline void kmemtrace_mark_alloc_node(enum kmemtrace_type_id type_id,
"bytes_req %lu bytes_alloc %lu gfp_flags %lu node %d",
type_id, call_site, (unsigned long) ptr,
(unsigned long) bytes_req, (unsigned long) bytes_alloc,
- (unsigned long) gfp_flags, node);
+ (__force unsigned long)gfp_flags, node);
}
static inline void kmemtrace_mark_free(enum kmemtrace_type_id type_id,
--
1.6.0.3.756.gb776d
On Fri, Nov 07, 2008 at 10:58:41AM -0800, Harvey Harrison wrote:
> --- a/include/linux/kmemtrace.h
> +++ b/include/linux/kmemtrace.h
> @@ -34,7 +34,7 @@ static inline void kmemtrace_mark_alloc_node(enum kmemtrace_type_id type_id,
> "bytes_req %lu bytes_alloc %lu gfp_flags %lu node %d",
> type_id, call_site, (unsigned long) ptr,
> (unsigned long) bytes_req, (unsigned long) bytes_alloc,
> - (unsigned long) gfp_flags, node);
> + (__force unsigned long)gfp_flags, node);
gfp_t is "unsigned int" actually. These casts are bogus.
Subject: How do I printk <type> correctly?
If variable is of Type use printk format specifier.
---------------------------------------------------------
int %d or %x
unsigned int %u or %x
long %ld ot %lx
unsigned long %lu or %lx
long long %lld or %llx
unsigned long long %llu or %llx
size_t %zu or %zx
ssize_t %zd or %zx
Raw pointer value SHOULD be printed with %p.
u64 SHOULD be printed with %llu/%llx, (unsigned long long):
printk("%llu", (unsigned long long)u64_var);
s64 SHOULD be printed with %lld/%llx, (long long):
printk("%lld", (long long)s64_var);
If type is dependent on config option (sector_t), use format specifier
of biggest type and explicitly cast to it.
Reminder: sizeof() result is of type size_t.
Thank you for your cooperation.
On Fri, 2008-11-07 at 22:20 +0300, Alexey Dobriyan wrote:
> On Fri, Nov 07, 2008 at 10:58:41AM -0800, Harvey Harrison wrote:
> > --- a/include/linux/kmemtrace.h
> > +++ b/include/linux/kmemtrace.h
> > @@ -34,7 +34,7 @@ static inline void kmemtrace_mark_alloc_node(enum kmemtrace_type_id type_id,
> > "bytes_req %lu bytes_alloc %lu gfp_flags %lu node %d",
> > type_id, call_site, (unsigned long) ptr,
> > (unsigned long) bytes_req, (unsigned long) bytes_alloc,
> > - (unsigned long) gfp_flags, node);
> > + (__force unsigned long)gfp_flags, node);
>
> gfp_t is "unsigned int" actually. These casts are bogus.
>
> Subject: How do I printk <type> correctly?
>
> If variable is of Type use printk format specifier.
> ---------------------------------------------------------
> int %d or %x
> unsigned int %u or %x
> long %ld ot %lx
> unsigned long %lu or %lx
> long long %lld or %llx
> unsigned long long %llu or %llx
> size_t %zu or %zx
> ssize_t %zd or %zx
>
Perhaps add gfp_t to the list ;-)
Thanks.
Harvey
* Harvey Harrison ([email protected]) wrote:
> On Fri, 2008-11-07 at 22:20 +0300, Alexey Dobriyan wrote:
> > On Fri, Nov 07, 2008 at 10:58:41AM -0800, Harvey Harrison wrote:
> > > --- a/include/linux/kmemtrace.h
> > > +++ b/include/linux/kmemtrace.h
> > > @@ -34,7 +34,7 @@ static inline void kmemtrace_mark_alloc_node(enum kmemtrace_type_id type_id,
> > > "bytes_req %lu bytes_alloc %lu gfp_flags %lu node %d",
> > > type_id, call_site, (unsigned long) ptr,
> > > (unsigned long) bytes_req, (unsigned long) bytes_alloc,
> > > - (unsigned long) gfp_flags, node);
> > > + (__force unsigned long)gfp_flags, node);
> >
> > gfp_t is "unsigned int" actually. These casts are bogus.
> >
> > Subject: How do I printk <type> correctly?
> >
> > If variable is of Type use printk format specifier.
> > ---------------------------------------------------------
> > int %d or %x
> > unsigned int %u or %x
> > long %ld ot %lx
> > unsigned long %lu or %lx
> > long long %lld or %llx
> > unsigned long long %llu or %llx
> > size_t %zu or %zx
> > ssize_t %zd or %zx
> >
>
> Perhaps add gfp_t to the list ;-)
>
I think a cast
(__force unsigned) could be required for checker ?
#ifdef __CHECKER__
#define __bitwise__ __attribute__((bitwise))
#else
#define __bitwise__
#endif
typedef unsigned __bitwise__ gfp_t;
OTOH, adding a
%uB to printk so it supports bitwise variables may not hurt...
Mathieu
> Thanks.
>
> Harvey
>
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
On Fri, 2008-11-07 at 14:38 -0500, Mathieu Desnoyers wrote:
> I think a cast
>
> (__force unsigned) could be required for checker ?
No, that's not the way bitwise works. Printk will treat it as an
unsigned it just fine. bitwise will only warn if you treat the value
as anything other than a bitmask (| & ^) will be fine as long as it
is done with another gfp_t....all the arithmatic operators +-* etc
will warn.
The following looks like the correct way to fix this.
From: Harvey Harrison <[email protected]>
Subject: [PATCH] kmemtrace: gfp_t is an unsigned int, not an unsigned long
Signed-off-by: Harvey Harrison <[email protected]>
---
include/linux/kmemtrace.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/kmemtrace.h b/include/linux/kmemtrace.h
index 5bea8ea..80e9a7a 100644
--- a/include/linux/kmemtrace.h
+++ b/include/linux/kmemtrace.h
@@ -31,10 +31,10 @@ static inline void kmemtrace_mark_alloc_node(enum kmemtrace_type_id type_id,
int node)
{
trace_mark(kmemtrace_alloc, "type_id %d call_site %lu ptr %lu "
- "bytes_req %lu bytes_alloc %lu gfp_flags %lu node %d",
+ "bytes_req %lu bytes_alloc %lu gfp_flags %u node %d",
type_id, call_site, (unsigned long) ptr,
(unsigned long) bytes_req, (unsigned long) bytes_alloc,
- (unsigned long) gfp_flags, node);
+ gfp_flags, node);
}
static inline void kmemtrace_mark_free(enum kmemtrace_type_id type_id,
--
1.6.0.3.756.gb776d
On Fri, Nov 07, 2008 at 11:50:29AM -0800, Harvey Harrison wrote:
> --- a/include/linux/kmemtrace.h
> +++ b/include/linux/kmemtrace.h
> @@ -31,10 +31,10 @@ static inline void kmemtrace_mark_alloc_node(enum kmemtrace_type_id type_id,
> int node)
> {
> trace_mark(kmemtrace_alloc, "type_id %d call_site %lu ptr %lu "
> - "bytes_req %lu bytes_alloc %lu gfp_flags %lu node %d",
> + "bytes_req %lu bytes_alloc %lu gfp_flags %u node %d",
> type_id, call_site, (unsigned long) ptr,
> (unsigned long) bytes_req, (unsigned long) bytes_alloc,
> - (unsigned long) gfp_flags, node);
> + gfp_flags, node);
All other casts are bogus too.
On Fri, Nov 07, 2008 at 10:20:29PM +0300, Alexey Dobriyan wrote:
> On Fri, Nov 07, 2008 at 10:58:41AM -0800, Harvey Harrison wrote:
> > --- a/include/linux/kmemtrace.h
> > +++ b/include/linux/kmemtrace.h
> > @@ -34,7 +34,7 @@ static inline void kmemtrace_mark_alloc_node(enum kmemtrace_type_id type_id,
> > "bytes_req %lu bytes_alloc %lu gfp_flags %lu node %d",
> > type_id, call_site, (unsigned long) ptr,
> > (unsigned long) bytes_req, (unsigned long) bytes_alloc,
> > - (unsigned long) gfp_flags, node);
> > + (__force unsigned long)gfp_flags, node);
>
> gfp_t is "unsigned int" actually. These casts are bogus.
>
> Subject: How do I printk <type> correctly?
>
> If variable is of Type use printk format specifier.
> ---------------------------------------------------------
> int %d or %x
> unsigned int %u or %x
> long %ld ot %lx
> unsigned long %lu or %lx
> long long %lld or %llx
> unsigned long long %llu or %llx
> size_t %zu or %zx
> ssize_t %zd or %zx
>
> Raw pointer value SHOULD be printed with %p.
>
> u64 SHOULD be printed with %llu/%llx, (unsigned long long):
>
> printk("%llu", (unsigned long long)u64_var);
>
> s64 SHOULD be printed with %lld/%llx, (long long):
>
> printk("%lld", (long long)s64_var);
>
> If type is dependent on config option (sector_t), use format specifier
> of biggest type and explicitly cast to it.
>
> Reminder: sizeof() result is of type size_t.
>
> Thank you for your cooperation.
Hi,
Actually, "%zu" was the first thing that crossed my mind too. But we
don't want to carry such types into the probe callbacks. It's a lot
easier to see which u* an unsigned long fits into than it is for
size_t. So we take care of this inside a wrapper; the sooner, the better.
Also take into account that debugging code usually casts pointers to
unsigned long. This can easily be seen by looking at _RET_IP_ definition
or SLAB code. I think there's a very good reason to do so, since it adds
opacity to something that's not supposed to be used as a pointer.
Cheers,
Eduard
On Fri, Nov 07, 2008 at 10:58:41AM -0800, Harvey Harrison wrote:
> gfp_t is a bitwise type, casting to unsigned long produces a
> warning. Suppress it with __force.
>
> Otherwise sparse complains thusly:
> include/linux/kmemtrace.h:33:2: warning: cast from restricted gfp_t
>
> Signed-off-by: Harvey Harrison <[email protected]>
> ---
> Eduard, this is a local patch I've had sitting around in my sparse testing
> tree. I'm really not sure what the appropriate format specifier is for a
> gfp_t, but I don't think the trace infrastructure has support for it
> anyway...so if you are going to keep casting to unsigned long you'll need
> this...perhaps with a comment why added.
>
> include/linux/kmemtrace.h | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/kmemtrace.h b/include/linux/kmemtrace.h
> index 5bea8ea..9d82085 100644
> --- a/include/linux/kmemtrace.h
> +++ b/include/linux/kmemtrace.h
> @@ -34,7 +34,7 @@ static inline void kmemtrace_mark_alloc_node(enum kmemtrace_type_id type_id,
> "bytes_req %lu bytes_alloc %lu gfp_flags %lu node %d",
> type_id, call_site, (unsigned long) ptr,
> (unsigned long) bytes_req, (unsigned long) bytes_alloc,
> - (unsigned long) gfp_flags, node);
> + (__force unsigned long)gfp_flags, node);
> }
>
> static inline void kmemtrace_mark_free(enum kmemtrace_type_id type_id,
> --
> 1.6.0.3.756.gb776d
Thanks. It looks like the right thing to do, especially that linux/gfp.h does
it, IIRC. Although there's a missing whitespace and the commit name is a
bit longish. I would recommend "kmemtrace: Suppress gfp_t casting
warning with __force.", and telling that __bitwise & sparse story within
the commit description. Could you fix it so Pekka can cleanly send it
to Linus?
Thanks,
Eduard
On Fri, Nov 07, 2008 at 10:58:41AM -0800, Harvey Harrison wrote:
> gfp_t is a bitwise type, casting to unsigned long produces a
> warning. Suppress it with __force.
>
> Otherwise sparse complains thusly:
> include/linux/kmemtrace.h:33:2: warning: cast from restricted gfp_t
>
> Signed-off-by: Harvey Harrison <[email protected]>
> ---
> Eduard, this is a local patch I've had sitting around in my sparse testing
> tree. I'm really not sure what the appropriate format specifier is for a
> gfp_t, but I don't think the trace infrastructure has support for it
> anyway...so if you are going to keep casting to unsigned long you'll need
> this...perhaps with a comment why added.
>
> include/linux/kmemtrace.h | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/kmemtrace.h b/include/linux/kmemtrace.h
> index 5bea8ea..9d82085 100644
> --- a/include/linux/kmemtrace.h
> +++ b/include/linux/kmemtrace.h
> @@ -34,7 +34,7 @@ static inline void kmemtrace_mark_alloc_node(enum kmemtrace_type_id type_id,
> "bytes_req %lu bytes_alloc %lu gfp_flags %lu node %d",
> type_id, call_site, (unsigned long) ptr,
> (unsigned long) bytes_req, (unsigned long) bytes_alloc,
> - (unsigned long) gfp_flags, node);
> + (__force unsigned long)gfp_flags, node);
> }
>
> static inline void kmemtrace_mark_free(enum kmemtrace_type_id type_id,
> --
> 1.6.0.3.756.gb776d
>
>
Sorry, the commit name is okay, my mistake.
* Eduard - Gabriel Munteanu ([email protected]) wrote:
> On Fri, Nov 07, 2008 at 10:20:29PM +0300, Alexey Dobriyan wrote:
> > On Fri, Nov 07, 2008 at 10:58:41AM -0800, Harvey Harrison wrote:
> > > --- a/include/linux/kmemtrace.h
> > > +++ b/include/linux/kmemtrace.h
> > > @@ -34,7 +34,7 @@ static inline void kmemtrace_mark_alloc_node(enum kmemtrace_type_id type_id,
> > > "bytes_req %lu bytes_alloc %lu gfp_flags %lu node %d",
> > > type_id, call_site, (unsigned long) ptr,
> > > (unsigned long) bytes_req, (unsigned long) bytes_alloc,
> > > - (unsigned long) gfp_flags, node);
> > > + (__force unsigned long)gfp_flags, node);
> >
> > gfp_t is "unsigned int" actually. These casts are bogus.
> >
> > Subject: How do I printk <type> correctly?
> >
> > If variable is of Type use printk format specifier.
> > ---------------------------------------------------------
> > int %d or %x
> > unsigned int %u or %x
> > long %ld ot %lx
> > unsigned long %lu or %lx
> > long long %lld or %llx
> > unsigned long long %llu or %llx
> > size_t %zu or %zx
> > ssize_t %zd or %zx
> >
> > Raw pointer value SHOULD be printed with %p.
> >
> > u64 SHOULD be printed with %llu/%llx, (unsigned long long):
> >
> > printk("%llu", (unsigned long long)u64_var);
> >
> > s64 SHOULD be printed with %lld/%llx, (long long):
> >
> > printk("%lld", (long long)s64_var);
> >
> > If type is dependent on config option (sector_t), use format specifier
> > of biggest type and explicitly cast to it.
> >
> > Reminder: sizeof() result is of type size_t.
> >
> > Thank you for your cooperation.
>
> Hi,
>
> Actually, "%zu" was the first thing that crossed my mind too. But we
> don't want to carry such types into the probe callbacks.
quote :
> It's a lot
> easier to see which u* an unsigned long fits into than it is for
> size_t.
why would you need to limit yourself to u8, u16, u32, u64 ?
sizeof(size_t) tells you for sure what the size of size_t is. You can
even export it to a trace header so that size is know when the trace is
analyzed, neat eh ? :)
Why would you ever want to create a macro to make typing more obscure
and to take considerably more space on architectures where a u64 is not
required ?
Mathieu
> So we take care of this inside a wrapper; the sooner, the better.
>
> Also take into account that debugging code usually casts pointers to
> unsigned long. This can easily be seen by looking at _RET_IP_ definition
> or SLAB code. I think there's a very good reason to do so, since it adds
> opacity to something that's not supposed to be used as a pointer.
>
>
> Cheers,
> Eduard
>
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
On Sat, Nov 08, 2008 at 12:50:16AM +0200, Eduard - Gabriel Munteanu wrote:
> On Fri, Nov 07, 2008 at 10:20:29PM +0300, Alexey Dobriyan wrote:
> > On Fri, Nov 07, 2008 at 10:58:41AM -0800, Harvey Harrison wrote:
> > > --- a/include/linux/kmemtrace.h
> > > +++ b/include/linux/kmemtrace.h
> > > @@ -34,7 +34,7 @@ static inline void kmemtrace_mark_alloc_node(enum kmemtrace_type_id type_id,
> > > "bytes_req %lu bytes_alloc %lu gfp_flags %lu node %d",
> > > type_id, call_site, (unsigned long) ptr,
> > > (unsigned long) bytes_req, (unsigned long) bytes_alloc,
> > > - (unsigned long) gfp_flags, node);
> > > + (__force unsigned long)gfp_flags, node);
> >
> > gfp_t is "unsigned int" actually. These casts are bogus.
> >
> > Subject: How do I printk <type> correctly?
> >
> > If variable is of Type use printk format specifier.
> > ---------------------------------------------------------
> > int %d or %x
> > unsigned int %u or %x
> > long %ld ot %lx
> > unsigned long %lu or %lx
> > long long %lld or %llx
> > unsigned long long %llu or %llx
> > size_t %zu or %zx
> > ssize_t %zd or %zx
> >
> > Raw pointer value SHOULD be printed with %p.
> >
> > u64 SHOULD be printed with %llu/%llx, (unsigned long long):
> >
> > printk("%llu", (unsigned long long)u64_var);
> >
> > s64 SHOULD be printed with %lld/%llx, (long long):
> >
> > printk("%lld", (long long)s64_var);
> >
> > If type is dependent on config option (sector_t), use format specifier
> > of biggest type and explicitly cast to it.
> >
> > Reminder: sizeof() result is of type size_t.
> >
> > Thank you for your cooperation.
>
> Hi,
>
> Actually, "%zu" was the first thing that crossed my mind too. But we
> don't want to carry such types into the probe callbacks. It's a lot
> easier to see which u* an unsigned long fits into than it is for
> size_t. So we take care of this inside a wrapper; the sooner, the better.
Format specifiers are apparently hard.
> Also take into account that debugging code usually casts pointers to
> unsigned long. This can easily be seen by looking at _RET_IP_ definition
> or SLAB code.
You got "const void *" in this wrapper.
> I think there's a very good reason to do so, since it adds
> opacity to something that's not supposed to be used as a pointer.
What is not supposed to be used as pointer? _RET_IP_?