Messages in this thread | | | From | Rikard Falkeborn <> | Date | Wed, 7 Aug 2019 22:07:53 +0200 | Subject | Re: [PATCH v2 2/2] linux/bits.h: Add compile time sanity check of GENMASK inputs |
| |
On Wed, Aug 07, 2019 at 09:52:33AM -0700, Guenter Roeck wrote: > On 8/7/19 7:55 AM, Masahiro Yamada wrote: > > On Wed, Aug 7, 2019 at 11:27 PM Guenter Roeck <linux@roeck-us.net> wrote: > > > > > > On Fri, Aug 02, 2019 at 01:03:58AM +0200, Rikard Falkeborn wrote: > > > > GENMASK() and GENMASK_ULL() are supposed to be called with the high bit > > > > as the first argument and the low bit as the second argument. Mixing > > > > them will return a mask with zero bits set. > > > > > > > > Recent commits show getting this wrong is not uncommon, see e.g. > > > > commit aa4c0c9091b0 ("net: stmmac: Fix misuses of GENMASK macro") and > > > > commit 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK > > > > macro"). > > > > > > > > To prevent such mistakes from appearing again, add compile time sanity > > > > checking to the arguments of GENMASK() and GENMASK_ULL(). If both the > > > > arguments are known at compile time, and the low bit is higher than the > > > > high bit, break the build to detect the mistake immediately. > > > > > > > > Since GENMASK() is used in declarations, BUILD_BUG_ON_ZERO() must be > > > > used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead > > > > of __builtin_constant_p(). > > > > > > > > If successful, BUILD_BUG_OR_ZERO() returns 0 of type size_t. To avoid > > > > problems with implicit conversions, cast the result of BUILD_BUG_OR_ZERO > > > > to unsigned long. > > > > > > > > Since both BUILD_BUG_ON_ZERO() and __is_constexpr() only uses sizeof() > > > > on the arguments passed to them, neither of them evaluate the expression > > > > unless it is a VLA. Therefore, GENMASK(1, x++) still behaves as > > > > expected. > > > > > > > > Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends > > > > available in assembly") made the macros in linux/bits.h available in > > > > assembly. Since neither BUILD_BUG_OR_ZERO() or __is_constexpr() are asm > > > > compatible, disable the checks if the file is included in an asm file. > > > > > > > > > > Who is going to fix the fallout ? For example, arm64:defconfig no longer > > > compiles with this patch applied. > > > > > > It seems to me that the benefit of catching misuses of GENMASK is much > > > less than the fallout from no longer compiling kernels, since those > > > kernels won't get any test coverage at all anymore. > > > > > > We cannot apply this until we fix all errors. > > > > I do not understand why Andrew picked up this so soon. > > > > The same was done with the fallthrough warning in mainline, which still results > in all "sh" builds failing there (and in -next, obviously). I don't understand > the logic either, but maybe it is the new normal. > > Guenter
Sorry about that. As Masahiro said, the patch was picked up too soon, I've asked Andrew and Stephen Rothwell to remove the patch in a separate email thread.
FWIW, there seems to be a patch for the build failure already (all arm builds seems to have the same build error):
https://lore.kernel.org/lkml/20190807192305.6604-1-natechancellor@gmail.com/T/#u
Rikard
| |