Re: [Patch] xfs_lowbit64 broken on ia32

[Lachlan McIlroy <lachlan@xxxxxxx>]

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@xxxxxxx>
> > > > > ---
> > > > > 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.