Casting 0 to a __bitwise type

Casting 0 to a __bitwise type

[Edward Cree]

I'm given to understand that bitwise types consider 0 'special', in that
 sparse will allow, say:
        typedef __u32 __bitwise __be32;
        __be32 value = 0;
However, it appears that if I try to cast a constant 0 to a bitwise type,
        static inline void ip6_fill_mask(__be32 *mask)
        {
                mask[0] = mask[1] = mask[2] = mask[3] = ~(__be32)0;
        }
sparse complains "warning: cast to restricted __be32".
Why is this?  Is there some subtle reason why the cast is incorrect, or
 is sparse just unable to notice that the value being cast is 0?
It allows it if I rewrite it with a local variable, making the cast
 implicit:
        static inline void ip6_fill_mask(__be32 *mask)
        {
                __be32 nothing = 0;

                mask[0] = mask[1] = mask[2] = mask[3] = ~nothing;
        }

-Ed
The information contained in this message is confidential and is intended for the addressee(s) only. If you have received this message in error, please notify the sender immediately and delete the message. Unless you are an addressee (or authorized to receive for an addressee), you may not use, copy or disclose to anyone this message or any information contained in this message. The unauthorized use, disclosure, copying or alteration of this message is strictly prohibited.

Re: Casting 0 to a __bitwise type

[Luc Van Oostenryck]

On Tue, Feb 14, 2017 at 09:36:40PM +0000, Edward Cree wrote:
> I'm given to understand that bitwise types consider 0 'special', in that
>  sparse will allow, say:
>         typedef __u32 __bitwise __be32;
>         __be32 value = 0;
> However, it appears that if I try to cast a constant 0 to a bitwise type,
>         static inline void ip6_fill_mask(__be32 *mask)
>         {
>                 mask[0] = mask[1] = mask[2] = mask[3] = ~(__be32)0;
>         }
> sparse complains "warning: cast to restricted __be32".

I can't reproduce this with the current head.
Wich version of sparse are you using? Can you retry with sparse's head?


Luc Van Oostenryck

Re: Casting 0 to a __bitwise type

[Edward Cree]

On 14/02/17 22:51, Luc Van Oostenryck wrote:
> On Tue, Feb 14, 2017 at 09:36:40PM +0000, Edward Cree wrote:
>> I'm given to understand that bitwise types consider 0 'special', in that
>>  sparse will allow, say:
>>         typedef __u32 __bitwise __be32;
>>         __be32 value = 0;
>> However, it appears that if I try to cast a constant 0 to a bitwise type,
>>         static inline void ip6_fill_mask(__be32 *mask)
>>         {
>>                 mask[0] = mask[1] = mask[2] = mask[3] = ~(__be32)0;
>>         }
>> sparse complains "warning: cast to restricted __be32".
> I can't reproduce this with the current head.
> Wich version of sparse are you using? Can you retry with sparse's head?
I was using v0.5.0, which appears to be the most recent tag.
With latest HEAD (9208e04972f2) I still reproduce.
*However*, on either version I can only reproduce it 'in situ' in my code, it
 turns out that the synthetic example I gave above does _not_ reproduce it.
sparse only gives the warning if the function is _used_; if I comment out the
 calls to the function in my 'real' code, the warning goes away.  The calls
 are passing a __be32[4] as the mask argument.
(The actual code that is hitting this is drivers/net/ethernet/sfc/ethtool.c
 in recent Linux trees.)
From this I was able to create a new synthetic example, which *does*
 reproduce the warning:

    typedef unsigned int __attribute__((bitwise)) __be32;

    static inline void ip6_fill_mask(__be32 *mask)
    {
        mask[0] = mask[1] = mask[2] = mask[3] = ~(__be32)0;
    }

    static void f(void)
    {
        __be32 mask[4];

        ip6_fill_mask(mask);
    }

Feeding the above program to sparse yields the output
"test.c:5:51: warning: cast to restricted __be32"

In fact the example can be reduced even further:

    typedef unsigned int __attribute__((bitwise)) __be32;

    static __be32 foo = (__be32)0;

yields "test.c:3:22: warning: cast to restricted __be32"

My apologies for previously providing an incorrect example; I should have
 tested it rather than assuming, as I did, that a static inline that was
 never called would still be type-checked.
-Ed
The information contained in this message is confidential and is intended for the addressee(s) only. If you have received this message in error, please notify the sender immediately and delete the message. Unless you are an addressee (or authorized to receive for an addressee), you may not use, copy or disclose to anyone this message or any information contained in this message. The unauthorized use, disclosure, copying or alteration of this message is strictly prohibited.

Re: Casting 0 to a __bitwise type

[Luc Van Oostenryck]

On Fri, Feb 17, 2017 at 11:01:53AM +0000, Edward Cree wrote:
> (The actual code that is hitting this is drivers/net/ethernet/sfc/ethtool.c
>  in recent Linux trees.)
> From this I was able to create a new synthetic example, which *does*
>  reproduce the warning:
> 
>     typedef unsigned int __attribute__((bitwise)) __be32;
> 
>     static inline void ip6_fill_mask(__be32 *mask)
>     {
>         mask[0] = mask[1] = mask[2] = mask[3] = ~(__be32)0;
>     }
> 
>     static void f(void)
>     {
>         __be32 mask[4];
> 
>         ip6_fill_mask(mask);
>     }
> 
> Feeding the above program to sparse yields the output
> "test.c:5:51: warning: cast to restricted __be32"
> 
> In fact the example can be reduced even further:
> 
>     typedef unsigned int __attribute__((bitwise)) __be32;
> 
>     static __be32 foo = (__be32)0;
> 
> yields "test.c:3:22: warning: cast to restricted __be32"
> 
> My apologies for previously providing an incorrect example; I should have
>  tested it rather than assuming, as I did, that a static inline that was
>  never called would still be type-checked.

No worries.

In fact this warning is correct, by design.
Your not supposed to cast a bitwise type, even zero.
Here is a few examples that doesn't give a warning:
	mask[1] = 0;
	mask[2] = ~msk;
	mask[3] = ~(__force __be32) 0;
	mask[4] = ~({ __be32 msk = 0; msk; });

The canonical way to do what you need is via the (__force ...) cast.
In fact, in the driver you're talking about, just a few lines above
this fill_mask() function, there is a few masks already defined that
do exactly this, like IP4_ADDR_FULL_MASK.

Luc

> The information contained in this message is confidential and is intended for the addressee(s) only. If you have received this message in error, please notify the sender immediately and delete the message. Unless you are an addressee (or authorized to receive for an addressee), you may not use, copy or disclose to anyone this message or any information contained in this message. The unauthorized use, disclosure, copying or alteration of this message is strictly prohibited.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Casting 0 to a __bitwise type

[Edward Cree]

On 17/02/17 12:37, Luc Van Oostenryck wrote:
> In fact this warning is correct, by design.
> Your not supposed to cast a bitwise type, even zero.
But you _are_ allowed to initialise a bitwise typed variable with zero...
This is inconsistent; if a value is implicitly convertible to a type, it
 should also be legal to cast it explicitly to that type.  Such is the
 case with all types in standard C.
So it's my opinion that the design is wrong.
> Here is a few examples that doesn't give a warning:
>       mask[1] = 0;
>       mask[2] = ~msk;
>       mask[3] = ~(__force __be32) 0;
>       mask[4] = ~({ __be32 msk = 0; msk; });
>
> The canonical way to do what you need is via the (__force ...) cast.
> In fact, in the driver you're talking about, just a few lines above
> this fill_mask() function, there is a few masks already defined that
> do exactly this, like IP4_ADDR_FULL_MASK.
Yes, and I'd actually like to be able to remove __force from those casts as
 well; __forced casts are a very big hammer that imho should be confined to
 things like architecture-specific code.  They should not be needed in
 drivers; they certainly should not be needed for a conversion that can
 occur implicitly.
You mention that ({ __be32 msk = 0; msk; }) works.  But surely that is a
 strictly less forceful cast than (__be32)0, since it is using an implicit
 conversion, and if that doesn't give a warning, then neither should the
 explicit cast.

So can you explain to me why 0 should be treated specially by _everything
 except_ the cast operator?

-Ed
The information contained in this message is confidential and is intended for the addressee(s) only. If you have received this message in error, please notify the sender immediately and delete the message. Unless you are an addressee (or authorized to receive for an addressee), you may not use, copy or disclose to anyone this message or any information contained in this message. The unauthorized use, disclosure, copying or alteration of this message is strictly prohibited.

Re: Casting 0 to a __bitwise type

