Libguestfs - Sep 2018 - Re: [PATCH nbdkit v2] common: isaligned: Use a macro instead of relying on implicit truncation.

On 9/17/18 3:39 PM, Nir Soffer wrote:
>> +#define IS_ALIGNED(size, align) ({              \
>> +      assert (is_power_of_2 ((align)));         \
>> +      !((size) & ((align) - 1));                \
>> +})
>>
> 
> But this version will happily accept singed int, and I think this code
> behavior with signed int is undefined.
Well, sort of. Bit shifts are very likely to hit undefined behavior on 
ints. But & and | are not shifts.
> 
> See
>
https://wiki.sei.cmu.edu/confluence/display/c/INT13-C.+Use+bitwise+operators+only+on+unsigned+operands
> 
> Why use a macro when we can use a function that is likely to be inlined
> anyway?
> This is not used in tight loops, so there is no reason to use a macro.
> 
> If you want to use a macro, see the kernel align macros:
Alas, the kernel has a more restrictive license (GPLv2 in general); 
which means we can't copy it into nbdkit.  (True, the definitions will 
look similar, but if we claim we copied from somewhere, that source had 
better be permissively licensed).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Tue, Sep 18, 2018 at 12:13 AM Eric Blake <eblake@redhat.com> wrote:
> On 9/17/18 3:39 PM, Nir Soffer wrote:
>
> >> +#define IS_ALIGNED(size, align) ({              \
> >> +      assert (is_power_of_2 ((align)));         \
> >> +      !((size) & ((align) - 1));                \
> >> +})
> >>
> >
> > But this version will happily accept singed int, and I think this code
> > behavior with signed int is undefined.
>
> Well, sort of. Bit shifts are very likely to hit undefined behavior on
> ints. But & and | are not shifts.
>
> >
> > See
> >
>
https://wiki.sei.cmu.edu/confluence/display/c/INT13-C.+Use+bitwise+operators+only+on+unsigned+operands
> >
> > Why use a macro when we can use a function that is likely to be
inlined
> > anyway?
> > This is not used in tight loops, so there is no reason to use a macro.
> >
> > If you want to use a macro, see the kernel align macros:
>
> Alas, the kernel has a more restrictive license (GPLv2 in general);
> which means we can't copy it into nbdkit.  (True, the definitions will
> look similar, but if we claim we copied from somewhere, that source had
> better be permissively licensed).
>
The FreeBSD version:

#define IS_ALIGNED <http://fxr.watson.org/fxr/ident?i=IS_ALIGNED>(n
<http://fxr.watson.org/fxr/ident?i=n>,align)     (!((uint32_t)(n
<http://fxr.watson.org/fxr/ident?i=n>) & (align - 1)))

http://fxr.watson.org/fxr/source/contrib/ncsw/inc/ncsw_ext.h#L182

Nir

On 9/17/18 4:41 PM, Nir Soffer wrote:
> The FreeBSD version:
> 
> #define IS_ALIGNED <http://fxr.watson.org/fxr/ident?i=IS_ALIGNED>(n
> <http://fxr.watson.org/fxr/ident?i=n>,align)     (!((uint32_t)(n
> <http://fxr.watson.org/fxr/ident?i=n>) & (align - 1)))
> 
> http://fxr.watson.org/fxr/source/contrib/ncsw/inc/ncsw_ext.h#L182
Which truncates to 32 bits.  But that's what we're trying to get rid of 
- by moving to a macro that lets us do full 64-bit alignment if needed.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org