[PATCH-ugly] kmemtrace: casting a gfp_t requires __force

[PATCH-ugly] kmemtrace: casting a gfp_t requires __force

[Harvey Harrison]

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 <harvey.harrison@gmail.com>
---
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

Re: [PATCH-ugly] kmemtrace: casting a gfp_t requires __force

[Alexey Dobriyan]

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.

Re: [PATCH-ugly] kmemtrace: casting a gfp_t requires __force

[Harvey Harrison]

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

Re: [PATCH-ugly] kmemtrace: casting a gfp_t requires __force

[Mathieu Desnoyers]

* Harvey Harrison (harvey.harrison@gmail.com) 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

Re: [PATCH-ugly] kmemtrace: casting a gfp_t requires __force

[Harvey Harrison]

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 <harvey.harrison@gmail.com>
Subject: [PATCH] kmemtrace: gfp_t is an unsigned int, not an unsigned long

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 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

Re: [PATCH-ugly] kmemtrace: casting a gfp_t requires __force

[Alexey Dobriyan]

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.

Re: [PATCH-ugly] kmemtrace: casting a gfp_t requires __force

[Eduard - Gabriel Munteanu]

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

Re: [PATCH-ugly] kmemtrace: casting a gfp_t requires __force

[Mathieu Desnoyers]

* Eduard - Gabriel Munteanu (eduard.munteanu@linux360.ro) 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

Re: [PATCH-ugly] kmemtrace: casting a gfp_t requires __force

[Alexey Dobriyan]

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_?

Re: [PATCH-ugly] kmemtrace: casting a gfp_t requires __force

[Eduard - Gabriel Munteanu]

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 <harvey.harrison@gmail.com>
> ---
> 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

Re: [PATCH-ugly] kmemtrace: casting a gfp_t requires __force

[Eduard - Gabriel Munteanu]

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 <harvey.harrison@gmail.com>
> ---
> 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.