[PATCH] asm-generic/bitops/fls64.h

[PATCH] asm-generic/bitops/fls64.h

[Виноградов Николай Михайлович]

bugfix in fls64 on a big endian systems(against 2.6.25).

Signed-off-by: Nickolay Vinogradov <nickolay@protei.ru>

--

diff --git a/include/asm-generic/bitops/fls64.h 
b/include/asm-generic/bitops/fls64.h
index 1b6b17c..2eedb6f 100644
--- a/include/asm-generic/bitops/fls64.h
+++ b/include/asm-generic/bitops/fls64.h
@@ -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_ */

-- 
Nickolay Vinogradov
Protei Research and Development Center
St.Petersburg, 194044, Russia
Tel.: +7 812 449 47 27

Re: [PATCH] asm-generic/bitops/fls64.h

[Andrew Morton]

On Sun, 04 May 2008 22:58:50 +0400
____________________ ______________ ____________________ <nickolay@protei.ru> wrote:

> bugfix in fls64 on a big endian systems(against 2.6.25).
> 
> Signed-off-by: Nickolay Vinogradov <nickolay@protei.ru>
> 
> --
> 
> diff --git a/include/asm-generic/bitops/fls64.h 
> b/include/asm-generic/bitops/fls64.h
> index 1b6b17c..2eedb6f 100644
> --- a/include/asm-generic/bitops/fls64.h
> +++ b/include/asm-generic/bitops/fls64.h
> @@ -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.

Re: [PATCH] asm-generic/bitops/fls64.h

[Nickolay Vinogradov]

Andrew Morton пишет:
> On Sun, 04 May 2008 22:58:50 +0400
> ____________________ ______________ ____________________ <nickolay@protei.ru> wrote:
> 
>> bugfix in fls64 on a big endian systems(against 2.6.25).
>>
>> Signed-off-by: Nickolay Vinogradov <nickolay@protei.ru>
>>
>> --
>>
>> diff --git a/include/asm-generic/bitops/fls64.h 
>> b/include/asm-generic/bitops/fls64.h
>> index 1b6b17c..2eedb6f 100644
>> --- a/include/asm-generic/bitops/fls64.h
>> +++ b/include/asm-generic/bitops/fls64.h
>> @@ -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.

Repair fls64() so that it works correctly when passed a u64.

-- 
Nickolay Vinogradov
Protei Research and Development Center
St.Petersburg, 194044, Russia
Tel.: +7 812 449 47 27

Re: [PATCH] asm-generic/bitops/fls64.h

[Andrew Morton]

On Tue, 13 May 2008 14:43:43 +0400 Nickolay Vinogradov <nickolay@protei.ru> wrote:

> Andrew Morton __________:
> > On Sun, 04 May 2008 22:58:50 +0400
> > ____________________ ______________ ____________________ <nickolay@protei.ru> wrote:
> > 
> >> bugfix in fls64 on a big endian systems(against 2.6.25).
> >>
> >> Signed-off-by: Nickolay Vinogradov <nickolay@protei.ru>
> >>
> >> --
> >>
> >> diff --git a/include/asm-generic/bitops/fls64.h 
> >> b/include/asm-generic/bitops/fls64.h
> >> index 1b6b17c..2eedb6f 100644
> >> --- a/include/asm-generic/bitops/fls64.h
> >> +++ b/include/asm-generic/bitops/fls64.h
> >> @@ -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.
> 
> 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()?

Re: [PATCH] asm-generic/bitops/fls64.h

[Nickolay Vinogradov]

Andrew Morton пишет:
> On Tue, 13 May 2008 14:43:43 +0400 Nickolay Vinogradov <nickolay@protei.ru> wrote:
> 
>> Andrew Morton __________:
>>> On Sun, 04 May 2008 22:58:50 +0400
>>> ____________________ ______________ ____________________ <nickolay@protei.ru> wrote:
>>>
>>>> bugfix in fls64 on a big endian systems(against 2.6.25).
>>>>
>>>> Signed-off-by: Nickolay Vinogradov <nickolay@protei.ru>
>>>>
>>>> --
>>>>
>>>> diff --git a/include/asm-generic/bitops/fls64.h 
>>>> b/include/asm-generic/bitops/fls64.h
>>>> index 1b6b17c..2eedb6f 100644
>>>> --- a/include/asm-generic/bitops/fls64.h
>>>> +++ b/include/asm-generic/bitops/fls64.h
>>>> @@ -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.
>> 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()?

include/asm-arm/bitops.h
fls() 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.

-- 
Nickolay Vinogradov
Protei Research and Development Center
St.Petersburg, 194044, Russia
Tel.: +7 812 449 47 27

Re: [PATCH] asm-generic/bitops/fls64.h

[Alexander van Heukelum]

