Strange results of DIV_ROUND_CLOSEST

DISCUSSION Juergen Beisert 2 participants 6 messages Dec 18, 2012 – Dec 19, 2012

Share & tools

Juergen Beisert

author

2012-12-18 15:06 UTC #0 lore raw

Hi list,

commit 263a523d18bca306016d75f5c8d5c57c37fe52fb changes the code of DIV_ROUND_CLOSEST in include/linux/kernel.h to fix a compile time warning.

But now feeding in a zero into this macro results into 4198403. Tested with gcc 4.4.3 and 4.7.2, on arch x86 and ARM.

I can reproduce this behaviour, when my ADC delivers a '0' value in the driver drivers/hwmon/s3c-hwmon.c in function s3c_hwmon_ch_show() with a current 3.7.1 kernel. The value is correct again, when the ADC delivers at least a '1'.

Any ideas how to fix it correctly?

jbe

Signature · 2 lines

Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

Guenter Roeck

2012-12-18 17:16 UTC #1 lore raw

Quoted 8 lines · Juergen Beisert

Hi list,
commit 263a523d18bca306016d75f5c8d5c57c37fe52fb changes the code of DIV_ROUND_CLOSEST in include/linux/kernel.h to fix a compile time warning.
But now feeding in a zero into this macro results into 4198403. Tested with gcc 4.4.3 and 4.7.2, on arch x86 and ARM.

Quoted 7 lines

I can reproduce this behaviour, when my ADC delivers a '0' value in the driver drivers/hwmon/s3c-hwmon.c in function s3c_hwmon_ch_show() with a current 3.7.1 kernel. The value is correct again, when the ADC delivers at least a '1'.
Any ideas how to fix it correctly?

Odd one. I ran the macro through a large number of values and divisors as well as various optimization options, with different compilers, and always get correct results.

What are your compile options, and what are the channel multiplier and dividers set to ?

Thanks,
Guenter

Juergen Beisert

author

2012-12-18 21:07 UTC #2 lore raw

Hi Guenter,

Quoted 13 lines · Guenter Roeck

Quoted 12 lines · Juergen Beisert
commit 263a523d18bca306016d75f5c8d5c57c37fe52fb changes the code of DIV_ROUND_CLOSEST in include/linux/kernel.h to fix a compile time warning.
But now feeding in a zero into this macro results into 4198403. Tested with gcc 4.4.3 and 4.7.2, on arch x86 and ARM.
I can reproduce this behaviour, when my ADC delivers a '0' value in the driver drivers/hwmon/s3c-hwmon.c in function s3c_hwmon_ch_show() with a current 3.7.1 kernel. The value is correct again, when the ADC delivers at least a '1'.

Quoted 8 lines

Quoted 1 line
Any ideas how to fix it correctly?
Odd one. I ran the macro through a large number of values and divisors as well as various optimization options, with different compilers, and always get correct results.
What are your compile options, and what are the channel multiplier and dividers set to ?

Refer the lines 177 to 182 in drivers/hwmon/s3c-hwmon.c. "cfg->mult" is '3300' in my case, and "cfg->div" is '1023'. And whenever s3c_hwmon_read_ch() returns '0' line 184 returns '4198403' since Linux-3.6. checked with my gcc-4.6.2 cross compiler for Linux-3.6 and with gcc-4.6.2 for Linux-3.7.

I did a quick test with this macro on my host with gcc-4.4.3 and a simple userland program and surprise, surprise:

result = DIV_ROUND_CLOSEST(0, 1023);

works as expected (result is 0), but

int x = 0;
unsigned y = 1023;
result = DIV_ROUND_CLOSEST(x, y);

gives me result = 4198403!

Strange.

Regards,
Juergen

Signature · 2 lines

Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

Guenter Roeck

2012-12-18 22:59 UTC #3 lore raw

Quoted 37 lines · Juergen Beisert

