* [PATCH v2] linux/kernel.h: Fix DIV_ROUND_CLOSEST with unsigned divisors @ 2012-12-19 14:40 Guenter Roeck 2012-12-19 21:47 ` Andrew Morton 2012-12-19 22:21 ` Jean Delvare 0 siblings, 2 replies; 10+ messages in thread From: Guenter Roeck @ 2012-12-19 14:40 UTC (permalink / raw) To: linux-kernel Cc: Andrew Morton, lm-sensors, Juergen Beisert, Guenter Roeck, Jean Delvare 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. As a result, DIV_ROUND_CLOSEST(0, 2U) and similar operations now return bad values, since the automatic conversion of expressions such as "0 - 2U/2" to unsigned was not taken into account. Fix by checking for the divisor variable type when deciding which operation to perform. This fixes DIV_ROUND_CLOSEST(0, 2U), but still returns bad values for negative dividends divided by unsigned divisors. Mark the latter case as unsupported. Reported-by: Juergen Beisert <jbe@pengutronix.de> Tested-by: Juergen Beisert <jbe@pengutronix.de> Cc: Jean Delvare <khali@linux-fr.org> Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- v2: Description update (v1 wasn't supposed to make it to lkml) include/linux/kernel.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/include/linux/kernel.h b/include/linux/kernel.h index d97ed58..45726dc 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -77,13 +77,15 @@ /* * Divide positive or negative dividend by positive divisor and round - * to closest integer. Result is undefined for negative divisors. + * to closest integer. Result is undefined for negative divisors and + * for negative dividends if the divisor variable type is unsigned. */ #define DIV_ROUND_CLOSEST(x, divisor)( \ { \ typeof(x) __x = x; \ typeof(divisor) __d = divisor; \ - (((typeof(x))-1) > 0 || (__x) > 0) ? \ + (((typeof(x))-1) > 0 || \ + ((typeof(divisor))-1) > 0 || (__x) > 0) ? \ (((__x) + ((__d) / 2)) / (__d)) : \ (((__x) - ((__d) / 2)) / (__d)); \ } \ -- 1.7.9.7 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] linux/kernel.h: Fix DIV_ROUND_CLOSEST with unsigned divisors 2012-12-19 14:40 [PATCH v2] linux/kernel.h: Fix DIV_ROUND_CLOSEST with unsigned divisors Guenter Roeck @ 2012-12-19 21:47 ` Andrew Morton 2012-12-19 22:41 ` Guenter Roeck 2012-12-19 22:21 ` Jean Delvare 1 sibling, 1 reply; 10+ messages in thread From: Andrew Morton @ 2012-12-19 21:47 UTC (permalink / raw) To: Guenter Roeck; +Cc: linux-kernel, lm-sensors, Juergen Beisert, Jean Delvare On Wed, 19 Dec 2012 06:40:15 -0800 Guenter Roeck <linux@roeck-us.net> 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. > As a result, DIV_ROUND_CLOSEST(0, 2U) and similar operations now return > bad values, since the automatic conversion of expressions such as "0 - 2U/2" > to unsigned was not taken into account. > > Fix by checking for the divisor variable type when deciding which operation > to perform. This fixes DIV_ROUND_CLOSEST(0, 2U), but still returns bad values > for negative dividends divided by unsigned divisors. Mark the latter case as > unsupported. The changelog didn't describe the end-user visible effects of the bug. Please always include this information. Because... The patch is applicable to 3.7.x. Should we backport it? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] linux/kernel.h: Fix DIV_ROUND_CLOSEST with unsigned divisors 2012-12-19 21:47 ` Andrew Morton @ 2012-12-19 22:41 ` Guenter Roeck 2012-12-20 10:22 ` Jean Delvare 0 siblings, 1 reply; 10+ messages in thread From: Guenter Roeck @ 2012-12-19 22:41 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel, lm-sensors, Juergen Beisert, Jean Delvare On Wed, Dec 19, 2012 at 01:47:21PM -0800, Andrew Morton wrote: > On Wed, 19 Dec 2012 06:40:15 -0800 > Guenter Roeck <linux@roeck-us.net> 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. > > As a result, DIV_ROUND_CLOSEST(0, 2U) and similar operations now return > > bad values, since the automatic conversion of expressions such as "0 - 2U/2" > > to unsigned was not taken into account. > > > > Fix by checking for the divisor variable type when deciding which operation > > to perform. This fixes DIV_ROUND_CLOSEST(0, 2U), but still returns bad values > > for negative dividends divided by unsigned divisors. Mark the latter case as > > unsupported. > > The changelog didn't describe the end-user visible effects of the bug. > Please always include this information. Because... > One observed effect is that the s2c_hwmon driver reports a value of 4198403 instead of 0 if the ADC reads 0. Other impact is unpredictable. Problem is seen if the divisor is an unsigned variable or constant and the dividend is less than (divisor/2). > The patch is applicable to 3.7.x. Should we backport it? > Yes. DIV_ROUND_CLOSEST is used throughout the kernel, and impact is unpredictable. 3.6 needs it as well. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] linux/kernel.h: Fix DIV_ROUND_CLOSEST with unsigned divisors 2012-12-19 22:41 ` Guenter Roeck @ 2012-12-20 10:22 ` Jean Delvare 2012-12-20 10:30 ` Juergen Beisert 2012-12-20 14:13 ` Guenter Roeck 0 siblings, 2 replies; 10+ messages in thread From: Jean Delvare @ 2012-12-20 10:22 UTC (permalink / raw) To: Guenter Roeck; +Cc: Andrew Morton, linux-kernel, lm-sensors, Juergen Beisert Hi Guenter, On Wed, 19 Dec 2012 14:41:22 -0800, Guenter Roeck wrote: > On Wed, Dec 19, 2012 at 01:47:21PM -0800, Andrew Morton wrote: > > The changelog didn't describe the end-user visible effects of the bug. > > Please always include this information. Because... > > One observed effect is that the s2c_hwmon driver reports a value of 4198403 > instead of 0 if the ADC reads 0. > > Other impact is unpredictable. Problem is seen if the divisor is an unsigned > variable or constant and the dividend is less than (divisor/2). Really? In my own testing, the problem only shows with dividend == 0, and even then, only when dividend is signed and divisor is not. DIV_ROUND_CLOSEST(5, 20U) returns 0 as expected, and so do DIV_ROUND_CLOSEST(0 / 20), DIV_ROUND_CLOSEST(0U / 20) and DIV_ROUND_CLOSEST(0U / 20U). Are your observations different? > > The patch is applicable to 3.7.x. Should we backport it? > > Yes. DIV_ROUND_CLOSEST is used throughout the kernel, and impact is > unpredictable. > > 3.6 needs it as well. -- Jean Delvare ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] linux/kernel.h: Fix DIV_ROUND_CLOSEST with unsigned divisors 2012-12-20 10:22 ` Jean Delvare @ 2012-12-20 10:30 ` Juergen Beisert 2012-12-20 11:00 ` Jean Delvare 2012-12-20 14:13 ` Guenter Roeck 1 sibling, 1 reply; 10+ messages in thread From: Juergen Beisert @ 2012-12-20 10:30 UTC (permalink / raw) To: Jean Delvare; +Cc: Guenter Roeck, Andrew Morton, linux-kernel, lm-sensors Hi Jean, Jean Delvare wrote: > On Wed, 19 Dec 2012 14:41:22 -0800, Guenter Roeck wrote: > > On Wed, Dec 19, 2012 at 01:47:21PM -0800, Andrew Morton wrote: > > > The changelog didn't describe the end-user visible effects of the bug. > > > Please always include this information. Because... > > > > One observed effect is that the s2c_hwmon driver reports a value of > > 4198403 instead of 0 if the ADC reads 0. > > > > Other impact is unpredictable. Problem is seen if the divisor is an > > unsigned variable or constant and the dividend is less than (divisor/2). > > Really? In my own testing, the problem only shows with dividend == 0, > and even then, only when dividend is signed and divisor is not. > DIV_ROUND_CLOSEST(5, 20U) returns 0 as expected, and so do > DIV_ROUND_CLOSEST(0 / 20), DIV_ROUND_CLOSEST(0U / 20) and > DIV_ROUND_CLOSEST(0U / 20U). > > Are your observations different? I tried it with this simple user-land program to get an idea what's going wrong in the s3c_hwmon.c ADC driver: #define DIV_ROUND_CLOSEST(x, divisor)( \ { \ typeof(x) __x = x; \ typeof(divisor) __d = divisor; \ (((typeof(x))-1) > 0 || (__x) > 0) ? \ (((__x) + ((__d) / 2)) / (__d)) : \ (((__x) - ((__d) / 2)) / (__d)); \ } \ ) int main(int argc, char *argv[]) { int x; unsigned y; printf("Constants\n"); printf("-1 -> %d\n", DIV_ROUND_CLOSEST(-1, 2)); printf("-1 -> %d\n", DIV_ROUND_CLOSEST(-1, 1023)); printf("0 -> %d\n", DIV_ROUND_CLOSEST(0, 1023)); printf("0 -> %d\n", DIV_ROUND_CLOSEST(0, 2)); printf("1 -> %d\n", DIV_ROUND_CLOSEST(1, 2)); printf("1 -> %d\n", DIV_ROUND_CLOSEST(3300, 1023)); printf("2 -> %d\n", DIV_ROUND_CLOSEST(6600, 1023)); printf("Variables\n"); x = -1; y = 2; printf("-1 -> %d\n", DIV_ROUND_CLOSEST(x, y)); x = -1; y = 1023; printf("-1 -> %d\n", DIV_ROUND_CLOSEST(x, y)); x = 0; y = 1023; printf("0 -> %d\n", DIV_ROUND_CLOSEST(x, y)); x = 3300; y = 1023; printf("3300 -> %d\n", DIV_ROUND_CLOSEST(3300, 1023)); x = 6600; y = 1023; printf("6600 -> %d\n", DIV_ROUND_CLOSEST(6600, 1023)); return 0; } Result is on my x86 host (same on my ARM target): Constants -1 -> -1 -1 -> 0 0 -> 0 0 -> 0 1 -> 1 1 -> 3 2 -> 6 Variables -1 -> 2147483647 -1 -> 4198403 0 -> 4198403 3300 -> 3 6600 -> 6 Regards, Juergen -- Pengutronix e.K. | Juergen Beisert | Linux Solutions for Science and Industry | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] linux/kernel.h: Fix DIV_ROUND_CLOSEST with unsigned divisors 2012-12-20 10:30 ` Juergen Beisert @ 2012-12-20 11:00 ` Jean Delvare 0 siblings, 0 replies; 10+ messages in thread From: Jean Delvare @ 2012-12-20 11:00 UTC (permalink / raw) To: Juergen Beisert; +Cc: Guenter Roeck, Andrew Morton, linux-kernel, lm-sensors Hi Juergen, On Thu, 20 Dec 2012 11:30:38 +0100, Juergen Beisert wrote: > Hi Jean, > > Jean Delvare wrote: > > On Wed, 19 Dec 2012 14:41:22 -0800, Guenter Roeck wrote: > > > One observed effect is that the s2c_hwmon driver reports a value of > > > 4198403 instead of 0 if the ADC reads 0. > > > > > > Other impact is unpredictable. Problem is seen if the divisor is an > > > unsigned variable or constant and the dividend is less than (divisor/2). > > > > Really? In my own testing, the problem only shows with dividend == 0, > > and even then, only when dividend is signed and divisor is not. > > DIV_ROUND_CLOSEST(5, 20U) returns 0 as expected, and so do > > DIV_ROUND_CLOSEST(0 / 20), DIV_ROUND_CLOSEST(0U / 20) and > > DIV_ROUND_CLOSEST(0U / 20U). > > > > Are your observations different? > > I tried it with this simple user-land program to get an idea what's going > wrong in the s3c_hwmon.c ADC driver: > > #define DIV_ROUND_CLOSEST(x, divisor)( \ > { \ > typeof(x) __x = x; \ > typeof(divisor) __d = divisor; \ > (((typeof(x))-1) > 0 || (__x) > 0) ? \ > (((__x) + ((__d) / 2)) / (__d)) : \ > (((__x) - ((__d) / 2)) / (__d)); \ > } \ > ) > > int main(int argc, char *argv[]) > { > int x; > unsigned y; > > printf("Constants\n"); > > printf("-1 -> %d\n", DIV_ROUND_CLOSEST(-1, 2)); > printf("-1 -> %d\n", DIV_ROUND_CLOSEST(-1, 1023)); > printf("0 -> %d\n", DIV_ROUND_CLOSEST(0, 1023)); > printf("0 -> %d\n", DIV_ROUND_CLOSEST(0, 2)); > printf("1 -> %d\n", DIV_ROUND_CLOSEST(1, 2)); > printf("1 -> %d\n", DIV_ROUND_CLOSEST(3300, 1023)); > printf("2 -> %d\n", DIV_ROUND_CLOSEST(6600, 1023)); This all works properly, because everything is signed here. > printf("Variables\n"); > > x = -1; y = 2; > printf("-1 -> %d\n", DIV_ROUND_CLOSEST(x, y)); > x = -1; y = 1023; > printf("-1 -> %d\n", DIV_ROUND_CLOSEST(x, y)); > x = 0; y = 1023; > printf("0 -> %d\n", DIV_ROUND_CLOSEST(x, y)); > x = 3300; y = 1023; > printf("3300 -> %d\n", DIV_ROUND_CLOSEST(3300, 1023)); > x = 6600; y = 1023; > printf("6600 -> %d\n", DIV_ROUND_CLOSEST(6600, 1023)); I don't think variables vs. constants make any difference. What makes a difference is signed vs. unsigned. You see failures here because y is unsigned. You'd see the same with the constants above by changing 2 to 2U and 1023 to 1023U. > > return 0; > } > > Result is on my x86 host (same on my ARM target): > > Constants > -1 -> -1 > -1 -> 0 > 0 -> 0 > 0 -> 0 > 1 -> 1 > 1 -> 3 > 2 -> 6 > Variables > -1 -> 2147483647 > -1 -> 4198403 > 0 -> 4198403 > 3300 -> 3 > 6600 -> 6 I see the same here with your test program. -- Jean Delvare ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] linux/kernel.h: Fix DIV_ROUND_CLOSEST with unsigned divisors 2012-12-20 10:22 ` Jean Delvare 2012-12-20 10:30 ` Juergen Beisert @ 2012-12-20 14:13 ` Guenter Roeck 1 sibling, 0 replies; 10+ messages in thread From: Guenter Roeck @ 2012-12-20 14:13 UTC (permalink / raw) To: Jean Delvare; +Cc: Andrew Morton, linux-kernel, lm-sensors, Juergen Beisert On Thu, Dec 20, 2012 at 11:22:02AM +0100, Jean Delvare wrote: > Hi Guenter, > > On Wed, 19 Dec 2012 14:41:22 -0800, Guenter Roeck wrote: > > On Wed, Dec 19, 2012 at 01:47:21PM -0800, Andrew Morton wrote: > > > The changelog didn't describe the end-user visible effects of the bug. > > > Please always include this information. Because... > > > > One observed effect is that the s2c_hwmon driver reports a value of 4198403 > > instead of 0 if the ADC reads 0. > > > > Other impact is unpredictable. Problem is seen if the divisor is an unsigned > > variable or constant and the dividend is less than (divisor/2). > > Really? In my own testing, the problem only shows with dividend == 0, and even > then, only when dividend is signed and divisor is not. DIV_ROUND_CLOSEST(5, > 20U) returns 0 as expected, and so do DIV_ROUND_CLOSEST(0 / 20), > DIV_ROUND_CLOSEST(0U / 20) and DIV_ROUND_CLOSEST(0U / 20U). > > Are your observations different? > Hmm, you are right - it only happens with 0. I thought I had also seen it with other values. > > > The patch is applicable to 3.7.x. Should we backport it? > > > > Yes. DIV_ROUND_CLOSEST is used throughout the kernel, and impact is > > unpredictable. > > > > 3.6 needs it as well. > Turns out 3.6 is EOL, so we'll only need it in 3.7. Guenter ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] linux/kernel.h: Fix DIV_ROUND_CLOSEST with unsigned divisors 2012-12-19 14:40 [PATCH v2] linux/kernel.h: Fix DIV_ROUND_CLOSEST with unsigned divisors Guenter Roeck 2012-12-19 21:47 ` Andrew Morton @ 2012-12-19 22:21 ` Jean Delvare 2012-12-19 23:01 ` Guenter Roeck 1 sibling, 1 reply; 10+ messages in thread From: Jean Delvare @ 2012-12-19 22:21 UTC (permalink / raw) To: Guenter Roeck; +Cc: linux-kernel, Andrew Morton, lm-sensors, Juergen Beisert 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... I wouldn't be surprised if there are bugs because of this in the kernel and in other projects. > As a result, DIV_ROUND_CLOSEST(0, 2U) and similar operations now return > bad values, since the automatic conversion of expressions such as "0 - 2U/2" > to unsigned was not taken into account. > > Fix by checking for the divisor variable type when deciding which operation > to perform. This fixes DIV_ROUND_CLOSEST(0, 2U), but still returns bad values > for negative dividends divided by unsigned divisors. Mark the latter case as > unsupported. True but this last issue isn't specific to the DIV_ROUND_CLOSEST implementation, it would also happen with a simple division. > Reported-by: Juergen Beisert <jbe@pengutronix.de> > Tested-by: Juergen Beisert <jbe@pengutronix.de> > Cc: Jean Delvare <khali@linux-fr.org> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > v2: Description update (v1 wasn't supposed to make it to lkml) > > include/linux/kernel.h | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h > index d97ed58..45726dc 100644 > --- a/include/linux/kernel.h > +++ b/include/linux/kernel.h > @@ -77,13 +77,15 @@ > > /* > * Divide positive or negative dividend by positive divisor and round > - * to closest integer. Result is undefined for negative divisors. > + * to closest integer. Result is undefined for negative divisors and > + * for negative dividends if the divisor variable type is unsigned. 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? > */ > #define DIV_ROUND_CLOSEST(x, divisor)( \ > { \ > typeof(x) __x = x; \ > typeof(divisor) __d = divisor; \ > - (((typeof(x))-1) > 0 || (__x) > 0) ? \ > + (((typeof(x))-1) > 0 || \ > + ((typeof(divisor))-1) > 0 || (__x) > 0) ? \ > (((__x) + ((__d) / 2)) / (__d)) : \ > (((__x) - ((__d) / 2)) / (__d)); \ > } \ Looks good. -- Jean Delvare ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] linux/kernel.h: Fix DIV_ROUND_CLOSEST with unsigned divisors 2012-12-19 22:21 ` Jean Delvare @ 2012-12-19 23:01 ` Guenter Roeck 2012-12-20 11:48 ` Jean Delvare 0 siblings, 1 reply; 10+ messages in thread From: Guenter Roeck @ 2012-12-19 23:01 UTC (permalink / raw) To: Jean Delvare; +Cc: linux-kernel, Andrew Morton, lm-sensors, Juergen Beisert 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 :). > > As a result, DIV_ROUND_CLOSEST(0, 2U) and similar operations now return > > bad values, since the automatic conversion of expressions such as "0 - 2U/2" > > to unsigned was not taken into account. > > > > Fix by checking for the divisor variable type when deciding which operation > > to perform. This fixes DIV_ROUND_CLOSEST(0, 2U), but still returns bad values > > for negative dividends divided by unsigned divisors. Mark the latter case as > > unsupported. > > True but this last issue isn't specific to the DIV_ROUND_CLOSEST > implementation, it would also happen with a simple division. > Correct, which is why I did not try to fix it. Still worth mentioning, though, in my opinion. > > Reported-by: Juergen Beisert <jbe@pengutronix.de> > > Tested-by: Juergen Beisert <jbe@pengutronix.de> > > Cc: Jean Delvare <khali@linux-fr.org> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > --- > > v2: Description update (v1 wasn't supposed to make it to lkml) > > > > include/linux/kernel.h | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h > > index d97ed58..45726dc 100644 > > --- a/include/linux/kernel.h > > +++ b/include/linux/kernel.h > > @@ -77,13 +77,15 @@ > > > > /* > > * Divide positive or negative dividend by positive divisor and round > > - * to closest integer. Result is undefined for negative divisors. > > + * to closest integer. Result is undefined for negative divisors and > > + * for negative dividends if the divisor variable type is unsigned. > > 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. Guenter > > */ > > #define DIV_ROUND_CLOSEST(x, divisor)( \ > > { \ > > typeof(x) __x = x; \ > > typeof(divisor) __d = divisor; \ > > - (((typeof(x))-1) > 0 || (__x) > 0) ? \ > > + (((typeof(x))-1) > 0 || \ > > + ((typeof(divisor))-1) > 0 || (__x) > 0) ? \ > > (((__x) + ((__d) / 2)) / (__d)) : \ > > (((__x) - ((__d) / 2)) / (__d)); \ > > } \ > > Looks good. > > -- > Jean Delvare > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] linux/kernel.h: Fix DIV_ROUND_CLOSEST with unsigned divisors 2012-12-19 23:01 ` Guenter Roeck @ 2012-12-20 11:48 ` Jean Delvare 0 siblings, 0 replies; 10+ messages in thread From: Jean Delvare @ 2012-12-20 11:48 UTC (permalink / raw) To: Guenter Roeck; +Cc: linux-kernel, Andrew Morton, lm-sensors, Juergen Beisert 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-12-20 14:13 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-12-19 14:40 [PATCH v2] linux/kernel.h: Fix DIV_ROUND_CLOSEST with unsigned divisors Guenter Roeck 2012-12-19 21:47 ` Andrew Morton 2012-12-19 22:41 ` Guenter Roeck 2012-12-20 10:22 ` Jean Delvare 2012-12-20 10:30 ` Juergen Beisert 2012-12-20 11:00 ` Jean Delvare 2012-12-20 14:13 ` Guenter Roeck 2012-12-19 22:21 ` Jean Delvare 2012-12-19 23:01 ` Guenter Roeck 2012-12-20 11:48 ` Jean Delvare
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox