1 << 31 redux

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

1 << 31 redux

Eitan Adler-4
Hi all,

A few years ago I fixed most of the cases where we used 1 << 31 in FreeBSD.
This expression is illegal in C. Since then the issue has arisen again.

https://reviews.freebsd.org/D13858 fixed most of the non-contrib cases.

I'd also like to see if we could find some more general solution, be it a
compiler warning, bit set macro, or otherwise.

--
Eitan Adler
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: 1 << 31 redux

Ed Schouten-6
2018-01-11 13:03 GMT+01:00 Eitan Adler <[hidden email]>:
> I'd also like to see if we could find some more general solution, be it a
> compiler warning, bit set macro, or otherwise.

Does Clang already offer a warning for this? If so, we should consider
adding it to WARNS=6.

--
Ed Schouten <[hidden email]>
Nuxi, 's-Hertogenbosch, the Netherlands
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: 1 << 31 redux

Dimitry Andric-4
On 11 Jan 2018, at 13:05, Ed Schouten <[hidden email]> wrote:
>
> 2018-01-11 13:03 GMT+01:00 Eitan Adler <[hidden email]>:
>> I'd also like to see if we could find some more general solution, be it a
>> compiler warning, bit set macro, or otherwise.
>
> Does Clang already offer a warning for this? If so, we should consider
> adding it to WARNS=6.

There is a -Wshift-sign-overflow flag, but it isn't enabled by default:

$ clang -Wshift-sign-overflow -c bar.c
bar.c:1:26: warning: signed shift result (0x80000000) sets the sign bit of the shift expression's type ('int') and becomes negative [-Wshift-sign-overflow]
int bar(void) { return 1 << 31; }
                       ~ ^  ~~

I would expect quite a lot of stuff to break if you enable it, though. :)

And of course, there is -fsanitize=undefined, which can catch this kind
of thing at runtime.

-Dimitry


signature.asc (230 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: 1 << 31 redux

Warner Losh
On Thu, Jan 11, 2018 at 6:37 AM, Dimitry Andric <[hidden email]> wrote:

> On 11 Jan 2018, at 13:05, Ed Schouten <[hidden email]> wrote:
> >
> > 2018-01-11 13:03 GMT+01:00 Eitan Adler <[hidden email]>:
> >> I'd also like to see if we could find some more general solution, be it
> a
> >> compiler warning, bit set macro, or otherwise.
> >
> > Does Clang already offer a warning for this? If so, we should consider
> > adding it to WARNS=6.
>
> There is a -Wshift-sign-overflow flag, but it isn't enabled by default:
>
> $ clang -Wshift-sign-overflow -c bar.c
> bar.c:1:26: warning: signed shift result (0x80000000) sets the sign bit of
> the shift expression's type ('int') and becomes negative
> [-Wshift-sign-overflow]
> int bar(void) { return 1 << 31; }
>                        ~ ^  ~~
>
> I would expect quite a lot of stuff to break if you enable it, though. :)
>
> And of course, there is -fsanitize=undefined, which can catch this kind
> of thing at runtime.
>

If we can't get people to fix the warnings we have in the tree now
(especially the kernel), why enable new warnings that will just be ignored?

Warner
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: 1 << 31 redux

Steve Kargl
On Thu, Jan 11, 2018 at 08:47:16AM -0700, Warner Losh wrote:

> On Thu, Jan 11, 2018 at 6:37 AM, Dimitry Andric <[hidden email]> wrote:
>
> > On 11 Jan 2018, at 13:05, Ed Schouten <[hidden email]> wrote:
> > >
> > > 2018-01-11 13:03 GMT+01:00 Eitan Adler <[hidden email]>:
> > >> I'd also like to see if we could find some more general solution, be it
> > a
> > >> compiler warning, bit set macro, or otherwise.
> > >
> > > Does Clang already offer a warning for this? If so, we should consider
> > > adding it to WARNS=6.
> >
> > There is a -Wshift-sign-overflow flag, but it isn't enabled by default:
> >
> > $ clang -Wshift-sign-overflow -c bar.c
> > bar.c:1:26: warning: signed shift result (0x80000000) sets the sign bit of
> > the shift expression's type ('int') and becomes negative
> > [-Wshift-sign-overflow]
> > int bar(void) { return 1 << 31; }
> >                        ~ ^  ~~
> >
> > I would expect quite a lot of stuff to break if you enable it, though. :)
> >
> > And of course, there is -fsanitize=undefined, which can catch this kind
> > of thing at runtime.
> >
>
> If we can't get people to fix the warnings we have in the tree now
> (especially the kernel), why enable new warnings that will just be ignored?

