Hi Arnd,
On Wed, May 29, 2024 at 04:39:45PM +0200, Arnd Bergmann wrote:
> On Fri, May 24, 2024, at 05:00, Andrew Morton wrote:
> > ------------------------------------------------------
> > From: Yury Norov <yury.norov@xxxxxxxxx>
> > Subject: gcc: disable '-Warray-bounds' for gcc-9
> > Date: Wed, 22 May 2024 15:58:30 -0700
> >
> > '-Warray-bounds' is already disabled for gcc-10+. Now that we've merged
> > bitmap_{read,write), I see the following error when building the kernel
> > with gcc-9.4 (Ubuntu 20.04.4 LTS) for x86_64 allmodconfig:
> >
> > drivers/pinctrl/pinctrl-cy8c95x0.c: In function
> > `cy8c95x0_read_regs_mask.isra.0':
> > include/linux/bitmap.h:756:18: error: array subscript [1,
> > 288230376151711744] is outside array bounds of `long unsigned int[1]'
> > [-Werror=array-bounds]
> > 756 | value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start +
> > nbits);
> > | ~~~^~~~~~~~~~~
> >
> > The immediate reason is that the commit b44759705f7d ("bitmap: make
> > bitmap_{get,set}_value8() use bitmap_{read,write}()") switched the
> > bitmap_get_value8() to an alias of bitmap_read(); the same for 'set'.
> >
> > Now; the code that triggers Warray-bounds, calls the function like this:
> >
> > #define MAX_BANK 8
> > #define BANK_SZ 8
> > #define MAX_LINE (MAX_BANK * BANK_SZ)
> > DECLARE_BITMAP(tval, MAX_LINE); // 64-bit map: unsigned long tval[1]
> >
> > read_val |= bitmap_get_value8(tval, i * BANK_SZ) & ~bits;
> >
> > bitmap_read() is implemented such that it may conditionally dereference a
> > pointer beyond the boundary like this:
> >
> > unsigned long offset = start % BITS_PER_LONG;
> > unsigned long space = BITS_PER_LONG - offset;
> >
> > if (space >= nbits)
> > return (map[index] >> offset) & BITMAP_LAST_WORD_MASK(nbits);
> >
> > value_low = map[index] & BITMAP_FIRST_WORD_MASK(start);
> > value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start + nbits);
> > return (value_low >> offset) | (value_high << space);
> >
> > In case of bitmap_get_value8(), it's impossible to violate the boundary
> > because 'space >= nbits' is never the true for byte-aligned 8-bit access.
> > So, this is clearly a false-positive.
> >
> > The same type of false-positives break my allmodconfig build in many
> > places. gcc-8, is clear, however.
>
> I'm not too happy about this one,
Neither me
> I think this is mixing up
> a couple of independent issues, and makes it harder to
> ever enable the warning again.
>
> The bitmap.h code looks suspicious to me, and if gcc is
> unable to analyze this as a false positive, it probably
> also can't optimize it correctly,
In the b44759705f7d Alexander says:
bloat-o-meter shows no difference in vmlinux and -2 bytes for
gpio-pca953x.ko, which says the optimization didn't suffer due to
that change. The converted helpers have the value width embedded
and always compile-time constant and that helps a lot.
Bloat-o-meter itself is not a measure of how effective the code is,
but it's a good hit that code generation before/after is at least
on par. Have you an evidence that the patch makes code generation
worse?
> so it may be better
> to either not have this as an inline function at all,
> or find an implementation that gcc can optimize better.
The functions look bulky but it boild to one or at max two words fetch
plus shifts. Inlining helps to generate better code, particularly in
the bitmap_get/set_value8 case because masks and offsets generation is
done at compile time.
We had quite a few cycles back then... Alexander, can you please
share on code generation, particularly inline vs outline versions?
> In the meantime, I would suggest reverting b44759705f7d
> ("bitmap: make bitmap_{get,set}_value8() use
> bitmap_{read,write}()"), until the implementation is
> improved to work without a warning.
I think there's nothing to improve. This is clearly a false-positive
GCC warning, and it should be fixed on GCC side.
> The other problem I see is that the warning is
> disabled globally even when building with W=123,
> and I think we should change it to always warn at
> least with W=1 regardless of the compiler version.
> It's also likely that the other false-positive
> warnings only happen when sanitizers are enabled,
> so we could turn it on by default without sanitizers
> and move it to W=1 with sanitizers.
Interesting. If sanitizers are involved, we should do like you said.
But this is a matter of a different patch, I think.
Thanks,
Yury
