[PATCH] BUILD_BUG_ON sucks

[PATCH] BUILD_BUG_ON sucks

[Alexey Dobriyan]

BUILD_BUG_ON should have never existed -- BUG_ON could upgrade itself to
compile-breaking version if compiler has enough information and this is
what patch does.

The only downside is that one can't write BUG_ON(1) anymore.

Note: this compile-breaks jbd, jbd2, tipc,
      what this code is supposed to do?

	journal = handle->h_transaction->t_journal;
	if (!journal_set_features(journal, 0, 0, JFS_FEATURE_INCOMPAT_REVOKE)) {
		J_ASSERT (!"Cannot set revoke feature!");
			 ^^^^
		return -EINVAL;
        }

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 arch/cris/arch-v10/kernel/io_interface_mux.c |    8 ++++----
 arch/powerpc/include/asm/bug.h               |    3 +--
 drivers/infiniband/core/cma.c                |    2 +-
 drivers/infiniband/core/mad.c                |    2 +-
 drivers/infiniband/hw/amso1100/c2_ae.c       |    2 +-
 drivers/infiniband/hw/cxgb3/cxio_hal.c       |    2 +-
 drivers/infiniband/hw/cxgb3/iwch_cm.c        |    6 +++---
 drivers/mmc/host/wbsd.c                      |    2 +-
 drivers/net/wireless/b43legacy/b43legacy.h   |    2 ++
 drivers/net/wireless/b43legacy/main.c        |   12 ++++++------
 drivers/net/wireless/b43legacy/phy.c         |    2 +-
 drivers/net/wireless/b43legacy/radio.c       |   22 +++++++++++-----------
 drivers/net/wireless/b43legacy/xmit.c        |   12 ++++++------
 drivers/scsi/device_handler/scsi_dh_emc.c    |    2 +-
 drivers/ssb/scan.c                           |    4 ++--
 drivers/ssb/ssb_private.h                    |    2 ++
 fs/jbd2/journal.c                            |    2 +-
 include/asm-generic/bug.h                    |    8 +++++++-
 include/asm-mips/bug.h                       |    5 ++++-
 kernel/sched.c                               |    2 +-
 net/bluetooth/rfcomm/tty.c                   |    2 +-
 21 files changed, 58 insertions(+), 46 deletions(-)

--- a/arch/cris/arch-v10/kernel/io_interface_mux.c
+++ b/arch/cris/arch-v10/kernel/io_interface_mux.c
@@ -650,7 +650,7 @@ int cris_request_io_interface(enum cris_io_interface ioif, const char *device_id
 			if_group_use = interfaces[ioif].group_f;
 			break;
 		default:
-			BUG_ON(1);
+			BUG();
 		}
 
 		if (if_group_use & grp->used) {
@@ -799,7 +799,7 @@ int cris_request_io_interface(enum cris_io_interface ioif, const char *device_id
 			if_group_use = interfaces[ioif].group_f;
 			break;
 		default:
-			BUG_ON(1);
+			BUG();
 		}
 		grp->used |= if_group_use;
 
@@ -891,11 +891,11 @@ void cris_free_io_interface(enum cris_io_interface ioif)
 			if_group_use = interfaces[ioif].group_f;
 			break;
 		default:
-			BUG_ON(1);
+			BUG();
 		}
 
 		if ((grp->used & if_group_use) != if_group_use)
-			BUG_ON(1);
+			BUG();
 		grp->used = grp->used & ~if_group_use;
 
 		group_set = clear_group_from_set(group_set, grp);
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -71,8 +71,7 @@
 
 #define BUG_ON(x) do {						\
 	if (__builtin_constant_p(x)) {				\
-		if (x)						\
-			BUG();					\
+		(void)sizeof(char[1 - 2 * !!(x)]);		\
 	} else {						\
 		__asm__ __volatile__(				\
 		"1:	"PPC_TLNEI"	%4,0\n"			\
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -1252,7 +1252,7 @@ static int cma_iw_handler(struct iw_cm_id *iw_id, struct iw_cm_event *iw_event)
 		event.event = RDMA_CM_EVENT_ESTABLISHED;
 		break;
 	default:
-		BUG_ON(1);
+		BUG();
 	}
 
 	event.status = iw_event->status;
--- a/drivers/infiniband/core/mad.c
+++ b/drivers/infiniband/core/mad.c
@@ -2249,7 +2249,7 @@ static void ib_mad_completion_handler(struct work_struct *work)
 				ib_mad_recv_done_handler(port_priv, &wc);
 				break;
 			default:
-				BUG_ON(1);
+				BUG();
 				break;
 			}
 		} else
--- a/drivers/infiniband/hw/amso1100/c2_ae.c
+++ b/drivers/infiniband/hw/amso1100/c2_ae.c
@@ -256,7 +256,7 @@ void c2_ae_event(struct c2_dev *c2dev, u32 mq_index)
 				cm_id->event_handler(cm_id, &cm_event);
 			break;
 		default:
-			BUG_ON(1);
+			BUG();
 			pr_debug("%s:%d Unexpected event_id=%d on QP=%p, "
 				"CM_ID=%p\n",
 				__func__, __LINE__,
--- a/drivers/infiniband/hw/cxgb3/cxio_hal.c
+++ b/drivers/infiniband/hw/cxgb3/cxio_hal.c
@@ -109,7 +109,7 @@ int cxio_hal_cq_op(struct cxio_rdev *rdev_p, struct t3_cq *cq,
 		while (!CQ_VLD_ENTRY(rptr, cq->size_log2, cqe)) {
 			udelay(1);
 			if (i++ > 1000000) {
-				BUG_ON(1);
+				BUG();
 				printk(KERN_ERR "%s: stalled rnic\n",
 				       rdev_p->dev_name);
 				return -EIO;
--- a/drivers/infiniband/hw/cxgb3/iwch_cm.c
+++ b/drivers/infiniband/hw/cxgb3/iwch_cm.c
@@ -1493,7 +1493,7 @@ static int peer_close(struct t3cdev *tdev, struct sk_buff *skb, void *ctx)
 		disconnect = 0;
 		break;
 	default:
-		BUG_ON(1);
+		BUG();
 	}
 	spin_unlock_irqrestore(&ep->com.lock, flags);
 	if (disconnect)
@@ -1590,7 +1590,7 @@ static int peer_abort(struct t3cdev *tdev, struct sk_buff *skb, void *ctx)
 		spin_unlock_irqrestore(&ep->com.lock, flags);
 		return CPL_RET_BUF_DONE;
 	default:
-		BUG_ON(1);
+		BUG();
 		break;
 	}
 	dst_confirm(ep->dst);
@@ -1653,7 +1653,7 @@ static int close_con_rpl(struct t3cdev *tdev, struct sk_buff *skb, void *ctx)
 	case DEAD:
 		break;
 	default:
-		BUG_ON(1);
+		BUG();
 		break;
 	}
 	spin_unlock_irqrestore(&ep->com.lock, flags);
--- a/drivers/mmc/host/wbsd.c
+++ b/drivers/mmc/host/wbsd.c
@@ -1416,7 +1416,7 @@ kfree:
 	/*
 	 * If we've gotten here then there is some kind of alignment bug
 	 */
-	BUG_ON(1);
+	BUG();
 
 	dma_unmap_single(mmc_dev(host->mmc), host->dma_addr,
 		WBSD_DMA_SIZE, DMA_BIDIRECTIONAL);
--- a/drivers/net/wireless/b43legacy/b43legacy.h
+++ b/drivers/net/wireless/b43legacy/b43legacy.h
@@ -342,12 +342,14 @@ enum {
 			BUG_ON(expr);					\
 		}							\
 	} while (0)
+# define B43legacy_BUG()	BUG()
 # define B43legacy_DEBUG	1
 #else
 /* This will evaluate the argument even if debugging is disabled. */
 static inline bool __b43legacy_warn_on_dummy(bool x) { return x; }
 # define B43legacy_WARN_ON(x)	__b43legacy_warn_on_dummy(unlikely(!!(x)))
 # define B43legacy_BUG_ON(x)	do { /* nothing */ } while (0)
+# define B43legacy_BUG()	do {} while (0)
 # define B43legacy_DEBUG	0
 #endif
 
--- a/drivers/net/wireless/b43legacy/main.c
+++ b/drivers/net/wireless/b43legacy/main.c
@@ -649,7 +649,7 @@ void b43legacy_dummy_transmission(struct b43legacy_wldev *dev)
 		buffer[0] = 0x000B846E;
 		break;
 	default:
