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

[Syed Nayyar Waris <syednwaris@gmail.com>]

On Sat, May 30, 2020 at 3:13 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Fri, May 29, 2020 at 11:07 PM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> > On Sat, May 30, 2020 at 12:08 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> 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@intel.com> 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?

Sorry Andy it is not working. Actually the warning will be thrown,
whenever there will be comparison between 'h' and 'l'. If one of them
is '0' and the other is unsigned variable.
In above, still there is comparison being done between 'h' and 'l', so
the warning is getting thrown.

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

Actually for:
(BIT(nbits) - 1)
When nbits will be BITS_PER_LONG it will be 1U << 32 (or 64). Isn't it ?
The expression,
BIT(64) - 1
can become unexpectedly zero (incorrectly).

>
> --
> With Best Regards,
> Andy Shevchenko