Re: [Qemu-devel] [PATCH v2] bitops.h: Add field32() and field64() functions to extract bitfields

[Eric Blake]

On 06/27/2012 07:15 AM, Avi Kivity wrote:
> On 06/27/2012 01:29 PM, Peter Maydell wrote:
>> Add field32() and field64() functions which extract a particular
>> bit field from a word and return it. Based on an idea by Jia Liu.
>>

>> +static inline uint64_t field64(uint64_t value, int start, int length)
>> +{
>> +    assert(start >= 0 && start <= 63 && length > 0 && start + length <= 64);
>> +    return (value >> start) & (~0ULL >> (64 - length));
>> +}
>> +
> 
> Undefined for length == 64, so better add that to the assert.

How so?  ~0ULL >> 0 is well-defined (a length of 64, coupled with a
start of 0, is effectively writing the entire uint64_t value).  While it
is unlikely that anyone would ever trigger this with start and length
being constants (field64(value, 0, 64) == value), it might be triggered
when start and length are computed from other code.

> 
> I suggest adding the analogous functions for writing.  I believe the
> common naming is extract/deposit.
> 
> static inline uint64_t deposit64(uint64_t *pvalue, unsigned start,
>                                  unsigned length, uint64_t fieldval)
> {
>     uint64_t mask = (((uint64_t)1 << length) - 1) << start;

Here, a length of 64 is indeed undefined, but for symmetry with allowing
a full 64-bit extraction from a start of bit 0, you could special case
the computation of that mask.

>     *pvalue = (*pvalue & ~mask) | ((fieldval << start) & mask);

As with the extraction function, it would be worth an assert that start
and length don't trigger undefined shifts.  It may be further worth
asserting that fieldval is within the range given by length, although I
could see cases of intentionally wanting to pass in -1 to set all bits
within the mask and a tight assert would prevent that usage.

You marked this function signature as returning uint64_t, but didn't
return anything.  I think the logical return is the new contents of
*pvalue.  Another logical choice would be marking the function void.

> 
> Useful for setting a bit to a specific value.

Indeed, having the pair of functions may be handy.

-- 
Eric Blake   address@hidden    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature