Re: [Patch] xfs_lowbit64 broken on ia32

[Lachlan McIlroy <lachlan@sgi.com>]

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?