From: Jean Delvare <khali@linux-fr.org>
To: Guenter Roeck <linux@roeck-us.net>
Cc: linux-kernel@vger.kernel.org,
Andrew Morton <akpm@linux-foundation.org>,
lm-sensors@lm-sensors.org, Juergen Beisert <jbe@pengutronix.de>
Subject: Re: [lm-sensors] [PATCH v2] linux/kernel.h: Fix DIV_ROUND_CLOSEST with unsigned divisors
Date: Thu, 20 Dec 2012 11:48:20 +0000 [thread overview]
Message-ID: <20121220124820.0069aa66@endymion.delvare> (raw)
In-Reply-To: <20121219230144.GB26863@roeck-us.net>
On Wed, 19 Dec 2012 15:01:44 -0800, Guenter Roeck wrote:
> On Wed, Dec 19, 2012 at 11:21:15PM +0100, Jean Delvare wrote:
> > Hi Guenter,
> >
> > On Wed, 19 Dec 2012 06:40:15 -0800, Guenter Roeck wrote:
> > > Commit 263a523 fixes a warning seen with W=1 due to change in
> > > DIV_ROUND_CLOSEST. Unfortunately, the C compiler converts divide operations
> > > with unsigned divisors to unsigned, even if the dividend is signed and
> > > negative (for example, -10 / 5U = 858993457). The C standard says "If one
> > > operand has unsigned int type, the other operand is converted to unsigned
> > > int", so the compiler is not to blame.
> >
> > This is surprising to say the least. But if the C standard says so...
>
> Agreed, but it is how it is.
>
> > I wouldn't be surprised if there are bugs because of this in the kernel
> > and in other projects.
>
> Might easily be. This might make a good interview question - I suspect many
> if not most engineers would fail it. At least I would have until yesterday :).
Neither did I. And I'm not sure I'll remember it in one year from now.
> > (...)
> > Thinking a bit more about this... Documenting the non-working cases is
> > great, however I don't really expect all developers to pay attention. I
> > can also imagine variable types changing from signed to unsigned later,
> > and never thinking this can introduce a bug.
> >
> > So, is there nothing we can do to spot at least the second issue at
> > build time? For regular division there's nothing we can do (although I
> > don't understand why gcc doesn't warn...) but here we get the
> > opportunity to report the issue, let's take it.
> >
> > And given that the divisor is almost always a constant,
> > maybe we can check for negative divisors too, this would be safer and
> > the code size increase would probably be very small in practice.
> > Opinions?
>
> Agreed, though we should fix the problem now and think about reporting
> afterwards.
Yes, that's a good plan.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
WARNING: multiple messages have this Message-ID (diff)
From: Jean Delvare <khali@linux-fr.org>
To: Guenter Roeck <linux@roeck-us.net>
Cc: linux-kernel@vger.kernel.org,
Andrew Morton <akpm@linux-foundation.org>,
lm-sensors@lm-sensors.org, Juergen Beisert <jbe@pengutronix.de>
Subject: Re: [PATCH v2] linux/kernel.h: Fix DIV_ROUND_CLOSEST with unsigned divisors
Date: Thu, 20 Dec 2012 12:48:20 +0100 [thread overview]
Message-ID: <20121220124820.0069aa66@endymion.delvare> (raw)
In-Reply-To: <20121219230144.GB26863@roeck-us.net>
On Wed, 19 Dec 2012 15:01:44 -0800, Guenter Roeck wrote:
> On Wed, Dec 19, 2012 at 11:21:15PM +0100, Jean Delvare wrote:
> > Hi Guenter,
> >
> > On Wed, 19 Dec 2012 06:40:15 -0800, Guenter Roeck wrote:
> > > Commit 263a523 fixes a warning seen with W=1 due to change in
> > > DIV_ROUND_CLOSEST. Unfortunately, the C compiler converts divide operations
> > > with unsigned divisors to unsigned, even if the dividend is signed and
> > > negative (for example, -10 / 5U = 858993457). The C standard says "If one
> > > operand has unsigned int type, the other operand is converted to unsigned
> > > int", so the compiler is not to blame.
> >
> > This is surprising to say the least. But if the C standard says so...
>
> Agreed, but it is how it is.
>
> > I wouldn't be surprised if there are bugs because of this in the kernel
> > and in other projects.
>
> Might easily be. This might make a good interview question - I suspect many
> if not most engineers would fail it. At least I would have until yesterday :).
Neither did I. And I'm not sure I'll remember it in one year from now.
> > (...)
> > Thinking a bit more about this... Documenting the non-working cases is
> > great, however I don't really expect all developers to pay attention. I
> > can also imagine variable types changing from signed to unsigned later,
> > and never thinking this can introduce a bug.
> >
> > So, is there nothing we can do to spot at least the second issue at
> > build time? For regular division there's nothing we can do (although I
> > don't understand why gcc doesn't warn...) but here we get the
> > opportunity to report the issue, let's take it.
> >
> > And given that the divisor is almost always a constant,
> > maybe we can check for negative divisors too, this would be safer and
> > the code size increase would probably be very small in practice.
> > Opinions?
>
> Agreed, though we should fix the problem now and think about reporting
> afterwards.
Yes, that's a good plan.
--
Jean Delvare
next prev parent reply other threads:[~2012-12-20 11:48 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-12-19 14:40 [lm-sensors] [PATCH v2] linux/kernel.h: Fix DIV_ROUND_CLOSEST with unsigned divisors Guenter Roeck
2012-12-19 14:40 ` Guenter Roeck
2012-12-19 21:47 ` [lm-sensors] " Andrew Morton
2012-12-19 21:47 ` Andrew Morton
2012-12-19 22:41 ` [lm-sensors] " Guenter Roeck
2012-12-19 22:41 ` Guenter Roeck
2012-12-20 10:22 ` [lm-sensors] " Jean Delvare
2012-12-20 10:22 ` Jean Delvare
2012-12-20 10:30 ` [lm-sensors] " Juergen Beisert
2012-12-20 10:30 ` Juergen Beisert
2012-12-20 11:00 ` [lm-sensors] " Jean Delvare
2012-12-20 11:00 ` Jean Delvare
2012-12-20 14:13 ` [lm-sensors] " Guenter Roeck
2012-12-20 14:13 ` Guenter Roeck
2012-12-19 22:21 ` [lm-sensors] " Jean Delvare
2012-12-19 22:21 ` Jean Delvare
2012-12-19 23:01 ` [lm-sensors] " Guenter Roeck
2012-12-19 23:01 ` Guenter Roeck
2012-12-20 11:48 ` Jean Delvare [this message]
2012-12-20 11:48 ` Jean Delvare
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20121220124820.0069aa66@endymion.delvare \
--to=khali@linux-fr.org \
--cc=akpm@linux-foundation.org \
--cc=jbe@pengutronix.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=lm-sensors@lm-sensors.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.