Hi Guenter,
Quoted 13 lines · Guenter Roeck
Quoted 12 lines · Juergen Beisert
commit 263a523d18bca306016d75f5c8d5c57c37fe52fb changes the code of DIV_ROUND_CLOSEST in include/linux/kernel.h to fix a compile time warning.
But now feeding in a zero into this macro results into 4198403. Tested with gcc 4.4.3 and 4.7.2, on arch x86 and ARM.
I can reproduce this behaviour, when my ADC delivers a '0' value in the driver drivers/hwmon/s3c-hwmon.c in function s3c_hwmon_ch_show() with a current 3.7.1 kernel. The value is correct again, when the ADC delivers at least a '1'.
Quoted 8 lines
Quoted 1 line
Any ideas how to fix it correctly?
Odd one. I ran the macro through a large number of values and divisors as well as various optimization options, with different compilers, and always get correct results.
What are your compile options, and what are the channel multiplier and dividers set to ?
Refer the lines 177 to 182 in drivers/hwmon/s3c-hwmon.c. "cfg->mult" is '3300' in my case, and "cfg->div" is '1023'. And whenever s3c_hwmon_read_ch() returns '0' line 184 returns '4198403' since Linux-3.6. checked with my gcc-4.6.2 cross compiler for Linux-3.6 and with gcc-4.6.2 for Linux-3.7.
I did a quick test with this macro on my host with gcc-4.4.3 and a simple userland program and surprise, surprise:
result = DIV_ROUND_CLOSEST(0, 1023);
works as expected (result is 0), but

Quoted 8 lines

int x = 0;
unsigned y = 1023;
result = DIV_ROUND_CLOSEST(x, y);
gives me result = 4198403!
Strange.

Yes. Try:

	printf("%d\n", -10 / 2U);

I am amazed. Apparently the compiler converts an expression to unsigned if the divisor is unsigned. Which of course completely defeats the purpose of the operation.

I'll try to come up with a fix. Let me know if you have an idea. Obviously this does not only affect the "0" case, but all operations with negative dividend and unsigned divisor.

Guenter

Guenter Roeck

2012-12-19 01:45 UTC #4 lore raw

In reply to Juergen Beisert

Quoted 35 lines · Juergen Beisert

Hi Guenter,
Quoted 13 lines · Guenter Roeck
Quoted 12 lines · Juergen Beisert
commit 263a523d18bca306016d75f5c8d5c57c37fe52fb changes the code of DIV_ROUND_CLOSEST in include/linux/kernel.h to fix a compile time warning.
But now feeding in a zero into this macro results into 4198403. Tested with gcc 4.4.3 and 4.7.2, on arch x86 and ARM.
I can reproduce this behaviour, when my ADC delivers a '0' value in the driver drivers/hwmon/s3c-hwmon.c in function s3c_hwmon_ch_show() with a current 3.7.1 kernel. The value is correct again, when the ADC delivers at least a '1'.
Quoted 8 lines
Quoted 1 line
Any ideas how to fix it correctly?
Odd one. I ran the macro through a large number of values and divisors as well as various optimization options, with different compilers, and always get correct results.
What are your compile options, and what are the channel multiplier and dividers set to ?
Refer the lines 177 to 182 in drivers/hwmon/s3c-hwmon.c. "cfg->mult" is '3300' in my case, and "cfg->div" is '1023'. And whenever s3c_hwmon_read_ch() returns '0' line 184 returns '4198403' since Linux-3.6. checked with my gcc-4.6.2 cross compiler for Linux-3.6 and with gcc-4.6.2 for Linux-3.7.
I did a quick test with this macro on my host with gcc-4.4.3 and a simple userland program and surprise, surprise:
result = DIV_ROUND_CLOSEST(0, 1023);

Quoted 8 lines

works as expected (result is 0), but
int x = 0;
unsigned y = 1023;
result = DIV_ROUND_CLOSEST(x, y);
gives me result = 4198403!

Juergen,

Can you test the following patch ?

Thanks,
Guenter

Quoted 1 line

From d4a639c6ace7cb0de247f59c38abac72d671f135 Mon Sep 17 00:00:00 2001

