* [PATCH] m68k: Patch for broken lsl64() "old code" with incorrect shift
@ 2011-06-11 3:50 Jonathan Elchison
2011-06-11 12:01 ` Geert Uytterhoeven
0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Elchison @ 2011-06-11 3:50 UTC (permalink / raw)
To: Geert Uytterhoeven, linux-m68k, Jiri Kosina; +Cc: linux-kernel
[1.] One line summary of the problem:
[PATCH] m68k: Patch for broken lsl64() "old code" with incorrect shift
[2.] Full description of the problem/report:
In arch/m68k/math-emu/multi_arith.h, lsl64() doesn't calculate a
correct HI_WORD(*dest) when count < 32, due to an incorrect shift
operation. This line:
HI_WORD(*dest) = (HI_WORD(*dest) << count) | (LO_WORD(*dest) >> count);
...should be...
HI_WORD(*dest) = (HI_WORD(*dest) << count) | (LO_WORD(*dest) >>
(32 - count));
This function is ifdef'd out, marked as "old code". However, I fell
into the same trap into which I suspect others have fallen (or will
fall): On rare occasion when I need to implement my own library
functions, I depend on the Linux kernel to provide solid code. This
bug, albeit in old code, led me astray.
I'm asking that this old code (lsl64) be corrected or removed.
[3.] Keywords (i.e., modules, networking, kernel):
kernel, arch, m68k, math-emu, multi_arith.h, lsl64
[4.] Kernel information
2.6.39.1
[5.] Most recent kernel version which did not have the bug:
Unknown
[6.] Output of Oops.. message (if applicable) with symbolic
information resolved (see Documentation/oops-tracing.txt)
N/A
[7.] A small shell script or example program which triggers the
problem (if possible)
-----------------------------------------------------------------
#include <stdio.h>
#define LO_WORD(ll) (((unsigned int *) &ll)[1])
#define HI_WORD(ll) (((unsigned int *) &ll)[0])
static inline void lsl64_broken(int count, unsigned long long *dest)
{
if (count < 32) {
HI_WORD(*dest) = (HI_WORD(*dest) << count)
| (LO_WORD(*dest) >> count);
LO_WORD(*dest) <<= count;
return;
}
count -= 32;
HI_WORD(*dest) = LO_WORD(*dest) << count;
LO_WORD(*dest) = 0;
}
static inline void lsl64_fixed(int count, unsigned long long *dest)
{
if (count < 32) {
HI_WORD(*dest) = (HI_WORD(*dest) << count)
| (LO_WORD(*dest) >> (32 - count));
LO_WORD(*dest) <<= count;
return;
}
count -= 32;
HI_WORD(*dest) = LO_WORD(*dest) << count;
LO_WORD(*dest) = 0;
}
static inline void lsr64(int count, unsigned long long *dest)
{
if (count < 32) {
LO_WORD(*dest) = (LO_WORD(*dest) >> count)
| (HI_WORD(*dest) << (32 - count));
HI_WORD(*dest) >>= count;
return;
}
count -= 32;
LO_WORD(*dest) = HI_WORD(*dest) >> count;
HI_WORD(*dest) = 0;
}
int main( void )
{
long long testVal = 0x0000100000001000L;
long long testVal_broken = testVal;
long long testVal_fixed = testVal;
printf( "begin testVal_broken =\t\t0x%016llx\n", testVal_broken );
lsr64( 1, &testVal_broken );
printf( "intermediate testVal_broken =\t0x%016llx\n", testVal_broken );
lsl64_broken( 1, &testVal_broken );
printf( "final testVal_broken =\t\t0x%016llx\n", testVal_broken );
printf( "final matches begin?\t\t%s\n", testVal_broken == testVal ?
"TRUE" : "FALSE" );
printf( "\nbegin testVal_fixed =\t\t0x%016llx\n", testVal_fixed );
lsr64( 1, &testVal_fixed );
printf( "intermediate testVal_fixed =\t0x%016llx\n", testVal_fixed );
lsl64_fixed( 1, &testVal_fixed );
printf( "final testVal_fixed =\t\t0x%016llx\n", testVal_fixed );
printf( "final matches begin?\t\t%s\n", testVal_fixed == testVal ?
"TRUE" : "FALSE" );
return 0;
}
-----------------------------------------------------------------
[8.] Environment
Found via code inspection
[9.] Patch
Signed-off-by: Jonathan Elchison <jelchison@gmail.com>
-----------------------------------------------------------------
diff -up linux-2.6/arch/m68k/math-emu/multi_arith.h
linux-2.6_orig/arch/m68k/math-emu/multi_arith.h
--- linux-2.6/arch/m68k/math-emu/multi_arith.h 2011-06-10
22:50:32.538711320 -0400
+++ linux-2.6_orig/arch/m68k/math-emu/multi_arith.h 2011-06-10
22:47:43.407285452 -0400
@@ -236,7 +236,7 @@ static inline void lsl64(int count, unsi
{
if (count < 32) {
HI_WORD(*dest) = (HI_WORD(*dest) << count)
- | (LO_WORD(*dest) >> (32 - count));
+ | (LO_WORD(*dest) >> count);
LO_WORD(*dest) <<= count;
return;
}
-----------------------------------------------------------------
[10.] Responses
Please personally CC me on responses to this post.
--
Jonathan Elchison
jelchison@gmail.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] m68k: Patch for broken lsl64() "old code" with incorrect shift
@ 2011-06-11 3:50 Jonathan Elchison
0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Elchison @ 2011-06-11 3:50 UTC (permalink / raw)
To: Geert Uytterhoeven, linux-m68k, Jiri Kosina; +Cc: linux-kernel
[1.] One line summary of the problem:
[PATCH] m68k: Patch for broken lsl64() "old code" with incorrect shift
[2.] Full description of the problem/report:
In arch/m68k/math-emu/multi_arith.h, lsl64() doesn't calculate a
correct HI_WORD(*dest) when count < 32, due to an incorrect shift
operation. This line:
HI_WORD(*dest) = (HI_WORD(*dest) << count) | (LO_WORD(*dest) >> count);
...should be...
HI_WORD(*dest) = (HI_WORD(*dest) << count) | (LO_WORD(*dest) >>
(32 - count));
This function is ifdef'd out, marked as "old code". However, I fell
into the same trap into which I suspect others have fallen (or will
fall): On rare occasion when I need to implement my own library
functions, I depend on the Linux kernel to provide solid code. This
bug, albeit in old code, led me astray.
I'm asking that this old code (lsl64) be corrected or removed.
[3.] Keywords (i.e., modules, networking, kernel):
kernel, arch, m68k, math-emu, multi_arith.h, lsl64
[4.] Kernel information
2.6.39.1
[5.] Most recent kernel version which did not have the bug:
Unknown
[6.] Output of Oops.. message (if applicable) with symbolic
information resolved (see Documentation/oops-tracing.txt)
N/A
[7.] A small shell script or example program which triggers the
problem (if possible)
-----------------------------------------------------------------
#include <stdio.h>
#define LO_WORD(ll) (((unsigned int *) &ll)[1])
#define HI_WORD(ll) (((unsigned int *) &ll)[0])
static inline void lsl64_broken(int count, unsigned long long *dest)
{
if (count < 32) {
HI_WORD(*dest) = (HI_WORD(*dest) << count)
| (LO_WORD(*dest) >> count);
LO_WORD(*dest) <<= count;
return;
}
count -= 32;
HI_WORD(*dest) = LO_WORD(*dest) << count;
LO_WORD(*dest) = 0;
}
static inline void lsl64_fixed(int count, unsigned long long *dest)
{
if (count < 32) {
HI_WORD(*dest) = (HI_WORD(*dest) << count)
| (LO_WORD(*dest) >> (32 - count));
LO_WORD(*dest) <<= count;
return;
}
count -= 32;
HI_WORD(*dest) = LO_WORD(*dest) << count;
LO_WORD(*dest) = 0;
}
static inline void lsr64(int count, unsigned long long *dest)
{
if (count < 32) {
LO_WORD(*dest) = (LO_WORD(*dest) >> count)
| (HI_WORD(*dest) << (32 - count));
HI_WORD(*dest) >>= count;
return;
}
count -= 32;
LO_WORD(*dest) = HI_WORD(*dest) >> count;
HI_WORD(*dest) = 0;
}
int main( void )
{
long long testVal = 0x0000100000001000L;
long long testVal_broken = testVal;
long long testVal_fixed = testVal;
printf( "begin testVal_broken =\t\t0x%016llx\n", testVal_broken );
lsr64( 1, &testVal_broken );
printf( "intermediate testVal_broken =\t0x%016llx\n", testVal_broken );
lsl64_broken( 1, &testVal_broken );
printf( "final testVal_broken =\t\t0x%016llx\n", testVal_broken );
printf( "final matches begin?\t\t%s\n", testVal_broken == testVal ?
"TRUE" : "FALSE" );
printf( "\nbegin testVal_fixed =\t\t0x%016llx\n", testVal_fixed );
lsr64( 1, &testVal_fixed );
printf( "intermediate testVal_fixed =\t0x%016llx\n", testVal_fixed );
lsl64_fixed( 1, &testVal_fixed );
printf( "final testVal_fixed =\t\t0x%016llx\n", testVal_fixed );
printf( "final matches begin?\t\t%s\n", testVal_fixed == testVal ?
"TRUE" : "FALSE" );
return 0;
}
-----------------------------------------------------------------
[8.] Environment
Found via code inspection
[9.] Patch
Signed-off-by: Jonathan Elchison <jelchison@gmail.com>
-----------------------------------------------------------------
diff -up linux-2.6/arch/m68k/math-emu/multi_arith.h
linux-2.6_orig/arch/m68k/math-emu/multi_arith.h
--- linux-2.6/arch/m68k/math-emu/multi_arith.h 2011-06-10
22:50:32.538711320 -0400
+++ linux-2.6_orig/arch/m68k/math-emu/multi_arith.h 2011-06-10
22:47:43.407285452 -0400
@@ -236,7 +236,7 @@ static inline void lsl64(int count, unsi
{
if (count < 32) {
HI_WORD(*dest) = (HI_WORD(*dest) << count)
- | (LO_WORD(*dest) >> (32 - count));
+ | (LO_WORD(*dest) >> count);
LO_WORD(*dest) <<= count;
return;
}
-----------------------------------------------------------------
[10.] Responses
Please personally CC me on responses to this post.
--
Jonathan Elchison
jelchison@gmail.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] m68k: Patch for broken lsl64() "old code" with incorrect shift
2011-06-11 3:50 [PATCH] m68k: Patch for broken lsl64() "old code" with incorrect shift Jonathan Elchison
@ 2011-06-11 12:01 ` Geert Uytterhoeven
0 siblings, 0 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2011-06-11 12:01 UTC (permalink / raw)
To: Jonathan Elchison
Cc: linux-m68k, Jiri Kosina, linux-kernel, David Huggins-Daines
On Sat, Jun 11, 2011 at 05:50, Jonathan Elchison <JElchison@gmail.com> wrote:
> In arch/m68k/math-emu/multi_arith.h, lsl64() doesn't calculate a
> correct HI_WORD(*dest) when count < 32, due to an incorrect shift
> operation. This line:
>
> HI_WORD(*dest) = (HI_WORD(*dest) << count) | (LO_WORD(*dest) >> count);
>
> ...should be...
>
> HI_WORD(*dest) = (HI_WORD(*dest) << count) | (LO_WORD(*dest) >>
> (32 - count));
>
> This function is ifdef'd out, marked as "old code". However, I fell
> into the same trap into which I suspect others have fallen (or will
> fall): On rare occasion when I need to implement my own library
> functions, I depend on the Linux kernel to provide solid code. This
> bug, albeit in old code, led me astray.
>
> I'm asking that this old code (lsl64) be corrected or removed.
I think it's better to just remove all #ifdefed out parts in the code.
> diff -up linux-2.6/arch/m68k/math-emu/multi_arith.h
> linux-2.6_orig/arch/m68k/math-emu/multi_arith.h
> --- linux-2.6/arch/m68k/math-emu/multi_arith.h 2011-06-10
> 22:50:32.538711320 -0400
> +++ linux-2.6_orig/arch/m68k/math-emu/multi_arith.h 2011-06-10
> 22:47:43.407285452 -0400
> @@ -236,7 +236,7 @@ static inline void lsl64(int count, unsi
> {
> if (count < 32) {
> HI_WORD(*dest) = (HI_WORD(*dest) << count)
> - | (LO_WORD(*dest) >> (32 - count));
> + | (LO_WORD(*dest) >> count);
> LO_WORD(*dest) <<= count;
> return;
> }
The patch is reversed?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] m68k: Patch for broken lsl64() "old code" with incorrect shift
@ 2011-06-11 12:01 ` Geert Uytterhoeven
0 siblings, 0 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2011-06-11 12:01 UTC (permalink / raw)
To: Jonathan Elchison
Cc: linux-m68k, Jiri Kosina, linux-kernel, David Huggins-Daines
On Sat, Jun 11, 2011 at 05:50, Jonathan Elchison <JElchison@gmail.com> wrote:
> In arch/m68k/math-emu/multi_arith.h, lsl64() doesn't calculate a
> correct HI_WORD(*dest) when count < 32, due to an incorrect shift
> operation. This line:
>
> HI_WORD(*dest) = (HI_WORD(*dest) << count) | (LO_WORD(*dest) >> count);
>
> ...should be...
>
> HI_WORD(*dest) = (HI_WORD(*dest) << count) | (LO_WORD(*dest) >>
> (32 - count));
>
> This function is ifdef'd out, marked as "old code". However, I fell
> into the same trap into which I suspect others have fallen (or will
> fall): On rare occasion when I need to implement my own library
> functions, I depend on the Linux kernel to provide solid code. This
> bug, albeit in old code, led me astray.
>
> I'm asking that this old code (lsl64) be corrected or removed.
I think it's better to just remove all #ifdefed out parts in the code.
> diff -up linux-2.6/arch/m68k/math-emu/multi_arith.h
> linux-2.6_orig/arch/m68k/math-emu/multi_arith.h
> --- linux-2.6/arch/m68k/math-emu/multi_arith.h 2011-06-10
> 22:50:32.538711320 -0400
> +++ linux-2.6_orig/arch/m68k/math-emu/multi_arith.h 2011-06-10
> 22:47:43.407285452 -0400
> @@ -236,7 +236,7 @@ static inline void lsl64(int count, unsi
> {
> if (count < 32) {
> HI_WORD(*dest) = (HI_WORD(*dest) << count)
> - | (LO_WORD(*dest) >> (32 - count));
> + | (LO_WORD(*dest) >> count);
> LO_WORD(*dest) <<= count;
> return;
> }
The patch is reversed?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] m68k: Patch for broken lsl64() "old code" with incorrect shift
2011-06-11 12:01 ` Geert Uytterhoeven
@ 2011-06-11 12:34 ` Jonathan Elchison
-1 siblings, 0 replies; 6+ messages in thread
From: Jonathan Elchison @ 2011-06-11 12:34 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: linux-m68k, Jiri Kosina, linux-kernel, David Huggins-Daines
On Sat, Jun 11, 2011 at 8:01 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> I think it's better to just remove all #ifdefed out parts in the code.
I'm good either way, as long as the broken "old code" is no longer present.
> The patch is reversed?
My apologies, you're correct. In case anyone is interested in the
correct patch:
Signed-off-by: Jonathan Elchison <jelchison@gmail.com>
-----------------------------------------------------------------
diff -up linux-2.6_orig/arch/m68k/math-emu/multi_arith.h
linux-2.6/arch/m68k/math-emu/multi_arith.h
--- linux-2.6_orig/arch/m68k/math-emu/multi_arith.h 2011-06-10
22:47:43.407285452 -0400
+++ linux-2.6/arch/m68k/math-emu/multi_arith.h 2011-06-10
22:50:32.538711320 -0400
@@ -236,7 +236,7 @@ static inline void lsl64(int count, unsi
{
if (count < 32) {
HI_WORD(*dest) = (HI_WORD(*dest) << count)
- | (LO_WORD(*dest) >> count);
+ | (LO_WORD(*dest) >> (32 - count));
LO_WORD(*dest) <<= count;
return;
}
-----------------------------------------------------------------
Please personally CC me on responses to this post.
v/r,
--
Jonathan Elchison
jelchison@gmail.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] m68k: Patch for broken lsl64() "old code" with incorrect shift
@ 2011-06-11 12:34 ` Jonathan Elchison
0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Elchison @ 2011-06-11 12:34 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: linux-m68k, Jiri Kosina, linux-kernel, David Huggins-Daines
On Sat, Jun 11, 2011 at 8:01 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> I think it's better to just remove all #ifdefed out parts in the code.
I'm good either way, as long as the broken "old code" is no longer present.
> The patch is reversed?
My apologies, you're correct. In case anyone is interested in the
correct patch:
Signed-off-by: Jonathan Elchison <jelchison@gmail.com>
-----------------------------------------------------------------
diff -up linux-2.6_orig/arch/m68k/math-emu/multi_arith.h
linux-2.6/arch/m68k/math-emu/multi_arith.h
--- linux-2.6_orig/arch/m68k/math-emu/multi_arith.h 2011-06-10
22:47:43.407285452 -0400
+++ linux-2.6/arch/m68k/math-emu/multi_arith.h 2011-06-10
22:50:32.538711320 -0400
@@ -236,7 +236,7 @@ static inline void lsl64(int count, unsi
{
if (count < 32) {
HI_WORD(*dest) = (HI_WORD(*dest) << count)
- | (LO_WORD(*dest) >> count);
+ | (LO_WORD(*dest) >> (32 - count));
LO_WORD(*dest) <<= count;
return;
}
-----------------------------------------------------------------
Please personally CC me on responses to this post.
v/r,
--
Jonathan Elchison
jelchison@gmail.com
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-06-11 12:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-06-11 3:50 [PATCH] m68k: Patch for broken lsl64() "old code" with incorrect shift Jonathan Elchison
2011-06-11 12:01 ` Geert Uytterhoeven
2011-06-11 12:01 ` Geert Uytterhoeven
2011-06-11 12:34 ` Jonathan Elchison
2011-06-11 12:34 ` Jonathan Elchison
-- strict thread matches above, loose matches on Subject: below --
2011-06-11 3:50 Jonathan Elchison
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.