abs() vs. abs64() (was: Re: [PATCH] fbdev: fix nearest mode search)

Discussion:

(too old to reply)

Geert Uytterhoeven

2010-11-18 06:40:28 UTC

[Don't use the obsolete linux-fbdev-devel address]

In the framebuffer subsystem the abs() macro is often used as a part of
the calculation of a Manhattan metric, which in turn is used as a measure
of similarity between video modes. The arguments of abs() are sometimes
unsigned numbers. This worked fine until commit a49c59c0, which changed
the definition of abs() to prevent truncation. As a result of this
u32 a = 0, b = 1;
u32 c = abs(a - b);

Indeed, the difference of 2 numbers is unsigned, as per C.

'c' will end up with a value of 0xffffffff instead of the expected 0x1.

This happens on 64-bit only, right?

Anyway, I think commit a49c59c0 is wrong: abs() operates on signed
(32-bit) numbers. For larger (64-bit signed) numbers, we have abs64().

A problem caused by this change and visible by the end user is that
framebuffer drivers relying on functions from modedb.c will fail to
find high resolution video modes similar to that explicitly requested
by the user if an exact match cannot be found (see e.g.
https://bugs.gentoo.org/show_bug.cgi?id=296539).
Fix this problem by casting all arguments of abs() to an int prior
to the macro evaluation in modedb.c and uvesafb.c.
---
diff --git a/drivers/video/modedb.c b/drivers/video/modedb.c
index 0a4dbdc..878bea1 100644
--- a/drivers/video/modedb.c
+++ b/drivers/video/modedb.c
if (refresh_specified && db[i].refresh == refresh) {
return 1;
} else {
- if (abs(db[i].refresh - refresh) < diff) {
- diff = abs(db[i].refresh - refresh);
+ if (abs((int)(db[i].refresh - refresh)) <
+ diff) {
+ diff = abs((int)(db[i].refresh -
+ refresh));
best = i;
}
}
for (i = 0; i < dbsize; i++) {
DPRINTK("Trying %ix%i\n", db[i].xres, db[i].yres);
if (!fb_try_mode(var, info, &db[i], bpp)) {
- tdiff = abs(db[i].xres - xres) +
- abs(db[i].yres - yres);
+ tdiff = abs((int)(db[i].xres - xres)) +
+ abs((int)(db[i].yres - yres));
/*
* Penalize modes with resolutions smaller
@@ -851,13 +853,13 @@ const struct fb_videomode *fb_find_nearest_mode(const struct fb_videomode *mode,
modelist = list_entry(pos, struct fb_modelist, list);
cmode = &modelist->mode;
- d = abs(cmode->xres - mode->xres) +
- abs(cmode->yres - mode->yres);
+ d = abs((int)(cmode->xres - mode->xres)) +
+ abs((int)(cmode->yres - mode->yres));
if (diff > d) {
diff = d;
best = cmode;
} else if (diff == d) {
- d = abs(cmode->refresh - mode->refresh);
+ d = abs((int)(cmode->refresh - mode->refresh));
if (diff_refresh > d) {
diff_refresh = d;
best = cmode;
diff --git a/drivers/video/uvesafb.c b/drivers/video/uvesafb.c
index 7b8839e..6621427 100644
--- a/drivers/video/uvesafb.c
+++ b/drivers/video/uvesafb.c
@@ -320,9 +320,9 @@ static int uvesafb_vbe_find_mode(struct uvesafb_par *par,
int i, match = -1, h = 0, d = 0x7fffffff;
for (i = 0; i < par->vbe_modes_cnt; i++) {
- h = abs(par->vbe_modes[i].x_res - xres) +
- abs(par->vbe_modes[i].y_res - yres) +
- abs(depth - par->vbe_modes[i].depth);
+ h = abs((int)(par->vbe_modes[i].x_res - xres)) +
+ abs((int)(par->vbe_modes[i].y_res - yres)) +
+ abs((int)(depth - par->vbe_modes[i].depth));
/*
* We have an exact match in terms of resolution
@@ -1375,7 +1375,7 @@ static int uvesafb_check_var(struct fb_var_screeninfo *var,
* which is theoretically incorrect, but which we'll try to handle
* here.
*/
- if (depth == 0 || abs(depth - var->bits_per_pixel) >= 8)
+ if (depth == 0 || abs((int)(depth - var->bits_per_pixel)) >= 8)
depth = var->bits_per_pixel;
match = uvesafb_vbe_find_mode(par, var->xres, var->yres, depth,

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ***@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
N��r��y��b�X��ǧv�^�)޺{.n�+��{��zX��ܨ}��Ơz�&j:+v��zZ+��+zf��h��~��i��z��w��?��&�)ߢf��^jǫy�m��@A�a��0��h��i

Andrew Morton

2010-11-19 22:08:44 UTC

Permalink

On Thu, 18 Nov 2010 07:40:19 +0100

Post by Geert Uytterhoeven
[Don't use the obsolete linux-fbdev-devel address]

In the framebuffer subsystem the abs() macro is often used as a part of
the calculation of a Manhattan metric, which in turn is used as a measure
of similarity between video modes. __The arguments of abs() are sometimes
unsigned numbers. __This worked fine until commit a49c59c0, which changed
the definition of abs() to prevent truncation. __As a result of this
__u32 a = 0, b = 1;
__u32 c = abs(a - b);

Indeed, the difference of 2 numbers is unsigned, as per C.

'c' will end up with a value of 0xffffffff instead of the expected 0x1.

This happens on 64-bit only, right?
Anyway, I think commit a49c59c0 is wrong: abs() operates on signed
(32-bit) numbers. For larger (64-bit signed) numbers, we have abs64().

How does this look?

From: Andrew Morton <***@linux-foundation.org>

Michal reports:

In the framebuffer subsystem the abs() macro is often used as a part of
the calculation of a Manhattan metric, which in turn is used as a measure
of similarity between video modes. The arguments of abs() are sometimes
unsigned numbers. This worked fine until commit a49c59c0 ("Make sure the
value in abs() does not get truncated if it is greater than 2^32:) , which
changed the definition of abs() to prevent truncation. As a result of
this change, in the following piece of code:

u32 a = 0, b = 1;
u32 c = abs(a - b);

'c' will end up with a value of 0xffffffff instead of the expected 0x1.

A problem caused by this change and visible by the end user is that
framebuffer drivers relying on functions from modedb.c will fail to find
high resolution video modes similar to that explicitly requested by the
user if an exact match cannot be found (see e.g.

Fix this by special-casing `long' types within abs().

This patch reduces x86_64 code size a bit - drivers/video/uvesafb.o shrunk
by 15 bytes, presumably because it is doing abs() on 4-byte quantities,
and expanding those to 8-byte longs adds code.

testcase:

#define oldabs(x) ({ \
long __x = (x); \
(__x < 0) ? -__x : __x; \
})

#define newabs(x) ({ \
long ret; \
if (sizeof(x) == sizeof(long)) { \
long __x = (x); \
ret = (__x < 0) ? -__x : __x; \
} else { \
int __x = (x); \
ret = (__x < 0) ? -__x : __x; \
} \
ret; \
})

typedef unsigned int u32;

main()
{
u32 a = 0;
u32 b = 1;
u32 oldc = oldabs(a - b);
u32 newc = newabs(a - b);

printf("%u %u\n", oldc, newc);
}

akpm:/home/akpm> gcc t.c
akpm:/home/akpm> ./a.out
4294967295 1

Reported-by: Michal Januszewski <***@gmail.com>
Cc: Rolf Eike Beer <eike-***@sf-tec.de
Cc: Geert Uytterhoeven <***@linux-m68k.org>
Signed-off-by: Andrew Morton <***@linux-foundation.org>
---

include/linux/kernel.h | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)

diff -puN include/linux/kernel.h~include-linux-kernelh-abs-fix-handling-of-32-bit-unsigneds-on-64-bit include/linux/kernel.h
--- a/include/linux/kernel.h~include-linux-kernelh-abs-fix-handling-of-32-bit-unsigneds-on-64-bit
+++ a/include/linux/kernel.h
@@ -143,9 +143,16 @@ extern int _cond_resched(void);

#define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)

-#define abs(x) ({ \
- long __x = (x); \
- (__x < 0) ? -__x : __x; \
+#define abs(x) ({ \
+ long ret; \
+ if (sizeof(x) == sizeof(long)) { \
+ long __x = (x); \
+ ret = (__x < 0) ? -__x : __x; \
+ } else { \
+ int __x = (x); \
+ ret = (__x < 0) ? -__x : __x; \
+ } \
+ ret; \
})

#define abs64(x) ({ \
_

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Michal Januszewski

2010-11-19 22:28:38 UTC

Permalink

Post by Andrew Morton
On Thu, 18 Nov 2010 07:40:19 +0100

Post by Geert Uytterhoeven

'c' will end up with a value of 0xffffffff instead of the expected 0x1.

This happens on 64-bit only, right?

Absolutely, I should have mentioned it in the patch description.

Post by Andrew Morton
How does this look?
[..]
---
include/linux/kernel.h | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff -puN include/linux/kernel.h~include-linux-kernelh-abs-fix-handling-of-32-bit-unsigneds-on-64-bit include/linux/kernel.h
--- a/include/linux/kernel.h~include-linux-kernelh-abs-fix-handling-of-32-bit-unsigneds-on-64-bit
+++ a/include/linux/kernel.h
@@ -143,9 +143,16 @@ extern int _cond_resched(void);
#define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
-#define abs(x) ({ \
- long __x = (x); \
- (__x < 0) ? -__x : __x; \
+#define abs(x) ({ \
+ long ret; \
+ if (sizeof(x) == sizeof(long)) { \
+ long __x = (x); \
+ ret = (__x < 0) ? -__x : __x; \
+ } else { \
+ int __x = (x); \
+ ret = (__x < 0) ? -__x : __x; \
+ } \
+ ret; \
})
#define abs64(x) ({ \
_

Looks good to me. I posted essentially the same thing some 3 months ago
(http://marc.info/?l=linux-kernel&m=128033094822201&w=2) but it then
failed to get any traction. At any rate, I like your version better as
it seems more readable.

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Andrew Morton

2010-11-19 23:04:39 UTC

Permalink

On Fri, 19 Nov 2010 23:28:23 +0100

Post by Michal Januszewski

Post by Andrew Morton
On Thu, 18 Nov 2010 07:40:19 +0100

Post by Geert Uytterhoeven

'c' will end up with a value of 0xffffffff instead of the expected 0x1.

This happens on 64-bit only, right?

Absolutely, I should have mentioned it in the patch description.

I spose we should document it. Does this look complete and accurate?

--- a/include/linux/kernel.h~include-linux-kernelh-abs-fix-handling-of-32-bit-unsigneds-on-64-bit-fix
+++ a/include/linux/kernel.h
@@ -143,6 +143,13 @@ extern int _cond_resched(void);

#define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)

+/*
+ * abs() handles unsigned and signed longs, ints, shorts and chars. For long
+ * types it returns a signed long. For int, short and char types it returns a
+ * signed int.
+ * abs() should not be used for 64-bit types (s64, u64, long long) - use abs64()
+ * for those.
+ */
#define abs(x) ({ \
long ret; \
if (sizeof(x) == sizeof(long)) { \
_

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Andrew Morton

2010-11-19 23:20:21 UTC

Permalink

On Fri, 19 Nov 2010 15:04:11 -0800

Post by Andrew Morton

Post by Michal Januszewski
Looks good to me. I posted essentially the same thing some 3 months ago
(http://marc.info/?l=linux-kernel&m=128033094822201&w=2) but it then
failed to get any traction. At any rate, I like your version better as
it seems more readable.

I spose we should document it. Does this look complete and accurate?
--- a/include/linux/kernel.h~include-linux-kernelh-abs-fix-handling-of-32-bit-unsigneds-on-64-bit-fix
+++ a/include/linux/kernel.h
@@ -143,6 +143,13 @@ extern int _cond_resched(void);
#define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
+/*
+ * abs() handles unsigned and signed longs, ints, shorts and chars. For long
+ * types it returns a signed long. For int, short and char types it returns a
+ * signed int.
+ * abs() should not be used for 64-bit types (s64, u64, long long) - use abs64()
+ * for those.
+ */
#define abs(x) ({ \
long ret; \
if (sizeof(x) == sizeof(long)) { \

Well that was a load of bollocks. 2nd attempt:

--- a/include/linux/kernel.h~include-linux-kernelh-abs-fix-handling-of-32-bit-unsigneds-on-64-bit-fix
+++ a/include/linux/kernel.h
@@ -143,6 +143,12 @@ extern int _cond_resched(void);

#define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)

+/*
+ * abs() handles unsigned and signed longs, ints, shorts and chars. For all
+ * input types abs() returns a signed long.
+ * abs() should not be used for 64-bit types (s64, u64, long long) - use abs64()
+ * for those.
+ */
#define abs(x) ({ \
long ret; \
if (sizeof(x) == sizeof(long)) { \
_

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Geert Uytterhoeven

2010-11-20 08:56:40 UTC

Permalink

Post by Andrew Morton
On Fri, 19 Nov 2010 15:04:11 -0800

I spose we should document it. Does this look complete and accurate?
--- a/include/linux/kernel.h~include-linux-kernelh-abs-fix-handling-of-32-bit-unsigneds-on-64-bit-fix
+++ a/include/linux/kernel.h
@@ -143,6 +143,13 @@ extern int _cond_resched(void);
#define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
+/*
+ * abs() handles unsigned and signed longs, ints, shorts and chars. For long
+ * types it returns a signed long. For int, short and char types it returns a
+ * signed int.
+ * abs() should not be used for 64-bit types (s64, u64, long long) - use abs64()
+ * for those.
+ */
#define abs(x) ({ \
long ret; \
if (sizeof(x) == sizeof(long)) { \

Yeah, I was just gonna complain "but long _is_ 64-bit on 64-bit platforms"...

Post by Andrew Morton
--- a/include/linux/kernel.h~include-linux-kernelh-abs-fix-handling-of-32-bit-unsigneds-on-64-bit-fix
+++ a/include/linux/kernel.h
@@ -143,6 +143,12 @@ extern int _cond_resched(void);
#define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
+/*
+ * abs() handles unsigned and signed longs, ints, shorts and chars. For all
+ * input types abs() returns a signed long.
+ * abs() should not be used for 64-bit types (s64, u64, long long) - use abs64()
+ * for those.
+ */
#define abs(x) ({ \
long ret; \
if (sizeof(x) == sizeof(long)) { \

After some second thinking, I think this is OK...

Post by Andrew Morton
of similarity between video modes. The arguments of abs() are sometimes
unsigned numbers. This worked fine until commit a49c59c0 ("Make sure the

The "sometimes" is when the parameter is the difference of 2 numbers, which is a
highly likely use case. Unlike most people's intuitive mathematical
feelings, a difference
is always unsigned in C (that was incorrect in the comment of Michal's
first version).
So I think it's worth mentioning that explicitly.

.. and please llet me do the third thinking during the rest of the day ;-)

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ***@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/