From: Guenter Roeck <[email protected]>
Date: Tue, 18 Dec 2012 15:48:44 -0800
Subject: [PATCH] linux/kernel.h: Fix DIV_ROUND_CLOSEST for unsigned divisors

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 (-10 / 5U = 858993457). As a result, DIV_ROUND_CLOSEST(0, 2U) and similar operations now return bad data.

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 data for negative dividends divided by unsigned divisors. Mark the latter case as unsupported.

Reported-by Juergen Beisert <[email protected]>

Signed-off-by Guenter Roeck <[email protected]>

---
An alternative would be to always convert divisors to signed:
int __d = (int)divisor; \
Not sure if that would be better.

 include/linux/kernel.h |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

include/linux/kernel.h +4 -2

index d97ed58..45726dc 100644
@@ -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));	\
 }							\

Signature · 1 line

1.7.9.7

Juergen Beisert

author

2012-12-19 07:35 UTC #5 lore raw

Hi Guenter,

Quoted 84 lines · Guenter Roeck

Quoted 34 lines · Juergen Beisert
Quoted 13 lines · Guenter Roeck
Quoted 12 lines · Juergen Beisert
commit 263a523d18bca306016d75f5c8d5c57c37fe52fb changes the code of DIV_ROUND_CLOSEST in include/linux/kernel.h to fix a compile time warning.
But now feeding in a zero into this macro results into 4198403. Tested with gcc 4.4.3 and 4.7.2, on arch x86 and ARM.
I can reproduce this behaviour, when my ADC delivers a '0' value in the driver drivers/hwmon/s3c-hwmon.c in function s3c_hwmon_ch_show() with a current 3.7.1 kernel. The value is correct again, when the ADC delivers at least a '1'.
Quoted 8 lines
Quoted 1 line
Any ideas how to fix it correctly?
Odd one. I ran the macro through a large number of values and divisors as well as various optimization options, with different compilers, and always get correct results.
What are your compile options, and what are the channel multiplier and dividers set to ?
Refer the lines 177 to 182 in drivers/hwmon/s3c-hwmon.c. "cfg->mult" is '3300' in my case, and "cfg->div" is '1023'. And whenever s3c_hwmon_read_ch() returns '0' line 184 returns '4198403' since Linux-3.6. checked with my gcc-4.6.2 cross compiler for Linux-3.6 and with gcc-4.6.2 for Linux-3.7.
I did a quick test with this macro on my host with gcc-4.4.3 and a simple userland program and surprise, surprise:
result = DIV_ROUND_CLOSEST(0, 1023);
Quoted 7 lines
works as expected (result is 0), but
int x = 0;
unsigned y = 1023;
result = DIV_ROUND_CLOSEST(x, y);
gives me result = 4198403!
Can you test the following patch ?
From d4a639c6ace7cb0de247f59c38abac72d671f135 Mon Sep 17 00:00:00 2001
From: Guenter Roeck <[email protected]>
Date: Tue, 18 Dec 2012 15:48:44 -0800
Subject: [PATCH] linux/kernel.h: Fix DIV_ROUND_CLOSEST for unsigned divisors
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 (-10 / 5U = 858993457). As a result, DIV_ROUND_CLOSEST(0, 2U) and similar operations now return bad data.
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 data for negative dividends divided by unsigned divisors. Mark the latter case as unsupported.
Reported-by Juergen Beisert <[email protected]>
Signed-off-by Guenter Roeck <[email protected]>
---
An alternative would be to always convert divisors to signed:
int __d = (int)divisor; \
Not sure if that would be better.
 include/linux/kernel.h |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
include/linux/kernel.h +2 -1
index d97ed58..45726dc 100644
@@ -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.
  */

include/linux/kernel.h line 91

Quoted 10 lines

include/linux/kernel.h +2 -1

 #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));	\
 }							\

You can add my "tested-by: Juergen Beisert <[email protected]>". With this patch at least my S3C2440 ADC driver reports correct values again. Keep in mind it should also be fixed in the stable 3.6/3.7 kernel.

Regards,
Juergen

Signature · 2 lines

Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |