[Patch] xfs_lowbit64 broken on ia32

[Patch] xfs_lowbit64 broken on ia32

[David Chinner]

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.

Signed-off-by: Dave Chinner <dgc@sgi.com>
---
 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.h	2007-11-02 13:44:45.000000000 +1100
+++ 2.6.x-xfs-new/fs/xfs/xfs_bit.h	2007-12-03 14:43:33.169851481 +1100
@@ -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) */

Re: [Patch] xfs_lowbit64 broken on ia32

[Lachlan McIlroy]

David Chinner wrote:
> 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.
> 
> Signed-off-by: Dave Chinner <dgc@sgi.com>
> ---
>  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.h	2007-11-02 13:44:45.000000000 +1100
> +++ 2.6.x-xfs-new/fs/xfs/xfs_bit.h	2007-12-03 14:43:33.169851481 +1100
> @@ -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()?

> +	return (v) ? find_first_bit((unsigned long *)&t, 64) : -1;
>  }
>  
>  /* Return whether bitmap is empty (1 == empty) */
> 
> 
> 

Re: [Patch] xfs_lowbit64 broken on ia32

[David Chinner]

On Wed, Dec 05, 2007 at 01:11:36PM +1100, Lachlan McIlroy wrote:
> David Chinner wrote:
> >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.
> >
> >Signed-off-by: Dave Chinner <dgc@sgi.com>
> >---
> > 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.h	2007-11-02 
> >13:44:45.000000000 +1100
> >+++ 2.6.x-xfs-new/fs/xfs/xfs_bit.h	2007-12-03 14:43:33.169851481 +1100
> >@@ -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.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: [Patch] xfs_lowbit64 broken on ia32

[Lachlan McIlroy]

David Chinner wrote:
> On Wed, Dec 05, 2007 at 01:11:36PM +1100, Lachlan McIlroy wrote:
>> David Chinner wrote:
>>> 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.
>>>
>>> Signed-off-by: Dave Chinner <dgc@sgi.com>
>>> ---
>>> 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.h	2007-11-02 
>>> 13:44:45.000000000 +1100
>>> +++ 2.6.x-xfs-new/fs/xfs/xfs_bit.h	2007-12-03 14:43:33.169851481 +1100
>>> @@ -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)?
> 

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?

Re: [Patch] xfs_lowbit64 broken on ia32

[David Chinner]

On Wed, Dec 05, 2007 at 04:37:01PM +1100, Lachlan McIlroy wrote:
> David Chinner wrote:
> >On Wed, Dec 05, 2007 at 01:11:36PM +1100, Lachlan McIlroy wrote:
> >>David Chinner wrote:
> >>>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.
> >>>
> >>>Signed-off-by: Dave Chinner <dgc@sgi.com>
> >>>---
> >>>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.h	2007-11-02 
> >>>13:44:45.000000000 +1100
> >>>+++ 2.6.x-xfs-new/fs/xfs/xfs_bit.h	2007-12-03 14:43:33.169851481 +1100
> >>>@@ -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)?
> >
> 
> 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?

Whatever. They are both the same.

xfs/xfs_types.h    <global> 41 typedef unsigned long long int __uint64_t;

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: [Patch] xfs_lowbit64 broken on ia32

[Lachlan McIlroy]

David Chinner wrote:
> On Wed, Dec 05, 2007 at 04:37:01PM +1100, Lachlan McIlroy wrote:
>> David Chinner wrote:
>>> On Wed, Dec 05, 2007 at 01:11:36PM +1100, Lachlan McIlroy wrote:
>>>> David Chinner wrote:
>>>>> 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.
>>>>>
>>>>> Signed-off-by: Dave Chinner <dgc@sgi.com>
>>>>> ---
>>>>> 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.h	2007-11-02 
>>>>> 13:44:45.000000000 +1100
>>>>> +++ 2.6.x-xfs-new/fs/xfs/xfs_bit.h	2007-12-03 14:43:33.169851481 +1100
>>>>> @@ -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)?
>>>
>> 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?
> 
> 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.

RE: [Patch] xfs_lowbit64 broken on ia32

[Alex Elder]

David Chinner wrote:
. . .
> > >Index: 2.6.x-xfs-new/fs/xfs/xfs_bit.h
> > >===================================================================
> > >--- 2.6.x-xfs-new.orig/fs/xfs/xfs_bit.h	2007-11-02 
> > >13:44:45.000000000 +1100
> > >+++ 2.6.x-xfs-new/fs/xfs/xfs_bit.h	2007-12-03 
> 14:43:33.169851481 +1100
> > >@@ -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)?

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?

[Jameel Akari]

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.

-- 
Jameel Akari

Re: [Patch] xfs_lowbit64 broken on ia32

[Timothy Shimmin]

Alex Elder wrote:
> David Chinner wrote:
> . . .
>>>> Index: 2.6.x-xfs-new/fs/xfs/xfs_bit.h
>>>> ===================================================================
>>>> --- 2.6.x-xfs-new.orig/fs/xfs/xfs_bit.h	2007-11-02 
>>>> 13:44:45.000000000 +1100
>>>> +++ 2.6.x-xfs-new/fs/xfs/xfs_bit.h	2007-12-03 
>> 14:43:33.169851481 +1100
>>>> @@ -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)?
> 
> 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

RE: [Patch] xfs_lowbit64 broken on ia32

[Alex Elder]

Timothy Shimmin wrote:
> 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

Re: [Patch] xfs_lowbit64 broken on ia32

[David Chinner]

On Wed, Dec 05, 2007 at 04:14:02AM -0800, Alex Elder wrote:
> David Chinner wrote:
> . . .
> > > >Index: 2.6.x-xfs-new/fs/xfs/xfs_bit.h
> > > >===================================================================
> > > >--- 2.6.x-xfs-new.orig/fs/xfs/xfs_bit.h	2007-11-02 
> > > >13:44:45.000000000 +1100
> > > >+++ 2.6.x-xfs-new/fs/xfs/xfs_bit.h	2007-12-03 
> > 14:43:33.169851481 +1100
> > > >@@ -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)?
> 
> 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.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

RE: [Patch] xfs_lowbit64 broken on ia32

[Alex Elder]

David Chinner wrote:
> On Wed, Dec 05, 2007 at 04:14:02AM -0800, Alex Elder wrote:
> > David Chinner wrote:
> > . . .
> > > > >Index: 2.6.x-xfs-new/fs/xfs/xfs_bit.h
> > > > 
> >===================================================================
> > > > >--- 2.6.x-xfs-new.orig/fs/xfs/xfs_bit.h	2007-11-02 
> > > > >13:44:45.000000000 +1100
> > > > >+++ 2.6.x-xfs-new/fs/xfs/xfs_bit.h	2007-12-03 
> > > 14:43:33.169851481 +1100
> > > > >@@ -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)?
> > 
> > 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.

> 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