diff mbox

bitops: fix types

Message ID a89790a096b0acbf5343d73bf7ded050c8ca2300.1341749532.git.blauwirbel@gmail.com
State New
Headers show

Commit Message

Blue Swirl July 8, 2012, 12:12 p.m. UTC 
From: Blue Swirl <blauwirbel@gmail.com>

Use 'unsigned int' for bit numbers. Return 'bool' for test functions. QEMU is
not the kernel, we don't need 'volatile'. Adjust asserts.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 bitops.h |   47 ++++++++++++++++++++++++-----------------------
 1 files changed, 24 insertions(+), 23 deletions(-)

Comments

Peter Maydell July 8, 2012, 2:04 p.m. UTC | #1 
On 8 July 2012 13:12,  <blauwirbel@gmail.com> wrote:
> -static inline uint64_t deposit64(uint64_t value, int start, int length,
> -                                 uint64_t fieldval)
> +static inline uint64_t deposit64(uint64_t value, unsigned int start,
> +                                 unsigned int length, uint64_t fieldval)
>  {
>      uint64_t mask;
> -    assert(start >= 0 && length > 0 && length <= 64 - start);
> +    assert(length > 0 && length <= 64 - start);

This breaks the assertion (consider the case of start == UINT_MAX
and length == 64).

In general, this patch is fixing something that isn't broken
and pointlessly diverging from the well-tested kernel versions
of these functions. It's also trying to do several things at once
(eg "drop volatile" is probably less controversial.)

-- PMM
Blue Swirl July 8, 2012, 6:32 p.m. UTC | #2 
On Sun, Jul 8, 2012 at 2:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 8 July 2012 13:12,  <blauwirbel@gmail.com> wrote:
>> -static inline uint64_t deposit64(uint64_t value, int start, int length,
>> -                                 uint64_t fieldval)
>> +static inline uint64_t deposit64(uint64_t value, unsigned int start,
>> +                                 unsigned int length, uint64_t fieldval)
>>  {
>>      uint64_t mask;
>> -    assert(start >= 0 && length > 0 && length <= 64 - start);
>> +    assert(length > 0 && length <= 64 - start);
>
> This breaks the assertion (consider the case of start == UINT_MAX
> and length == 64).

The original is equally buggy in other cases since there is no bound
check for the upper limit. I think this should catch all:

assert(start < 64 && length > 0 && length <= 64  && start + length  <= 64);

>
> In general, this patch is fixing something that isn't broken
> and pointlessly diverging from the well-tested kernel versions
> of these functions.

This is not pointless. We are using plenty of other things from
various sources and there is very little effort to keep things
identical to originals (except for KVM kernel headers).

> It's also trying to do several things at once
> (eg "drop volatile" is probably less controversial.)

I agree.

>
> -- PMM
Peter Maydell July 8, 2012, 6:54 p.m. UTC | #3 
On 8 July 2012 19:32, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sun, Jul 8, 2012 at 2:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 8 July 2012 13:12,  <blauwirbel@gmail.com> wrote:
>>> -static inline uint64_t deposit64(uint64_t value, int start, int length,
>>> -                                 uint64_t fieldval)
>>> +static inline uint64_t deposit64(uint64_t value, unsigned int start,
>>> +                                 unsigned int length, uint64_t fieldval)
>>>  {
>>>      uint64_t mask;
>>> -    assert(start >= 0 && length > 0 && length <= 64 - start);
>>> +    assert(length > 0 && length <= 64 - start);
>>
>> This breaks the assertion (consider the case of start == UINT_MAX
>> and length == 64).
>
> The original is equally buggy in other cases since there is no bound
> check for the upper limit.

For what upper limit? Overlong length or start should both be caught
by the third condition in the signed case.


-- PMM
Blue Swirl July 8, 2012, 7:12 p.m. UTC | #4 
On Sun, Jul 8, 2012 at 6:54 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 8 July 2012 19:32, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Sun, Jul 8, 2012 at 2:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 8 July 2012 13:12,  <blauwirbel@gmail.com> wrote:
>>>> -static inline uint64_t deposit64(uint64_t value, int start, int length,
>>>> -                                 uint64_t fieldval)
>>>> +static inline uint64_t deposit64(uint64_t value, unsigned int start,
>>>> +                                 unsigned int length, uint64_t fieldval)
>>>>  {
>>>>      uint64_t mask;
>>>> -    assert(start >= 0 && length > 0 && length <= 64 - start);
>>>> +    assert(length > 0 && length <= 64 - start);
>>>
>>> This breaks the assertion (consider the case of start == UINT_MAX
>>> and length == 64).
>>
>> The original is equally buggy in other cases since there is no bound
>> check for the upper limit.
>
> For what upper limit? Overlong length or start should both be caught
> by the third condition in the signed case.

Nice. Why is it written like that, I'd use
start + length <= 64?

>
>
> -- PMM
Peter Maydell July 8, 2012, 7:32 p.m. UTC | #5 
On 8 July 2012 20:12, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sun, Jul 8, 2012 at 6:54 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 8 July 2012 19:32, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> On Sun, Jul 8, 2012 at 2:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> On 8 July 2012 13:12,  <blauwirbel@gmail.com> wrote:
>>>>> -static inline uint64_t deposit64(uint64_t value, int start, int length,
>>>>> -                                 uint64_t fieldval)
>>>>> +static inline uint64_t deposit64(uint64_t value, unsigned int start,
>>>>> +                                 unsigned int length, uint64_t fieldval)
>>>>>  {
>>>>>      uint64_t mask;
>>>>> -    assert(start >= 0 && length > 0 && length <= 64 - start);
>>>>> +    assert(length > 0 && length <= 64 - start);
>>>>
>>>> This breaks the assertion (consider the case of start == UINT_MAX
>>>> and length == 64).
>>>
>>> The original is equally buggy in other cases since there is no bound
>>> check for the upper limit.
>>
>> For what upper limit? Overlong length or start should both be caught
>> by the third condition in the signed case.
>
> Nice. Why is it written like that, I'd use
> start + length <= 64?

That would fail to handle the case of start == length == INT_MAX.

-- PMM
Blue Swirl July 8, 2012, 7:39 p.m. UTC | #6 
On Sun, Jul 8, 2012 at 7:32 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 8 July 2012 20:12, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Sun, Jul 8, 2012 at 6:54 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 8 July 2012 19:32, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>> On Sun, Jul 8, 2012 at 2:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>> On 8 July 2012 13:12,  <blauwirbel@gmail.com> wrote:
>>>>>> -static inline uint64_t deposit64(uint64_t value, int start, int length,
>>>>>> -                                 uint64_t fieldval)
>>>>>> +static inline uint64_t deposit64(uint64_t value, unsigned int start,
>>>>>> +                                 unsigned int length, uint64_t fieldval)
>>>>>>  {
>>>>>>      uint64_t mask;
>>>>>> -    assert(start >= 0 && length > 0 && length <= 64 - start);
>>>>>> +    assert(length > 0 && length <= 64 - start);
>>>>>
>>>>> This breaks the assertion (consider the case of start == UINT_MAX
>>>>> and length == 64).
>>>>
>>>> The original is equally buggy in other cases since there is no bound
>>>> check for the upper limit.
>>>
>>> For what upper limit? Overlong length or start should both be caught
>>> by the third condition in the signed case.
>>
>> Nice. Why is it written like that, I'd use
>> start + length <= 64?
>
> That would fail to handle the case of start == length == INT_MAX.

 64 - INT_MAX = 0x80000040 (maybe off by one), which should still
trigger assert(INT_MAX <= 0x80000040)

>
> -- PMM
Peter Maydell July 8, 2012, 7:51 p.m. UTC | #7 
On 8 July 2012 20:39, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sun, Jul 8, 2012 at 7:32 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 8 July 2012 20:12, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> On Sun, Jul 8, 2012 at 6:54 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> On 8 July 2012 19:32, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>>> On Sun, Jul 8, 2012 at 2:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>>> On 8 July 2012 13:12,  <blauwirbel@gmail.com> wrote:
>>>>>>> -static inline uint64_t deposit64(uint64_t value, int start, int length,
>>>>>>> -                                 uint64_t fieldval)
>>>>>>> +static inline uint64_t deposit64(uint64_t value, unsigned int start,
>>>>>>> +                                 unsigned int length, uint64_t fieldval)
>>>>>>>  {
>>>>>>>      uint64_t mask;
>>>>>>> -    assert(start >= 0 && length > 0 && length <= 64 - start);
>>>>>>> +    assert(length > 0 && length <= 64 - start);
>>>>>>
>>>>>> This breaks the assertion (consider the case of start == UINT_MAX
>>>>>> and length == 64).
>>>>>
>>>>> The original is equally buggy in other cases since there is no bound
>>>>> check for the upper limit.
>>>>
>>>> For what upper limit? Overlong length or start should both be caught
>>>> by the third condition in the signed case.
>>>
>>> Nice. Why is it written like that, I'd use
>>> start + length <= 64?
>>
>> That would fail to handle the case of start == length == INT_MAX.
>
>  64 - INT_MAX = 0x80000040 (maybe off by one), which should still
> trigger assert(INT_MAX <= 0x80000040).

Nope, because "64 - start" here is signed arithmetic, so 64 - INT_MAX
underflows and gives you a negative number, and INT_MAX is not <= -ve.
(We went through this in reviews for the initial patch.)

-- PMM
Peter Maydell July 8, 2012, 10:41 p.m. UTC | #8 
On 8 July 2012 20:51, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 8 July 2012 20:39, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Sun, Jul 8, 2012 at 7:32 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 8 July 2012 20:12, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>> Nice. Why is it written like that, I'd use
>>>> start + length <= 64?
>>>
>>> That would fail to handle the case of start == length == INT_MAX.
>>
>>  64 - INT_MAX = 0x80000040 (maybe off by one), which should still
>> trigger assert(INT_MAX <= 0x80000040).
>
> Nope, because "64 - start" here is signed arithmetic, so 64 - INT_MAX
> underflows and gives you a negative number, and INT_MAX is not <= -ve.
> (We went through this in reviews for the initial patch.)

Sorry, my reply is confused, because your email had a structure
implying that you are asserting that 'start + length <= 64' is a
working check, but the calculation you do is for 'length <= 64 - start',
and I ended up writing garbage as a result of my failure to parse it.

For clarity (all assuming signed int parameters as the code currently
uses, since this was a question about the current code):
 * "start + length <= 64" doesn't work because signed arithmetic means
the case of start = length = INT_MAX overflows and the condition
incorrectly doesn't trigger.
 * "length <= 64 - start" does work, because there is no underflow
or overflow, and the condition correctly triggers.
 assert(INT_MAX <= 0x80000040)
 * although "assert(INT_MAX <= 0x80000041)" will trigger if written
out literally, "assert(INT_MAX <= (64 - INT_MAX))" will not, even
though 64 - INT_MAX is 0x80000041. (the literal hex constant will
be an unsigned value, so we end up doing an unsigned comparison,
whereas 64 - INT_MAX is signed and we do a signed comparison).

Isn't C great?

-- PMM
Markus Armbruster July 9, 2012, 7:38 a.m. UTC | #9 
Peter Maydell <peter.maydell@linaro.org> writes:

> On 8 July 2012 13:12,  <blauwirbel@gmail.com> wrote:
>> -static inline uint64_t deposit64(uint64_t value, int start, int length,
>> -                                 uint64_t fieldval)
>> +static inline uint64_t deposit64(uint64_t value, unsigned int start,
>> +                                 unsigned int length, uint64_t fieldval)
>>  {
>>      uint64_t mask;
>> -    assert(start >= 0 && length > 0 && length <= 64 - start);
>> +    assert(length > 0 && length <= 64 - start);
>
> This breaks the assertion (consider the case of start == UINT_MAX
> and length == 64).
>
> In general, this patch is fixing something that isn't broken
> and pointlessly diverging from the well-tested kernel versions
> of these functions. It's also trying to do several things at once
> (eg "drop volatile" is probably less controversial.)

Agree on all points.
diff mbox

Patch

diff --git a/bitops.h b/bitops.h
index b967ef3..ae9d66f 100644
--- a/bitops.h
+++ b/bitops.h
@@ -28,7 +28,7 @@ 
  *
  * Undefined if no bit exists, so code should check against 0 first.
  */
-static unsigned long bitops_ffsl(unsigned long word)
+static unsigned int bitops_ffsl(unsigned long word)
 {
 	int num = 0;
 
@@ -66,7 +66,7 @@  static unsigned long bitops_ffsl(unsigned long word)
  *
  * Undefined if no set bit exists, so code should check against 0 first.
  */
-static inline unsigned long bitops_flsl(unsigned long word)
+static inline unsigned int bitops_flsl(unsigned long word)
 {
 	int num = BITS_PER_LONG - 1;
 
@@ -104,7 +104,7 @@  static inline unsigned long bitops_flsl(unsigned long word)
  *
  * Undefined if no zero exists, so code should check against ~0UL first.
  */
-static inline unsigned long ffz(unsigned long word)
+static inline unsigned int ffz(unsigned long word)
 {
     return bitops_ffsl(~word);
 }
@@ -114,7 +114,7 @@  static inline unsigned long ffz(unsigned long word)
  * @nr: the bit to set
  * @addr: the address to start counting from
  */
-static inline void set_bit(int nr, volatile unsigned long *addr)
+static inline void set_bit(unsigned int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -127,7 +127,7 @@  static inline void set_bit(int nr, volatile unsigned long *addr)
  * @nr: Bit to clear
  * @addr: Address to start counting from
  */
-static inline void clear_bit(int nr, volatile unsigned long *addr)
+static inline void clear_bit(unsigned int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -140,7 +140,7 @@  static inline void clear_bit(int nr, volatile unsigned long *addr)
  * @nr: Bit to change
  * @addr: Address to start counting from
  */
-static inline void change_bit(int nr, volatile unsigned long *addr)
+static inline void change_bit(unsigned int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -153,7 +153,7 @@  static inline void change_bit(int nr, volatile unsigned long *addr)
  * @nr: Bit to set
  * @addr: Address to count from
  */
-static inline int test_and_set_bit(int nr, volatile unsigned long *addr)
+static inline bool test_and_set_bit(unsigned int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -168,7 +168,7 @@  static inline int test_and_set_bit(int nr, volatile unsigned long *addr)
  * @nr: Bit to clear
  * @addr: Address to count from
  */
-static inline int test_and_clear_bit(int nr, volatile unsigned long *addr)
+static inline bool test_and_clear_bit(unsigned int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -183,7 +183,7 @@  static inline int test_and_clear_bit(int nr, volatile unsigned long *addr)
  * @nr: Bit to change
  * @addr: Address to count from
  */
-static inline int test_and_change_bit(int nr, volatile unsigned long *addr)
+static inline bool test_and_change_bit(unsigned int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -198,7 +198,7 @@  static inline int test_and_change_bit(int nr, volatile unsigned long *addr)
  * @nr: bit number to test
  * @addr: Address to start counting from
  */
-static inline int test_bit(int nr, const volatile unsigned long *addr)
+static inline bool test_bit(unsigned int nr, const unsigned long *addr)
 {
 	return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
 }
@@ -210,8 +210,7 @@  static inline int test_bit(int nr, const volatile unsigned long *addr)
  *
  * Returns the bit number of the first set bit, or size.
  */
-unsigned long find_last_bit(const unsigned long *addr,
-                            unsigned long size);
+unsigned long find_last_bit(const unsigned long *addr, unsigned long size);
 
 /**
  * find_next_bit - find the next set bit in a memory region
@@ -220,7 +219,7 @@  unsigned long find_last_bit(const unsigned long *addr,
  * @size: The bitmap size in bits
  */
 unsigned long find_next_bit(const unsigned long *addr,
-				   unsigned long size, unsigned long offset);
+                            unsigned long size, unsigned long offset);
 
 /**
  * find_next_zero_bit - find the next cleared bit in a memory region
@@ -282,9 +281,10 @@  static inline unsigned long hweight_long(unsigned long w)
  *
  * Returns: the value of the bit field extracted from the input value.
  */
-static inline uint32_t extract32(uint32_t value, int start, int length)
+static inline uint32_t extract32(uint32_t value, unsigned int start,
+                                 unsigned int length)
 {
-    assert(start >= 0 && length > 0 && length <= 32 - start);
+    assert(length > 0 && length <= 32 - start);
     return (value >> start) & (~0U >> (32 - length));
 }
 
@@ -301,9 +301,10 @@  static inline uint32_t extract32(uint32_t value, int start, int length)
  *
  * Returns: the value of the bit field extracted from the input value.
  */
-static inline uint64_t extract64(uint64_t value, int start, int length)
+static inline uint64_t extract64(uint64_t value, unsigned int start,
+                                 unsigned int length)
 {
-    assert(start >= 0 && length > 0 && length <= 64 - start);
+    assert(length > 0 && length <= 64 - start);
     return (value >> start) & (~0ULL >> (64 - length));
 }
 
@@ -324,11 +325,11 @@  static inline uint64_t extract64(uint64_t value, int start, int length)
  *
  * Returns: the modified @value.
  */
-static inline uint32_t deposit32(uint32_t value, int start, int length,
-                                 uint32_t fieldval)
+static inline uint32_t deposit32(uint32_t value, unsigned int start,
+                                 unsigned int length, uint32_t fieldval)
 {
     uint32_t mask;
-    assert(start >= 0 && length > 0 && length <= 32 - start);
+    assert(length > 0 && length <= 32 - start);
     mask = (~0U >> (32 - length)) << start;
     return (value & ~mask) | ((fieldval << start) & mask);
 }
@@ -350,11 +351,11 @@  static inline uint32_t deposit32(uint32_t value, int start, int length,
  *
  * Returns: the modified @value.
  */
-static inline uint64_t deposit64(uint64_t value, int start, int length,
-                                 uint64_t fieldval)
+static inline uint64_t deposit64(uint64_t value, unsigned int start,
+                                 unsigned int length, uint64_t fieldval)
 {
     uint64_t mask;
-    assert(start >= 0 && length > 0 && length <= 64 - start);
+    assert(length > 0 && length <= 64 - start);
     mask = (~0ULL >> (64 - length)) << start;
     return (value & ~mask) | ((fieldval << start) & mask);
 }