Fixing "cast truncates bits from constant value"

Fixing "cast truncates bits from constant value"

[Pavel Roskin]

Hello!

There was a discussion in the linux-wireless mailing list about
silencing a sparse warning in the b43 driver:

http://marc.info/?t=125020183400006&r=1&w=2

It turns out the fixes for the warnings would make the code worse, not
better.  It's not what sparse should try to do.

Here's the simplest case:

static unsigned short test(void)
{
        return (unsigned short)~0x8000;
}

test.c:3:32: warning: cast truncates bits from constant value (ffff7fff
becomes 7fff)

A side note - "0x" should be used with user visible output.

0x8000 could be a constant defined in a header, so just using 0x7fff is
not an option, if the code readability is to be preserved.

In my opinion, an explicit cast should be enough to suppress the
warning.  But it's not.  Moreover, it's a "superwarning" that cannot
even be suppressed by the "force" attribute!  This source still
generates a warning:

static unsigned short test(void)
{
        return (__attribute__((force)) unsigned short)~0x8000;
}

There is also an inconsistency.  This source produces a warning:

static unsigned short test(void)
{
        return 0x0ffff000U;
}

But this source doesn't:

static unsigned short test(void)
{
        return 0xfffff000U;
}

In both cases bits are dropped and the value (a large positive number)
is changed.

I'm not currently subscribed to this list, please copy me.

-- 
Regards,
Pavel Roskin

Re: Fixing "cast truncates bits from constant value"

[Christopher Li]

On Mon, Aug 17, 2009 at 2:02 PM, Pavel Roskin<proski@gnu.org> wrote:
> static unsigned short test(void)
> {
>        return (unsigned short)~0x8000;
> }
>
> test.c:3:32: warning: cast truncates bits from constant value (ffff7fff
> becomes 7fff)

That warning is legit because you ARE truncating constant value here.

> In my opinion, an explicit cast should be enough to suppress the
> warning.  But it's not.  Moreover, it's a "superwarning" that cannot
> even be suppressed by the "force" attribute!  This source still
> generates a warning:
>
> static unsigned short test(void)
> {
>        return (__attribute__((force)) unsigned short)~0x8000;

Sparse currently does not thing than just mark it as forced.


>
> But this source doesn't:
>
> static unsigned short test(void)
> {
>        return 0xfffff000U;
> }

I think sparse consider it as signed extend.
Truncating all 0xffff is consider OK for the negative case.

Chris
--
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: Fixing "cast truncates bits from constant value"

[Pavel Roskin]

On Tue, 2009-08-18 at 22:36 -0700, Christopher Li wrote:
> On Mon, Aug 17, 2009 at 2:02 PM, Pavel Roskin<proski@gnu.org> wrote:
> > static unsigned short test(void)
> > {
> >        return (unsigned short)~0x8000;
> > }
> >
> > test.c:3:32: warning: cast truncates bits from constant value (ffff7fff
> > becomes 7fff)
> 
> That warning is legit because you ARE truncating constant value here.

I believe it's not, as it's an explicit cast.  Sure, it's debatable
whether users want to be warned about it.  But suppose we keep the
warning.  What would be the fix for the above code?  The only fix I can
think of would be to use AND with 0xffff:

static unsigned short test(void)
{
      return (unsigned short)(0xffff & ~0x8000);
}

I think it's ugly.  And that's what b43 developers think, so the sparce
warning remains unfixed.

> > In my opinion, an explicit cast should be enough to suppress the
> > warning.  But it's not.  Moreover, it's a "superwarning" that cannot
> > even be suppressed by the "force" attribute!  This source still
> > generates a warning:
> >
> > static unsigned short test(void)
> > {
> >        return (__attribute__((force)) unsigned short)~0x8000;
> 
> Sparse currently does not thing than just mark it as forced.

The whole point of having that attribute is to suppress warnings.

> > But this source doesn't:
> >
> > static unsigned short test(void)
> > {
> >        return 0xfffff000U;
> > }
> 
> I think sparse consider it as signed extend.
> Truncating all 0xffff is consider OK for the negative case.

That's wrong, as there are no signed integers involved in the above
example.

-- 
Regards,
Pavel Roskin

Re: Fixing "cast truncates bits from constant value"

[Christopher Li]

On Wed, Aug 19, 2009 at 4:28 PM, Pavel Roskin<proski@gnu.org> wrote:
>
> static unsigned short test(void)
> {
>      return (unsigned short)(0xffff & ~0x8000);
> }
>
> I think it's ugly.  And that's what b43 developers think, so the sparce
> warning remains unfixed.


You can use a macro with nice name to do the convert plus AND 0xffff.
e.g.

return short_value(~0x8000);


>
>> > In my opinion, an explicit cast should be enough to suppress the
>> > warning.  But it's not.  Moreover, it's a "superwarning" that cannot
>> > even be suppressed by the "force" attribute!  This source still
>> > generates a warning:
>> >
>> > static unsigned short test(void)
>> > {
>> >        return (__attribute__((force)) unsigned short)~0x8000;
>>
>> Sparse currently does not thing than just mark it as forced.
>
> The whole point of having that attribute is to suppress warnings.

Yes, there is a lot of place sparse is complete yet. If any one want
to send patches. I am glad to review it.

>> > static unsigned short test(void)
>> > {
>> >        return 0xfffff000U;
>> > }
>>
>> I think sparse consider it as signed extend.
>> Truncating all 0xffff is consider OK for the negative case.
>
> That's wrong, as there are no signed integers involved in the above
> example.

True. But sparse is not a validations tool. It just point out the possible
warnings. If a warning has too much false positives. It is pretty much
useless. What you are suggesting will cause too much unnecessary
warnings. I haven't try it myself, but I suspect the kernel using negative
error code in pointers might trigger this.

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