[Don't use the obsolete linux-fbdev-devel address]
On Thu, Nov 18, 2010 at 00:08, Michal Januszewski <[email protected]> wrote:
> 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
> change, in the following piece of code:
>
> 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.
>
> Signed-off-by: Michal Januszewski <[email protected]>
> ---
> 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
> @@ -636,8 +636,10 @@ done:
> 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;
> }
> }
> @@ -654,8 +656,8 @@ done:
> 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 -- [email protected]
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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Thu, 18 Nov 2010 07:40:19 +0100
Geert Uytterhoeven <[email protected]> wrote:
> [Don't use the obsolete linux-fbdev-devel address]
>
> On Thu, Nov 18, 2010 at 00:08, Michal Januszewski <[email protected]> wrote:
> > 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
> > change, in the following piece of code:
> >
> > __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).
How does this look?
From: Andrew Morton <[email protected]>
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 <[email protected]>
Cc: Rolf Eike Beer <[email protected]
Cc: Geert Uytterhoeven <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
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) ({ \
_
On Fri, Nov 19, 2010 at 02:07:21PM -0800, Andrew Morton wrote:
> On Thu, 18 Nov 2010 07:40:19 +0100
> Geert Uytterhoeven <[email protected]> wrote:
> > > '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.
> How does this look?
>
> [..]
>
> Reported-by: Michal Januszewski <[email protected]>
> Cc: Rolf Eike Beer <[email protected]
> Cc: Geert Uytterhoeven <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> ---
>
> 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
On Fri, 19 Nov 2010 23:28:23 +0100
Michal Januszewski <[email protected]> wrote:
> On Fri, Nov 19, 2010 at 02:07:21PM -0800, Andrew Morton wrote:
> > On Thu, 18 Nov 2010 07:40:19 +0100
> > Geert Uytterhoeven <[email protected]> wrote:
>
> > > > '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.
>
> > How does this look?
> >
> > [..]
> >
> > Reported-by: Michal Januszewski <[email protected]>
> > Cc: Rolf Eike Beer <[email protected]
> > Cc: Geert Uytterhoeven <[email protected]>
> > Signed-off-by: Andrew Morton <[email protected]>
> > ---
> >
> > 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.
>
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)) { \
_
On Fri, 19 Nov 2010 15:04:11 -0800
Andrew Morton <[email protected]> wrote:
> > 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)) { \
_
On Sat, Nov 20, 2010 at 00:19, Andrew Morton <[email protected]> wrote:
> On Fri, 19 Nov 2010 15:04:11 -0800
> Andrew Morton <[email protected]> wrote:
>
>> > 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:
Yeah, I was just gonna complain "but long _is_ 64-bit on 64-bit platforms"...
> --- 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...
In the first patch:
> 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 -- [email protected]
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