asm-generic/bitops/fls64.h
Subsystems (2)
PATCH 1/1 asm-generic/bitops/fls64.h
bugfix in fls64 on a big endian systems(against 2.6.25).
--
include/asm-generic/bitops/fls64.h +1 -1
@@ -8,7 +8,7 @@ static inline int fls64(__u64 x)__u32 h = x >>32 ;if (h)return fls (h) +32 ; - return fls(x); + return fls((__u32)x); }#endif /* _ASM_GENERIC_BITOPS_FLS64_H_ */
Signature · 4 lines
Nickolay Vinogradov Protei Research and Development Center St.Petersburg, 194044, Russia Tel.: +7 812 449 47 27
Quoted 12 lines · On Sun, 04 May 2008 22:58:50 +0400 ____________________ ______________…
bugfix in fls64 on a big endian systems(against 2.6.25).
Signed-off-by Nickolay Vinogradov <[email protected]>--
include/asm-generic/bitops/fls64.h +0
-0
include/asm-generic/bitops/fls64.h static inline int fls64(__u64 x) · line 14
Quoted 8 lines
include/asm-generic/bitops/fls64.h +1
-1@@ -8,7 +8,7 @@ static inline int fls64(__u64 x)__u32 h = x >>32 ;if (h)return fls (h) +32 ; - return fls(x); + return fls((__u32)x); }#endif /* _ASM_GENERIC_BITOPS_FLS64_H_ */
Please describe the bug which you are fixing?
Perhaps a more robust fix to <whatever the bug is> would be to repair fls() so that it works correctly when passed a u64. Perhaps.
Andrew Morton пишет:
Quoted 21 lines
Quoted 18 lines · On Sun, 04 May 2008 22:58:50 +0400 ____________________ ______________…
bugfix in fls64 on a big endian systems(against 2.6.25).
Signed-off-by Nickolay Vinogradov <[email protected]>--
include/asm-generic/bitops/fls64.h +1
-1@@ -8,7 +8,7 @@ static inline int fls64(__u64 x)__u32 h = x >>32 ;if (h)return fls (h) +32 ; -return fls (x); +return fls ((__u32 )x); }
Quoted 6 lines
include/asm-generic/bitops/fls64.h static inline int fls64(__u64 x) · line 14
Quoted 1 line
include/asm-generic/bitops/fls64.h +0
-0#endif /* _ASM_GENERIC_BITOPS_FLS64_H_ */ Please describe the bug which you are fixing?
Perhaps a more robust fix to <whatever the bug is> would be to repair
fls()so that it works correctly when passed a u64. Perhaps.
Repair fls64() so that it works correctly when passed a u64.
Signature · 4 lines
Nickolay Vinogradov Protei Research and Development Center St.Petersburg, 194044, Russia Tel.: +7 812 449 47 27
Quoted 12 lines
bugfix in fls64 on a big endian systems(against 2.6.25).
Signed-off-by Nickolay Vinogradov <[email protected]>--
include/asm-generic/bitops/fls64.h +0
-0
include/asm-generic/bitops/fls64.h static inline int fls64(__u64 x) · line 12
Quoted 6 lines
include/asm-generic/bitops/fls64.h +1
-1@@ -8,7 +8,7 @@ static inline int fls64(__u64 x)__u32 h = x >>32 ;if (h)return fls (h) +32 ; -return fls (x); +return fls ((__u32 )x); }
Hi Nickolay,
The change is ok, I guess, but the cast should be a no-op (fls takes an int, which is always 32 bit in linux). What is the problem you are seeing? Does fls64() return a wrong value in some cases? If so, what cpu? Which values?
Why would this be a bug on big endian systems only? There is no pointer magic involved, so the compiler should take care of the casts in a correct way.
Maybe you see a compiler warning? Which compiler version?
(also note that current (development) kernels now have separate versions for 32-bit and 64-bit environments.)
Greetings,
Alexander
Quoted 15 lines
include/asm-generic/bitops/fls64.h +0
-0#endif /* _ASM_GENERIC_BITOPS_FLS64_H_ */ Signature · 10 lines
Nickolay Vinogradov Protei Research and Development Center St.Petersburg, 194044, Russia Tel.: +7 812 449 47 27 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Signature · 6 lines
Alexander van Heukelum [email protected] -- http://www.fastmail.fm - Or how I learned to stop worrying and love email again
Alexander van Heukelum пишет:
Quoted 7 lines
Hi Nickolay,
The change is ok, I guess, but the cast should be a no-op (fls takes an int, which is always 32 bit in linux). What is the problem you are seeing? Does
fls64()return a wrong value in some cases? If so, what cpu? Which values?
Quoted 8 lines
Why would this be a bug on big endian systems only? There is no pointer magic involved, so the compiler should take care of the casts in a correct way.
Maybe you see a compiler warning? Which compiler version?
(also note that current (development) kernels now have separate versions for 32-bit and 64-bit environments.)
Because fls() is a macro for asm-arm:
#define fls(x) \
( __builtin_constant_p(x) ? constant_fls(x) : \
({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc"); 32-__r; }) )
We can fix it right here:
include/asm-arm/bitops.h +1 -1
@@ -279,7 +279,7 @@ static inline int constant_fls(int x)#define fls (x) \ ( __builtin_constant_p(x) ? constant_fls(x) : \ - ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc"); 32-__r; }) ) + ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"((__u32)x) : "cc"); 32-__r; }) )#define ffs (x) ({ unsigned long __t = (x); fls(__t & -__t); })#define __ffs (x) (ffs(x) - 1)#define ffz (x) __ffs( ~(x) )
Signature · 4 lines
Nickolay Vinogradov Protei Research and Development Center St.Petersburg, 194044, Russia Tel.: +7 812 449 47 27
Quoted 40 lines · Nickolay Vinogradov
Alexander van Heukelum пишет:
Quoted 7 lines
Hi Nickolay,
The change is ok, I guess, but the cast should be a no-op (fls takes an int, which is always 32 bit in linux). What is the problem you are seeing? Does
fls64()return a wrong value in some cases? If so, what cpu? Which values?Quoted 8 lines
Why would this be a bug on big endian systems only? There is no pointer magic involved, so the compiler should take care of the casts in a correct way.
Maybe you see a compiler warning? Which compiler version?
(also note that current (development) kernels now have separate versions for 32-bit and 64-bit environments.)
Because
fls()is a macro for asm-arm:#define fls(x) \ ( __builtin_constant_p(x) ? constant_fls(x) : \ ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc");32-__r; }) )
We can fix it right here:
include/asm-arm/bitops.h +1
-1@@ -279,7 +279,7 @@ static inline int constant_fls(int x)#define fls (x) \ ( __builtin_constant_p(x) ? constant_fls(x) : \ - ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc"); 32-__r; }) ) + ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"((__u32)x) : "cc"); 32-__r; }) )#define ffs (x) ({ unsigned long __t = (x); fls(__t & -__t); })
Quoted 2 lines
#define __ffs(x) (ffs(x) - 1) #define ffz(x) __ffs( ~(x) )
Hmm, indeed.
Maybe: ({ int __r, __x = (x); asm("clz\t%0, %1" : "=r"(__r) : "r"(__x) :
"cc"); 32-__r; }) ?
This is a 32-bit machine, right? Doesn't the compiler complain about feeding a long long into a 32-bit register?
Greetings,
Alexander
Signature · 5 lines
Alexander van Heukelum [email protected] -- http://www.fastmail.fm - I mean, what is it about a decent email service?
Quoted 20 lines · Alexander van Heukelum
Quoted 18 lines · Nickolay Vinogradov
Alexander van Heukelum пишет:
Quoted 15 lines
Hi Nickolay,
The change is ok, I guess, but the cast should be a no-op (fls takes an int, which is always 32 bit in linux). What is the problem you are seeing? Does
fls64()return a wrong value in some cases? If so, what cpu? Which values?Why would this be a bug on big endian systems only? There is no pointer magic involved, so the compiler should take care of the casts in a correct way.
Maybe you see a compiler warning? Which compiler version?
(also note that current (development) kernels now have separate versions for 32-bit and 64-bit environments.)
Quoted 8 lines
Quoted 8 lines
Because
fls()is a macro for asm-arm:#define fls(x) \ ( __builtin_constant_p(x) ? constant_fls(x) : \ ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc");32-__r; }) )
We can fix it right here:
No. "fls" is for finding the last set bit in an _int_. It is not supposed to have random crap passed to it, such as types longer than sizeof(int).
If you're going to pass long long (64-bit) arguments to fls, and then cast them to a u32, you're truncating the value, and you'll get the wrong answer if bit 33 or greater is set. If you don't actually care about the upper bits, don't pass a 64-bit quantity to fls().
If you want to use fls with a long long, use fls64 instead. Or for top marks, use a u64 and fls64.
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Quoted 34 lines · Russell King
Quoted 20 lines · Alexander van Heukelum
Quoted 18 lines · Nickolay Vinogradov
Alexander van Heukelum пишет:
Quoted 15 lines
Hi Nickolay,
The change is ok, I guess, but the cast should be a no-op (fls takes an int, which is always 32 bit in linux). What is the problem you are seeing? Does
fls64()return a wrong value in some cases? If so, what cpu? Which values?Why would this be a bug on big endian systems only? There is no pointer magic involved, so the compiler should take care of the casts in a correct way.
Maybe you see a compiler warning? Which compiler version?
(also note that current (development) kernels now have separate versions for 32-bit and 64-bit environments.)
Quoted 8 lines
Quoted 8 lines
Because
fls()is a macro for asm-arm:#define fls(x) \ ( __builtin_constant_p(x) ? constant_fls(x) : \ ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc");32-__r; }) )
We can fix it right here:
No. "fls" is for finding the last set bit in an _int_. It is not supposed to have random crap passed to it, such as types longer than sizeof(int).
Quoted 7 lines
If you're going to pass long long (64-bit) arguments to fls, and then cast them to a u32, you're truncating the value, and you'll get the wrong answer if bit 33 or greater is set. If you don't actually care about the upper bits, don't pass a 64-bit quantity to
fls().If you want to use fls with a long long, use fls64 instead. Or for top marks, use a u64 and fls64.
But that was the problem we began with: the generic fls64 passes an u64 to fls. Nickolay's original patch solves that by putting a cast to u32 in fls64. I did not, however, understand why the cast was needed. It should not be needed for correctness, imho, because fls is expected to behave as if it was a function "int fls(int)", like ffs. Values passed to fls should thus be truncated if necessary.
Making the truncation explicit in fls64 is still a good idea, though.
Greetings,
Alexander
Quoted 4 lines
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Signature · 5 lines
Alexander van Heukelum [email protected] -- http://www.fastmail.fm - The way an email service should be
Quoted 28 lines · Russell King
Quoted 27 lines · Alexander van Heukelum
Quoted 25 lines · Nickolay Vinogradov
Alexander van Heukelum пишет:
Quoted 7 lines
Hi Nickolay,
The change is ok, I guess, but the cast should be a no-op (fls takes an int, which is always 32 bit in linux). What is the problem you are seeing? Does
fls64()return a wrong value in some cases? If so, what cpu? Which values?Quoted 8 lines
Why would this be a bug on big endian systems only? There is no pointer magic involved, so the compiler should take care of the casts in a correct way.
Maybe you see a compiler warning? Which compiler version?
(also note that current (development) kernels now have separate versions for 32-bit and 64-bit environments.)
Because
fls()is a macro for asm-arm:#define fls(x) \ ( __builtin_constant_p(x) ? constant_fls(x) : \ ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc");32-__r; }) )
Quoted 5 lines
Quoted 1 line
Quoted 1 line
We can fix it right here:
No. "fls" is for finding the last set bit in an _int_. It is not supposed to have random crap passed to it, such as types longer than sizeof(int).
If you write fls as an (inline) function then the argument is implicitly converted.
Andreas.
Signature · 4 lines
Andreas Schwab, SuSE Labs, [email protected] SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different."
Alexander van Heukelum пишет:
Quoted 9 lines
Quoted 9 lines
No. "fls" is for finding the last set bit in an _int_. It is not supposed to have random crap passed to it, such as types longer than sizeof(int).
If you're going to pass long long (64-bit) arguments to fls, and then cast them to a u32, you're truncating the value, and you'll get the wrong answer if bit 33 or greater is set. If you don't actually care about the upper bits, don't pass a 64-bit quantity to
fls().
Quoted 6 lines
Quoted 2 lines
If you want to use fls with a long long, use fls64 instead. Or for top marks, use a u64 and fls64.
But that was the problem we began with: the generic fls64 passes an u64 to fls. Nickolay's original patch solves that by putting a cast to u32 in fls64. I did not, however, understand why the cast was needed.
The cast was needed because fls is a macro, not a function.
Signature · 4 lines
Nickolay Vinogradov Protei Research and Development Center St.Petersburg, 194044, Russia Tel.: +7 812 449 47 27
Quoted 23 lines · On Tue, 13 May 2008 14:43:43 +0400 Nickolay Vinogradov <[email protected]>…
Andrew Morton __________:
Quoted 22 lines
Quoted 19 lines · On Sun, 04 May 2008 22:58:50 +0400 ____________________ ______________…
bugfix in fls64 on a big endian systems(against 2.6.25).
Signed-off-by Nickolay Vinogradov <[email protected]>--
include/asm-generic/bitops/fls64.h +1
-1@@ -8,7 +8,7 @@ static inline int fls64(__u64 x)__u32 h = x >>32 ;if (h)return fls (h) +32 ; -return fls (x); +return fls ((__u32 )x); }
Quoted 9 lines
Quoted 6 lines
include/asm-generic/bitops/fls64.h static inline int fls64(__u64 x) · line 14
Quoted 1 line
include/asm-generic/bitops/fls64.h +0
-0#endif /* _ASM_GENERIC_BITOPS_FLS64_H_ */ Please describe the bug which you are fixing?
Perhaps a more robust fix to <whatever the bug is> would be to repair
fls()so that it works correctly when passed a u64. Perhaps.Repair
fls64()so that it works correctly when passed a u64.
Yes, but what's wrong with it now?
The fls() in include/asm-generic/bitops/fls.h takes an int.
The fls() in include/asm-x86/bitops.h takes an int.
So both of these will already trucate the incoming argument to 32-bits.
It seems that you are using a version of fls() which doesn't do this. Why? Which architecture are you using? Would it not be more robust to fix fls()?
Andrew Morton пишет:
Quoted 34 lines · On Tue, 13 May 2008 14:43:43 +0400 Nickolay Vinogradov <[email protected]>…
Quoted 23 lines
Andrew Morton __________:
Quoted 21 lines
Quoted 18 lines · On Sun, 04 May 2008 22:58:50 +0400 ____________________ ______________…
bugfix in fls64 on a big endian systems(against 2.6.25).
Signed-off-by Nickolay Vinogradov <[email protected]>--
include/asm-generic/bitops/fls64.h +1
-1@@ -8,7 +8,7 @@ static inline int fls64(__u64 x)__u32 h = x >>32 ;if (h)return fls (h) +32 ; -return fls (x); +return fls ((__u32 )x); }Quoted 7 lines
Quoted 5 lines
include/asm-generic/bitops/fls64.h static inline int fls64(__u64 x) · line 14
Quoted 1 line
include/asm-generic/bitops/fls64.h +0
-0#endif /* _ASM_GENERIC_BITOPS_FLS64_H_ */ Please describe the bug which you are fixing?
Perhaps a more robust fix to <whatever the bug is> would be to repair
fls()so that it works correctly when passed a u64. Perhaps.Repair
fls64()so that it works correctly when passed a u64.Yes, but what's wrong with it now?
Quoted 8 lines
The
fls()ininclude/asm-generic/bitops/fls.htakes an int.The
fls()ininclude/asm-x86/bitops.htakes an int.So both of these will already trucate the incoming argument to 32-bits.
It seems that you are using a version of
fls()which doesn't do this. Why? Which architecture are you using? Would it not be more robust to fixfls()?
include/asm-arm/bitops.hfls() defined here as a macro, not as a function with 32-bit argument. Therefore, there are two choices:
1. explicit cast to u32 before calling.
2. rewrite fls macro as an inline function.
Signature · 4 lines
Nickolay Vinogradov Protei Research and Development Center St.Petersburg, 194044, Russia Tel.: +7 812 449 47 27
Quoted 30 lines · On Tue, 13 May 2008 16:35:25 +0200 Andreas Schwab <[email protected]> wrote:
Quoted 28 lines · Russell King
Quoted 27 lines · Alexander van Heukelum
Quoted 25 lines · Nickolay Vinogradov
Alexander van Heukelum пишет:
Quoted 7 lines
Hi Nickolay,
The change is ok, I guess, but the cast should be a no-op (fls takes an int, which is always 32 bit in linux). What is the problem you are seeing? Does
fls64()return a wrong value in some cases? If so, what cpu? Which values?Quoted 8 lines
Why would this be a bug on big endian systems only? There is no pointer magic involved, so the compiler should take care of the casts in a correct way.
Maybe you see a compiler warning? Which compiler version?
(also note that current (development) kernels now have separate versions for 32-bit and 64-bit environments.)
Because
fls()is a macro for asm-arm:#define fls(x) \ ( __builtin_constant_p(x) ? constant_fls(x) : \ ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc");32-__r; }) )
Quoted 9 lines
Quoted 5 lines
Quoted 1 line
Quoted 1 line
We can fix it right here:
No. "fls" is for finding the last set bit in an _int_. It is not supposed to have random crap passed to it, such as types longer than sizeof(int).
If you write fls as an (inline) function then the argument is implicitly converted.
Yes, and that's what other architectures do.
It's not pretty, but I do think the arm implementation should zap the upper 32 bits as other architectures do, and as one would expect from the usual C type conversion rules.
Converting it to a C implementation would be a suitable means...