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

This is a discussion on [PATCH-ugly] kmemtrace: casting a gfp_t requires __force - Kernel ; 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 --- Eduard, this is a local patch I've had sitting around ...

+ Reply to Thread
Results 1 to 10 of 10

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

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

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



    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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 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.

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    * 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 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    Subject: [PATCH] kmemtrace: gfp_t is an unsigned int, not an unsigned long

    Signed-off-by: Harvey Harrison
    ---
    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



    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    * 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 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    > ---
    > 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.

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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 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_?
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread