xfs_lowbit64 broken on ia32
PATCH 1/1 xfs_lowbit64 broken on ia32
The recent change to the internals of xfs_lowbit64 broke it on ia32 - the code treats the 64bit value as an unsigned long which is only 32 bits on ia32 and hence throws away the high 32 bits. 64 bit platforms are not affected.
Tested with a userspace implementation comparing the original code with the new code.
--- fs/xfs/xfs_bit.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
fs/xfs/xfs_bit.h +2 -2
@@ -68,8 +68,8 @@ static inline int xfs_lowbit32(__uint32_/* Get low bit set out of 64-bit argument, -1 if none set */ static inline int xfs_lowbit64 (__uint64_t v) { -unsigned long t = v; -return (v) ?find_first_bit (&t,64 ) :-1 ; +unsigned long long t = v; +return (v) ?find_first_bit ((unsigned long *)&t,64 ) :-1 ; }/* Return whether bitmap is empty (1 == empty) */
Quoted 18 lines · David Chinner
The recent change to the internals of
xfs_lowbit64broke it on ia32 - the code treats the 64bit value as an unsigned long which is only 32 bits on ia32 and hence throws away the high 32 bits. 64 bit platforms are not affected.Tested with a userspace implementation comparing the original code with the new code.
Signed-off-by Dave Chinner <[email protected]>--- fs/xfs/xfs_bit.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)Index:
2.6.x-xfs-new/fs/xfs/xfs_bit.h
===================================================================
fs/xfs/xfs_bit.h static inline int xfs_lowbit32(__uint32_ · line 71
Quoted 6 lines
fs/xfs/xfs_bit.h +1
-2@@ -68,8 +68,8 @@ static inline int xfs_lowbit32(__uint32_/* Get low bit set out of 64-bit argument, -1 if none set */ static inline int xfs_lowbit64 (__uint64_t v) { -unsigned long t = v; -return (v) ?find_first_bit (&t,64 ) :-1 ; +unsigned long long t = v;
Why create a local copy? Why not just pass v into find_first_bit()?
Quoted 7 lines
fs/xfs/xfs_bit.h +1
-0+return (v) ?find_first_bit ((unsigned long *)&t,64 ) :-1 ; }/* Return whether bitmap is empty (1 == empty) */
Quoted 14 lines · Lachlan McIlroy
Quoted 13 lines · David Chinner
The recent change to the internals of
xfs_lowbit64broke it on ia32 - the code treats the 64bit value as an unsigned long which is only 32 bits on ia32 and hence throws away the high 32 bits. 64 bit platforms are not affected.Tested with a userspace implementation comparing the original code with the new code.
Signed-off-by Dave Chinner <[email protected]>---
fs/xfs/xfs_bit.h| 4 ++--1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 13 lines
fs/xfs/xfs_bit.h static inline int xfs_lowbit32(__uint32_ · line 71
Quoted 12 lines
Index:
2.6.x-xfs-new/fs/xfs/xfs_bit.h
===================================================================
---2.6.x-xfs-new.orig/fs/xfs/xfs_bit.h2007-11-02
13:44:45.000000000 +1100
+++2.6.x-xfs-new/fs/xfs/xfs_bit.h2007-12-03 14:43:33.169851481 +1100fs/xfs/xfs_bit.h +1
-2@@ -68,8 +68,8 @@ static inline int xfs_lowbit32(__uint32_/* Get low bit set out of 64-bit argument, -1 if none set */ static inline int xfs_lowbit64 (__uint64_t v) { -unsigned long t = v; -return (v) ?find_first_bit (&t,64 ) :-1 ; +unsigned long long t = v;Why create a local copy? Why not just pass v into
find_first_bit()?
Because I thought that taking the address of a function parameter was a big no-no because the result is undefined (i.e. platform and compiler dependent)?
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 29 lines · David Chinner
Quoted 14 lines · Lachlan McIlroy
Quoted 13 lines · David Chinner
The recent change to the internals of
xfs_lowbit64broke it on ia32 - the code treats the 64bit value as an unsigned long which is only 32 bits on ia32 and hence throws away the high 32 bits. 64 bit platforms are not affected.Tested with a userspace implementation comparing the original code with the new code.
Signed-off-by Dave Chinner <[email protected]>---
fs/xfs/xfs_bit.h| 4 ++--1 file changed, 2 insertions(+), 2 deletions(-)Quoted 13 lines
fs/xfs/xfs_bit.h static inline int xfs_lowbit32(__uint32_ · line 71
Quoted 12 lines
Index:
2.6.x-xfs-new/fs/xfs/xfs_bit.h
===================================================================
---2.6.x-xfs-new.orig/fs/xfs/xfs_bit.h2007-11-02
13:44:45.000000000 +1100
+++2.6.x-xfs-new/fs/xfs/xfs_bit.h2007-12-03 14:43:33.169851481 +1100fs/xfs/xfs_bit.h +1
-2@@ -68,8 +68,8 @@ static inline int xfs_lowbit32(__uint32_/* Get low bit set out of 64-bit argument, -1 if none set */ static inline int xfs_lowbit64 (__uint64_t v) { -unsigned long t = v; -return (v) ?find_first_bit (&t,64 ) :-1 ; +unsigned long long t = v;Why create a local copy? Why not just pass v into
find_first_bit()?
Quoted 4 lines
Because I thought that taking the address of a function parameter was a big no-no because the result is undefined (i.e. platform and compiler dependent)?
Really? Didn't occur to me but then I can't say for sure I've ever used the address of an argument. If we need a local copy then why not use __unit64_t instead of unsigned long long?
Quoted 30 lines · Lachlan McIlroy
Quoted 29 lines · David Chinner
Quoted 27 lines · Lachlan McIlroy
Quoted 19 lines · David Chinner
The recent change to the internals of
xfs_lowbit64broke it on ia32 - the code treats the 64bit value as an unsigned long which is only 32 bits on ia32 and hence throws away the high 32 bits. 64 bit platforms are not affected.Tested with a userspace implementation comparing the original code with the new code.
Signed-off-by Dave Chinner <[email protected]>---
fs/xfs/xfs_bit.h| 4 ++--1 file changed, 2 insertions(+), 2 deletions(-)Index:
2.6.x-xfs-new/fs/xfs/xfs_bit.h
===================================================================
---2.6.x-xfs-new.orig/fs/xfs/xfs_bit.h2007-11-02
13:44:45.000000000 +1100
+++2.6.x-xfs-new/fs/xfs/xfs_bit.h2007-12-03 14:43:33.169851481 +1100fs/xfs/xfs_bit.h static inline int xfs_lowbit32(__uint32_ · line 71
Quoted 6 lines
fs/xfs/xfs_bit.h +1
-2@@ -68,8 +68,8 @@ static inline int xfs_lowbit32(__uint32_/* Get low bit set out of 64-bit argument, -1 if none set */ static inline int xfs_lowbit64 (__uint64_t v) { -unsigned long t = v; -return (v) ?find_first_bit (&t,64 ) :-1 ; +unsigned long long t = v;Why create a local copy? Why not just pass v into
find_first_bit()?
Quoted 8 lines
Quoted 4 lines
Because I thought that taking the address of a function parameter was a big no-no because the result is undefined (i.e. platform and compiler dependent)?
Really? Didn't occur to me but then I can't say for sure I've ever used the address of an argument. If we need a local copy then why not use
__unit64_tinstead of unsigned long long?
Whatever. They are both the same.
xfs/xfs_types.h <global> 41 typedef unsigned long long int __uint64_t;
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 34 lines · David Chinner
Quoted 33 lines · Lachlan McIlroy
Quoted 32 lines · David Chinner
Quoted 14 lines · Lachlan McIlroy
Quoted 13 lines · David Chinner
The recent change to the internals of
xfs_lowbit64broke it on ia32 - the code treats the 64bit value as an unsigned long which is only 32 bits on ia32 and hence throws away the high 32 bits. 64 bit platforms are not affected.Tested with a userspace implementation comparing the original code with the new code.
Signed-off-by Dave Chinner <[email protected]>---
fs/xfs/xfs_bit.h| 4 ++--1 file changed, 2 insertions(+), 2 deletions(-)Quoted 13 lines
fs/xfs/xfs_bit.h static inline int xfs_lowbit32(__uint32_ · line 71
Quoted 12 lines
Index:
2.6.x-xfs-new/fs/xfs/xfs_bit.h
===================================================================
---2.6.x-xfs-new.orig/fs/xfs/xfs_bit.h2007-11-02
13:44:45.000000000 +1100
+++2.6.x-xfs-new/fs/xfs/xfs_bit.h2007-12-03 14:43:33.169851481 +1100fs/xfs/xfs_bit.h +1
-2@@ -68,8 +68,8 @@ static inline int xfs_lowbit32(__uint32_/* Get low bit set out of 64-bit argument, -1 if none set */ static inline int xfs_lowbit64 (__uint64_t v) { -unsigned long t = v; -return (v) ?find_first_bit (&t,64 ) :-1 ; +unsigned long long t = v;Why create a local copy? Why not just pass v into
find_first_bit()?Because I thought that taking the address of a function parameter was a big no-no because the result is undefined (i.e. platform and compiler dependent)?
Quoted 8 lines
Quoted 3 lines
Really? Didn't occur to me but then I can't say for sure I've ever used the address of an argument. If we need a local copy then why not use
__unit64_tinstead of unsigned long long?Whatever. They are both the same.
xfs/xfs_types.h<global> 41 typedef unsigned long long int__uint64_t;
Figured they would be. Using the same type probably would have avoided the bug in the first place. Otherwise the change is good.
David Chinner wrote:
. . .
Quoted 15 lines
Quoted 5 lines
Quoted 5 lines
Index:
2.6.x-xfs-new/fs/xfs/xfs_bit.h
===================================================================
---2.6.x-xfs-new.orig/fs/xfs/xfs_bit.h2007-11-02
13:44:45.000000000 +1100
+++2.6.x-xfs-new/fs/xfs/xfs_bit.h2007-12-0314:43:33.169851481 +1100
Quoted 8 lines
fs/xfs/xfs_bit.h static inline int xfs_lowbit32(__uint32_ · line 71
Quoted 7 lines
fs/xfs/xfs_bit.h +1
-2@@ -68,8 +68,8 @@ static inline int xfs_lowbit32(__uint32_/* Get low bit set out of 64-bit argument, -1 if none set */ static inline int xfs_lowbit64 (__uint64_t v) { -unsigned long t = v; -return (v) ?find_first_bit (&t,64 ) :-1 ; +unsigned long long t = v;Why create a local copy? Why not just pass v into
find_first_bit()?
Quoted 3 lines
Because I thought that taking the address of a function parameter was a big no-no because the result is undefined (i.e. platform and compiler dependent)?
I've never heard of this, and it's incorrect--at least with respect to standard C. (But that's not to say in practice some compiler does it wrong.) Unless it's a real (details known) problem you shouldn't try to work around it.
-Alex
Which (lib)acl for 2.4.35?
The story: We have some old RH7.3 machines that had been running the old 2.4.18_SGI_XFS1.1 from back in the day. Moving these to a new version of VMware requires upgrading to something more recent, and 2.4.35.3/SMP was built from the mainline.
The situation:
Seems to work, except ACL support is broken. I'm pretty sure this is due to the userland libraries and tools being so out of date (machine has 2.0.9-0). Which versions do I need, and where do I get them? CVS?
I assume that all the other utilities (things in xfsprogs like mkfs, and xfsdump) will need similar updates as well.
The immediate need is to get ACLs working, but it'd be nice if I can patch up the rest of it one last time until we can rebuild these VMs.
Signature · 1 line
Jameel Akari
Quoted 20 lines · Alex Elder
David Chinner wrote:
. . .Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
Index:
2.6.x-xfs-new/fs/xfs/xfs_bit.h
===================================================================
---2.6.x-xfs-new.orig/fs/xfs/xfs_bit.h2007-11-02
13:44:45.000000000 +1100
+++2.6.x-xfs-new/fs/xfs/xfs_bit.h2007-12-0314:43:33.169851481 +1100
Quoted 8 lines
fs/xfs/xfs_bit.h static inline int xfs_lowbit32(__uint32_ · line 71
Quoted 7 lines
fs/xfs/xfs_bit.h +1
-2@@ -68,8 +68,8 @@ static inline int xfs_lowbit32(__uint32_/* Get low bit set out of 64-bit argument, -1 if none set */ static inline int xfs_lowbit64 (__uint64_t v) { -unsigned long t = v; -return (v) ?find_first_bit (&t,64 ) :-1 ; +unsigned long long t = v;Why create a local copy? Why not just pass v into
find_first_bit()?Quoted 3 lines
Because I thought that taking the address of a function parameter was a big no-no because the result is undefined (i.e. platform and compiler dependent)?
Quoted 7 lines
I've never heard of this, and it's incorrect--at least with respect to standard C. (But that's not to say in practice some compiler does it wrong.) Unless it's a real (details known) problem you shouldn't try to work around it.
-Alex
I've never heard of that either.
(Then again, I didn't know until recently that they changed C
to allow "flexible array members" in C99
http://tigcc.ticalc.org/doc/gnuexts.html#SEC75)
--Tim
Quoted 4 lines · Timothy Shimmin
I've never heard of that either.
(Then again, I didn't know until recently that they changed C
to allow "flexible array members" in C99
http://tigcc.ticalc.org/doc/gnuexts.html#SEC75)
Yeah there's lots of fancy new stuff in C99.
Like variable length arrays, language-supported complex (sqrt(-1)) data types, C++ style initializations (mid-functions). And you can declare your loop index variable inside a for loop's parentheses (its scope is defined only for the loop).
I'm not ready to use most of that; it has less value than some of the "old" extensions like (void *) and 0-length arrays. But some things are defined very precisely and that's good.
-Alex
Quoted 17 lines · Alex Elder
David Chinner wrote:
. . .Quoted 15 lines
Quoted 5 lines
Quoted 5 lines
Index:
2.6.x-xfs-new/fs/xfs/xfs_bit.h
===================================================================
---2.6.x-xfs-new.orig/fs/xfs/xfs_bit.h2007-11-02
13:44:45.000000000 +1100
+++2.6.x-xfs-new/fs/xfs/xfs_bit.h2007-12-0314:43:33.169851481 +1100
Quoted 8 lines
fs/xfs/xfs_bit.h static inline int xfs_lowbit32(__uint32_ · line 71
Quoted 7 lines
fs/xfs/xfs_bit.h +1
-2@@ -68,8 +68,8 @@ static inline int xfs_lowbit32(__uint32_/* Get low bit set out of 64-bit argument, -1 if none set */ static inline int xfs_lowbit64 (__uint64_t v) { -unsigned long t = v; -return (v) ?find_first_bit (&t,64 ) :-1 ; +unsigned long long t = v;Why create a local copy? Why not just pass v into
find_first_bit()?
Quoted 8 lines
Quoted 3 lines
Because I thought that taking the address of a function parameter was a big no-no because the result is undefined (i.e. platform and compiler dependent)?
I've never heard of this, and it's incorrect--at least with respect to standard C. (But that's not to say in practice some compiler does it wrong.) Unless it's a real (details known) problem you shouldn't try to work around it.
It caused me pain about 10 years ago with gcc 2.? and m68k platform, so I've just avoided doing it ever since.
IMO, taking the address of a function parameter is also bad coding practice because it usually indicates a bug in the code. i.e. you should have passed a pointer to the function or you should be using a local variable rather than abusing the function parameter in strange ways.
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 24 lines · David Chinner
Quoted 17 lines · Alex Elder
David Chinner wrote:
. . .Quoted 2 lines
Quoted 2 lines
Quoted 1 line
Index:
2.6.x-xfs-new/fs/xfs/xfs_bit.h===================================================================
Quoted 12 lines
Quoted 3 lines
Quoted 3 lines
---
2.6.x-xfs-new.orig/fs/xfs/xfs_bit.h2007-11-02
13:44:45.000000000 +1100
+++2.6.x-xfs-new/fs/xfs/xfs_bit.h2007-12-0314:43:33.169851481 +1100
Quoted 8 lines
fs/xfs/xfs_bit.h static inline int xfs_lowbit32(__uint32_ · line 71
Quoted 7 lines
fs/xfs/xfs_bit.h +1
-2@@ -68,8 +68,8 @@ static inline int xfs_lowbit32(__uint32_/* Get low bit set out of 64-bit argument, -1 if none set */ static inline int xfs_lowbit64 (__uint64_t v) { -unsigned long t = v; -return (v) ?find_first_bit (&t,64 ) :-1 ; +unsigned long long t = v;Why create a local copy? Why not just pass v into
find_first_bit()?Quoted 5 lines
Quoted 4 lines
Because I thought that taking the address of a function parameter was a big no-no because the result is undefined (i.e. platform and compiler dependent)?
Quoted 7 lines
Quoted 4 lines
I've never heard of this, and it's incorrect--at least with respect to standard C. (But that's not to say in practice some compiler does it wrong.) Unless it's a real (details known) problem you shouldn't try to work around it.
It caused me pain about 10 years ago with gcc 2.? and m68k platform, so I've just avoided doing it ever since.
I figured an experience like this was behind it.
Quoted 5 lines
IMO, taking the address of a function parameter is also bad coding practice because it usually indicates a bug in the code. i.e. you should have passed a pointer to the function or you should be using a local variable rather than abusing the function parameter in strange ways.
I'm undecided. I agree there's something aesthetically wrong about it. Technically I'm not so sure. Maybe a longjmp() can lead bad things happening when you do this, I don't know. Anyway, this is pretty picky. The change was fine.
-Alex