> bugfix in fls64 on a big endian systems(against 2.6.25).
> 
> Signed-off-by: Nickolay Vinogradov <nickolay@protei.ru>
> 
> --
> 
> diff --git a/include/asm-generic/bitops/fls64.h 
> b/include/asm-generic/bitops/fls64.h
> index 1b6b17c..2eedb6f 100644
> --- a/include/asm-generic/bitops/fls64.h
> +++ b/include/asm-generic/bitops/fls64.h
> @@ -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

>   #endif /* _ASM_GENERIC_BITOPS_FLS64_H_ */
> 
> -- 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 
-- 
  Alexander van Heukelum
  heukelum@fastmail.fm

-- 
http://www.fastmail.fm - Or how I learned to stop worrying and
                          love email again

Re: [PATCH] asm-generic/bitops/fls64.h

[Nickolay Vinogradov]

Alexander van Heukelum пишет:

> 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.)

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:

diff --git a/include/asm-arm/bitops.h b/include/asm-arm/bitops.h
index 5c60bfc..ce3fb6f 100644
--- a/include/asm-arm/bitops.h
+++ b/include/asm-arm/bitops.h
@@ -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) )


-- 
Nickolay Vinogradov
Protei Research and Development Center
St.Petersburg, 194044, Russia
Tel.: +7 812 449 47 27

Re: [PATCH] asm-generic/bitops/fls64.h

[Alexander van Heukelum]

On Tue, 13 May 2008 16:29:04 +0400, "Nickolay Vinogradov"
<nickolay@protei.ru> said:
> Alexander van Heukelum &#1087;&#1080;&#1096;&#1077;&#1090;:
> 
> > 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.)
> 
> 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:
> 
> diff --git a/include/asm-arm/bitops.h b/include/asm-arm/bitops.h
> index 5c60bfc..ce3fb6f 100644
> --- a/include/asm-arm/bitops.h
> +++ b/include/asm-arm/bitops.h
> @@ -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) )

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
-- 
  Alexander van Heukelum
  heukelum@fastmail.fm

-- 
http://www.fastmail.fm - I mean, what is it about a decent email service?

Re: [PATCH] asm-generic/bitops/fls64.h

[Russell King]

On Tue, May 13, 2008 at 03:24:13PM +0200, Alexander van Heukelum wrote:
> On Tue, 13 May 2008 16:29:04 +0400, "Nickolay Vinogradov"
> <nickolay@protei.ru> said:
> > Alexander van Heukelum &#1087;&#1080;&#1096;&#1077;&#1090;:
> > 
> > > 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.)
> > 
> > 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.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH] asm-generic/bitops/fls64.h

[Alexander van Heukelum]

On Tue, 13 May 2008 14:58:39 +0100, "Russell King"
<rmk+lkml@arm.linux.org.uk> said:
> On Tue, May 13, 2008 at 03:24:13PM +0200, Alexander van Heukelum wrote:
> > On Tue, 13 May 2008 16:29:04 +0400, "Nickolay Vinogradov"
> > <nickolay@protei.ru> said:
> > > Alexander van Heukelum &#1087;&#1080;&#1096;&#1077;&#1090;:
> > > 
> > > > 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.)
> > > 
> > > 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.

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

> -- 
> Russell King
>  Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
>  maintainer of:
-- 
  Alexander van Heukelum
  heukelum@fastmail.fm

-- 
http://www.fastmail.fm - The way an email service should be

Re: [PATCH] asm-generic/bitops/fls64.h

[Nickolay Vinogradov]

Alexander van Heukelum пишет:

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

-- 
Nickolay Vinogradov
Protei Research and Development Center
St.Petersburg, 194044, Russia
Tel.: +7 812 449 47 27

Re: [PATCH] asm-generic/bitops/fls64.h

[Andreas Schwab]

Russell King <rmk+lkml@arm.linux.org.uk> writes:

> On Tue, May 13, 2008 at 03:24:13PM +0200, Alexander van Heukelum wrote:
>> On Tue, 13 May 2008 16:29:04 +0400, "Nickolay Vinogradov"
>> <nickolay@protei.ru> said:
>> > Alexander van Heukelum &#1087;&#1080;&#1096;&#1077;&#1090;:
>> > 
>> > > 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.)
>> > 
>> > 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 write fls as an (inline) function then the argument is implicitly
converted.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
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."

Re: [PATCH] asm-generic/bitops/fls64.h

[Andrew Morton]

On Tue, 13 May 2008 16:35:25 +0200 Andreas Schwab <schwab@suse.de> wrote:

> Russell King <rmk+lkml@arm.linux.org.uk> writes:
> 
> > On Tue, May 13, 2008 at 03:24:13PM +0200, Alexander van Heukelum wrote:
> >> On Tue, 13 May 2008 16:29:04 +0400, "Nickolay Vinogradov"
> >> <nickolay@protei.ru> said:
> >> > Alexander van Heukelum &#1087;&#1080;&#1096;&#1077;&#1090;:
> >> > 
> >> > > 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.)
> >> > 
> >> > 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 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...