[Luc Van Oostenryck]

On Fri, Feb 17, 2017 at 01:13:51PM +0000, Edward Cree wrote:
> On 17/02/17 12:37, Luc Van Oostenryck wrote:
> > In fact this warning is correct, by design.
> > Your not supposed to cast a bitwise type, even zero.
> But you _are_ allowed to initialise a bitwise typed variable with zero...
> This is inconsistent; if a value is implicitly convertible to a type, it
>  should also be legal to cast it explicitly to that type.  Such is the
>  case with all types in standard C.
> So it's my opinion that the design is wrong.
> > Here is a few examples that doesn't give a warning:
> >       mask[1] = 0;
> >       mask[2] = ~msk;
> >       mask[3] = ~(__force __be32) 0;
> >       mask[4] = ~({ __be32 msk = 0; msk; });
> >
> > The canonical way to do what you need is via the (__force ...) cast.
> > In fact, in the driver you're talking about, just a few lines above
> > this fill_mask() function, there is a few masks already defined that
> > do exactly this, like IP4_ADDR_FULL_MASK.
> Yes, and I'd actually like to be able to remove __force from those casts as
>  well; __forced casts are a very big hammer that imho should be confined to
>  things like architecture-specific code.  They should not be needed in
>  drivers; they certainly should not be needed for a conversion that can
>  occur implicitly.

Well, it's a bit the same for all casts: you should not abuse them but theys
have their uses.

> You mention that ({ __be32 msk = 0; msk; }) works.  But surely that is a
>  strictly less forceful cast than (__be32)0, since it is using an implicit
>  conversion, and if that doesn't give a warning, then neither should the
>  explicit cast.
> 
> So can you explain to me why 0 should be treated specially by _everything
>  except_ the cast operator?

It was a design decision, justified (I think) by:
- 0 is already magic in C (can be used to initialize integer *and* pointers)
- since 0 is a constant that make sense for all sizes, have the same 
  representation in big endian and little endian, ... it can be used for
  all bitwise types.

I think the answer to your question could be: "since it's best to avoid casts,
especially unnneded ones for 0 zero you don't need a cast, it's good to warn
about it".

Luc

Re: Casting 0 to a __bitwise type

[Edward Cree]

On 17/02/17 14:16, Luc Van Oostenryck wrote:
> On Fri, Feb 17, 2017 at 01:13:51PM +0000, Edward Cree wrote:
>> So can you explain to me why 0 should be treated specially by _everything
>>  except_ the cast operator?
> It was a design decision, justified (I think) by:
> - 0 is already magic in C (can be used to initialize integer *and* pointers)
> - since 0 is a constant that make sense for all sizes, have the same
>   representation in big endian and little endian, ... it can be used for
>   all bitwise types.
I agree so far: 0 is special...
> I think the answer to your question could be: "since it's best to avoid casts,
> especially unnneded ones for 0 zero you don't need a cast, it's good to warn
> about it".
...but suddenly in a cast it's not special any more?

I _do_ need a cast, because once I've got a zero of type __be32 I then want to
 operate on it, specifically by taking its complement - which of course is an
 entirely legal thing to do with a bitwise type.
And the cast has to be there, because sparse doesn't accept "__be32 x = ~0;" and
 quite rightly too as integer promotions make it nontrivial to determine whether
 that is correct.  The complement operation is only bitwise-safe if applied to a
 bitwise type, hence the type conversion must happen before the complement.
Anyway, after digging my way around innards of sparse I've come up with a patch
 that appears to do what I want, I'll post it as a new thread.  Then we'll have
 something slightly more concrete to argue about ;)

-Ed

PS. I've also noticed that sparse doesn't seem to pay attention to the -Wbitwise
 command line option any more, it always performs the bitwise checks whether the
 user asked for them or not.  The only results for "git grep Wbitwise" are the
 code to parse the option, command lines in regression tests, and the man page.
Personally that doesn't bother me, because I want the checks enabled anyway, but
 I suppose it's a bug that they can't be turned off.
--
The information contained in this message is confidential and is intended for the addressee(s) only. If you have received this message in error, please notify the sender immediately and delete the message. Unless you are an addressee (or authorized to receive for an addressee), you may not use, copy or disclose to anyone this message or any information contained in this message. The unauthorized use, disclosure, copying or alteration of this message is strictly prohibited.