Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Andy Shevchenko]

On Fri, May 29, 2020 at 11:07 PM Syed Nayyar Waris <syednwaris@xxxxxxxxx> wrote:
> On Sat, May 30, 2020 at 12:08 AM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > On Fri, May 29, 2020 at 11:38:18PM +0530, Syed Nayyar Waris wrote:
> > > On Sun, May 24, 2020 at 8:15 PM kbuild test robot <lkp@xxxxxxxxx> wrote:

...

> > > Taking the example statement (in my patch) where compilation warning
> > > is getting reported:
> > > return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> > >
> > > 'nbits' is of type 'unsigned long'.
> > > In above, the sanity check is comparing '0' with unsigned value. And
> > > unsigned value can't be less than '0' ever, hence the warning.
> > > But this warning will occur whenever there will be '0' as one of the
> > > 'argument' and an unsigned variable as another 'argument' for GENMASK.

> > Proper fix is to fix GENMASK(), but allowed workaround is to use
> >         (BIT(nbits) - 1)
> > instead.

> When I used BIT macro (earlier), I had faced a problem. I want to tell
> you about that.
>
> Inside functions 'bitmap_set_value' and 'bitmap_get_value' when nbits (or
> clump size) is BITS_PER_LONG, unexpected calculation happens.
>
> Explanation:
> Actually when nbits (clump size) is 64 (BITS_PER_LONG is 64 on my computer),
> (BIT(nbits) - 1)
> gives a value of zero and when this zero is ANDed with any value, it
> makes it full zero. This is unexpected and incorrect calculation happening.
>
> What actually happens is in the macro expansion of BIT(64), that is 1
> << 64, the '1' overflows from leftmost bit position (most significant
> bit) and re-enters at the rightmost bit position (least significant
> bit), therefore 1 << 64 becomes '0x1', and when another '1' is
> subtracted from this, the final result becomes 0.
>
> Since this macro is being used in both bitmap_get_value and
> bitmap_set_value functions, it will give unexpected results when nbits or clump
> size is BITS_PER_LONG (32 or 64 depending on arch).

I see, something like
https://elixir.bootlin.com/linux/latest/source/include/linux/dma-mapping.h#L139
should be done.
But yes, let's try to fix GENMASK().

So, if we modify the following

  #define GENMASK_INPUT_CHECK(h, l) \
    (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
    __builtin_constant_p((l) > (h)), (l) > (h), 0)))

to be

  #define GENMASK_INPUT_CHECK(h, l) \
    (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
    __builtin_constant_p((l) > (h)), (l) ? (l) > (h) : 0, 0)))

would it work?

> William also knows about this issue:
> "This is undefined behavior in the C standard (section 6.5.7 in the N1124)"

I think it is about 6.5.7.3  here, 1U << 31 (or 63) is okay.

--
With Best Regards,
Andy Shevchenko