-		B43legacy_BUG_ON(1);
+		B43legacy_BUG();
 		return;
 	}
 
@@ -2010,7 +2010,7 @@ static void b43legacy_rate_memory_init(struct b43legacy_wldev *dev)
 		b43legacy_rate_memory_write(dev, B43legacy_CCK_RATE_11MB, 0);
 		break;
 	default:
-		B43legacy_BUG_ON(1);
+		B43legacy_BUG();
 	}
 }
 
@@ -2032,7 +2032,7 @@ static void b43legacy_mgmtframe_txantenna(struct b43legacy_wldev *dev,
 		ant |= B43legacy_TX4_PHY_ANTLAST;
 		break;
 	default:
-		B43legacy_BUG_ON(1);
+		B43legacy_BUG();
 	}
 
 	/* FIXME We also need to set the other flags of the PHY control
@@ -2442,7 +2442,7 @@ static const char *phymode_to_string(unsigned int phymode)
 	case B43legacy_PHYMODE_G:
 		return "G";
 	default:
-		B43legacy_BUG_ON(1);
+		B43legacy_BUG();
 	}
 	return "";
 }
@@ -2887,7 +2887,7 @@ static int b43legacy_phy_versioning(struct b43legacy_wldev *dev)
 			unsupported = 1;
 		break;
 	default:
-		B43legacy_BUG_ON(1);
+		B43legacy_BUG();
 	}
 	if (unsupported) {
 		b43legacyerr(dev->wl, "FOUND UNSUPPORTED RADIO "
@@ -3563,7 +3563,7 @@ static int b43legacy_wireless_core_attach(struct b43legacy_wldev *dev)
 			have_gphy = 1;
 			break;
 		default:
-			B43legacy_BUG_ON(1);
+			B43legacy_BUG();
 		}
 	}
 	dev->phy.gmode = (have_gphy || have_bphy);
--- a/drivers/net/wireless/b43legacy/phy.c
+++ b/drivers/net/wireless/b43legacy/phy.c
@@ -1760,7 +1760,7 @@ static s8 b43legacy_phy_estimate_power_out(struct b43legacy_wldev *dev, s8 tssi)
 		dbm = phy->tssi2dbm[tmp];
 		break;
 	default:
-		B43legacy_BUG_ON(1);
+		B43legacy_BUG();
 	}
 
 	return dbm;
--- a/drivers/net/wireless/b43legacy/radio.c
+++ b/drivers/net/wireless/b43legacy/radio.c
@@ -131,7 +131,7 @@ u16 b43legacy_radio_read16(struct b43legacy_wldev *dev, u16 offset)
 		offset |= 0x80;
 		break;
 	default:
-		B43legacy_BUG_ON(1);
+		B43legacy_BUG();
 	}
 
 	b43legacy_write16(dev, B43legacy_MMIO_RADIO_CONTROL, offset);
@@ -811,7 +811,7 @@ void b43legacy_calc_nrssi_slope(struct b43legacy_wldev *dev)
 		b43legacy_calc_nrssi_threshold(dev);
 		break;
 	default:
-		B43legacy_BUG_ON(1);
+		B43legacy_BUG();
 	}
 }
 
@@ -910,7 +910,7 @@ void b43legacy_calc_nrssi_threshold(struct b43legacy_wldev *dev)
 		}
 		break;
 	default:
-		B43legacy_BUG_ON(1);
+		B43legacy_BUG();
 	}
 }
 
@@ -946,7 +946,7 @@ static u16 _stack_restore(u32 *stackptr,
 			continue;
 		return ((*stackptr & 0xFFFF0000) >> 16);
 	}
-	B43legacy_BUG_ON(1);
+	B43legacy_BUG();
 
 	return 0;
 }
@@ -1227,7 +1227,7 @@ b43legacy_radio_interference_mitigation_enable(struct b43legacy_wldev *dev,
 		b43legacy_calc_nrssi_slope(dev);
 		break;
 	default:
-		B43legacy_BUG_ON(1);
+		B43legacy_BUG();
 	}
 }
 
@@ -1325,7 +1325,7 @@ b43legacy_radio_interference_mitigation_disable(struct b43legacy_wldev *dev,
 		b43legacy_calc_nrssi_slope(dev);
 		break;
 	default:
-		B43legacy_BUG_ON(1);
+		B43legacy_BUG();
 	}
 }
 
@@ -1419,7 +1419,7 @@ static u16 b43legacy_get_812_value(struct b43legacy_wldev *dev, u8 lpd)
 			case LPD(1, 0, 0):
 				return 0x30B3;
 			default:
-				B43legacy_BUG_ON(1);
+				B43legacy_BUG();
 			}
 		} else {
 			switch (lpd) {
@@ -1432,7 +1432,7 @@ static u16 b43legacy_get_812_value(struct b43legacy_wldev *dev, u8 lpd)
 			case LPD(1, 0, 0):
 				return 0x20B3;
 			default:
-				B43legacy_BUG_ON(1);
+				B43legacy_BUG();
 			}
 		}
 	} else {
@@ -1474,7 +1474,7 @@ static u16 b43legacy_get_812_value(struct b43legacy_wldev *dev, u8 lpd)
 			case LPD(1, 0, 0):
 				return (0x2093 | loop_or);
 			default:
-				B43legacy_BUG_ON(1);
+				B43legacy_BUG();
 			}
 		} else {
 			switch (lpd) {
@@ -1486,7 +1486,7 @@ static u16 b43legacy_get_812_value(struct b43legacy_wldev *dev, u8 lpd)
 			case LPD(1, 0, 0):
 				return (0x0093 | loop_or);
 			default:
-				B43legacy_BUG_ON(1);
+				B43legacy_BUG();
 			}
 		}
 	}
@@ -2111,7 +2111,7 @@ void b43legacy_radio_turn_on(struct b43legacy_wldev *dev)
 		B43legacy_WARN_ON(err);
 		break;
 	default:
-		B43legacy_BUG_ON(1);
+		B43legacy_BUG();
 	}
 	phy->radio_on = 1;
 }
--- a/drivers/net/wireless/b43legacy/xmit.c
+++ b/drivers/net/wireless/b43legacy/xmit.c
@@ -49,7 +49,7 @@ static u8 b43legacy_plcp_get_bitrate_idx_cck(struct b43legacy_plcp_hdr6 *plcp)
 	case 0x6E:
 		return 3;
 	}
-	B43legacy_BUG_ON(1);
+	B43legacy_BUG();
 	return -1;
 }
 
@@ -77,7 +77,7 @@ static u8 b43legacy_plcp_get_bitrate_idx_ofdm(struct b43legacy_plcp_hdr6 *plcp,
 	case 0xC:
 		return base + 7;
 	}
-	B43legacy_BUG_ON(1);
+	B43legacy_BUG();
 	return -1;
 }
 
@@ -93,7 +93,7 @@ u8 b43legacy_plcp_get_ratecode_cck(const u8 bitrate)
 	case B43legacy_CCK_RATE_11MB:
 		return 0x6E;
 	}
-	B43legacy_BUG_ON(1);
+	B43legacy_BUG();
 	return 0;
 }
 
@@ -117,7 +117,7 @@ u8 b43legacy_plcp_get_ratecode_ofdm(const u8 bitrate)
 	case B43legacy_OFDM_RATE_54MB:
 		return 0xC;
 	}
-	B43legacy_BUG_ON(1);
+	B43legacy_BUG();
 	return 0;
 }
 
@@ -180,7 +180,7 @@ static u8 b43legacy_calc_fallback_rate(u8 bitrate)
 	case B43legacy_OFDM_RATE_54MB:
 		return B43legacy_OFDM_RATE_48MB;
 	}
-	B43legacy_BUG_ON(1);
+	B43legacy_BUG();
 	return 0;
 }
 
@@ -289,7 +289,7 @@ static int generate_txhdr_fw3(struct b43legacy_wldev *dev,
 		phy_ctl |= B43legacy_TX4_PHY_ANT1;
 		break;
 	default:
-		B43legacy_BUG_ON(1);
+		B43legacy_BUG();
 	}
 
 	/* MAC control */
--- a/drivers/scsi/device_handler/scsi_dh_emc.c
+++ b/drivers/scsi/device_handler/scsi_dh_emc.c
@@ -298,7 +298,7 @@ static struct request *get_req(struct scsi_device *sdev, int cmd,
 		memset(buffer, 0, len);
 		break;
 	default:
-		BUG_ON(1);
+		BUG();
 		break;
 	}
 
