These GENMASK uses are inverted argument order and the
actual masks produced are incorrect. Fix them.
Add checkpatch tests to help avoid more misuses too.
Joe Perches (12):
checkpatch: Add GENMASK tests
clocksource/drivers/npcm: Fix misuse of GENMASK macro
drm: aspeed_gfx: Fix misuse of GENMASK macro
iio: adc: max9611: Fix misuse of GENMASK macro
irqchip/gic-v3-its: Fix misuse of GENMASK macro
mmc: meson-mx-sdio: Fix misuse of GENMASK macro
net: ethernet: mediatek: Fix misuses of GENMASK macro
net: stmmac: Fix misuses of GENMASK macro
rtw88: Fix misuse of GENMASK macro
phy: amlogic: G12A: Fix misuse of GENMASK macro
staging: media: cedrus: Fix misuse of GENMASK macro
ASoC: wcd9335: Fix misuse of GENMASK macro
drivers/clocksource/timer-npcm7xx.c | 2 +-
drivers/gpu/drm/aspeed/aspeed_gfx.h | 2 +-
drivers/iio/adc/max9611.c | 2 +-
drivers/irqchip/irq-gic-v3-its.c | 2 +-
drivers/mmc/host/meson-mx-sdio.c | 2 +-
drivers/net/ethernet/mediatek/mtk_eth_soc.h | 2 +-
drivers/net/ethernet/mediatek/mtk_sgmii.c | 2 +-
drivers/net/ethernet/stmicro/stmmac/descs.h | 2 +-
drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 4 ++--
drivers/net/wireless/realtek/rtw88/rtw8822b.c | 2 +-
drivers/phy/amlogic/phy-meson-g12a-usb2.c | 2 +-
drivers/staging/media/sunxi/cedrus/cedrus_regs.h | 2 +-
scripts/checkpatch.pl | 15 +++++++++++++++
sound/soc/codecs/wcd-clsh-v2.c | 2 +-
14 files changed, 29 insertions(+), 14 deletions(-)
--
2.15.0
Arguments are supposed to be ordered high then low.
Signed-off-by: Joe Perches <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/descs.h | 2 +-
drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/descs.h b/drivers/net/ethernet/stmicro/stmmac/descs.h
index 10429b05f932..9f0b9a9e63b3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/descs.h
+++ b/drivers/net/ethernet/stmicro/stmmac/descs.h
@@ -123,7 +123,7 @@
#define ETDES1_BUFFER2_SIZE_SHIFT 16
/* Extended Receive descriptor definitions */
-#define ERDES4_IP_PAYLOAD_TYPE_MASK GENMASK(2, 6)
+#define ERDES4_IP_PAYLOAD_TYPE_MASK GENMASK(6, 2)
#define ERDES4_IP_HDR_ERR BIT(3)
#define ERDES4_IP_PAYLOAD_ERR BIT(4)
#define ERDES4_IP_CSUM_BYPASSED BIT(5)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index 6d5cba4075eb..4c5db00c1d18 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -192,7 +192,7 @@ static const struct emac_variant emac_variant_h6 = {
/* Used in RX_CTL1*/
#define EMAC_RX_MD BIT(1)
-#define EMAC_RX_TH_MASK GENMASK(4, 5)
+#define EMAC_RX_TH_MASK GENMASK(5, 4)
#define EMAC_RX_TH_32 0
#define EMAC_RX_TH_64 (0x1 << 4)
#define EMAC_RX_TH_96 (0x2 << 4)
@@ -203,7 +203,7 @@ static const struct emac_variant emac_variant_h6 = {
/* Used in TX_CTL1*/
#define EMAC_TX_MD BIT(1)
#define EMAC_TX_NEXT_FRM BIT(2)
-#define EMAC_TX_TH_MASK GENMASK(8, 10)
+#define EMAC_TX_TH_MASK GENMASK(10, 8)
#define EMAC_TX_TH_64 0
#define EMAC_TX_TH_128 (0x1 << 8)
#define EMAC_TX_TH_192 (0x2 << 8)
--
2.15.0
Arguments are supposed to be ordered high then low.
Signed-off-by: Joe Perches <[email protected]>
---
drivers/net/wireless/realtek/rtw88/rtw8822b.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822b.c b/drivers/net/wireless/realtek/rtw88/rtw8822b.c
index 1172f6c0605b..d61d534396c7 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8822b.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8822b.c
@@ -997,7 +997,7 @@ static void rtw8822b_do_iqk(struct rtw_dev *rtwdev)
rtw_write_rf(rtwdev, RF_PATH_A, RF_DTXLOK, RFREG_MASK, 0x0);
reload = !!rtw_read32_mask(rtwdev, REG_IQKFAILMSK, BIT(16));
- iqk_fail_mask = rtw_read32_mask(rtwdev, REG_IQKFAILMSK, GENMASK(0, 7));
+ iqk_fail_mask = rtw_read32_mask(rtwdev, REG_IQKFAILMSK, GENMASK(7, 0));
rtw_dbg(rtwdev, RTW_DBG_PHY,
"iqk counter=%d reload=%d do_iqk_cnt=%d n_iqk_fail(mask)=0x%02x\n",
counter, reload, ++do_iqk_cnt, iqk_fail_mask);
--
2.15.0
Arguments are supposed to be ordered high then low.
Signed-off-by: Joe Perches <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 35500801dc2b..730fbe0e2a9d 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -185,7 +185,7 @@ static struct its_collection *dev_event_to_col(struct its_device *its_dev,
static struct its_collection *valid_col(struct its_collection *col)
{
- if (WARN_ON_ONCE(col->target_address & GENMASK_ULL(0, 15)))
+ if (WARN_ON_ONCE(col->target_address & GENMASK_ULL(15, 0)))
return NULL;
return col;
--
2.15.0
Arguments are supposed to be ordered high then low.
Signed-off-by: Joe Perches <[email protected]>
---
drivers/net/ethernet/mediatek/mtk_eth_soc.h | 2 +-
drivers/net/ethernet/mediatek/mtk_sgmii.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index 876ce6798709..221012ecb845 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -712,7 +712,7 @@ struct mtk_soc_data {
#define MTK_MAX_DEVS 2
#define MTK_SGMII_PHYSPEED_AN BIT(31)
-#define MTK_SGMII_PHYSPEED_MASK GENMASK(0, 2)
+#define MTK_SGMII_PHYSPEED_MASK GENMASK(2, 0)
#define MTK_SGMII_PHYSPEED_1000 BIT(0)
#define MTK_SGMII_PHYSPEED_2500 BIT(1)
#define MTK_HAS_FLAGS(flags, _x) (((flags) & (_x)) == (_x))
diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index 136f90ce5a65..ff509d42d818 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -82,7 +82,7 @@ int mtk_sgmii_setup_mode_force(struct mtk_sgmii *ss, int id)
return -EINVAL;
regmap_read(ss->regmap[id], ss->ana_rgc3, &val);
- val &= ~GENMASK(2, 3);
+ val &= ~GENMASK(3, 2);
mode = ss->flags[id] & MTK_SGMII_PHYSPEED_MASK;
val |= (mode == MTK_SGMII_PHYSPEED_1000) ? 0 : BIT(2);
regmap_write(ss->regmap[id], ss->ana_rgc3, val);
--
2.15.0
Commit-ID: 20faba848752901de23a4d45a1174d64d2069dde
Gitweb: https://git.kernel.org/tip/20faba848752901de23a4d45a1174d64d2069dde
Author: Joe Perches <[email protected]>
AuthorDate: Tue, 9 Jul 2019 22:04:18 -0700
Committer: Thomas Gleixner <[email protected]>
CommitDate: Wed, 10 Jul 2019 11:04:17 +0200
irqchip/gic-v3-its: Fix misuse of GENMASK macro
Arguments are supposed to be ordered high then low.
Signed-off-by: Joe Perches <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Acked-by: Marc Zyngier <[email protected]>
Link: https://lkml.kernel.org/r/ab5deb4fc3cd604cb620054770b7d00016d736bc.1562734889.git.joe@perches.com
---
drivers/irqchip/irq-gic-v3-its.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 35500801dc2b..730fbe0e2a9d 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -185,7 +185,7 @@ static struct its_collection *dev_event_to_col(struct its_device *its_dev,
static struct its_collection *valid_col(struct its_collection *col)
{
- if (WARN_ON_ONCE(col->target_address & GENMASK_ULL(0, 15)))
+ if (WARN_ON_ONCE(col->target_address & GENMASK_ULL(15, 0)))
return NULL;
return col;
On 10/07/2019 06:04, Joe Perches wrote:
> Arguments are supposed to be ordered high then low.
>
> Signed-off-by: Joe Perches <[email protected]>
> ---
> drivers/irqchip/irq-gic-v3-its.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 35500801dc2b..730fbe0e2a9d 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -185,7 +185,7 @@ static struct its_collection *dev_event_to_col(struct its_device *its_dev,
>
> static struct its_collection *valid_col(struct its_collection *col)
> {
> - if (WARN_ON_ONCE(col->target_address & GENMASK_ULL(0, 15)))
> + if (WARN_ON_ONCE(col->target_address & GENMASK_ULL(15, 0)))
> return NULL;
>
> return col;
>
Well caught.
Acked-by: Marc Zyngier <[email protected]>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
From: Joe Perches <[email protected]>
Date: Jul/10/2019, 06:04:21 (UTC+00:00)
> Arguments are supposed to be ordered high then low.
>
> Signed-off-by: Joe Perches <[email protected]>
If you submit another version please add:
Fixes: 293e4365a1ad ("stmmac: change descriptor layout")
Fixes: 9f93ac8d4085 ("net-next: stmmac: Add dwmac-sun8i")
---
Thanks,
Jose Miguel Abreu
Hi Joe,
On 10.07.2019 07:04, Joe Perches wrote:
> These GENMASK uses are inverted argument order and the
> actual masks produced are incorrect. Fix them.
>
> Add checkpatch tests to help avoid more misuses too.
>
> Joe Perches (12):
> checkpatch: Add GENMASK tests
> clocksource/drivers/npcm: Fix misuse of GENMASK macro
> drm: aspeed_gfx: Fix misuse of GENMASK macro
> iio: adc: max9611: Fix misuse of GENMASK macro
> irqchip/gic-v3-its: Fix misuse of GENMASK macro
> mmc: meson-mx-sdio: Fix misuse of GENMASK macro
> net: ethernet: mediatek: Fix misuses of GENMASK macro
> net: stmmac: Fix misuses of GENMASK macro
> rtw88: Fix misuse of GENMASK macro
> phy: amlogic: G12A: Fix misuse of GENMASK macro
> staging: media: cedrus: Fix misuse of GENMASK macro
> ASoC: wcd9335: Fix misuse of GENMASK macro
>
> drivers/clocksource/timer-npcm7xx.c | 2 +-
> drivers/gpu/drm/aspeed/aspeed_gfx.h | 2 +-
> drivers/iio/adc/max9611.c | 2 +-
> drivers/irqchip/irq-gic-v3-its.c | 2 +-
> drivers/mmc/host/meson-mx-sdio.c | 2 +-
> drivers/net/ethernet/mediatek/mtk_eth_soc.h | 2 +-
> drivers/net/ethernet/mediatek/mtk_sgmii.c | 2 +-
> drivers/net/ethernet/stmicro/stmmac/descs.h | 2 +-
> drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 4 ++--
> drivers/net/wireless/realtek/rtw88/rtw8822b.c | 2 +-
> drivers/phy/amlogic/phy-meson-g12a-usb2.c | 2 +-
> drivers/staging/media/sunxi/cedrus/cedrus_regs.h | 2 +-
> scripts/checkpatch.pl | 15 +++++++++++++++
> sound/soc/codecs/wcd-clsh-v2.c | 2 +-
> 14 files changed, 29 insertions(+), 14 deletions(-)
>
After adding following compile time check:
------
diff --git a/Makefile b/Makefile
index 5102b2bbd224..ac4ea5f443a9 100644
--- a/Makefile
+++ b/Makefile
@@ -457,7 +457,7 @@ KBUILD_AFLAGS := -D__ASSEMBLY__ -fno-PIE
KBUILD_CFLAGS := -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs \
-fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE \
-Werror=implicit-function-declaration
-Werror=implicit-int \
- -Wno-format-security \
+ -Wno-format-security -Werror=div-by-zero \
-std=gnu89
KBUILD_CPPFLAGS := -D__KERNEL__
KBUILD_AFLAGS_KERNEL :=
diff --git a/include/linux/bits.h b/include/linux/bits.h
index 669d69441a62..61d74b103055 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -19,11 +19,11 @@
* GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
*/
#define GENMASK(h, l) \
- (((~UL(0)) - (UL(1) << (l)) + 1) & \
+ (((~UL(0)) - (UL(1) << (l)) + 1 + 0/((h) >= (l))) & \
(~UL(0) >> (BITS_PER_LONG - 1 - (h))))
#define GENMASK_ULL(h, l) \
- (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
+ (((~ULL(0)) - (ULL(1) << (l)) + 1 + 0/((h) >= (l))) & \
(~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
#endif /* __LINUX_BITS_H */
-------
I was able to detect one more GENMASK misue (AARCH64, allyesconfig):
CC drivers/phy/rockchip/phy-rockchip-inno-hdmi.o
In file included from ../include/linux/bitops.h:5:0,
from ../include/linux/kernel.h:12,
from ../include/linux/clk.h:13,
from ../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:9:
../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c: In function
‘inno_hdmi_phy_rk3328_power_on’:
../include/linux/bits.h:22:37: error: division by zero [-Werror=div-by-zero]
(((~UL(0)) - (UL(1) << (l)) + 1 + 0/((h) >= (l))) & \
^
../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:24:42: note: in
expansion of macro ‘GENMASK’
#define UPDATE(x, h, l) (((x) << (l)) & GENMASK((h), (l)))
^~~~~~~
../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:201:50: note: in
expansion of macro ‘UPDATE’
#define RK3328_TERM_RESISTOR_CALIB_SPEED_7_0(x) UPDATE(x, 7, 9)
^~~~~~
../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:1046:26: note: in
expansion of macro ‘RK3328_TERM_RESISTOR_CALIB_SPEED_7_0’
inno_write(inno, 0xc6, RK3328_TERM_RESISTOR_CALIB_SPEED_7_0(v));
Of course I do not advise to add the check as is to Kernel - it is
undefined behavior area AFAIK.
Regards
Andrzej
Trimming CC-list.
> It'd can't be done as it's used in declarations
> and included in asm files and it uses the UL()
> macro.
Can the BUILD_BUG_ON_ZERO() macro be used instead? It works in
declarations. I don't know if it works in asm-files, but the below
changes builds an x86-64 allyesconfig without problems (after the rest
of this series have been applied.
/Rikard
---
include/linux/bits.h | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/include/linux/bits.h b/include/linux/bits.h
index 669d69441a62..52e747d27f87 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -2,6 +2,7 @@
#ifndef __LINUX_BITS_H
#define __LINUX_BITS_H
+#include <linux/build_bug.h>
#include <linux/const.h>
#include <asm/bitsperlong.h>
@@ -19,11 +20,15 @@
* GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
*/
#define GENMASK(h, l) \
+ (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
+ __is_constexpr(h) && __is_constexpr(l), (l) > (h), 0)) + \
(((~UL(0)) - (UL(1) << (l)) + 1) & \
- (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
+ (~UL(0) >> (BITS_PER_LONG - 1 - (h)))))
#define GENMASK_ULL(h, l) \
+ (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
+ __is_constexpr(h) && __is_constexpr(l), (l) > (h), 0)) + \
(((~ULL(0)) - (ULL(1) << (l)) + 1) & \
- (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
+ (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h)))))
#endif /* __LINUX_BITS_H */
--
2.22.0
On Sat, 2019-07-27 at 21:54 +0200, Rikard Falkeborn wrote:
> Trimming CC-list.
>
> > It'd can't be done as it's used in declarations
> > and included in asm files and it uses the UL()
> > macro.
>
> Can the BUILD_BUG_ON_ZERO() macro be used instead? It works in
> declarations. I don't know if it works in asm-files, but the below
> changes builds an x86-64 allyesconfig without problems (after the rest
> of this series have been applied.
Maybe, fine by me if it works.
Perhaps you should submit this and let the build-bot
verify it.
> /Rikard
>
> ---
> include/linux/bits.h | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 669d69441a62..52e747d27f87 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -2,6 +2,7 @@
> #ifndef __LINUX_BITS_H
> #define __LINUX_BITS_H
>
> +#include <linux/build_bug.h>
> #include <linux/const.h>
> #include <asm/bitsperlong.h>
>
> @@ -19,11 +20,15 @@
> * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> */
> #define GENMASK(h, l) \
> + (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> + __is_constexpr(h) && __is_constexpr(l), (l) > (h), 0)) + \
> (((~UL(0)) - (UL(1) << (l)) + 1) & \
> - (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> + (~UL(0) >> (BITS_PER_LONG - 1 - (h)))))
>
> #define GENMASK_ULL(h, l) \
> + (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> + __is_constexpr(h) && __is_constexpr(l), (l) > (h), 0)) + \
> (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> - (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
> + (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h)))))
>
> #endif /* __LINUX_BITS_H */
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_OR_ZERO() must be
used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead
of __builtin_constant_p().
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.
Signed-off-by: Rikard Falkeborn <[email protected]>
---
Joe Perches sent a series to fix the existing misuses of GENMASK() that
needs to be merged before this to avoid build failures. Currently, 7 of
the patches were not in Linus tree, and 2 were not in linux-next.
Also, there's currently no asm users of bits.h, but since it was made
asm-compatible just two weeks ago it would be a shame to break it right
away...
include/linux/bits.h | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/include/linux/bits.h b/include/linux/bits.h
index 669d69441a62..73489579eef9 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -18,12 +18,22 @@
* position @h. For example
* GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
*/
+#ifndef __ASSEMBLY__
+#include <linux/build_bug.h>
+#define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
+ __is_constexpr(h) && __is_constexpr(l), (l) > (h), 0))
+#else
+#define GENMASK_INPUT_CHECK(h, l) 0
+#endif
+
#define GENMASK(h, l) \
+ (GENMASK_INPUT_CHECK(h, l) + \
(((~UL(0)) - (UL(1) << (l)) + 1) & \
- (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
+ (~UL(0) >> (BITS_PER_LONG - 1 - (h)))))
#define GENMASK_ULL(h, l) \
+ (GENMASK_INPUT_CHECK(h, l) + \
(((~ULL(0)) - (ULL(1) << (l)) + 1) & \
- (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
+ (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h)))))
#endif /* __LINUX_BITS_H */
--
2.22.0
On Thu, Aug 1, 2019 at 4:04 AM Rikard Falkeborn
<[email protected]> 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_OR_ZERO() must be
> used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead
> of __builtin_constant_p().
>
> 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.
>
> Signed-off-by: Rikard Falkeborn <[email protected]>
> ---
> Joe Perches sent a series to fix the existing misuses of GENMASK() that
> needs to be merged before this to avoid build failures. Currently, 7 of
> the patches were not in Linus tree, and 2 were not in linux-next.
>
> Also, there's currently no asm users of bits.h, but since it was made
> asm-compatible just two weeks ago it would be a shame to break it right
> away...
>
> include/linux/bits.h | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 669d69441a62..73489579eef9 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -18,12 +18,22 @@
> * position @h. For example
> * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> */
> +#ifndef __ASSEMBLY__
> +#include <linux/build_bug.h>
> +#define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> + __is_constexpr(h) && __is_constexpr(l), (l) > (h), 0))
> +#else
> +#define GENMASK_INPUT_CHECK(h, l) 0
> +#endif
> +
> #define GENMASK(h, l) \
> + (GENMASK_INPUT_CHECK(h, l) + \
> (((~UL(0)) - (UL(1) << (l)) + 1) & \
> - (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> + (~UL(0) >> (BITS_PER_LONG - 1 - (h)))))
>
> #define GENMASK_ULL(h, l) \
> + (GENMASK_INPUT_CHECK(h, l) + \
> (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> - (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
> + (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h)))))
This is getting cluttered with so many parentheses.
One way of clean-up is to rename the current macros as follows:
GENMASK() -> __GENMASK()
GENMASK_UL() -> __GENMASK_ULL()
Then,
#define GENMASK(h, l) (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
#define GENMASK_ULL(h, l) (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
--
Best Regards
Masahiro Yamada
On Thu, 2019-08-01 at 11:50 +0900, Masahiro Yamada wrote:
> On Thu, Aug 1, 2019 at 4:04 AM Rikard Falkeborn
> <[email protected]> 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_OR_ZERO() must be
> > used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead
> > of __builtin_constant_p().
> >
> > 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.
> >
> > Signed-off-by: Rikard Falkeborn <[email protected]>
> > ---
> > Joe Perches sent a series to fix the existing misuses of GENMASK() that
> > needs to be merged before this to avoid build failures. Currently, 7 of
> > the patches were not in Linus tree, and 2 were not in linux-next.
> >
> > Also, there's currently no asm users of bits.h, but since it was made
> > asm-compatible just two weeks ago it would be a shame to break it right
> > away...
[]
> > diff --git a/include/linux/bits.h b/include/linux/bits.h
[]
> > #define GENMASK(h, l) \
> > + (GENMASK_INPUT_CHECK(h, l) + \
> > (((~UL(0)) - (UL(1) << (l)) + 1) & \
> > - (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> > + (~UL(0) >> (BITS_PER_LONG - 1 - (h)))))
> >
> > #define GENMASK_ULL(h, l) \
> > + (GENMASK_INPUT_CHECK(h, l) + \
> > (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> > - (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
> > + (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h)))))
>
> This is getting cluttered with so many parentheses.
>
> One way of clean-up is to rename the current macros as follows:
>
> GENMASK() -> __GENMASK()
> GENMASK_UL() -> __GENMASK_ULL()
>
> Then,
>
> #define GENMASK(h, l) (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> #define GENMASK_ULL(h, l) (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
Much nicer. It may be better still to use avoid
multiple dereferences of each argument.
Also it'd be useful to rename h and l to something like
high_bit and low_bit or high and low.
On Wed, Jul 31, 2019 at 07:57:48PM -0700, Joe Perches wrote:
> On Thu, 2019-08-01 at 11:50 +0900, Masahiro Yamada wrote:
> > On Thu, Aug 1, 2019 at 4:04 AM Rikard Falkeborn
> > <[email protected]> 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.
> >
> > This is getting cluttered with so many parentheses.
> >
> > One way of clean-up is to rename the current macros as follows:
> >
> > GENMASK() -> __GENMASK()
> > GENMASK_UL() -> __GENMASK_ULL()
> >
> > Then,
> >
> > #define GENMASK(h, l) (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > #define GENMASK_ULL(h, l) (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
>
> Much nicer. It may be better still to use avoid
> multiple dereferences of each argument.
Much nicer indeed, I changed it accordingly. There are no multiple
dererences of the arguments. GENMASK_INPUT_CHECK() and __is_constexpr()
both use sizeof() on the input arguments, which does not evaluate the
argument (unless the argument is a VLA, which is not allowed).
> Also it'd be useful to rename h and l to something like
> high_bit and low_bit or high and low.
Agreed.
Be a little more verbose to improve readability.
Signed-off-by: Rikard Falkeborn <[email protected]>
---
Changes in v2:
- This patch is new in v2
include/linux/bits.h | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/include/linux/bits.h b/include/linux/bits.h
index 669d69441a62..d4466aa42a9c 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -14,16 +14,16 @@
#define BITS_PER_BYTE 8
/*
- * Create a contiguous bitmask starting at bit position @l and ending at
- * position @h. For example
+ * Create a contiguous bitmask starting at bit position @low and ending at
+ * position @high. For example
* GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
*/
-#define GENMASK(h, l) \
- (((~UL(0)) - (UL(1) << (l)) + 1) & \
- (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
+#define GENMASK(high, low) \
+ (((~UL(0)) - (UL(1) << (low)) + 1) & \
+ (~UL(0) >> (BITS_PER_LONG - 1 - (high))))
-#define GENMASK_ULL(h, l) \
- (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
- (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
+#define GENMASK_ULL(high, low) \
+ (((~ULL(0)) - (ULL(1) << (low)) + 1) & \
+ (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (high))))
#endif /* __LINUX_BITS_H */
--
2.22.0
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.
Signed-off-by: Rikard Falkeborn <[email protected]>
---
Changes in v2:
- Add comment about why inputs are not checked when used in asm file
- Use UL(0) instead of 0
- Extract mask creation in a separate macro to improve readability
- Use high and low instead of h and l (part of this was extracted to a
separate patch)
- Updated commit message
Joe Perches sent a series to fix the existing misuses of GENMASK() that
needs to be merged before this to avoid build failures. Currently, 6 of
the patches are not in Linus tree, and 2 are not in linux-next.
include/linux/bits.h | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/include/linux/bits.h b/include/linux/bits.h
index d4466aa42a9c..955e9e43c4a5 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -18,12 +18,30 @@
* position @high. For example
* GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
*/
-#define GENMASK(high, low) \
+#ifndef __ASSEMBLY__
+#include <linux/build_bug.h>
+#define GENMASK_INPUT_CHECK(high, low) \
+ ((unsigned long)BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
+ __is_constexpr(high) && __is_constexpr(low), \
+ (low) > (high), UL(0))))
+#else
+/*
+ * BUILD_BUG_ON_ZERO and __is_constexpr() are not available in h files
+ * included from asm files, disable the input check if that is the case.
+ */
+#define GENMASK_INPUT_CHECK(high, low) UL(0)
+#endif
+
+#define __GENMASK(high, low) \
(((~UL(0)) - (UL(1) << (low)) + 1) & \
(~UL(0) >> (BITS_PER_LONG - 1 - (high))))
+#define GENMASK(high, low) \
+ (GENMASK_INPUT_CHECK(high, low) + __GENMASK(high, low))
-#define GENMASK_ULL(high, low) \
+#define __GENMASK_ULL(high, low) \
(((~ULL(0)) - (ULL(1) << (low)) + 1) & \
(~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (high))))
+#define GENMASK_ULL(high, low) \
+ (GENMASK_INPUT_CHECK(high, low) + __GENMASK_ULL(high, low))
#endif /* __LINUX_BITS_H */
--
2.22.0
On Fri, 2019-08-02 at 01:03 +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.
>
> Signed-off-by: Rikard Falkeborn <[email protected]>
> ---
> Changes in v2:
> - Add comment about why inputs are not checked when used in asm file
> - Use UL(0) instead of 0
> - Extract mask creation in a separate macro to improve readability
> - Use high and low instead of h and l (part of this was extracted to a
> separate patch)
> - Updated commit message
>
> Joe Perches sent a series to fix the existing misuses of GENMASK() that
> needs to be merged before this to avoid build failures. Currently, 6 of
> the patches are not in Linus tree, and 2 are not in linux-next.
Thanks Rikard.
It wouldn't surprise me if this change finds more misuses
as the compiler will perform substitutions on #define
values where the search I did was just for decimal uses.
For instance, this new macro should build error on:
#define FOO 5
#define BAR 6
GENMASK(FOO, BAR)
On Sat, Aug 3, 2019 at 12:03 PM Masahiro Yamada
<[email protected]> wrote:
>
> BTW, v2 is already inconsistent.
> If you wanted GENMASK_INPUT_CHECK() to return 'unsigned long',,
> you would have to cast (low) > (high) as well:
>
> (unsigned long)((low) > (high)), UL(0))))
>
> This is totally redundant, and weird.
I take back this comment.
You added (unsigned long) to the beginning of this macro.
So, the type is consistent, but I believe all casts should be removed.
--
Best Regards
Masahiro Yamada
On Sat, Aug 03, 2019 at 12:12:46PM +0900, Masahiro Yamada wrote:
> On Sat, Aug 3, 2019 at 12:03 PM Masahiro Yamada
> <[email protected]> wrote:
>
> >
> > BTW, v2 is already inconsistent.
> > If you wanted GENMASK_INPUT_CHECK() to return 'unsigned long',,
> > you would have to cast (low) > (high) as well:
> >
> > (unsigned long)((low) > (high)), UL(0))))
> >
> > This is totally redundant, and weird.
>
> I take back this comment.
> You added (unsigned long) to the beginning of this macro.
> So, the type is consistent, but I believe all casts should be removed.
Maybe you're right. BUILD_BUG_ON_ZERO returns size_t regardless of
inputs. I was worried that on some platform, size_t would be larger than
unsigned long (as far as I could see, the standard does not give any
guarantees), and thus all of a sudden GENMASK would be 8 bytes instead
of 4, but perhaps that is not a problem?
On Sun, Aug 4, 2019 at 3:36 AM Rikard Falkeborn
<[email protected]> wrote:
>
> On Sat, Aug 03, 2019 at 12:12:46PM +0900, Masahiro Yamada wrote:
> > On Sat, Aug 3, 2019 at 12:03 PM Masahiro Yamada
> > <[email protected]> wrote:
> >
> > >
> > > BTW, v2 is already inconsistent.
> > > If you wanted GENMASK_INPUT_CHECK() to return 'unsigned long',,
> > > you would have to cast (low) > (high) as well:
> > >
> > > (unsigned long)((low) > (high)), UL(0))))
> > >
> > > This is totally redundant, and weird.
> >
> > I take back this comment.
> > You added (unsigned long) to the beginning of this macro.
> > So, the type is consistent, but I believe all casts should be removed.
>
> Maybe you're right. BUILD_BUG_ON_ZERO returns size_t regardless of
> inputs. I was worried that on some platform, size_t would be larger than
> unsigned long (as far as I could see, the standard does not give any
> guarantees), and thus all of a sudden GENMASK would be 8 bytes instead
> of 4, but perhaps that is not a problem?
How about adding (int) cast to BUILD_BUG_ON_ZERO() ?
--
Best Regards
Masahiro Yamada
On Sun, Aug 04, 2019 at 03:45:16PM +0900, Masahiro Yamada wrote:
> On Sun, Aug 4, 2019 at 3:36 AM Rikard Falkeborn
> <[email protected]> wrote:
> >
> > On Sat, Aug 03, 2019 at 12:12:46PM +0900, Masahiro Yamada wrote:
> > > On Sat, Aug 3, 2019 at 12:03 PM Masahiro Yamada
> > > <[email protected]> wrote:
> > >
> > > >
> > > > BTW, v2 is already inconsistent.
> > > > If you wanted GENMASK_INPUT_CHECK() to return 'unsigned long',,
> > > > you would have to cast (low) > (high) as well:
> > > >
> > > > (unsigned long)((low) > (high)), UL(0))))
> > > >
> > > > This is totally redundant, and weird.
> > >
> > > I take back this comment.
> > > You added (unsigned long) to the beginning of this macro.
> > > So, the type is consistent, but I believe all casts should be removed.
> >
> > Maybe you're right. BUILD_BUG_ON_ZERO returns size_t regardless of
> > inputs. I was worried that on some platform, size_t would be larger than
> > unsigned long (as far as I could see, the standard does not give any
> > guarantees), and thus all of a sudden GENMASK would be 8 bytes instead
> > of 4, but perhaps that is not a problem?
>
>
> How about adding (int) cast to BUILD_BUG_ON_ZERO() ?
I'll have a look.
Hi Rikard,
On Tue, Aug 6, 2019 at 4:55 AM Rikard Falkeborn
<[email protected]> wrote:
>
> On Sun, Aug 04, 2019 at 03:45:16PM +0900, Masahiro Yamada wrote:
> > On Sun, Aug 4, 2019 at 3:36 AM Rikard Falkeborn
> > <[email protected]> wrote:
> > >
> > > On Sat, Aug 03, 2019 at 12:12:46PM +0900, Masahiro Yamada wrote:
> > > > On Sat, Aug 3, 2019 at 12:03 PM Masahiro Yamada
> > > > <[email protected]> wrote:
> > > >
> > > > >
> > > > > BTW, v2 is already inconsistent.
> > > > > If you wanted GENMASK_INPUT_CHECK() to return 'unsigned long',,
> > > > > you would have to cast (low) > (high) as well:
> > > > >
> > > > > (unsigned long)((low) > (high)), UL(0))))
> > > > >
> > > > > This is totally redundant, and weird.
> > > >
> > > > I take back this comment.
> > > > You added (unsigned long) to the beginning of this macro.
> > > > So, the type is consistent, but I believe all casts should be removed.
> > >
> > > Maybe you're right. BUILD_BUG_ON_ZERO returns size_t regardless of
> > > inputs. I was worried that on some platform, size_t would be larger than
> > > unsigned long (as far as I could see, the standard does not give any
> > > guarantees), and thus all of a sudden GENMASK would be 8 bytes instead
> > > of 4, but perhaps that is not a problem?
> >
> >
> > How about adding (int) cast to BUILD_BUG_ON_ZERO() ?
>
> I'll have a look.
I found a more important problem in this patch.
You used __is_constexpr(), which is defined in <linux/kernel.h>.
This header does not include <linux/kernel.h>,
so this header is not self-contained anymore.
The following test code fails to build:
#include <linux/bits.h>
unsigned long foo(unsigned long in_bits)
{
return in_bits & GENMASK(5, 3);
}
However, you cannot include <linux/kernel.h> from <linux/bits.h>.
See the log of 8bd9cb51daac89337295b6f037b0486911e1b408
This header was split out to not pull in <linux/bitops.h>
Including <linux/kernel.h> pulls in <linux/bitops.h> again.
In summary, please use __builtin_constant_p()
instead of __is_constexpr().
You can shorten __builtin_constant_p(high) && __builtin_constant_p(low)
into __builtin_constant_p((low) > (high)).
How about this?
#define GENMASK_INPUT_CHECK(high, low) \
BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
__builtin_constant_p((low) > (high)), (low) > (high), 0))
--
Best Regards
Masahiro Yamada
Hi Masahiro,
On Wed, Aug 07, 2019 at 12:19:36AM +0900, Masahiro Yamada wrote:
> Hi Rikard,
>
>
> On Tue, Aug 6, 2019 at 4:55 AM Rikard Falkeborn
> <[email protected]> wrote:
> >
> > On Sun, Aug 04, 2019 at 03:45:16PM +0900, Masahiro Yamada wrote:
> > > On Sun, Aug 4, 2019 at 3:36 AM Rikard Falkeborn
> > > <[email protected]> wrote:
> > > >
> > > > On Sat, Aug 03, 2019 at 12:12:46PM +0900, Masahiro Yamada wrote:
> > > > > On Sat, Aug 3, 2019 at 12:03 PM Masahiro Yamada
> > > > > <[email protected]> wrote:
> > > > >
> > > > > >
> > > > > > BTW, v2 is already inconsistent.
> > > > > > If you wanted GENMASK_INPUT_CHECK() to return 'unsigned long',,
> > > > > > you would have to cast (low) > (high) as well:
> > > > > >
> > > > > > (unsigned long)((low) > (high)), UL(0))))
> > > > > >
> > > > > > This is totally redundant, and weird.
> > > > >
> > > > > I take back this comment.
> > > > > You added (unsigned long) to the beginning of this macro.
> > > > > So, the type is consistent, but I believe all casts should be removed.
> > > >
> > > > Maybe you're right. BUILD_BUG_ON_ZERO returns size_t regardless of
> > > > inputs. I was worried that on some platform, size_t would be larger than
> > > > unsigned long (as far as I could see, the standard does not give any
> > > > guarantees), and thus all of a sudden GENMASK would be 8 bytes instead
> > > > of 4, but perhaps that is not a problem?
> > >
> > >
> > > How about adding (int) cast to BUILD_BUG_ON_ZERO() ?
> >
> > I'll have a look.
>
>
>
> I found a more important problem in this patch.
>
> You used __is_constexpr(), which is defined in <linux/kernel.h>.
>
> This header does not include <linux/kernel.h>,
> so this header is not self-contained anymore.
>
> The following test code fails to build:
>
> #include <linux/bits.h>
> unsigned long foo(unsigned long in_bits)
> {
> return in_bits & GENMASK(5, 3);
> }
>
>
>
> However, you cannot include <linux/kernel.h> from <linux/bits.h>.
> See the log of 8bd9cb51daac89337295b6f037b0486911e1b408
>
> This header was split out to not pull in <linux/bitops.h>
> Including <linux/kernel.h> pulls in <linux/bitops.h> again.
>
>
> In summary, please use __builtin_constant_p()
> instead of __is_constexpr().
>
>
> You can shorten __builtin_constant_p(high) && __builtin_constant_p(low)
> into __builtin_constant_p((low) > (high)).
>
>
> How about this?
>
>
> #define GENMASK_INPUT_CHECK(high, low) \
> BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> __builtin_constant_p((low) > (high)), (low) > (high), 0))
>
>
> --
> Best Regards
> Masahiro Yamada
Thanks for the feedback, your version looks much cleaner than mine. I
*think* I had a reason for using __is_constexpr() instead of
__builtin_constant_p but I'll try a full rebuild to see if something
comes up.
Best Regards,
Rikard Falkeborn
On Tue, 2019-08-06 at 21:27 +0200, Rikard Falkeborn wrote:
> On Wed, Aug 07, 2019 at 12:19:36AM +0900, Masahiro Yamada wrote:
> > How about this?
> > #define GENMASK_INPUT_CHECK(high, low) \
> > BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > __builtin_constant_p((low) > (high)), (low) > (high), 0))
> Thanks for the feedback, your version looks much cleaner than mine. I
> *think* I had a reason for using __is_constexpr() instead of
> __builtin_constant_p but I'll try a full rebuild to see if something
> comes up.
Perhaps a statement expression so high and low aren't possibly
evaluated multiple times?
#define GENMASK_INPUT_CHECK(high, low) \
({ \
typeof(high) _high = high; \
typeof(low) _low = low; \
BUILD_BUG_ON_ZERO(__builtin_constant_p(_low > _high, \
_low > _high, \
0)) \
})
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.
Guenter
On Wed, Aug 7, 2019 at 11:27 PM Guenter Roeck <[email protected]> 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.
--
Best Regards
Masahiro Yamada
On 8/7/19 7:55 AM, Masahiro Yamada wrote:
> On Wed, Aug 7, 2019 at 11:27 PM Guenter Roeck <[email protected]> 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
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 <[email protected]> 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/[email protected]/T/#u
Rikard
On Tue, Aug 06, 2019 at 02:15:54PM -0700, Joe Perches wrote:
> On Tue, 2019-08-06 at 21:27 +0200, Rikard Falkeborn wrote:
> > On Wed, Aug 07, 2019 at 12:19:36AM +0900, Masahiro Yamada wrote:
> > > How about this?
> > > #define GENMASK_INPUT_CHECK(high, low) \
> > > BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > > __builtin_constant_p((low) > (high)), (low) > (high), 0))
> > Thanks for the feedback, your version looks much cleaner than mine. I
> > *think* I had a reason for using __is_constexpr() instead of
> > __builtin_constant_p but I'll try a full rebuild to see if something
> > comes up.
>
> Perhaps a statement expression so high and low aren't possibly
> evaluated multiple times?
>
> #define GENMASK_INPUT_CHECK(high, low) \
> ({ \
> typeof(high) _high = high; \
> typeof(low) _low = low; \
> BUILD_BUG_ON_ZERO(__builtin_constant_p(_low > _high, \
> _low > _high, \
> 0)) \
> })
>
>
That doesn't work I think (even after adding __builtin_choose_expr).
Even so, high and low are not evaluated multiple times (they're not
evaluated at all in GENMASK_INPUT_CHECK, if they were, the arguments
would be evaluated twice since they're evaluated in __GENMASK as well.
__builtin_constant_p does not seem to evaluate it's expression (even
though I didn't manage to find that spelled out in the docs, but since
__builtin_constant_p is evaluated at compile time it makes sense that it
doesn't), and __builtin_choose_expr does not evaluate the operand that
is not chosen (this is actually in the docs). Even if it was,
BUIlD_BUG_ON_ZERO uses sizeof its argument, which is evaluated at compile
time (unless the argument is a VLA).
So this should be safe.
On Wed, 2019-08-07 at 23:55 +0900, Masahiro Yamada wrote:
> On Wed, Aug 7, 2019 at 11:27 PM Guenter Roeck <[email protected]> wrote:
[]
> > 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.
I think it makes complete sense to break -next (not mainline)
and force
people to fix defects. Especially these types of
defects that are
trivial to fix.
I already sent patches a month ago for all decimal only
defective uses of GENMASK
https://lore.kernel.org/lkml/[email protected]/
A couple of which have _still_ not been picked up.
These have been applied in -next:
1 9e037bdf743cc081858423ad4123824e846b2358 media: staging: media: cedrus: Fix misuse of GENMASK macro
2 5ff29d836d1beb347080bd96e6321c811a8e3f62 rtw88: Fix misuse of GENMASK macro
3 665e985c2f41bebc3e6cee7e04c36a44afbc58f7 mmc: meson-mx-sdio: Fix misuse of GENMASK macro
4 f7408a3d5b5fd10571a653d1a81ce9167c62727f ASoC: wcd9335: Fix misuse of GENMASK macro
5 ae8cc91a7d85e018c0c267f580820b2bb558cd48 iio: adc: max9611: Fix misuse of GENMASK macro
6 aa4c0c9091b0bb4cb261bbe0718d17c2834c4690 net: stmmac: Fix misuses of GENMASK macro
7 937a944090cca2f19458fd037a8aff61c546f0cd net: ethernet: mediatek: Fix misuses of GENMASK macro
8 9bdd7bb3a8447fe841cd37ddd9e0a6974b06a0bb clocksource/drivers/npcm: Fix misuse of GENMASK macro
9 20faba848752901de23a4d45a1174d64d2069dde irqchip/gic-v3-its: Fix misuse of GENMASK macro
These have not:
(and that's a rather sad indictment of lk process defects)
[PATCH 03/12] drm: aspeed_gfx: Fix misuse of GENMASK macro
https://lore.kernel.org/lkml/cddd7ad7e9f81dec1e86c106f04229d21fc21920.1562734889.git.joe@perches.com/
[PATCH 10/12] phy: amlogic: G12A: Fix misuse of GENMASK macro
https://lore.kernel.org/lkml/d149d2851f9aa2425c927cb8e311e20c4b83e186.1562734889.git.joe@perches.com/
At least these last two do not seem to have actual uses.
On 8/7/19 5:07 PM, Joe Perches wrote:
> On Wed, 2019-08-07 at 23:55 +0900, Masahiro Yamada wrote:
>> On Wed, Aug 7, 2019 at 11:27 PM Guenter Roeck <[email protected]> wrote:
> []
>>> 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.
>
> I think it makes complete sense to break -next (not mainline)
> and force
> people to fix defects. Especially these types of
> defects that are
> trivial to fix.
>
I don't think this (from next-20190807):
Build results:
total: 158 pass: 137 fail: 21
Qemu test results:
total: 391 pass: 318 fail: 73
is very useful. The situation is bad enough for newly introduced problems.
It is all but impossible to get fixes for all problems discovered (or introduced)
by adding checks like this one. In some cases, no one will care. In others,
no one will pick up patches. Sometimes people won't know or realize that
they are expected to fix something. Making the situation worse, the failures
introduced by the new checks will hide other accumulating problems.
arch/sh has failed to build in mainline since 7/27 and in -next since
next-20190711, due to the added "fallthrough" warning. I don't think
that is too useful either. Ok, that situation may be a sign that the
architecture isn't maintained as well as it should, but I don't think
that this warrants breaking it on purpose in the hope to trigger
some kind of reaction.
I don't mind if new checks are introduced, and I agree that it is useful
and makes sense. But the checks should only be introduced after a reasonable
attempt was made to fix _all_ associated problems. That doesn't mean that
the entire work has to be done by the person introducing the check, but I
do see that person responsible for making sure (or a reasonable definition
of "make sure") that all problems are fixed before actually introducing
the check. Yes, I understand, this is a lot of work, but adding checks
and letting all hell break loose can not be the answer.
Guenter
On Wed, 2019-08-07 at 17:58 -0700, Guenter Roeck wrote:
> On 8/7/19 5:07 PM, Joe Perches wrote:
> > On Wed, 2019-08-07 at 23:55 +0900, Masahiro Yamada wrote:
> > > On Wed, Aug 7, 2019 at 11:27 PM Guenter Roeck <[email protected]> wrote:
> > []
> > > > 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.
> >
> > I think it makes complete sense to break -next (not mainline)
> > and force people to fix defects. Especially these types of
> > defects that are trivial to fix.
> >
>
> I don't think this (from next-20190807):
>
> Build results:
> total: 158 pass: 137 fail: 21
> Qemu test results:
> total: 391 pass: 318 fail: 73
>
> is very useful. The situation is bad enough for newly introduced problems.
> It is all but impossible to get fixes for all problems discovered (or introduced)
> by adding checks like this one. In some cases, no one will care. In others,
> no one will pick up patches. Sometimes people won't know or realize that
> they are expected to fix something. Making the situation worse, the failures
> introduced by the new checks will hide other accumulating problems.
>
> arch/sh has failed to build in mainline since 7/27 and in -next since
> next-20190711, due to the added "fallthrough" warning. I don't think
> that is too useful either. Ok, that situation may be a sign that the
> architecture isn't maintained as well as it should, but I don't think
> that this warrants breaking it on purpose in the hope to trigger
> some kind of reaction.
>
> I don't mind if new checks are introduced, and I agree that it is useful
> and makes sense. But the checks should only be introduced after a reasonable
> attempt was made to fix _all_ associated problems. That doesn't mean that
> the entire work has to be done by the person introducing the check, but I
> do see that person responsible for making sure (or a reasonable definition
> of "make sure") that all problems are fixed before actually introducing
> the check. Yes, I understand, this is a lot of work, but adding checks
> and letting all hell break loose can not be the answer.
No hell is unleashed.
It's -next, an integration build, not mainline.
On 8/7/19 6:08 PM, Joe Perches wrote:
> On Wed, 2019-08-07 at 17:58 -0700, Guenter Roeck wrote:
>> On 8/7/19 5:07 PM, Joe Perches wrote:
>>> On Wed, 2019-08-07 at 23:55 +0900, Masahiro Yamada wrote:
>>>> On Wed, Aug 7, 2019 at 11:27 PM Guenter Roeck <[email protected]> wrote:
>>> []
>>>>> 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.
>>>
>>> I think it makes complete sense to break -next (not mainline)
>>> and force people to fix defects. Especially these types of
>>> defects that are trivial to fix.
>>>
>>
>> I don't think this (from next-20190807):
>>
>> Build results:
>> total: 158 pass: 137 fail: 21
>> Qemu test results:
>> total: 391 pass: 318 fail: 73
>>
>> is very useful. The situation is bad enough for newly introduced problems.
>> It is all but impossible to get fixes for all problems discovered (or introduced)
>> by adding checks like this one. In some cases, no one will care. In others,
>> no one will pick up patches. Sometimes people won't know or realize that
>> they are expected to fix something. Making the situation worse, the failures
>> introduced by the new checks will hide other accumulating problems.
>>
>> arch/sh has failed to build in mainline since 7/27 and in -next since
>> next-20190711, due to the added "fallthrough" warning. I don't think
>> that is too useful either. Ok, that situation may be a sign that the
>> architecture isn't maintained as well as it should, but I don't think
>> that this warrants breaking it on purpose in the hope to trigger
>> some kind of reaction.
>>
>> I don't mind if new checks are introduced, and I agree that it is useful
>> and makes sense. But the checks should only be introduced after a reasonable
>> attempt was made to fix _all_ associated problems. That doesn't mean that
>> the entire work has to be done by the person introducing the check, but I
>> do see that person responsible for making sure (or a reasonable definition
>> of "make sure") that all problems are fixed before actually introducing
>> the check. Yes, I understand, this is a lot of work, but adding checks
>> and letting all hell break loose can not be the answer.
>
> No hell is unleashed.
>
> It's -next, an integration build, not mainline.
>
... and the breakages introduced in -next are making it into mainline
without being fixed, as I just pointed out above. That by itself is bad.
It is much worse if the breakage is introduced on purpose.
The criteria for -next _used_ to be "ready for mainline". If breaking -next
on purpose is the new normal, no one should be surprised if it will be tested
even less than it is today.
Guenter
Hi Rikard,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc3 next-20190807]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Rikard-Falkeborn/linux-bits-h-Clarify-macro-argument-names/20190805-024030
config: i386-defconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-10) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All warnings (new ones prefixed by >>):
In file included from include/linux/bits.h:22:0,
from arch/x86/include/asm/msr-index.h:5,
from arch/x86/boot/cpucheck.c:28:
>> include/linux/build_bug.h:49:0: warning: "BUILD_BUG_ON" redefined
#define BUILD_BUG_ON(condition) \
In file included from arch/x86/boot/cpucheck.c:22:0:
arch/x86/boot/boot.h:31:0: note: this is the location of the previous definition
#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
vim +/BUILD_BUG_ON +49 include/linux/build_bug.h
bc6245e5efd70c Ian Abbott 2017-07-10 40
bc6245e5efd70c Ian Abbott 2017-07-10 41 /**
bc6245e5efd70c Ian Abbott 2017-07-10 42 * BUILD_BUG_ON - break compile if a condition is true.
bc6245e5efd70c Ian Abbott 2017-07-10 43 * @condition: the condition which the compiler should know is false.
bc6245e5efd70c Ian Abbott 2017-07-10 44 *
bc6245e5efd70c Ian Abbott 2017-07-10 45 * If you have some code which relies on certain constants being equal, or
bc6245e5efd70c Ian Abbott 2017-07-10 46 * some other compile-time-evaluated condition, you should use BUILD_BUG_ON to
bc6245e5efd70c Ian Abbott 2017-07-10 47 * detect if someone changes it.
bc6245e5efd70c Ian Abbott 2017-07-10 48 */
bc6245e5efd70c Ian Abbott 2017-07-10 @49 #define BUILD_BUG_ON(condition) \
bc6245e5efd70c Ian Abbott 2017-07-10 50 BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
bc6245e5efd70c Ian Abbott 2017-07-10 51
:::::: The code at line 49 was first introduced by commit
:::::: bc6245e5efd70c41eaf9334b1b5e646745cb0fb3 bug: split BUILD_BUG stuff out into <linux/build_bug.h>
:::::: TO: Ian Abbott <[email protected]>
:::::: CC: Linus Torvalds <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Rikard,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc3 next-20190807]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Rikard-Falkeborn/linux-bits-h-Clarify-macro-argument-names/20190805-024030
config: x86_64-randconfig-h003-201931 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-10) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All warnings (new ones prefixed by >>):
In file included from include/linux/bits.h:22:0,
from include/linux/bitops.h:5,
from include/linux/kernel.h:12,
from include/linux/delay.h:22,
from drivers/iio/adc/max9611.c:20:
drivers/iio/adc/max9611.c: In function 'max9611_init':
include/linux/build_bug.h:16:45: error: negative width in bit-field '<anonymous>'
#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:(-!!(e)); }))
^
>> include/linux/bits.h:24:18: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
((unsigned long)BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
^~~~~~~~~~~~~~~~~
>> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(high, low) + __GENMASK(high, low))
^~~~~~~~~~~~~~~~~~~
>> drivers/iio/adc/max9611.c:86:28: note: in expansion of macro 'GENMASK'
#define MAX9611_TEMP_MASK GENMASK(7, 15)
^~~~~~~
>> drivers/iio/adc/max9611.c:483:17: note: in expansion of macro 'MAX9611_TEMP_MASK'
regval = ret & MAX9611_TEMP_MASK;
^~~~~~~~~~~~~~~~~
--
In file included from include/linux/bits.h:22:0,
from include/linux/bitops.h:5,
from include/linux/kernel.h:12,
from include/linux/delay.h:22,
from drivers/iio//adc/max9611.c:20:
drivers/iio//adc/max9611.c: In function 'max9611_init':
include/linux/build_bug.h:16:45: error: negative width in bit-field '<anonymous>'
#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:(-!!(e)); }))
^
>> include/linux/bits.h:24:18: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
((unsigned long)BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
^~~~~~~~~~~~~~~~~
>> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(high, low) + __GENMASK(high, low))
^~~~~~~~~~~~~~~~~~~
drivers/iio//adc/max9611.c:86:28: note: in expansion of macro 'GENMASK'
#define MAX9611_TEMP_MASK GENMASK(7, 15)
^~~~~~~
drivers/iio//adc/max9611.c:483:17: note: in expansion of macro 'MAX9611_TEMP_MASK'
regval = ret & MAX9611_TEMP_MASK;
^~~~~~~~~~~~~~~~~
vim +/BUILD_BUG_ON_ZERO +24 include/linux/bits.h
15
16 /*
17 * Create a contiguous bitmask starting at bit position @low and ending at
18 * position @high. For example
19 * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
20 */
21 #ifndef __ASSEMBLY__
> 22 #include <linux/build_bug.h>
23 #define GENMASK_INPUT_CHECK(high, low) \
> 24 ((unsigned long)BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
25 __is_constexpr(high) && __is_constexpr(low), \
26 (low) > (high), UL(0))))
27 #else
28 /*
29 * BUILD_BUG_ON_ZERO and __is_constexpr() are not available in h files
30 * included from asm files, disable the input check if that is the case.
31 */
32 #define GENMASK_INPUT_CHECK(high, low) UL(0)
33 #endif
34
35 #define __GENMASK(high, low) \
36 (((~UL(0)) - (UL(1) << (low)) + 1) & \
37 (~UL(0) >> (BITS_PER_LONG - 1 - (high))))
38 #define GENMASK(high, low) \
> 39 (GENMASK_INPUT_CHECK(high, low) + __GENMASK(high, low))
40
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Fri, Aug 2, 2019 at 8:04 AM Rikard Falkeborn
<[email protected]> wrote:
>
> Be a little more verbose to improve readability.
>
> Signed-off-by: Rikard Falkeborn <[email protected]>
BTW, I do not understand what the improvement is.
I tend to regard this as a noise commit.
> ---
> Changes in v2:
> - This patch is new in v2
>
> include/linux/bits.h | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 669d69441a62..d4466aa42a9c 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -14,16 +14,16 @@
> #define BITS_PER_BYTE 8
>
> /*
> - * Create a contiguous bitmask starting at bit position @l and ending at
> - * position @h. For example
> + * Create a contiguous bitmask starting at bit position @low and ending at
> + * position @high. For example
> * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> */
> -#define GENMASK(h, l) \
> - (((~UL(0)) - (UL(1) << (l)) + 1) & \
> - (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> +#define GENMASK(high, low) \
> + (((~UL(0)) - (UL(1) << (low)) + 1) & \
> + (~UL(0) >> (BITS_PER_LONG - 1 - (high))))
>
> -#define GENMASK_ULL(h, l) \
> - (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> - (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
> +#define GENMASK_ULL(high, low) \
> + (((~ULL(0)) - (ULL(1) << (low)) + 1) & \
> + (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (high))))
>
> #endif /* __LINUX_BITS_H */
> --
> 2.22.0
>
--
Best Regards
Masahiro Yamada
On Thu, Aug 08, 2019 at 12:46:45PM +0900, Masahiro Yamada wrote:
> On Fri, Aug 2, 2019 at 8:04 AM Rikard Falkeborn
> <[email protected]> wrote:
> >
> > Be a little more verbose to improve readability.
> >
> > Signed-off-by: Rikard Falkeborn <[email protected]>
>
> BTW, I do not understand what the improvement is.
> I tend to regard this as a noise commit.
Fair enough, I'll drop it and keep l and h as argument names.
On Thu, 2019-08-08 at 12:46 +0900, Masahiro Yamada wrote:
> On Fri, Aug 2, 2019 at 8:04 AM Rikard Falkeborn
> <[email protected]> wrote:
> > Be a little more verbose to improve readability.
> >
> > Signed-off-by: Rikard Falkeborn <[email protected]>
>
> BTW, I do not understand what the improvement is.
> I tend to regard this as a noise commit.
Non verbose naming clarity is good.
Perhaps adding kernel-doc is good too.
>
> > ---
> > Changes in v2:
> > - This patch is new in v2
> >
> > include/linux/bits.h | 16 ++++++++--------
> > 1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/linux/bits.h b/include/linux/bits.h
> > index 669d69441a62..d4466aa42a9c 100644
> > --- a/include/linux/bits.h
> > +++ b/include/linux/bits.h
> > @@ -14,16 +14,16 @@
> > #define BITS_PER_BYTE 8
> >
> > /*
> > - * Create a contiguous bitmask starting at bit position @l and ending at
> > - * position @h. For example
> > + * Create a contiguous bitmask starting at bit position @low and ending at
> > + * position @high. For example
> > * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> > */
> > -#define GENMASK(h, l) \
> > - (((~UL(0)) - (UL(1) << (l)) + 1) & \
> > - (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> > +#define GENMASK(high, low) \
> > + (((~UL(0)) - (UL(1) << (low)) + 1) & \
> > + (~UL(0) >> (BITS_PER_LONG - 1 - (high))))
> >
> > -#define GENMASK_ULL(h, l) \
> > - (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> > - (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
> > +#define GENMASK_ULL(high, low) \
> > + (((~ULL(0)) - (ULL(1) << (low)) + 1) & \
> > + (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (high))))
> >
> > #endif /* __LINUX_BITS_H */
> > --
> > 2.22.0
> >
>
>
Hello,
A new attempt to try to add build time validity checks of GENMASK (and
GENMASK_ULL) inputs. There main differences from v2:
Remove a define of BUILD_BUG_ON in x86/boot to avoid a compiler warning
about redefining BUILD_BUG_ON. Instead, use the common one from
include/.
Drop patch 2 in v2 where GENMASK arguments where made more verbose.
Add a cast in the BUILD_BUG_ON_ZERO macro change the type to int to
avoid the somewhat clumpsy casts of BUILD_BUG_ON_ZERO. The second patch
in this series adds such a cast to BUILD_BUG_ON_ZERO, which makes it
possible to avoid casts when using BUILD_BUG_ON_ZERO in patch 3.
I have checked all users of BUILD_BUG_ON_ZERO and I did not find a case
where adding a cast to int would affect existing users but I'd feel much
more comfortable if someone else double (or tripple) checked (there are
~80 instances plus ~10 copies in tools). Perhaps I should have CC:d
maintainers of files using BUILD_BUG_ON_ZERO?
Finally, use __builtin_constant_p instead of __is_constexpr. This avoids
pulling in kernel.h in bits.h.
Joe Perches sent a patch series to fix existing misuses, currently there
are five such misuses (which patches pending) left in Linus tree and two
(with patches pending) in linux-next. Those patches should fix all
"simple" misuses of GENMASK (cases where the arguments are numerical
constants). Pushing v2 to linux-next also revealed an arm-specific
misuse where GENMASK was used in another macro (and also broke the
arm-builds). There is a patch to fix that by Nathan Chancellor (not in
linux-next yet). Those patches should be merged before the last patch of
this series to avoid breaking builds.
Changelog
Since v2
- Use __builtin_constant_p instead of __is_constexpr to avoid pulling
in kernel.h (that include was missing in v2, so the header was no
longer builable standalone
- add cast to BUILD_BUG_ON_ZERO to make the type int
- Remove unnecessary casts due to the above
- Drop patch that renamed macro arguments
Since v1
- Add comment about why inputs are not checked when used in asm file
- Use UL(0) instead of 0
- Extract mask creation in a separate macro to improve readability
- Use high and low instead of h and l (part of this was extracted to a
separate patch)
- Updated commit message
Rikard Falkeborn (3):
x86/boot: Use common BUILD_BUG_ON
linux/build_bug.h: Change type to int
linux/bits.h: Add compile time sanity check of GENMASK inputs
arch/x86/boot/boot.h | 2 --
arch/x86/boot/main.c | 1 +
include/linux/bits.h | 21 +++++++++++++++++++--
include/linux/build_bug.h | 4 ++--
4 files changed, 22 insertions(+), 6 deletions(-)
--
2.22.0
Defining BUILD_BUG_ON causes redefinition warnings when adding includes
of include/linux/build_bug.h in files unrelated to x86/boot.
For example, adding an include of build_bug.h to include/linux/bits.h
shows the following warnings:
CC arch/x86/boot/cpucheck.o
In file included from ./include/linux/bits.h:22,
from ./arch/x86/include/asm/msr-index.h:5,
from arch/x86/boot/cpucheck.c:28:
./include/linux/build_bug.h:49: warning: "BUILD_BUG_ON" redefined
49 | #define BUILD_BUG_ON(condition) \
|
In file included from arch/x86/boot/cpucheck.c:22:
arch/x86/boot/boot.h:31: note: this is the location of the previous definition
31 | #define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
|
The macro was added to boot.h in commit 62bd0337d0c4 ("Top header file
for new x86 setup code"). At that time, BUILD_BUG_ON was defined in
kernel.h. Presumably BUILD_BUG_ON was redefined to avoid pulling in
kernel.h. Since then, BUILD_BUG_ON and similar macros have been split to
a separate header file.
Signed-off-by: Rikard Falkeborn <[email protected]>
---
arch/x86/boot/boot.h | 2 --
arch/x86/boot/main.c | 1 +
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index 19eca14b49a0..ca866f1cca2e 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -28,8 +28,6 @@
#include "cpuflags.h"
/* Useful macros */
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
-
#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
extern struct setup_header hdr;
diff --git a/arch/x86/boot/main.c b/arch/x86/boot/main.c
index 996df3d586f0..c5e55d2c55d0 100644
--- a/arch/x86/boot/main.c
+++ b/arch/x86/boot/main.c
@@ -13,6 +13,7 @@
#include "boot.h"
#include "string.h"
+#include <linux/build_bug.h>
struct boot_params boot_params __attribute__((aligned(16)));
--
2.22.0
Having BUILD_BUG_ON_ZERO produce a value of type size_t leads to awkward
casts in cases where the result needs to be signed, or of smaller type
than size_t. To avoid this, cast the value to int instead and rely on
implicit type conversions when a larger or unsigned type is needed.
Suggested-by: Masahiro Yamada <[email protected]>
Signed-off-by: Rikard Falkeborn <[email protected]>
---
Changes in v3:
- This patch is new in v3
include/linux/build_bug.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
index 0fe5426f2bdc..e3a0be2c90ad 100644
--- a/include/linux/build_bug.h
+++ b/include/linux/build_bug.h
@@ -9,11 +9,11 @@
#else /* __CHECKER__ */
/*
* Force a compilation error if condition is true, but also produce a
- * result (of value 0 and type size_t), so the expression can be used
+ * result (of value 0 and type int), so the expression can be used
* e.g. in a structure initializer (or where-ever else comma expressions
* aren't permitted).
*/
-#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:(-!!(e)); }))
+#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
#endif /* __CHECKER__ */
/* Force a compilation error if a constant expression is not a power of 2 */
--
2.22.0
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
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().
__builtin_constant_p does not evaluate is argument, it only checks if it
is a constant or not at compile time, and __builtin_choose_expr does not
evaluate the expression that is not chosen. Therefore, GENMASK(x++, 0)
does only evaluate x++ once.
Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends
available in assembly") made the macros in linux/bits.h available in
assembly. Since BUILD_BUG_OR_ZERO() is not asm compatible, disable the
checks if the file is included in an asm file.
Signed-off-by: Rikard Falkeborn <[email protected]>
---
Changes in v3:
- Changed back to shorter macro argument names
- Remove casts and use 0 instead of UL(0) in GENMASK_INPUT_CHECK(),
since all results in GENMASK_INPUT_CHECK() are now ints. Update
commit message to reflect that.
Changes in v2:
- Add comment about why inputs are not checked when used in asm file
- Use UL(0) instead of 0
- Extract mask creation in a separate macro to improve readability
- Use high and low instead of h and l (part of this was extracted to a
separate patch)
- Updated commit message
Joe Perches sent a series to fix the existing misuses of GENMASK() that
needs to be merged before this to avoid build failures. Currently, 5 of
the patches are not in Linus tree, and 2 are not in linux-next. There is
also a patch pending by Nathan Chancellor that also needs to be merged
before this patch is merged to avoid build failures.
include/linux/bits.h | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/include/linux/bits.h b/include/linux/bits.h
index 669d69441a62..4ba0fb609239 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -18,12 +18,29 @@
* position @h. For example
* GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
*/
-#define GENMASK(h, l) \
+#ifndef __ASSEMBLY__
+#include <linux/build_bug.h>
+#define GENMASK_INPUT_CHECK(h, l) \
+ (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
+ __builtin_constant_p((l) > (h)), (l) > (h), 0)))
+#else
+/*
+ * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
+ * disable the input check if that is the case.
+ */
+#define GENMASK_INPUT_CHECK(h, l) 0
+#endif
+
+#define __GENMASK(h, l) \
(((~UL(0)) - (UL(1) << (l)) + 1) & \
(~UL(0) >> (BITS_PER_LONG - 1 - (h))))
+#define GENMASK(h, l) \
+ (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
-#define GENMASK_ULL(h, l) \
+#define __GENMASK_ULL(h, l) \
(((~ULL(0)) - (ULL(1) << (l)) + 1) & \
(~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
+#define GENMASK_ULL(h, l) \
+ (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
#endif /* __LINUX_BITS_H */
--
2.22.0
On Sun, Aug 11, 2019 at 08:49:35PM +0200, Rikard Falkeborn wrote:
> Hello,
>
> A new attempt to try to add build time validity checks of GENMASK (and
> GENMASK_ULL) inputs. There main differences from v2:
>
> Remove a define of BUILD_BUG_ON in x86/boot to avoid a compiler warning
> about redefining BUILD_BUG_ON. Instead, use the common one from
> include/.
>
> Drop patch 2 in v2 where GENMASK arguments where made more verbose.
>
> Add a cast in the BUILD_BUG_ON_ZERO macro change the type to int to
> avoid the somewhat clumpsy casts of BUILD_BUG_ON_ZERO. The second patch
> in this series adds such a cast to BUILD_BUG_ON_ZERO, which makes it
> possible to avoid casts when using BUILD_BUG_ON_ZERO in patch 3.
>
> I have checked all users of BUILD_BUG_ON_ZERO and I did not find a case
> where adding a cast to int would affect existing users but I'd feel much
> more comfortable if someone else double (or tripple) checked (there are
> ~80 instances plus ~10 copies in tools). Perhaps I should have CC:d
> maintainers of files using BUILD_BUG_ON_ZERO?
>
> Finally, use __builtin_constant_p instead of __is_constexpr. This avoids
> pulling in kernel.h in bits.h.
Cool; I like this. I spent some time convincing myself that the
side-effects really aren't double-evaluated, and it looks fine to me. :)
For the series:
Reviewed-by: Kees Cook <[email protected]>
-Kees
Commit-ID: f6fabe25f01cb09db51644780193f5ea6c44e04e
Gitweb: https://git.kernel.org/tip/f6fabe25f01cb09db51644780193f5ea6c44e04e
Author: Rikard Falkeborn <[email protected]>
AuthorDate: Sun, 11 Aug 2019 20:49:36 +0200
Committer: Thomas Gleixner <[email protected]>
CommitDate: Mon, 12 Aug 2019 20:13:56 +0200
x86/boot: Use common BUILD_BUG_ON
Defining BUILD_BUG_ON causes redefinition warnings when adding includes of
include/linux/build_bug.h in files unrelated to x86/boot. For example,
adding an include of build_bug.h to include/linux/bits.h shows the
following warnings:
CC arch/x86/boot/cpucheck.o
In file included from ./include/linux/bits.h:22,
from ./arch/x86/include/asm/msr-index.h:5,
from arch/x86/boot/cpucheck.c:28:
./include/linux/build_bug.h:49: warning: "BUILD_BUG_ON" redefined
49 | #define BUILD_BUG_ON(condition) \
|
In file included from arch/x86/boot/cpucheck.c:22:
arch/x86/boot/boot.h:31: note: this is the location of the previous definition
31 | #define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
|
The macro was added to boot.h in commit 62bd0337d0c4 ("Top header file for
new x86 setup code"). At that time, BUILD_BUG_ON was defined in
kernel.h. Presumably BUILD_BUG_ON was redefined to avoid pulling in
kernel.h. Since then, BUILD_BUG_ON and similar macros have been split to a
separate header file.
Signed-off-by: Rikard Falkeborn <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/boot/boot.h | 2 --
arch/x86/boot/main.c | 1 +
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index 19eca14b49a0..ca866f1cca2e 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -28,8 +28,6 @@
#include "cpuflags.h"
/* Useful macros */
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
-
#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
extern struct setup_header hdr;
diff --git a/arch/x86/boot/main.c b/arch/x86/boot/main.c
index 996df3d586f0..e3add857c2c9 100644
--- a/arch/x86/boot/main.c
+++ b/arch/x86/boot/main.c
@@ -10,6 +10,7 @@
/*
* Main module for the real-mode kernel code
*/
+#include <linux/build_bug.h>
#include "boot.h"
#include "string.h"
Commit-ID: d5a1baddf1585885868cbab55989401fb97118c6
Gitweb: https://git.kernel.org/tip/d5a1baddf1585885868cbab55989401fb97118c6
Author: Rikard Falkeborn <[email protected]>
AuthorDate: Sun, 11 Aug 2019 20:49:36 +0200
Committer: Thomas Gleixner <[email protected]>
CommitDate: Fri, 16 Aug 2019 14:15:50 +0200
x86/boot: Use common BUILD_BUG_ON
Defining BUILD_BUG_ON causes redefinition warnings when adding includes of
include/linux/build_bug.h in files unrelated to x86/boot. For example,
adding an include of build_bug.h to include/linux/bits.h shows the
following warnings:
CC arch/x86/boot/cpucheck.o
In file included from ./include/linux/bits.h:22,
from ./arch/x86/include/asm/msr-index.h:5,
from arch/x86/boot/cpucheck.c:28:
./include/linux/build_bug.h:49: warning: "BUILD_BUG_ON" redefined
49 | #define BUILD_BUG_ON(condition) \
|
In file included from arch/x86/boot/cpucheck.c:22:
arch/x86/boot/boot.h:31: note: this is the location of the previous definition
31 | #define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
|
The macro was added to boot.h in commit 62bd0337d0c4 ("Top header file for
new x86 setup code"). At that time, BUILD_BUG_ON was defined in
kernel.h. Presumably BUILD_BUG_ON was redefined to avoid pulling in
kernel.h. Since then, BUILD_BUG_ON and similar macros have been split to a
separate header file.
Signed-off-by: Rikard Falkeborn <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/boot/boot.h | 2 --
arch/x86/boot/main.c | 1 +
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index 19eca14b49a0..ca866f1cca2e 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -28,8 +28,6 @@
#include "cpuflags.h"
/* Useful macros */
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
-
#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
extern struct setup_header hdr;
diff --git a/arch/x86/boot/main.c b/arch/x86/boot/main.c
index 996df3d586f0..e3add857c2c9 100644
--- a/arch/x86/boot/main.c
+++ b/arch/x86/boot/main.c
@@ -10,6 +10,7 @@
/*
* Main module for the real-mode kernel code
*/
+#include <linux/build_bug.h>
#include "boot.h"
#include "string.h"
On Sun, Aug 11, 2019 at 08:49:35PM +0200, Rikard Falkeborn wrote:
> Hello,
>
> A new attempt to try to add build time validity checks of GENMASK (and
> GENMASK_ULL) inputs. There main differences from v2:
>
> Remove a define of BUILD_BUG_ON in x86/boot to avoid a compiler warning
> about redefining BUILD_BUG_ON. Instead, use the common one from
> include/.
>
> Drop patch 2 in v2 where GENMASK arguments where made more verbose.
>
> Add a cast in the BUILD_BUG_ON_ZERO macro change the type to int to
> avoid the somewhat clumpsy casts of BUILD_BUG_ON_ZERO. The second patch
> in this series adds such a cast to BUILD_BUG_ON_ZERO, which makes it
> possible to avoid casts when using BUILD_BUG_ON_ZERO in patch 3.
>
> I have checked all users of BUILD_BUG_ON_ZERO and I did not find a case
> where adding a cast to int would affect existing users but I'd feel much
> more comfortable if someone else double (or tripple) checked (there are
> ~80 instances plus ~10 copies in tools). Perhaps I should have CC:d
> maintainers of files using BUILD_BUG_ON_ZERO?
>
> Finally, use __builtin_constant_p instead of __is_constexpr. This avoids
> pulling in kernel.h in bits.h.
>
> Joe Perches sent a patch series to fix existing misuses, currently there
> are five such misuses (which patches pending) left in Linus tree and two
> (with patches pending) in linux-next. Those patches should fix all
> "simple" misuses of GENMASK (cases where the arguments are numerical
> constants). Pushing v2 to linux-next also revealed an arm-specific
> misuse where GENMASK was used in another macro (and also broke the
> arm-builds). There is a patch to fix that by Nathan Chancellor (not in
> linux-next yet). Those patches should be merged before the last patch of
> this series to avoid breaking builds.
>
Ping.
The current status is that patch 1 has been merged into Linus tree
through the x86 tree. The patches mentioned about actually fixing the
existing misuses have all been merged except two of Joe Perches patches
[0], [1], but those patches fixes misuse in unused macros, and will not
affect anyones build.
I have testbuilt the two remaining patches rebased on top of Linus tree
and on linux-next for aarch64 and x86 without seeing any problems (when
v2 of the series went into linux-next, those were the only archs where
any problems were spotted (build warning on x86, build error on
aarch64)).
[0] https://lore.kernel.org/lkml/cddd7ad7e9f81dec1e86c106f04229d21fc21920.1562734889.git.joe@perches.com/
[1] https://lore.kernel.org/lkml/d149d2851f9aa2425c927cb8e311e20c4b83e186.1562734889.git.joe@perches.com/
Rikard
On Mon, Aug 12, 2019 at 3:50 AM Rikard Falkeborn
<[email protected]> wrote:
>
> Having BUILD_BUG_ON_ZERO produce a value of type size_t leads to awkward
> casts in cases where the result needs to be signed, or of smaller type
> than size_t. To avoid this, cast the value to int instead and rely on
> implicit type conversions when a larger or unsigned type is needed.
>
> Suggested-by: Masahiro Yamada <[email protected]>
> Signed-off-by: Rikard Falkeborn <[email protected]>
Reviewed-by: Masahiro Yamada <[email protected]>
> Changes in v3:
> - This patch is new in v3
>
> include/linux/build_bug.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
> index 0fe5426f2bdc..e3a0be2c90ad 100644
> --- a/include/linux/build_bug.h
> +++ b/include/linux/build_bug.h
> @@ -9,11 +9,11 @@
> #else /* __CHECKER__ */
> /*
> * Force a compilation error if condition is true, but also produce a
> - * result (of value 0 and type size_t), so the expression can be used
> + * result (of value 0 and type int), so the expression can be used
> * e.g. in a structure initializer (or where-ever else comma expressions
> * aren't permitted).
> */
> -#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:(-!!(e)); }))
> +#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> #endif /* __CHECKER__ */
>
> /* Force a compilation error if a constant expression is not a power of 2 */
> --
> 2.22.0
>
--
Best Regards
Masahiro Yamada
On Mon, Aug 12, 2019 at 3:50 AM Rikard Falkeborn
<[email protected]> 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
> 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().
>
> __builtin_constant_p does not evaluate is argument, it only checks if it
> is a constant or not at compile time, and __builtin_choose_expr does not
> evaluate the expression that is not chosen. Therefore, GENMASK(x++, 0)
> does only evaluate x++ once.
>
> Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends
> available in assembly") made the macros in linux/bits.h available in
> assembly. Since BUILD_BUG_OR_ZERO() is not asm compatible, disable the
> checks if the file is included in an asm file.
>
> Signed-off-by: Rikard Falkeborn <[email protected]>
Reviewed-by: Masahiro Yamada <[email protected]>
> ---
> Changes in v3:
> - Changed back to shorter macro argument names
> - Remove casts and use 0 instead of UL(0) in GENMASK_INPUT_CHECK(),
> since all results in GENMASK_INPUT_CHECK() are now ints. Update
> commit message to reflect that.
>
> Changes in v2:
> - Add comment about why inputs are not checked when used in asm file
> - Use UL(0) instead of 0
> - Extract mask creation in a separate macro to improve readability
> - Use high and low instead of h and l (part of this was extracted to a
> separate patch)
> - Updated commit message
>
> Joe Perches sent a series to fix the existing misuses of GENMASK() that
> needs to be merged before this to avoid build failures. Currently, 5 of
> the patches are not in Linus tree, and 2 are not in linux-next. There is
> also a patch pending by Nathan Chancellor that also needs to be merged
> before this patch is merged to avoid build failures.
>
> include/linux/bits.h | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 669d69441a62..4ba0fb609239 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -18,12 +18,29 @@
> * position @h. For example
> * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> */
> -#define GENMASK(h, l) \
> +#ifndef __ASSEMBLY__
> +#include <linux/build_bug.h>
> +#define GENMASK_INPUT_CHECK(h, l) \
> + (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> + __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> +#else
> +/*
> + * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> + * disable the input check if that is the case.
> + */
> +#define GENMASK_INPUT_CHECK(h, l) 0
> +#endif
> +
> +#define __GENMASK(h, l) \
> (((~UL(0)) - (UL(1) << (l)) + 1) & \
> (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> +#define GENMASK(h, l) \
> + (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>
> -#define GENMASK_ULL(h, l) \
> +#define __GENMASK_ULL(h, l) \
> (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
> +#define GENMASK_ULL(h, l) \
> + (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
>
> #endif /* __LINUX_BITS_H */
> --
> 2.22.0
>
--
Best Regards
Masahiro Yamada
Hi Rikard,
On Sun, Aug 11, 2019 at 8:52 PM Rikard Falkeborn
<[email protected]> 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
> 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().
>
> __builtin_constant_p does not evaluate is argument, it only checks if it
> is a constant or not at compile time, and __builtin_choose_expr does not
> evaluate the expression that is not chosen. Therefore, GENMASK(x++, 0)
> does only evaluate x++ once.
>
> Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends
> available in assembly") made the macros in linux/bits.h available in
> assembly. Since BUILD_BUG_OR_ZERO() is not asm compatible, disable the
> checks if the file is included in an asm file.
>
> Signed-off-by: Rikard Falkeborn <[email protected]>
> ---
> Changes in v3:
> - Changed back to shorter macro argument names
> - Remove casts and use 0 instead of UL(0) in GENMASK_INPUT_CHECK(),
> since all results in GENMASK_INPUT_CHECK() are now ints. Update
> commit message to reflect that.
>
> Changes in v2:
> - Add comment about why inputs are not checked when used in asm file
> - Use UL(0) instead of 0
> - Extract mask creation in a separate macro to improve readability
> - Use high and low instead of h and l (part of this was extracted to a
> separate patch)
> - Updated commit message
>
> Joe Perches sent a series to fix the existing misuses of GENMASK() that
> needs to be merged before this to avoid build failures. Currently, 5 of
> the patches are not in Linus tree, and 2 are not in linux-next. There is
> also a patch pending by Nathan Chancellor that also needs to be merged
> before this patch is merged to avoid build failures.
>
> include/linux/bits.h | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 669d69441a62..4ba0fb609239 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -18,12 +18,29 @@
> * position @h. For example
> * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> */
> -#define GENMASK(h, l) \
> +#ifndef __ASSEMBLY__
> +#include <linux/build_bug.h>
> +#define GENMASK_INPUT_CHECK(h, l) \
> + (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> + __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> +#else
> +/*
> + * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> + * disable the input check if that is the case.
> + */
> +#define GENMASK_INPUT_CHECK(h, l) 0
> +#endif
> +
> +#define __GENMASK(h, l) \
> (((~UL(0)) - (UL(1) << (l)) + 1) & \
> (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> +#define GENMASK(h, l) \
> + (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>
> -#define GENMASK_ULL(h, l) \
> +#define __GENMASK_ULL(h, l) \
> (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
> +#define GENMASK_ULL(h, l) \
> + (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
>
> #endif /* __LINUX_BITS_H */
This is now commit 0fd35cd30a2fece1 ("linux/bits.h: add compile time sanity
check of GENMASK inputs") in next-20191008.
<[email protected]> reported the following failure in sun3_defconfig,
which I managed to reproduce with gcc-4.6.3:
lib/842/842_compress.c: In function '__split_add_bits':
lib/842/842_compress.c:164:25: error: first argument to
'__builtin_choose_expr' not a constant
lib/842/842_compress.c:164:25: error: bit-field '<anonymous>'
width not an integer constant
scripts/Makefile.build:265: recipe for target
'lib/842/842_compress.o' failed
__split_add_bits() calls GENMASK_ULL() with a non-constant.
However __split_add_bits() itself is called with constants only.
Apparently gcc fails to inline __split_add_bits().
Adding inline or always_inline doesn't help.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Geert,
On Tue, Oct 8, 2019 at 4:23 PM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Rikard,
>
> On Sun, Aug 11, 2019 at 8:52 PM Rikard Falkeborn
> <[email protected]> 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
> > 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().
> >
> > __builtin_constant_p does not evaluate is argument, it only checks if it
> > is a constant or not at compile time, and __builtin_choose_expr does not
> > evaluate the expression that is not chosen. Therefore, GENMASK(x++, 0)
> > does only evaluate x++ once.
> >
> > Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends
> > available in assembly") made the macros in linux/bits.h available in
> > assembly. Since BUILD_BUG_OR_ZERO() is not asm compatible, disable the
> > checks if the file is included in an asm file.
> >
> > Signed-off-by: Rikard Falkeborn <[email protected]>
> > ---
> > Changes in v3:
> > - Changed back to shorter macro argument names
> > - Remove casts and use 0 instead of UL(0) in GENMASK_INPUT_CHECK(),
> > since all results in GENMASK_INPUT_CHECK() are now ints. Update
> > commit message to reflect that.
> >
> > Changes in v2:
> > - Add comment about why inputs are not checked when used in asm file
> > - Use UL(0) instead of 0
> > - Extract mask creation in a separate macro to improve readability
> > - Use high and low instead of h and l (part of this was extracted to a
> > separate patch)
> > - Updated commit message
> >
> > Joe Perches sent a series to fix the existing misuses of GENMASK() that
> > needs to be merged before this to avoid build failures. Currently, 5 of
> > the patches are not in Linus tree, and 2 are not in linux-next. There is
> > also a patch pending by Nathan Chancellor that also needs to be merged
> > before this patch is merged to avoid build failures.
> >
> > include/linux/bits.h | 21 +++++++++++++++++++--
> > 1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/bits.h b/include/linux/bits.h
> > index 669d69441a62..4ba0fb609239 100644
> > --- a/include/linux/bits.h
> > +++ b/include/linux/bits.h
> > @@ -18,12 +18,29 @@
> > * position @h. For example
> > * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> > */
> > -#define GENMASK(h, l) \
> > +#ifndef __ASSEMBLY__
> > +#include <linux/build_bug.h>
> > +#define GENMASK_INPUT_CHECK(h, l) \
> > + (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > + __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> > +#else
> > +/*
> > + * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> > + * disable the input check if that is the case.
> > + */
> > +#define GENMASK_INPUT_CHECK(h, l) 0
> > +#endif
> > +
> > +#define __GENMASK(h, l) \
> > (((~UL(0)) - (UL(1) << (l)) + 1) & \
> > (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> > +#define GENMASK(h, l) \
> > + (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> >
> > -#define GENMASK_ULL(h, l) \
> > +#define __GENMASK_ULL(h, l) \
> > (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> > (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
> > +#define GENMASK_ULL(h, l) \
> > + (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
> >
> > #endif /* __LINUX_BITS_H */
>
> This is now commit 0fd35cd30a2fece1 ("linux/bits.h: add compile time sanity
> check of GENMASK inputs") in next-20191008.
>
> <[email protected]> reported the following failure in sun3_defconfig,
> which I managed to reproduce with gcc-4.6.3:
Oh dear.
I was able to reproduce this for gcc 4.7 or 4.8,
but I did not see any problem for gcc 4.9+
Perhaps, is this due to broken __builtin_choose_expr or __builtin_constant_p
for old compilers?
>
> lib/842/842_compress.c: In function '__split_add_bits':
> lib/842/842_compress.c:164:25: error: first argument to
> '__builtin_choose_expr' not a constant
> lib/842/842_compress.c:164:25: error: bit-field '<anonymous>'
> width not an integer constant
> scripts/Makefile.build:265: recipe for target
> 'lib/842/842_compress.o' failed
>
> __split_add_bits() calls GENMASK_ULL() with a non-constant.
> However __split_add_bits() itself is called with constants only.
> Apparently gcc fails to inline __split_add_bits().
> Adding inline or always_inline doesn't help.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
--
Best Regards
Masahiro Yamada
On Tue, Oct 8, 2019 at 4:44 PM Masahiro Yamada
<[email protected]> wrote:
>
> Hi Geert,
>
> On Tue, Oct 8, 2019 at 4:23 PM Geert Uytterhoeven <[email protected]> wrote:
> >
> > Hi Rikard,
> >
> > On Sun, Aug 11, 2019 at 8:52 PM Rikard Falkeborn
> > <[email protected]> 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
> > > 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().
> > >
> > > __builtin_constant_p does not evaluate is argument, it only checks if it
> > > is a constant or not at compile time, and __builtin_choose_expr does not
> > > evaluate the expression that is not chosen. Therefore, GENMASK(x++, 0)
> > > does only evaluate x++ once.
> > >
> > > Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends
> > > available in assembly") made the macros in linux/bits.h available in
> > > assembly. Since BUILD_BUG_OR_ZERO() is not asm compatible, disable the
> > > checks if the file is included in an asm file.
> > >
> > > Signed-off-by: Rikard Falkeborn <[email protected]>
> > > ---
> > > Changes in v3:
> > > - Changed back to shorter macro argument names
> > > - Remove casts and use 0 instead of UL(0) in GENMASK_INPUT_CHECK(),
> > > since all results in GENMASK_INPUT_CHECK() are now ints. Update
> > > commit message to reflect that.
> > >
> > > Changes in v2:
> > > - Add comment about why inputs are not checked when used in asm file
> > > - Use UL(0) instead of 0
> > > - Extract mask creation in a separate macro to improve readability
> > > - Use high and low instead of h and l (part of this was extracted to a
> > > separate patch)
> > > - Updated commit message
> > >
> > > Joe Perches sent a series to fix the existing misuses of GENMASK() that
> > > needs to be merged before this to avoid build failures. Currently, 5 of
> > > the patches are not in Linus tree, and 2 are not in linux-next. There is
> > > also a patch pending by Nathan Chancellor that also needs to be merged
> > > before this patch is merged to avoid build failures.
> > >
> > > include/linux/bits.h | 21 +++++++++++++++++++--
> > > 1 file changed, 19 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/linux/bits.h b/include/linux/bits.h
> > > index 669d69441a62..4ba0fb609239 100644
> > > --- a/include/linux/bits.h
> > > +++ b/include/linux/bits.h
> > > @@ -18,12 +18,29 @@
> > > * position @h. For example
> > > * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> > > */
> > > -#define GENMASK(h, l) \
> > > +#ifndef __ASSEMBLY__
> > > +#include <linux/build_bug.h>
> > > +#define GENMASK_INPUT_CHECK(h, l) \
> > > + (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > > + __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> > > +#else
> > > +/*
> > > + * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> > > + * disable the input check if that is the case.
> > > + */
> > > +#define GENMASK_INPUT_CHECK(h, l) 0
> > > +#endif
> > > +
> > > +#define __GENMASK(h, l) \
> > > (((~UL(0)) - (UL(1) << (l)) + 1) & \
> > > (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> > > +#define GENMASK(h, l) \
> > > + (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > >
> > > -#define GENMASK_ULL(h, l) \
> > > +#define __GENMASK_ULL(h, l) \
> > > (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> > > (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
> > > +#define GENMASK_ULL(h, l) \
> > > + (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
> > >
> > > #endif /* __LINUX_BITS_H */
> >
> > This is now commit 0fd35cd30a2fece1 ("linux/bits.h: add compile time sanity
> > check of GENMASK inputs") in next-20191008.
> >
> > <[email protected]> reported the following failure in sun3_defconfig,
> > which I managed to reproduce with gcc-4.6.3:
>
> Oh dear.
>
> I was able to reproduce this for gcc 4.7 or 4.8,
> but I did not see any problem for gcc 4.9+
>
> Perhaps, is this due to broken __builtin_choose_expr or __builtin_constant_p
> for old compilers?
>
>
>
>
>
>
>
> >
> > lib/842/842_compress.c: In function '__split_add_bits':
> > lib/842/842_compress.c:164:25: error: first argument to
> > '__builtin_choose_expr' not a constant
> > lib/842/842_compress.c:164:25: error: bit-field '<anonymous>'
> > width not an integer constant
> > scripts/Makefile.build:265: recipe for target
> > 'lib/842/842_compress.o' failed
> >
> > __split_add_bits() calls GENMASK_ULL() with a non-constant.
> > However __split_add_bits() itself is called with constants only.
> > Apparently gcc fails to inline __split_add_bits().
> > Adding inline or always_inline doesn't help.
> >
If this is broken for GCC < 4.9,
we might be able to workaround it as follows:
diff --git a/include/linux/bits.h b/include/linux/bits.h
index 4ba0fb609239..f00417baf545 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -18,7 +18,7 @@
* position @h. For example
* GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
*/
-#ifndef __ASSEMBLY__
+#if !defined(__ASSEMBLY__) && (!defined(CONFIG_CC_IS_GCC) ||
CONFIG_GCC_VERSION >= 49000)
#include <linux/build_bug.h>
#define GENMASK_INPUT_CHECK(h, l) \
(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
--
Best Regards
Masahiro Yamada
On Tue, Oct 08, 2019 at 04:52:17PM +0900, Masahiro Yamada wrote:
> On Tue, Oct 8, 2019 at 4:44 PM Masahiro Yamada
> <[email protected]> wrote:
> >
> > Hi Geert,
> >
> > On Tue, Oct 8, 2019 at 4:23 PM Geert Uytterhoeven <[email protected]> wrote:
> > >
> > > Hi Rikard,
> > >
> > > On Sun, Aug 11, 2019 at 8:52 PM Rikard Falkeborn
> > > <[email protected]> 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
> > > > 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().
> > > >
> > > > __builtin_constant_p does not evaluate is argument, it only checks if it
> > > > is a constant or not at compile time, and __builtin_choose_expr does not
> > > > evaluate the expression that is not chosen. Therefore, GENMASK(x++, 0)
> > > > does only evaluate x++ once.
> > > >
> > > > Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends
> > > > available in assembly") made the macros in linux/bits.h available in
> > > > assembly. Since BUILD_BUG_OR_ZERO() is not asm compatible, disable the
> > > > checks if the file is included in an asm file.
> > > >
> > > > Signed-off-by: Rikard Falkeborn <[email protected]>
> > > > ---
> > > > Changes in v3:
> > > > - Changed back to shorter macro argument names
> > > > - Remove casts and use 0 instead of UL(0) in GENMASK_INPUT_CHECK(),
> > > > since all results in GENMASK_INPUT_CHECK() are now ints. Update
> > > > commit message to reflect that.
> > > >
> > > > Changes in v2:
> > > > - Add comment about why inputs are not checked when used in asm file
> > > > - Use UL(0) instead of 0
> > > > - Extract mask creation in a separate macro to improve readability
> > > > - Use high and low instead of h and l (part of this was extracted to a
> > > > separate patch)
> > > > - Updated commit message
> > > >
> > > > Joe Perches sent a series to fix the existing misuses of GENMASK() that
> > > > needs to be merged before this to avoid build failures. Currently, 5 of
> > > > the patches are not in Linus tree, and 2 are not in linux-next. There is
> > > > also a patch pending by Nathan Chancellor that also needs to be merged
> > > > before this patch is merged to avoid build failures.
> > > >
> > > > include/linux/bits.h | 21 +++++++++++++++++++--
> > > > 1 file changed, 19 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/include/linux/bits.h b/include/linux/bits.h
> > > > index 669d69441a62..4ba0fb609239 100644
> > > > --- a/include/linux/bits.h
> > > > +++ b/include/linux/bits.h
> > > > @@ -18,12 +18,29 @@
> > > > * position @h. For example
> > > > * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> > > > */
> > > > -#define GENMASK(h, l) \
> > > > +#ifndef __ASSEMBLY__
> > > > +#include <linux/build_bug.h>
> > > > +#define GENMASK_INPUT_CHECK(h, l) \
> > > > + (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > > > + __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> > > > +#else
> > > > +/*
> > > > + * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> > > > + * disable the input check if that is the case.
> > > > + */
> > > > +#define GENMASK_INPUT_CHECK(h, l) 0
> > > > +#endif
> > > > +
> > > > +#define __GENMASK(h, l) \
> > > > (((~UL(0)) - (UL(1) << (l)) + 1) & \
> > > > (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> > > > +#define GENMASK(h, l) \
> > > > + (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > > >
> > > > -#define GENMASK_ULL(h, l) \
> > > > +#define __GENMASK_ULL(h, l) \
> > > > (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> > > > (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
> > > > +#define GENMASK_ULL(h, l) \
> > > > + (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
> > > >
> > > > #endif /* __LINUX_BITS_H */
> > >
> > > This is now commit 0fd35cd30a2fece1 ("linux/bits.h: add compile time sanity
> > > check of GENMASK inputs") in next-20191008.
> > >
> > > <[email protected]> reported the following failure in sun3_defconfig,
> > > which I managed to reproduce with gcc-4.6.3:
> >
> > Oh dear.
> >
> > I was able to reproduce this for gcc 4.7 or 4.8,
> > but I did not see any problem for gcc 4.9+
> >
> > Perhaps, is this due to broken __builtin_choose_expr or __builtin_constant_p
> > for old compilers?
> >
> >
> >
> >
> >
> >
> >
> > >
> > > lib/842/842_compress.c: In function '__split_add_bits':
> > > lib/842/842_compress.c:164:25: error: first argument to
> > > '__builtin_choose_expr' not a constant
> > > lib/842/842_compress.c:164:25: error: bit-field '<anonymous>'
> > > width not an integer constant
> > > scripts/Makefile.build:265: recipe for target
> > > 'lib/842/842_compress.o' failed
> > >
> > > __split_add_bits() calls GENMASK_ULL() with a non-constant.
> > > However __split_add_bits() itself is called with constants only.
> > > Apparently gcc fails to inline __split_add_bits().
> > > Adding inline or always_inline doesn't help.
> > >
>
>
> If this is broken for GCC < 4.9,
> we might be able to workaround it as follows:
>
>
>
>
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 4ba0fb609239..f00417baf545 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -18,7 +18,7 @@
> * position @h. For example
> * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> */
> -#ifndef __ASSEMBLY__
> +#if !defined(__ASSEMBLY__) && (!defined(CONFIG_CC_IS_GCC) ||
> CONFIG_GCC_VERSION >= 49000)
> #include <linux/build_bug.h>
> #define GENMASK_INPUT_CHECK(h, l) \
> (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
>
>
> --
> Best Regards
> Masahiro Yamada
Hi Masahiro, Geert,
It seems it is broken for GCC < 4.9, see [1]. I'll try disabling it for
too old compilers and resend.
In the meantime, I guess it should be dropped from linux-next?
Rikard
[1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449
Hello,
Add build time validity checks of GENMASK (and GENMASK_ULL) inputs.
The main differences from v3:
- Patch v3 1/3 was merged into Linus tree through the x86 tree and is
not part of this series any longer.
- Disable the input check for GCC < 4.9 due to a gcc bug.
Joe Perches sent a patch series to fix existing misuses, currently there
are two remaining such misuses (which patches pending) left in Linus
tree. However, the remaining two cases are in unused macros and will not
break any builds until someone tries to use them. There was also an
arm-specific misuse which have also been fixed since v2 of this patchset
was merged to linux-next. When v3 of this patchset was included in
linux-next, there were not additional build failures or warnings
(there are some failed builds due to other patches though), however
<[email protected]> and Geert Uytterhoeven reported that it broke
compilation with old versions of gcc so a v4 is needed.
Changelog
Since v3
- Patch v2 1/3 (the x86 build warning) has been merged into Linus tree
through the x86 tree (and is therefore not part of v4).
- Disable the GENMASK input check if GCC version < 4.9 due to a
compiler bug [0].
[0]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449
Since v2
- Use __builtin_constant_p instead of __is_constexpr to avoid pulling
in kernel.h (that include was missing in v2, so the header was no
longer builable standalone
- add cast to BUILD_BUG_ON_ZERO to make the type int
- Remove unnecessary casts due to the above
- Drop patch that renamed macro arguments
Since v1
- Add comment about why inputs are not checked when used in asm file
- Use UL(0) instead of 0
- Extract mask creation in a separate macro to improve readability
- Use high and low instead of h and l (part of this was extracted to a
separate patch)
- Updated commit message
Rikard Falkeborn (2):
linux/build_bug.h: Change type to int
linux/bits.h: Add compile time sanity check of GENMASK inputs
include/linux/bits.h | 22 ++++++++++++++++++++--
include/linux/build_bug.h | 4 ++--
2 files changed, 22 insertions(+), 4 deletions(-)
--
2.23.0