Create WARNS=7 with -Werror added to command line option.
Edit the various *.mk files to force WARNS=7
Watch warnings get fixed.

--
Steve
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: 1 << 31 redux

Eitan Adler-4
In reply to this post by Warner Losh
On 11 January 2018 at 07:47, Warner Losh <[hidden email]> wrote:
>
>
> If we can't get people to fix the warnings we have in the tree now
> (especially the kernel), why enable new warnings that will just be ignored?
>

We've been doing a reasonable job with warnings until now. Either way, at
higher warnings levels we should add what we can.



--
Eitan Adler
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: 1 << 31 redux

Rodney W. Grimes-4
> On 11 January 2018 at 07:47, Warner Losh <[hidden email]> wrote:
> >
> >
> > If we can't get people to fix the warnings we have in the tree now
> > (especially the kernel), why enable new warnings that will just be ignored?
> >
>
> We've been doing a reasonable job with warnings until now. Either way, at
> higher warnings levels we should add what we can.

Is not this 1U<<31 -> signed value really just sweeping the bigger
issue that we are using signed values in unsigned ways?

--
Rod Grimes                                                 [hidden email]
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: 1 << 31 redux

Kyle Evans
In reply to this post by Eitan Adler-4
On Thu, Jan 11, 2018 at 6:03 AM, Eitan Adler <[hidden email]> wrote:

> Hi all,
>
> A few years ago I fixed most of the cases where we used 1 << 31 in FreeBSD.
> This expression is illegal in C. Since then the issue has arisen again.
>
> https://reviews.freebsd.org/D13858 fixed most of the non-contrib cases.
>
> I'd also like to see if we could find some more general solution, be it a
> compiler warning, bit set macro, or otherwise.
>

For what it's worth, I've really come to like and appreciate NetBSD's
approach with __BIT/__BITS. See [1] for implementation, [2] for usage.

[1] http://src.illumos.org/source/xref/netbsd-src/sys/sys/cdefs.h#577
[2] http://src.illumos.org/source/xref/netbsd-src/sys/arch/arm/sunxi/sunxi_usbphy.c#L44
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: 1 << 31 redux

Lowell Gilbert-14
In reply to this post by Rodney W. Grimes-4
"Rodney W. Grimes" <[hidden email]> writes:

> Is not this 1U<<31 -> signed value really just sweeping the bigger
> issue that we are using signed values in unsigned ways?

It might be more appropriate to say that we are using unsigned values,
but not telling the compiler that they aren't signed. No?
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: 1 << 31 redux

Julian Elischer-5
In reply to this post by Kyle Evans
On 14/1/18 1:34 am, Kyle Evans wrote:

> On Thu, Jan 11, 2018 at 6:03 AM, Eitan Adler <[hidden email]> wrote:
>> Hi all,
>>
>> A few years ago I fixed most of the cases where we used 1 << 31 in FreeBSD.
>> This expression is illegal in C. Since then the issue has arisen again.
>>
>> https://reviews.freebsd.org/D13858 fixed most of the non-contrib cases.
>>
>> I'd also like to see if we could find some more general solution, be it a
>> compiler warning, bit set macro, or otherwise.
>>
> For what it's worth, I've really come to like and appreciate NetBSD's
> approach with __BIT/__BITS. See [1] for implementation, [2] for usage.
>
> [1] http://src.illumos.org/source/xref/netbsd-src/sys/sys/cdefs.h#577

I like __BIT() but it should give a compile error if the number is too
large rather than just setting it to 0.
in fact I think I think it should take a target and use the size of
the target rather than assuming int.

> [2] http://src.illumos.org/source/xref/netbsd-src/sys/arch/arm/sunxi/sunxi_usbphy.c#L44
> _______________________________________________
> [hidden email] mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "[hidden email]"
>

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"