--- a/drivers/ssb/scan.c
+++ b/drivers/ssb/scan.c
@@ -203,7 +203,7 @@ void ssb_iounmap(struct ssb_bus *bus)
 #ifdef CONFIG_SSB_PCIHOST
 		pci_iounmap(bus->host_pci, bus->mmio);
 #else
-		SSB_BUG_ON(1); /* Can't reach this code. */
+		SSB_BUG(); /* Can't reach this code. */
 #endif
 		break;
 	}
@@ -227,7 +227,7 @@ static void __iomem *ssb_ioremap(struct ssb_bus *bus,
 #ifdef CONFIG_SSB_PCIHOST
 		mmio = pci_iomap(bus->host_pci, 0, ~0UL);
 #else
-		SSB_BUG_ON(1); /* Can't reach this code. */
+		SSB_BUG(); /* Can't reach this code. */
 #endif
 		break;
 	}
--- a/drivers/ssb/ssb_private.h
+++ b/drivers/ssb/ssb_private.h
@@ -23,10 +23,12 @@
 #ifdef CONFIG_SSB_DEBUG
 # define SSB_WARN_ON(x)		WARN_ON(x)
 # define SSB_BUG_ON(x)		BUG_ON(x)
+# define SSB_BUG()		BUG()
 #else
 static inline int __ssb_do_nothing(int x) { return x; }
 # define SSB_WARN_ON(x)		__ssb_do_nothing(unlikely(!!(x)))
 # define SSB_BUG_ON(x)		__ssb_do_nothing(unlikely(!!(x)))
+# define SSB_BUG()		do {} while (0)
 #endif
 
 
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -731,7 +731,7 @@ static int jbd2_seq_history_show(struct seq_file *seq, void *v)
 				ts->u.chp.cs_written, ts->u.chp.cs_dropped,
 				ts->u.chp.cs_forced_to_close);
 	else
-		J_ASSERT(0);
+		BUG();
 	return 0;
 }
 
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -28,7 +28,13 @@ struct bug_entry {
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0)
+#define BUG_ON(condition)						\
+	do {								\
+		if (__builtin_constant_p(condition))			\
+			(void)sizeof(char[1 - 2 * !!(condition)]);	\
+		else if (unlikely(condition))				\
+			BUG();						\
+	} while (0)
 #endif
 
 #ifndef __WARN
--- a/include/asm-mips/bug.h
+++ b/include/asm-mips/bug.h
@@ -18,7 +18,10 @@ do {									\
 
 #define BUG_ON(condition)						\
 do {									\
-	__asm__ __volatile__("tne $0, %0, %1"				\
+	if (__builtin_constant_p(condition))				\
+		(void)sizeof(char[1 - 2 * !!(condition)]);		\
+	else
+		__asm__ __volatile__("tne $0, %0, %1"			\
 			     : : "r" (condition), "i" (BRK_BUG));	\
 } while (0)
 
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -2798,7 +2798,7 @@ static int double_lock_balance(struct rq *this_rq, struct rq *busiest)
 	if (unlikely(!irqs_disabled())) {
 		/* printk() doesn't work good under rq->lock */
 		spin_unlock(&this_rq->lock);
-		BUG_ON(1);
+		BUG();
 	}
 	if (unlikely(!spin_trylock(&busiest->lock))) {
 		if (busiest < this_rq) {
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -331,7 +331,7 @@ static void rfcomm_dev_del(struct rfcomm_dev *dev)
 	BT_DBG("dev %p", dev);
 
 	if (test_bit(RFCOMM_TTY_RELEASED, &dev->flags))
-		BUG_ON(1);
+		BUG();
 	else
 		set_bit(RFCOMM_TTY_RELEASED, &dev->flags);
 

Re: [PATCH] BUILD_BUG_ON sucks

[Rusty Russell]

On Saturday 16 August 2008 20:09:48 Alexey Dobriyan wrote:
> BUILD_BUG_ON should have never existed -- BUG_ON could upgrade itself to
> compile-breaking version if compiler has enough information and this is
> what patch does.
>
> The only downside is that one can't write BUG_ON(1) anymore.

Interesting idea, but I've come to actually like the semantic explicitness of 
BUILD_BUG_ON.  There's a difference between "we should never get here" 
and "this should never exist".

But maybe I just like it because we have it.  At very least BUILD_BUG_ON 
should definitely compile-barf on a non-constant expr, and vice versa for 
BUG_ON().

Note that BUG_ON() is a hack caused by lack of attribute((cold)).  "if (x) 
BUG()" is clearer, and possible in the long run as people upgrade compilers.

Cheers,
Rusty.

Re: [PATCH] BUILD_BUG_ON sucks

[Andrew Morton]

On Sat, 16 Aug 2008 14:09:48 +0400 Alexey Dobriyan <adobriyan@gmail.com> wrote:

>       what this code is supposed to do?
> 
> 	journal = handle->h_transaction->t_journal;
> 	if (!journal_set_features(journal, 0, 0, JFS_FEATURE_INCOMPAT_REVOKE)) {
> 		J_ASSERT (!"Cannot set revoke feature!");
> 			 ^^^^

lol.  It's been there since I merged ext3 in 2.4.15.  Probably it was
in sct's ext3 patches in the RH kernel.

Don't change it - it might be important!

Re: [PATCH] BUILD_BUG_ON sucks

[Linus Torvalds]

On Sat, 16 Aug 2008, Rusty Russell wrote:
> 
> Interesting idea, but I've come to actually like the semantic explicitness of 
> BUILD_BUG_ON.  There's a difference between "we should never get here" 
> and "this should never exist".

Agreed. I think Alexey's patch is broken.

The thing is, BUILD_BUG_ON() is a different thing. It says "this is a 
build error", while BUG_ON() says "this is an error if we reach it".

Very different.

The fact that you broke BUG_ON(1) should have made you think. Sometimes 
the "1" isn't necessarily a constant one. It might be

	if (something_that_can_never_happen_in_some_configuration) {
		...
		BUG_ON(CONFIG_XYZZY);
		...
	}

where the BUG_ON(1) is absolutely *not* the same thing as BUILD_BUG_ON().

			Linus

[PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, 16 Aug 2008, Rusty Russell wrote:
> > 
> > Interesting idea, but I've come to actually like the semantic explicitness of 
> > BUILD_BUG_ON.  There's a difference between "we should never get here" 
> > and "this should never exist".
> 
> Agreed. I think Alexey's patch is broken.
> 
> The thing is, BUILD_BUG_ON() is a different thing. It says "this is a 
> build error", while BUG_ON() says "this is an error if we reach it".
> 
> Very different.

agreed.

There's one aspect of BUILD_BUG_ON() that is quite dangerous though: it 
does not 'upgrade' into a runtime check if an expression is not 
constant. And it does not warn either. So BUILD_BUG_ON() can degrade 
into a no-op very silently, and that is inherently dangerous.

That aspect bit me once: i added a BUILD_BUG_ON() under the assumption 
that it would catch a mis-sized virtual memory sizing detail in 
arch/x86/, but it just remained silent.

To fix these problems i've added the two commits below to tip/core/debug 
[one to extend BUILD_BUG_ON, one to clean up its location] - any 
objections against that direction? I've started testing it through to 
make sure we dont have any stale non-constant BUILD_BUG_ON() instances 
around.

( Note, i have not changed BUILD_BUG_ON_ZERO() because that is used in 
  structure initializers so no comma expression can be used in them. 
  Such structure initializers wont allow non-constant expressions 
  anyway, so there's not much extra value in checking for that. )

( Note #2, BUILD_BUG_ON() had to remain a macro, so that
  __builtin_constant_expression_p() can do its work. )

	Ingo

>From f5b5d41dd51a31fe70e3a04fb80a3b90b84c6a4e Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Sun, 17 Aug 2008 11:58:58 +0200
Subject: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

constant expressions get detected at build time via:

  kernel/sched.c: In function ‘test':
  kernel/sched.c:9187: error: size of array ‘type name' is negative
  make[1]: *** [kernel/sched.o] Error 1

but non-constant expressions (for example BUILD_BUG_ON(variable)) simply
get discarded by the compiler - turning BUILD_BUG_ON() into a dangerous
construct.

So add another layer at the link level to detect such mishaps:

  kernel/built-in.o: In function `test':
  : undefined reference to `__BUILD_BUG_ON_non_constant'

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 include/linux/kernel.h |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 2651f80..36c841e 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -467,8 +467,22 @@ struct sysinfo {
 	char _f[20-2*sizeof(long)-sizeof(int)];	/* Padding: libc5 uses this.. */
 };
 
-/* Force a compilation error if condition is true */
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+/*
+ * Force a compilation error if condition is true [array index becomes
+ * negative], and a linker error if condition is not constant [non-defined
+ * variable is used as an array index]:
+ *
+ * ( The linker trick relies on gcc optimizing out a multiplication with
+ *   constant zero - which should be reasonable enough. )
+ */
+extern unsigned int __BUILD_BUG_ON_non_constant;
+
+#define BUILD_BUG_ON(condition)					\
+do {								\
+	(void)sizeof(char[1 - 2*!!(condition)]);		\
+	if (!__builtin_constant_p(condition))			\
+		__BUILD_BUG_ON_non_constant++;			\
+} while (0)
 
 /* 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

>From 7c516ee411f38cffbd4ab09b089c210202f9bd0f Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Sun, 17 Aug 2008 12:18:01 +0200
Subject: [PATCH] debug, x86: move BUILD_BUG_ON() and __FUNCTION__

move BUILD_BUG_ON variants and the __FUNCTION__ definition from
kernel.h to compiler.h.

Besides being the correct location for such trivial wrappers around
compiler functionality, this also allows the removal of a duplicate
(and now slighly incompatible) definition of BUILD_BUG_ON from
arch/x86/boot/boot.h.

[ boot.h cannot just include kernel.h to pick up the new definition of
  BUILD_BUG_ON(), as it is also built into user-space utilities on the
  host system. ]

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/boot/boot.h     |    3 ---
 include/linux/compiler.h |   30 ++++++++++++++++++++++++++++++
 include/linux/kernel.h   |   26 --------------------------
 3 files changed, 30 insertions(+), 29 deletions(-)

diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index 616b804..f09b79a 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -27,9 +27,6 @@
 #include "bitops.h"
 #include <asm/cpufeature.h>
 
-/* Useful macros */
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
-
 extern struct setup_header hdr;
 extern struct boot_params boot_params;
 
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index c8bd2da..727862f 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -194,4 +194,34 @@ extern void __chk_io_ptr(const volatile void __iomem *);
  */
 #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
 
+/*
+ * Force a compilation error if condition is true [array index becomes
+ * negative], and a linker error if condition is not constant [non-defined
+ * variable is used as an array index]:
+ *
+ * ( The linker trick relies on gcc optimizing out a multiplication with
+ *   constant zero - which should be reasonable enough. )
+ */
+#ifndef __ASSEMBLY__
+extern unsigned int __BUILD_BUG_ON_non_constant;
+#endif
+
+#define BUILD_BUG_ON(condition)					\
+do {								\
+	(void)sizeof(char[1 - 2*!!(condition)]);		\
+	if (!__builtin_constant_p(condition))			\
+		__BUILD_BUG_ON_non_constant++;			\
+} while (0)
+
+/*
+ * 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
+ * e.g. in a structure initializer (or where-ever else comma expressions
+ * aren't permitted):
+ */
+#define BUILD_BUG_ON_ZERO(e) (sizeof(char[1 - 2 * !!(e)]) - 1)
+
+/* Trap pasters of __FUNCTION__ at compile-time */
+#define __FUNCTION__ (__func__)
+
 #endif /* __LINUX_COMPILER_H */
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 36c841e..1ceafa4 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -467,32 +467,6 @@ struct sysinfo {
 	char _f[20-2*sizeof(long)-sizeof(int)];	/* Padding: libc5 uses this.. */
 };
 
-/*
- * Force a compilation error if condition is true [array index becomes
- * negative], and a linker error if condition is not constant [non-defined
- * variable is used as an array index]:
- *
- * ( The linker trick relies on gcc optimizing out a multiplication with
- *   constant zero - which should be reasonable enough. )
- */
-extern unsigned int __BUILD_BUG_ON_non_constant;
-
-#define BUILD_BUG_ON(condition)					\
-do {								\
-	(void)sizeof(char[1 - 2*!!(condition)]);		\
-	if (!__builtin_constant_p(condition))			\
-		__BUILD_BUG_ON_non_constant++;			\
-} while (0)
-
-/* 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
-   e.g. in a structure initializer (or where-ever else comma expressions
-   aren't permitted). */
-#define BUILD_BUG_ON_ZERO(e) (sizeof(char[1 - 2 * !!(e)]) - 1)
-
-/* Trap pasters of __FUNCTION__ at compile-time */
-#define __FUNCTION__ (__func__)
-
 /* This helps us to avoid #ifdef CONFIG_NUMA */
 #ifdef CONFIG_NUMA
 #define NUMA_BUILD 1

Re: [PATCH] BUILD_BUG_ON sucks

[Theodore Tso]

On Sat, Aug 16, 2008 at 10:46:58AM -0700, Andrew Morton wrote:
> >       what this code is supposed to do?
> > 
> > 	journal = handle->h_transaction->t_journal;
> > 	if (!journal_set_features(journal, 0, 0, JFS_FEATURE_INCOMPAT_REVOKE)) {
> > 		J_ASSERT (!"Cannot set revoke feature!");
> > 			 ^^^^
> 
> lol.  It's been there since I merged ext3 in 2.4.15.  Probably it was
> in sct's ext3 patches in the RH kernel.
> 
> Don't change it - it might be important!

Heh!  Well, it does the right thing, and doesn't take any extra text
space assuming a vaguely competent C compiler optimizer.  :-)

I'm pretty sure that back in the 2.4 days, we didn't have BUG_ON.  We
should do a s/J_ASSERT/BUG_ON/g pass over all of fs/jbd and fs/jbd2.
I'll submit patches for application when the 2.6.27 merge window opens
up --- or is this an obvious enough and safe enough transformation
that it will get accepted mainline at this point?

     	     	 	  	      	   - Ted

Re: [PATCH] BUILD_BUG_ON sucks

[Andrew Morton]

On Sun, 17 Aug 2008 08:19:06 -0400 Theodore Tso <tytso@mit.edu> wrote:

> On Sat, Aug 16, 2008 at 10:46:58AM -0700, Andrew Morton wrote:
> > >       what this code is supposed to do?
> > > 
> > > 	journal = handle->h_transaction->t_journal;
> > > 	if (!journal_set_features(journal, 0, 0, JFS_FEATURE_INCOMPAT_REVOKE)) {
> > > 		J_ASSERT (!"Cannot set revoke feature!");
> > > 			 ^^^^
> > 
> > lol.  It's been there since I merged ext3 in 2.4.15.  Probably it was
> > in sct's ext3 patches in the RH kernel.
> > 
> > Don't change it - it might be important!
> 
> Heh!  Well, it does the right thing, and doesn't take any extra text
> space assuming a vaguely competent C compiler optimizer.  :-)
> 
> I'm pretty sure that back in the 2.4 days, we didn't have BUG_ON.  We
> should do a s/J_ASSERT/BUG_ON/g pass over all of fs/jbd and fs/jbd2.
> I'll submit patches for application when the 2.6.27 merge window opens
> up --- or is this an obvious enough and safe enough transformation
> that it will get accepted mainline at this point?
> 

May as well get it over and done with.

We presently have a mix of J_ASSERT, J_ASSERT_JH and J_ASSERT_BH.  That
will become BUG_ON, J_ASSERT_JH and J_ASSERT_BH.  Which a is slightly
unpleasing loss of consistency but whatever.

Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[Linus Torvalds]

On Sun, 17 Aug 2008, Ingo Molnar wrote:
> +/*
> + * Force a compilation error if condition is true [array index becomes
> + * negative], and a linker error if condition is not constant [non-defined
> + * variable is used as an array index]:
> + *
> + * ( The linker trick relies on gcc optimizing out a multiplication with
> + *   constant zero - which should be reasonable enough. )
> + */
> +extern unsigned int __BUILD_BUG_ON_non_constant;
> +
> +#define BUILD_BUG_ON(condition)					\
> +do {								\
> +	(void)sizeof(char[1 - 2*!!(condition)]);		\
> +	if (!__builtin_constant_p(condition))			\
> +		__BUILD_BUG_ON_non_constant++;			\
> +} while (0)

Gag me now.

Why not just do

 #define __BBO(c)                sizeof(const char[1 - 2*!!(c)])
 #define __BBONC(c)              __BBO(!__builtin_constant_p(c))
 #define BUILD_BUG_ON_ZERO(c)    (__BBO(c) - __BBONC(c))
 #define BUILD_BUG_ON(c)         (void)BUILD_BUG_ON_ZERO(c)

and be done with it?

The rules are trivial:

 - __BBO() causes a warning if 'c' is a constant non-zero, returns a
   (size_t) 1 otherwise

 - __BBONC() causes a warning if 'c' is not a constant, returns a
   (size_t) 1 otherwise

And then BUILD_BUG_ON[_ZERO] are totally _trivial_ on top of those.

Yeah, yeah, I didn't test this, but it looks a hell of a lot simpler, and 
gives the warning about non-constant issues at compile time with line 
numbers rather than at link-time.

		Linus

Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Sun, 17 Aug 2008, Ingo Molnar wrote:
> > +/*
> > + * Force a compilation error if condition is true [array index becomes
> > + * negative], and a linker error if condition is not constant [non-defined
> > + * variable is used as an array index]:
> > + *
> > + * ( The linker trick relies on gcc optimizing out a multiplication with
> > + *   constant zero - which should be reasonable enough. )
> > + */
> > +extern unsigned int __BUILD_BUG_ON_non_constant;
> > +
> > +#define BUILD_BUG_ON(condition)					\
> > +do {								\
> > +	(void)sizeof(char[1 - 2*!!(condition)]);		\
> > +	if (!__builtin_constant_p(condition))			\
> > +		__BUILD_BUG_ON_non_constant++;			\
> > +} while (0)
> 
> Gag me now.
> 
> Why not just do
> 
>  #define __BBO(c)                sizeof(const char[1 - 2*!!(c)])
>  #define __BBONC(c)              __BBO(!__builtin_constant_p(c))
>  #define BUILD_BUG_ON_ZERO(c)    (__BBO(c) - __BBONC(c))
>  #define BUILD_BUG_ON(c)         (void)BUILD_BUG_ON_ZERO(c)
> 
> and be done with it?

yeah, i first tried a few variants of that (compile-time warnings are 
much better than link time warnings), but none worked when i tested 
various failure modes.

try the patch below - it only gives this error during build:

 kernel/sched.c: In function ‘test':
 kernel/sched.c:9193: error: size of array ‘type name' is negative
 make[1]: *** [kernel/sched.o] Error 1

it doesnt notice the error on the next line. I suspect this is because 
__builtin_constant_p() is special (or broken). I tried it with gcc 4.2.3 
and 4.3.0.

	Ingo

Index: linux/kernel/sched.c
===================================================================
--- linux.orig/kernel/sched.c
+++ linux/kernel/sched.c
@@ -9181,3 +9181,16 @@ struct cgroup_subsys cpuacct_subsys = {
 	.subsys_id = cpuacct_subsys_id,
 };
 #endif	/* CONFIG_CGROUP_CPUACCT */
+
+#define __BBO(c)                sizeof(const char[1 - 2*!!(c)])
+#define __BBONC(c)              __BBO(!__builtin_constant_p(c))
+#define BUILD_BUG_ON_ZERO2(c)    (__BBO(c) - __BBONC(c))
+#define BUILD_BUG_ON2(c)         (void)BUILD_BUG_ON_ZERO(c)
+
+void test(void)
+{
+	BUILD_BUG_ON2(0);
+	BUILD_BUG_ON2(1);
+	BUILD_BUG_ON2(panic_timeout);
+}
+

Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> +#define BUILD_BUG_ON_ZERO2(c)    (__BBO(c) - __BBONC(c))
> +#define BUILD_BUG_ON2(c)         (void)BUILD_BUG_ON_ZERO(c)
                                          ^^^^^^^^^^^^^^^^^
Lars Gullik Bjønnes pointed out that this should have been ZERO2 not 
ZERO - but same end result.

	Ingo

Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[Linus Torvalds]

On Sun, 17 Aug 2008, Ingo Molnar wrote:
> 
> try the patch below - it only gives this error during build:

Well, you didn't do it right:

> +#define __BBO(c)                sizeof(const char[1 - 2*!!(c)])
> +#define __BBONC(c)              __BBO(!__builtin_constant_p(c))
> +#define BUILD_BUG_ON_ZERO2(c)    (__BBO(c) - __BBONC(c))
> +#define BUILD_BUG_ON2(c)         (void)BUILD_BUG_ON_ZERO(c)

Look at the #define of BUILD_BUG_ON2 a bit more.

Hint: you're using the _wrong_ BUILD_BUG_ON_ZERO. The old one, not the v2 
one!

That said, with that fixed, there's still something wrong. It does seem 
like gcc has some very odd interaction there with __builtin_constant_p. 
Odd.

			Linus

Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Sun, 17 Aug 2008, Ingo Molnar wrote:
> > 
> > try the patch below - it only gives this error during build:
> 
> Well, you didn't do it right:
> 
> > +#define __BBO(c)                sizeof(const char[1 - 2*!!(c)])
> > +#define __BBONC(c)              __BBO(!__builtin_constant_p(c))
> > +#define BUILD_BUG_ON_ZERO2(c)    (__BBO(c) - __BBONC(c))
> > +#define BUILD_BUG_ON2(c)         (void)BUILD_BUG_ON_ZERO(c)
> 
> Look at the #define of BUILD_BUG_ON2 a bit more.
> 
> Hint: you're using the _wrong_ BUILD_BUG_ON_ZERO. The old one, not the v2 
> one!

yeah, i already tried various variants earlier today so i really didnt try
that hard with yours. (and i pointed out this mistake in the previous mail)

> That said, with that fixed, there's still something wrong. It does seem 
> like gcc has some very odd interaction there with __builtin_constant_p. 
> Odd.

yeah. I tried various integer arithmetic expressions (which the array trick
relies on) and it didnt work as expected - it's always zero. It only makes
a difference when used in comparisons. (and that's where the kernel uses
__builtin_constant_p quite heavily, and it works fine there.)

Odd indeed.

	Ingo

Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[Rusty Russell]

On Monday 18 August 2008 03:33:19 Ingo Molnar wrote:
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > Gag me now.
> >
> > Why not just do
> >
> >  #define __BBO(c)                sizeof(const char[1 - 2*!!(c)])
> >  #define __BBONC(c)              __BBO(!__builtin_constant_p(c))
> >  #define BUILD_BUG_ON_ZERO(c)    (__BBO(c) - __BBONC(c))
> >  #define BUILD_BUG_ON(c)         (void)BUILD_BUG_ON_ZERO(c)
> >
> > and be done with it?
>
> yeah, i first tried a few variants of that (compile-time warnings are
> much better than link time warnings), but none worked when i tested
> various failure modes.

Hey, I thought I was the "undisputed ruler of Ugly-land".

How about this instead:

#define BUILD_BUG_ON(condition)						\
do {									\
	static struct { char arr[1 - 2*!!(condition)]; } x __maybe_unused;	\
} while(0)

Cheers,
Rusty.

Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[Ingo Molnar]

* Rusty Russell <rusty@rustcorp.com.au> wrote:

> On Monday 18 August 2008 03:33:19 Ingo Molnar wrote:
> > * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > > Gag me now.
> > >
> > > Why not just do
> > >
> > >  #define __BBO(c)                sizeof(const char[1 - 2*!!(c)])
> > >  #define __BBONC(c)              __BBO(!__builtin_constant_p(c))
> > >  #define BUILD_BUG_ON_ZERO(c)    (__BBO(c) - __BBONC(c))
> > >  #define BUILD_BUG_ON(c)         (void)BUILD_BUG_ON_ZERO(c)
> > >
> > > and be done with it?
> >
> > yeah, i first tried a few variants of that (compile-time warnings are
> > much better than link time warnings), but none worked when i tested
> > various failure modes.
> 
> Hey, I thought I was the "undisputed ruler of Ugly-land".
> 
> How about this instead:
> 
> #define BUILD_BUG_ON(condition)						\
> do {									\
> 	static struct { char arr[1 - 2*!!(condition)]; } x __maybe_unused;	\
> } while(0)

hm, have you tried it and do we get a severe enough link error about 
that? If the macro gets ignored by the compiler that's really a hard 
error - such things are essential safeguards of kernel sanity:

	/*
	 * Build-time sanity checks on the kernel image and module
	 * area mappings. (these are purely build-time and produce no code)
	 */
	BUILD_BUG_ON(MODULES_VADDR < KERNEL_IMAGE_START);
	BUILD_BUG_ON(MODULES_VADDR-KERNEL_IMAGE_START < KERNEL_IMAGE_SIZE);
	BUILD_BUG_ON(MODULES_LEN + KERNEL_IMAGE_SIZE > 2*PUD_SIZE);
	BUILD_BUG_ON((KERNEL_IMAGE_START & ~PMD_MASK) != 0);
	BUILD_BUG_ON((MODULES_VADDR & ~PMD_MASK) != 0);
	BUILD_BUG_ON(!(MODULES_VADDR > __START_KERNEL));
	BUILD_BUG_ON(!(((MODULES_END - 1) & PGDIR_MASK) ==
				(__START_KERNEL & PGDIR_MASK)));
	BUILD_BUG_ON(__fix_to_virt(__end_of_fixed_addresses) <= MODULES_END);

( and propagating them into runtime failures not only increases bloat,
  it also makes failures harder to debug. These checks 'run' _early_. )

Link time warnings are easy enough to miss.

So unless there's a better way of doing it all at compile time (i'd 
really prefer that!) i'd prefer the link time error about botched 
BUILD_BUG_ON() conditions - as my commits introduce.

	Ingo

Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[Boaz Harrosh]

Ingo Molnar wrote:
> * Rusty Russell <rusty@rustcorp.com.au> wrote:
> 
>> On Monday 18 August 2008 03:33:19 Ingo Molnar wrote:
>>> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>>> Gag me now.
>>>>
>>>> Why not just do
>>>>
>>>>  #define __BBO(c)                sizeof(const char[1 - 2*!!(c)])
>>>>  #define __BBONC(c)              __BBO(!__builtin_constant_p(c))
>>>>  #define BUILD_BUG_ON_ZERO(c)    (__BBO(c) - __BBONC(c))
>>>>  #define BUILD_BUG_ON(c)         (void)BUILD_BUG_ON_ZERO(c)
>>>>
>>>> and be done with it?
>>> yeah, i first tried a few variants of that (compile-time warnings are
>>> much better than link time warnings), but none worked when i tested
>>> various failure modes.
>> Hey, I thought I was the "undisputed ruler of Ugly-land".
>>
>> How about this instead:
>>
>> #define BUILD_BUG_ON(condition)						\
>> do {									\
>> 	static struct { char arr[1 - 2*!!(condition)]; } x __maybe_unused;	\
>> } while(0)
> 
> hm, have you tried it and do we get a severe enough link error about 
> that? If the macro gets ignored by the compiler that's really a hard 
> error - such things are essential safeguards of kernel sanity:
> 
> 	/*
> 	 * Build-time sanity checks on the kernel image and module
> 	 * area mappings. (these are purely build-time and produce no code)
> 	 */
> 	BUILD_BUG_ON(MODULES_VADDR < KERNEL_IMAGE_START);
> 	BUILD_BUG_ON(MODULES_VADDR-KERNEL_IMAGE_START < KERNEL_IMAGE_SIZE);
> 	BUILD_BUG_ON(MODULES_LEN + KERNEL_IMAGE_SIZE > 2*PUD_SIZE);
> 	BUILD_BUG_ON((KERNEL_IMAGE_START & ~PMD_MASK) != 0);
> 	BUILD_BUG_ON((MODULES_VADDR & ~PMD_MASK) != 0);
> 	BUILD_BUG_ON(!(MODULES_VADDR > __START_KERNEL));
> 	BUILD_BUG_ON(!(((MODULES_END - 1) & PGDIR_MASK) ==
> 				(__START_KERNEL & PGDIR_MASK)));
> 	BUILD_BUG_ON(__fix_to_virt(__end_of_fixed_addresses) <= MODULES_END);
> 
> ( and propagating them into runtime failures not only increases bloat,
>   it also makes failures harder to debug. These checks 'run' _early_. )
> 
> Link time warnings are easy enough to miss.
> 
> So unless there's a better way of doing it all at compile time (i'd 
> really prefer that!) i'd prefer the link time error about botched 
> BUILD_BUG_ON() conditions - as my commits introduce.
> 
> 	Ingo
> --

#define BUILD_BUG_ON(condition)						\
do {
	enum { bad = !!(condition)}; 									\
	static struct { char arr[1 - 2*bad]; } x __maybe_unused;	\
} while(0)

the enum definition will not let in anything not compile-time constant.
But then I fail on: (include/linux/virtio_config.h:99)

	if (__builtin_constant_p(fbit))
		BUILD_BUG_ON(fbit >= 32);

is that code broken?

Boaz

Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[Boaz Harrosh]

Boaz Harrosh wrote:
> Ingo Molnar wrote:
>> * Rusty Russell <rusty@rustcorp.com.au> wrote:
>>
>>> On Monday 18 August 2008 03:33:19 Ingo Molnar wrote:
>>>> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>>>> Gag me now.
>>>>>
>>>>> Why not just do
>>>>>
>>>>>  #define __BBO(c)                sizeof(const char[1 - 2*!!(c)])
>>>>>  #define __BBONC(c)              __BBO(!__builtin_constant_p(c))
>>>>>  #define BUILD_BUG_ON_ZERO(c)    (__BBO(c) - __BBONC(c))
>>>>>  #define BUILD_BUG_ON(c)         (void)BUILD_BUG_ON_ZERO(c)
>>>>>
>>>>> and be done with it?
>>>> yeah, i first tried a few variants of that (compile-time warnings are
>>>> much better than link time warnings), but none worked when i tested
>>>> various failure modes.
>>> Hey, I thought I was the "undisputed ruler of Ugly-land".
>>>
>>> How about this instead:
>>>
>>> #define BUILD_BUG_ON(condition)						\
>>> do {									\
>>> 	static struct { char arr[1 - 2*!!(condition)]; } x __maybe_unused;	\
>>> } while(0)
>> hm, have you tried it and do we get a severe enough link error about 
>> that? If the macro gets ignored by the compiler that's really a hard 
>> error - such things are essential safeguards of kernel sanity:
>>
>> 	/*
>> 	 * Build-time sanity checks on the kernel image and module
>> 	 * area mappings. (these are purely build-time and produce no code)
>> 	 */
>> 	BUILD_BUG_ON(MODULES_VADDR < KERNEL_IMAGE_START);
>> 	BUILD_BUG_ON(MODULES_VADDR-KERNEL_IMAGE_START < KERNEL_IMAGE_SIZE);
>> 	BUILD_BUG_ON(MODULES_LEN + KERNEL_IMAGE_SIZE > 2*PUD_SIZE);
>> 	BUILD_BUG_ON((KERNEL_IMAGE_START & ~PMD_MASK) != 0);
>> 	BUILD_BUG_ON((MODULES_VADDR & ~PMD_MASK) != 0);
>> 	BUILD_BUG_ON(!(MODULES_VADDR > __START_KERNEL));
>> 	BUILD_BUG_ON(!(((MODULES_END - 1) & PGDIR_MASK) ==
>> 				(__START_KERNEL & PGDIR_MASK)));
>> 	BUILD_BUG_ON(__fix_to_virt(__end_of_fixed_addresses) <= MODULES_END);
>>
>> ( and propagating them into runtime failures not only increases bloat,
>>   it also makes failures harder to debug. These checks 'run' _early_. )
>>
>> Link time warnings are easy enough to miss.
>>
>> So unless there's a better way of doing it all at compile time (i'd 
>> really prefer that!) i'd prefer the link time error about botched 
>> BUILD_BUG_ON() conditions - as my commits introduce.
>>
>> 	Ingo
>> --
> 
> #define BUILD_BUG_ON(condition)						\
> do {
> 	enum { bad = !!(condition)}; 									\
> 	static struct { char arr[1 - 2*bad]; } x __maybe_unused;	\
> } while(0)
> 
> the enum definition will not let in anything not compile-time constant.
> But then I fail on: (include/linux/virtio_config.h:99)
> 
> 	if (__builtin_constant_p(fbit))
> 		BUILD_BUG_ON(fbit >= 32);
> 
> is that code broken?
> 
> Boaz
> 

with this patch:
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index aaa998f..91d98f2 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -468,7 +468,12 @@ struct sysinfo {
 };
 
 /* Force a compilation error if condition is true */
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+// #define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+#define BUILD_BUG_ON(condition)					\
+do { 								\
+	enum { bad = !!(condition)}; 				\
+	static struct { char arr[1 - 2*bad]; } x __maybe_unused;\
+} while(0)
 
 /* 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


I found another BUILD_BUG_ON() none const bug, here:

diff --git a/drivers/net/niu.c b/drivers/net/niu.c
index e4765b7..1469a38 100644
--- a/drivers/net/niu.c
+++ b/drivers/net/niu.c
@@ -5133,9 +5133,9 @@ static void niu_init_tx_bmac(struct niu *np, u64 min, u64 max)
 
 static void niu_init_tx_mac(struct niu *np)
 {
-	u64 min, max;
+	u64 max;
 
-	min = 64;
+	enum { min = 64 } ;
 	if (np->dev->mtu > ETH_DATA_LEN)
 		max = 9216;
 	else

Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[Ingo Molnar]

* Boaz Harrosh <bharrosh@panasas.com> wrote:

> > Link time warnings are easy enough to miss.
> > 
> > So unless there's a better way of doing it all at compile time (i'd 
> > really prefer that!) i'd prefer the link time error about botched 
> > BUILD_BUG_ON() conditions - as my commits introduce.
> > 
> > 	Ingo
> > --
> 
> #define BUILD_BUG_ON(condition)						\
> do {
> 	enum { bad = !!(condition)}; 									\
> 	static struct { char arr[1 - 2*bad]; } x __maybe_unused;	\
> } while(0)
> 
> the enum definition will not let in anything not compile-time constant.

nice trick!

> But then I fail on: (include/linux/virtio_config.h:99)
> 
> 	if (__builtin_constant_p(fbit))
> 		BUILD_BUG_ON(fbit >= 32);
> 
> is that code broken?

hmm ... that's a bit sad, gcc ought to have been able to figure this 
out. Can this be fixed somehow, without losing the strength of the 
checking here? I think we should not change BUILD_BUG_ON() to make it 
less useful.

	Ingo

Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[Boaz Harrosh]

Ingo Molnar wrote:
> * Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>>> Link time warnings are easy enough to miss.
>>>
>>> So unless there's a better way of doing it all at compile time (i'd 
>>> really prefer that!) i'd prefer the link time error about botched 
>>> BUILD_BUG_ON() conditions - as my commits introduce.
>>>
>>> 	Ingo
>>> --
>> #define BUILD_BUG_ON(condition)						\
>> do {
>> 	enum { bad = !!(condition)}; 									\
>> 	static struct { char arr[1 - 2*bad]; } x __maybe_unused;	\
>> } while(0)
>>
>> the enum definition will not let in anything not compile-time constant.
> 
> nice trick!
> 
>> But then I fail on: (include/linux/virtio_config.h:99)
>>
>> 	if (__builtin_constant_p(fbit))
>> 		BUILD_BUG_ON(fbit >= 32);
>>
>> is that code broken?
> 
> hmm ... that's a bit sad, gcc ought to have been able to figure this 
> out. Can this be fixed somehow, without losing the strength of the 
> checking here? I think we should not change BUILD_BUG_ON() to make it 
> less useful.
> 
> 	Ingo

The BUILD_BUG_ON I proposed is not less useful. Actually with current
BUILD_BUG_ON above code does nothing, since fbit is a function parameter
it will translate to nothing. Certainly it will not check the size of passed
parameter, since that was converted on the stack to unsigned int. So my
definition will catch the bad programing here.

If the user of virtio_has_feature() must pass a compile-time constant then
it must be converted to a MACRO, and then the BUILD_BUG_ON will work.
Or it should be changed to a BUG_ON() if fbit is a runtime variable.

>From what I understood this is what you wanted, some way to make sure
a BUILD_BUG_ON will check that the check is actually useful (compile-time)
and is not silently ignored.

Same thing at the other place I sent.

Boaz

Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[Ingo Molnar]

* Boaz Harrosh <bharrosh@panasas.com> wrote:

> If the user of virtio_has_feature() must pass a compile-time constant 
> then it must be converted to a MACRO, and then the BUILD_BUG_ON will 
> work. Or it should be changed to a BUG_ON() if fbit is a runtime 
> variable.

well, that's the question i'm asking: that sort of proposed 
BUILD_BUG_ON() variantcannot be used in inline functions like 
virtio_has_feature() does. If we get forced back to macros that's not an 
improvement.

Maybe the link-time last-line-of-defense mechanism i posed is the most 
flexible one perhaps after all? (it's ugly too but none of this is 
particularly pretty)

hm?

	Ingo

Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[Boaz Harrosh]

Ingo Molnar wrote:
> * Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>> If the user of virtio_has_feature() must pass a compile-time constant 
>> then it must be converted to a MACRO, and then the BUILD_BUG_ON will 
>> work. Or it should be changed to a BUG_ON() if fbit is a runtime 
>> variable.
> 

The use of __builtin_constant_p in inline functions is broken. This
is because it will give different results depending on the -O level
used. So I think that using it in the Kernel with inlines is plain
broken. And should be discouraged.

That said, my trick with enum is exactly the same as __builtin_constant_p
when -O is off, that is, does not traverse inline. But it is consistent
across any optimization.

> well, that's the question i'm asking: that sort of proposed 
> BUILD_BUG_ON() variantcannot be used in inline functions like 
> virtio_has_feature() does. If we get forced back to macros that's not an 
> improvement.
> 

I think it is an improvement, in a sense that now we think something is happening
but get silently ignored if compilation conditions are different, and/or the programmer
had a mistake. The new way will show us what code will be produced in the worse 
case and will error if wrong.
  
> Maybe the link-time last-line-of-defense mechanism i posed is the most 
> flexible one perhaps after all? (it's ugly too but none of this is 
> particularly pretty)
> 

The link-time gives the same results. Only warns at link time instead of
compile time. The difference between our approaches is the use of
__builtin_constant_p which is suppose to work cross inline stack boundary,
but in effect it does not if the optimization is not just right.

> hm?
> 
> 	Ingo

Here is gcc documentation about __builtin_constant_p:

— Built-in Function: int __builtin_constant_p (exp)

    You can use the built-in function __builtin_constant_p to determine if a value is known to be constant at compile-time and hence that GCC can perform constant-folding on expressions involving that value. The argument of the function is the value to test. The function returns the integer 1 if the argument is known to be a compile-time constant and 0 if it is not known to be a compile-time constant. A return of 0 does not indicate that the value is not a constant, but merely that GCC cannot prove it is a constant with the specified value of the -O option.

    You would typically use this function in an embedded application where memory was a critical resource. If you have some complex calculation, you may want it to be folded if it involves constants, but need to call a function if it does not. For example:

              #define Scale_Value(X)      \
                (__builtin_constant_p (X) \
                ? ((X) * SCALE + OFFSET) : Scale (X))
         

    You may use this built-in function in either a macro or an inline function. However, if you use it in an inlined function and pass an argument of the function as the argument to the built-in, GCC will never return 1 when you call the inline function with a string constant or compound literal (see Compound Literals) and will not return 1 when you pass a constant numeric value to the inline function unless you specify the -O option.

    You may also use __builtin_constant_p in initializers for static data. For instance, you can write

              static const int table[] = {
                 __builtin_constant_p (EXPRESSION) ? (EXPRESSION) : -1,
                 /* ... */
              };
         

    This is an acceptable initializer even if EXPRESSION is not a constant expression. GCC must be more conservative about evaluating the built-in in this case, because it has no opportunity to perform optimization.

    Previous versions of GCC did not accept this built-in in data initializers. The earliest version where it is completely safe is 3.0.1. 


I have tried the test below:
#include <stdio.h>

#define __maybe_unused			__attribute__((unused))

#define BUILD_BUG_ON_ORIG(condition) ((void)sizeof(char[1 - 2*!!(condition)]))

#define BUILD_BUG_ON_B(condition)				\
do { 								\
	enum { bad = !!(condition)}; 				\
	static struct { char arr[1 - 2*bad]; } x __maybe_unused;\
} while(0)

#define BUILD_BUG_ON_R(condition)						\
do {									\
	static struct { char arr[1 - 2*!!(condition)]; } x __maybe_unused;	\
} while(0)

extern unsigned int __BUILD_BUG_ON_non_constant;
#define BUILD_BUG_ON_I(condition)				\
do {								\
	(void)sizeof(char[1 - 2*!!(condition)]);		\
	if (!__builtin_constant_p(condition))			\
		__BUILD_BUG_ON_non_constant++;			\
} while (0)

#define BUILD_BUG_ON BUILD_BUG_ON_R

int main()
{
	int var;

	var = random();

	BUILD_BUG_ON(2 < 1);
	BUILD_BUG_ON(1 < 2);
	BUILD_BUG_ON(var < 2);

	printf("var=%d", var);
	return 0;
}

where I changed #define BUILD_BUG_ON BUILD_BUG_ON_X to the three
variants (ORIG/B/R/I) here is what I get (optimization is off).

_ORIG:
2 < 1:   good (is silent)
1 < 2:   good (error report)
var < 2: bad (just ignored)

_B && _R:
2 < 1:   good (is silent)
1 < 2:   good (error report)
var < 2: good (error report)

_I: (optimization is off)
2 < 1:   bad (link time error)
1 < 2:   good (error report)
var < 2: good- (link time error)

So I think the BUILD_BUG_ON_R should be accepted. This will force
two changes in current Kernel (i386 allmodconfig), which in my
opinion are case 3 above and should be fixed anyway.

Please propose other tests we should try, for example with cross
inline-functions/macros.

Boaz

Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[adobriyan]

On Wed, Aug 20, 2008 at 03:31:53PM +0300, Boaz Harrosh wrote:
> Ingo Molnar wrote:
> > * Boaz Harrosh <bharrosh@panasas.com> wrote:
> > 
> >> If the user of virtio_has_feature() must pass a compile-time constant 
> >> then it must be converted to a MACRO, and then the BUILD_BUG_ON will 
> >> work. Or it should be changed to a BUG_ON() if fbit is a runtime 
> >> variable.
> > 
> 
> The use of __builtin_constant_p in inline functions is broken. This
> is because it will give different results depending on the -O level
> used. So I think that using it in the Kernel with inlines is plain
> broken. And should be discouraged.

See how kmalloc() works.

Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[Boaz Harrosh]

adobriyan@gmail.com wrote:
> On Wed, Aug 20, 2008 at 03:31:53PM +0300, Boaz Harrosh wrote:
>> Ingo Molnar wrote:
>>> * Boaz Harrosh <bharrosh@panasas.com> wrote:
>>>
>>>> If the user of virtio_has_feature() must pass a compile-time constant 
>>>> then it must be converted to a MACRO, and then the BUILD_BUG_ON will 
>>>> work. Or it should be changed to a BUG_ON() if fbit is a runtime 
>>>> variable.
>> The use of __builtin_constant_p in inline functions is broken. This
>> is because it will give different results depending on the -O level
>> used. So I think that using it in the Kernel with inlines is plain
>> broken. And should be discouraged.
> 
> See how kmalloc() works.
> 
Right:

static inline void *kmalloc(size_t size, gfp_t flags)
{
	if (__builtin_constant_p(size)) {
	 	... do some stuff
	else
		return __kmalloc_xxx(..)
}

So if optimization is on we might gain some cycles
but we must make sure we have the proper else clause.

In any way virtio_has_feature() is broken and no amount
of optimization may ever fix it. Please look at it:

static inline bool virtio_has_feature(const struct virtio_device *vdev,
				      unsigned int fbit)
{
	/* Did you forget to fix assumptions on max features? */
	if (__builtin_constant_p(fbit))
		BUILD_BUG_ON(sizeof(fbit) >= 32);


the __builtin_constant_p(fbit) will never help.
sizeof(fbit) is always sizeof(unsigned int) in any case. Only
a macro can help here. Sorry

Boaz

Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[Boaz Harrosh]

Ingo Molnar wrote:
> From: Ingo Molnar <mingo@elte.hu>
> Date: Sun, 17 Aug 2008 12:18:01 +0200
> Subject: [PATCH] debug, x86: move BUILD_BUG_ON() and __FUNCTION__
> 
> move BUILD_BUG_ON variants and the __FUNCTION__ definition from
> kernel.h to compiler.h.
> 
> Besides being the correct location for such trivial wrappers around
> compiler functionality, this also allows the removal of a duplicate
> (and now slighly incompatible) definition of BUILD_BUG_ON from
> arch/x86/boot/boot.h.
> 
> [ boot.h cannot just include kernel.h to pick up the new definition of
>   BUILD_BUG_ON(), as it is also built into user-space utilities on the
>   host system. ]
> 
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/x86/boot/boot.h     |    3 ---
>  include/linux/compiler.h |   30 ++++++++++++++++++++++++++++++
>  include/linux/kernel.h   |   26 --------------------------
>  3 files changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
> index 616b804..f09b79a 100644
> --- a/arch/x86/boot/boot.h
> +++ b/arch/x86/boot/boot.h
> @@ -27,9 +27,6 @@
>  #include "bitops.h"
>  #include <asm/cpufeature.h>
>  
> -/* Useful macros */
> -#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
> -
>  extern struct setup_header hdr;
>  extern struct boot_params boot_params;
>  
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index c8bd2da..727862f 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -194,4 +194,34 @@ extern void __chk_io_ptr(const volatile void __iomem *);
>   */
>  #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
>  
> +/*
> + * Force a compilation error if condition is true [array index becomes
> + * negative], and a linker error if condition is not constant [non-defined
> + * variable is used as an array index]:
> + *
> + * ( The linker trick relies on gcc optimizing out a multiplication with
> + *   constant zero - which should be reasonable enough. )
> + */
> +#ifndef __ASSEMBLY__
> +extern unsigned int __BUILD_BUG_ON_non_constant;
> +#endif
> +
> +#define BUILD_BUG_ON(condition)					\
> +do {								\
> +	(void)sizeof(char[1 - 2*!!(condition)]);		\
> +	if (!__builtin_constant_p(condition))			\
> +		__BUILD_BUG_ON_non_constant++;			\
> +} while (0)
> +
> +/*
> + * 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
> + * e.g. in a structure initializer (or where-ever else comma expressions
> + * aren't permitted):
> + */
> +#define BUILD_BUG_ON_ZERO(e) (sizeof(char[1 - 2 * !!(e)]) - 1)
> +
> +/* Trap pasters of __FUNCTION__ at compile-time */
> +#define __FUNCTION__ (__func__)
> +
>  #endif /* __LINUX_COMPILER_H */
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 36c841e..1ceafa4 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -467,32 +467,6 @@ struct sysinfo {
>  	char _f[20-2*sizeof(long)-sizeof(int)];	/* Padding: libc5 uses this.. */
>  };
>  
> -/*
> - * Force a compilation error if condition is true [array index becomes
> - * negative], and a linker error if condition is not constant [non-defined
> - * variable is used as an array index]:
> - *
> - * ( The linker trick relies on gcc optimizing out a multiplication with
> - *   constant zero - which should be reasonable enough. )
> - */
> -extern unsigned int __BUILD_BUG_ON_non_constant;
> -
> -#define BUILD_BUG_ON(condition)					\
> -do {								\
> -	(void)sizeof(char[1 - 2*!!(condition)]);		\
> -	if (!__builtin_constant_p(condition))			\
> -		__BUILD_BUG_ON_non_constant++;			\
> -} while (0)
> -
> -/* 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
> -   e.g. in a structure initializer (or where-ever else comma expressions
> -   aren't permitted). */
> -#define BUILD_BUG_ON_ZERO(e) (sizeof(char[1 - 2 * !!(e)]) - 1)
> -
> -/* Trap pasters of __FUNCTION__ at compile-time */
> -#define __FUNCTION__ (__func__)
> -
>  /* This helps us to avoid #ifdef CONFIG_NUMA */
>  #ifdef CONFIG_NUMA
>  #define NUMA_BUILD 1
> --
Ingo Hi!

I got bitten by this duplication too. Please submit an equivalent patch
for today's code. That could be nice.

Then later we can take care of BUILD_BUG_ON which ever variant.

Thanks
Boaz

Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[Ingo Molnar]

* Boaz Harrosh <bharrosh@panasas.com> wrote:

> Ingo Molnar wrote:
> > * Boaz Harrosh <bharrosh@panasas.com> wrote:
> > 
> >> If the user of virtio_has_feature() must pass a compile-time constant 
> >> then it must be converted to a MACRO, and then the BUILD_BUG_ON will 
> >> work. Or it should be changed to a BUG_ON() if fbit is a runtime 
> >> variable.
> > 
> 
> The use of __builtin_constant_p in inline functions is broken. This is 
> because it will give different results depending on the -O level used. 
> So I think that using it in the Kernel with inlines is plain broken. 
> And should be discouraged.

ok - that's convincing, so your approach is superior to my hack on 
multiple levels.

Could you please (re-)send your patch(es) with a changelog, etc., so 
that i can apply it and drop my patches?

Thanks,

	Ingo

Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[Rusty Russell]

On Wednesday 20 August 2008 02:33:59 Boaz Harrosh wrote:
> If the user of virtio_has_feature() must pass a compile-time constant then
> it must be converted to a MACRO, and then the BUILD_BUG_ON will work.
> Or it should be changed to a BUG_ON() if fbit is a runtime variable.

I was (ab)using the current slackness of BUILD_BUG_ON() to be a noop on 
non-constants.  We can replace with a BUG_ON if we fix BUILD_BUG_ON() to 
insist on a compile-time constant; I think that's still an overall win.

Cheers,
Rusty.