[00/02] add BUILD_BUG_DECL assertion (for 3.4??)

[00/02] add BUILD_BUG_DECL assertion (for 3.4??)

[Jim Cromie]

0001-bug.h-add-BUILD_BUG_DECL-usable-at-file-scope.patch
0002-bug.h-add-test-demo-module.patch (DONT APPLY)

this patch (0001) adds new bug.h macro, BUILD_BUG_DECL(name, cond),
which unlike other *BUG* assertions is usable at file scope.  Its
primary purpose is to enforce identical sizes of 2 separate arrays,
which but for considerations of packing/padding/section, would be
together in a struct.

const char const *names[] = { "bart", "lisa", "homer", "marge" };
int a[] = {1,2,3,4};
int b[] = {1,2,3,5};
long d[] = {1,2};

BUILD_BUG_DECL(foo, ARRAY_SIZE(a) != ARRAY_SIZE(b));
BUILD_BUG_DECL(buz, sizeof(a) != sizeof(b));	// good
BUILD_BUG_DECL(a, sizeof(a) != sizeof(d));	// ok on x32, error x64
BUILD_BUG_DECL(b, ARRAY_SIZE(a) != ARRAY_SIZE(names));	// good

macro expands as:
static __attribute__ ((__section__(".init.data"))) struct {
       int BUILD_BUG_DECL_buz[1 - 2*!!(sizeof(a) != sizeof(b))];
} BUILD_BUG_DECL_buz[0] __attribute__((unused));



I wanted to ask for this in 3.4, but see CAVEATS

= its low risk (famous last words)

= has many immediate bug-prevention applications

For example (perhaps a bad one, I only eyeballed the tables
themselves): in drivers/net/wireless/b43/tables_lpphy.c, these 2
tables are the same size.  Should that be enforced ??

  static const u16 lpphy_rev0_ofdm_cck_gain_table[] = {...}
  static const u16 lpphy_rev1_ofdm_cck_gain_table[] = {...}

Whether or not this example is appropriate, I think its tautological
that there are pairs of arrays in the code that must match on length
for proper operation; this would enforce them, with trivial patches.

= other *BUG* assertions use do{}while, so they cant work at file scope.

BUILD_BUG_DECL is declarative, so it does work at file scope.  It
declares an unused 0 length array in __initdata, so shouldnt create
storage.

I sent a similar patch previously as part of dynamic-debug patches,
Jason Baron liked it, Rusty Russell thought it was redundant and not
neccessary in that context.  I dont think I adequately explained (or
even mentioned) the file-scope aspect.  Im not cc'g them, theyre both
*busy* atm.

CAVEATS

Macro includes __attribute__((unused)), but it seems ineffective.  I
also tried deref, but compiler (cc (GCC) 4.6.3 20120306 (Red Hat
4.6.3-2)) warned about it, so I yanked it.

Macro does seem to create storage, so I put it in __initdata.
However, referencing it from a non--init function doesnt give a
compiler warning.  Further, calling that function after boot gives a
paging-request error, and traceback.  See patch 0002.

IIUC, this is expected, since __initdata has been dropped after boot
is complete.  What I dont get is why the compiler allowed the
references; I've seen __initdata/section warnings for similar problems
in the past.


thanks
Jim Cromie

[PATCH 1/2] bug.h: add BUILD_BUG_DECL, usable at file scope

[Jim Cromie]

This adds BUILD_BUG_DECL, primarily to check sizes of file-scoped
arrays etc:

  const char const *names[] = { "bart", "lisa", "homer", "marge" };
  int a[] = {1,2,3,4}, b[] = {1,2,3,5}, c[] = {1,2}
  long d[] = {1,2};
  BUILD_BUG_DECL(Luke, sizeof(a) != sizeof(d));			// ok, but iffy usage
  BUILD_BUG_DECL(Han, sizeof(a) != sizeof(b));			// good
  BUILD_BUG_DECL(Obi, ARRAY_SIZE(a) != ARRAY_SIZE(b));		// better
  BUILD_BUG_DECL(Yoda, ARRAY_SIZE(a) != ARRAY_SIZE(names));	// good, on different types
  BUILD_BUG_DECL(Darth, sizeof(a) != sizeof(c));		// compile err

example 1 expands as:
    static __attribute__ ((__section__(".init.data"))) struct {
       int BUILD_BUG_DECL_Luke[1 - 2*!!(sizeof(a) != sizeof(b))];
    } BUILD_BUG_DECL_Luke[0]  __attribute__((unused));

The name parameter distinguishes multiple uses in the same scope, but
is otherwise arbitrary.  You can reuse the name of one of the checked
vars, or pick something easy to find on the rare occaision when the
assertion breaks the build.

example 5 yields:
    error: size of array ‘BUILD_BUG_DECL_Darth’ is negative

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 include/linux/bug.h |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index 72961c3..c76e6f6 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -17,6 +17,7 @@ struct pt_regs;
 #define BUILD_BUG_ON_NULL(e) ((void*)0)
 #define BUILD_BUG_ON(condition)
 #define BUILD_BUG() (0)
+#define BUILD_BUG_DECL(name, condition)
 #else /* __CHECKER__ */
 
 /* Force a compilation error if a constant expression is not a power of 2 */
@@ -70,6 +71,20 @@ extern int __build_bug_on_failed;
 		__build_bug_failed();				\
 	} while (0)
 
+/**
+ * BUILD_BUG_DECL - check declared objects
+ * @name: distinguishes multiple uses at same scope.
+ * @cond: false expr, typically like sizeof(a) != sizeof(b)
+ *
+ * This works at file-scope too, and supports checks like:
+ * BUILD_BUG_DECL(foo, sizeof(a) != sizeof(b));
+ * BUILD_BUG_DECL(id_strings, ARRAY_SIZE(id_strings) != ARRAY_SIZE(id_vals));
+ */
+#define BUILD_BUG_DECL(name, cond)				\
+	static __initdata struct {				\
+		int BUILD_BUG_DECL_ ## name[1 - 2*!!(cond)];	\
+	} BUILD_BUG_DECL_ ##name[0] __attribute__((unused))
+
 #endif	/* __CHECKER__ */
 
 #ifdef CONFIG_GENERIC_BUG
-- 
1.7.7.6

=?y?q?=5BPATCH=202/2=5D=20bug=2Eh=3A=20add=20test/demo=20module?=

[Jim Cromie]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 11199 bytes --]

add drivers/misc/build-asserts.c to test BUILD_BUG_DECL.

BUILD_BUG_DECL works as intended, but __attribute((unused)) appears to
be ineffective in the macro:

Code run at __init time is able to reference the storage w/o warnings
(no section complaints, even though those functions are not explicitly
marked as __init - I guess thats inferred..).

Code run from mod-param getter/setter callbacks pukes badly.
I guess this is to be expected, since __initdata is dropped
once boot is complete.  But why no compile warnings ??

root@voyage:~# cat /sys/module/build_asserts/parameters/*
build_asserts.cbint_get: got bufp:c65cd000='' kp:c8bb967c
build_asserts.cbint_get: kp-name:cbint
build_asserts.cbint_get: kp-arg:c8bb9113
build_asserts.cbint_get: kp-ops:c8bb973c
build_asserts.cbint_get: kp-ops-set:c8bb9179
build_asserts.cbint_get: kp-ops-get:c8bb9000
build_asserts.cbint_get: cbint_get me:c8bb9000
BUG: unable to handle kernel paging request at c8bbb000
IP: [<c8bb90d8>] cbint_get+0xd8/0x113 [build_asserts]
*pde = 06c76067 *pte = 00000000
Oops: 0000 [#1] PREEMPT
Modules linked in: build_asserts scx200_gpio scx200_hrt pc8736x_gpio nsc_gpio pc87360 hwmon_vid scx200_acb acx_mac80211(O) arc4 rtl8180 mac80211 eeprom_93cx6 scx200 cfg80211 ohci_hcd pata_sc1200 [last unloaded: build_asserts]

Pid: 1364, comm: cat Tainted: G           O 3.4.0-rc1-ske+ #24
EIP: 0060:[<c8bb90d8>] EFLAGS: 00010282 CPU: 0
EIP is at cbint_get+0xd8/0x113 [build_asserts]
EAX: 00000035 EBX: c65cd000 ECX: c7450000 EDX: 00000003
ESI: c8bb967c EDI: c65cd000 EBP: c7451efc ESP: c7451ee0
 DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068
CR0: 8005003b CR2: c8bbb000 CR3: 066b7000 CR4: 00000000
DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
DR6: ffff0ff0 DR7: 00000400
Process cat (pid: 1364, ti=c7450000 task=c6524f80 task.ti=c7450000)
Stack:
 c8bb945e c8bb96bc c8bb9000 c65cd000 c8bb967c ffffffff c675c270 c7451f10
 c012f536 c675c270 c8bb9790 c012f503 c7451f24 c012f35b c6507740 c60deb40
 c043eb28 c7451f70 c02010d5 c6753680 00000000 c7451f44 c027fe90 c6753680
Call Trace:
 [<c8bb9000>] ? 0xc8bb8fff
 [<c012f536>] param_attr_show+0x33/0x57
 [<c012f503>] ? __kernel_param_unlock+0x14/0x14
 [<c012f35b>] module_attr_show+0x21/0x26
 [<c02010d5>] sysfs_read_file+0x9e/0x154
 [<c027fe90>] ? security_file_permission+0x27/0x2b
 [<c01bb5ee>] ? rw_verify_area+0xd0/0xf2
 [<c01bb956>] vfs_read+0x8d/0xd3
 [<c0201037>] ? sysfs_open_file+0x1fa/0x1fa
 [<c01bb9de>] sys_read+0x42/0x63
 [<c043592c>] syscall_call+0x7/0xb
Code: 04 24 3a 94 bb c8 89 44 24 08 e8 89 7c 87 f7 c7 44 24 08 00 90 bb c8 c7 44 24 04 bc 96 bb c8 c7 04 24 5e 94 bb c8 e8 6d 7c 87 f7 <a1> 00 b0 bb c8 c7 44 24 04 bc 96 bb c8 89 44 24 0c 89 44 24 08
EIP: [<c8bb90d8>] cbint_get+0xd8/0x113 [build_asserts] SS:ESP 0068:c7451ee0
CR2: 00000000c8bbb000
---[ end trace bfbcc6aee803d03b ]---

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 drivers/misc/Makefile        |    2 +
 drivers/misc/build-asserts.c |  239 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 241 insertions(+), 0 deletions(-)
 create mode 100644 drivers/misc/build-asserts.c

diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 3e1d8010..5aff6bb 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -49,3 +49,5 @@ obj-y				+= carma/
 obj-$(CONFIG_USB_SWITCH_FSA9480) += fsa9480.o
 obj-$(CONFIG_ALTERA_STAPL)	+=altera-stapl/
 obj-$(CONFIG_MAX8997_MUIC)	+= max8997-muic.o
+
+obj-m			+= build-asserts.o
diff --git a/drivers/misc/build-asserts.c b/drivers/misc/build-asserts.c
new file mode 100644
index 0000000..95e8567
--- /dev/null
+++ b/drivers/misc/build-asserts.c
@@ -0,0 +1,239 @@
+
+#define pr_fmt(fmt)  KBUILD_MODNAME ".%s: " fmt, __FUNCTION__
+
+#include <linux/kernel.h>
+#include <linux/bug.h>
+#include <linux/stringify.h>
+#include <linux/module.h>
+#include <linux/init.h>
+
+int a[] = {1,2,3,4};
+int b[] = {1,2,3,5};
+int c[] = {3,5};
+long d[] = {1,2};
+
+const char *names[] = { "bart", "lisa", "homer", "marge" };
+
+struct bridge {
+	int member_ids[4];
+	char *names[4];
+	char *snacks[3];	// hungry ?
+} clubs[4];
+
+BUILD_BUG_DECL(foo, ARRAY_SIZE(a) != ARRAY_SIZE(b));
+BUILD_BUG_DECL(buz, sizeof(a) != sizeof(b));		// good
+// BUILD_BUG_DECL(a, sizeof(a) != sizeof(d));		// brittle
+BUILD_BUG_DECL(b, ARRAY_SIZE(a) != ARRAY_SIZE(names));	// good, on different types
+
+// BUILD_BUG_DECL(d, ARRAY_SIZE(a) != ARRAY_SIZE(d));	// compile err, correct
+// BUILD_BUG_DECL(bonk, sizeof(a) != sizeof(names));	// breaks, correct
+// BUILD_BUG_DECL(ac, sizeof(a) != sizeof(c));		// err
+
+
+BUILD_BUG_DECL(joe, ARRAY_SIZE(a) != ARRAY_SIZE(clubs));
+BUILD_BUG_DECL(bob, ARRAY_SIZE(a) != ARRAY_SIZE(clubs[0].member_ids));
+BUILD_BUG_DECL(mike, ARRAY_SIZE(a) != ARRAY_SIZE(clubs[0].names));
+
+// BUILD_BUG_DECL(sue, ARRAY_SIZE(a) != ARRAY_SIZE(clubs[0].snacks));
+// cant use typeof like this
+// BUILD_BUG_DECL(ike, typeof(clubs[0].snacks) != typeof(clubs[0].names));
+
+union {
+	int a;
+	long b;
+	char c;
+	char *s;
+	int arr[4];
+} pba;
+
+// these cause warning at file scope, but ok inside fn.
+// warning: variably modified ‘field’ at file scope [enabled by default]
+// BUILD_BUG_DECL(x, (void*)&pba.a != (void*)&pba.b);
+// BUILD_BUG_DECL(y, (void*)&pba.a != (void*)&pba.c);
+
+int cmp_ints(int sz, int *arg1, int *arg2)
+{
+	int i;
+
+	// NG: compiler doesnt know the arrays passed.
+	// BUILD_BUG_DECL(y, ARRAY_SIZE(*arg1) != ARRAY_SIZE(*arg2));
+
+	pr_notice("all equal ?\n");
+	for (i=0; i<sz; i++) {
+		pr_notice("%d ?= %d\n", arg1[i], arg2[i]);
+		if (arg1[i] != arg2[i]) {
+			pr_notice("nope\n");
+			return 1;
+		}
+	}
+	pr_notice("yup\n");
+	i = BUILD_BUG_DECL_mike[0].BUILD_BUG_DECL_mike[0];
+	pr_notice("i-mike %d\n", i);
+	return 0;
+}
+
+// ok, but hmm
+BUILD_BUG_DECL(fn, sizeof(cmp_ints) != sizeof(cmp_ints));
+
+int use_cmp_ints(void)
+{
+	int i = 20, j = 30;
+
+	BUILD_BUG_DECL(foo, ARRAY_SIZE(a) != ARRAY_SIZE(b));
+	BUILD_BUG_DECL(a, (void*)&pba.a != (void*)&pba.b);
+
+	pr_notice("hello\n");
+	cmp_ints(4, a, b);
+
+	pr_notice("i:%d j:%d\n", i, j);
+
+	// derefs w/o error !!
+	i = BUILD_BUG_DECL_a[0].BUILD_BUG_DECL_a[0];
+	j = BUILD_BUG_DECL_mike[0].BUILD_BUG_DECL_mike[0];
+
+	pr_notice("access a? %d, %d\n", i,
+		BUILD_BUG_DECL_a[0].BUILD_BUG_DECL_a[0]);
+	pr_notice("access mike? %d, %d\n", i,
+		BUILD_BUG_DECL_mike[0].BUILD_BUG_DECL_mike[0]);
+
+	//BUILD_BUG_DECL_a[0].a_sizecheck = 1;
+	return 0;
+}
+// ok, but obviously useless
+BUILD_BUG_DECL(fn2, sizeof(cmp_ints) != sizeof(use_cmp_ints));
+
+/*
+  for some reason, refs to BUILD_BUG_DECL_* declarations works in
+  *cmp_ints(), despite unused attribute and placement in __initdata.
+  Maybe this is cuz *cmp_ints() is (so far) called from module_init,
+  where it should work.
+
+  So code below adds mod-params and getter/setters to try to invoke it
+  during runtime, to see what happens.
+
+  Apparently what happens is kernel fault.  That makes some sense
+  given that __init is no longer current, and has been dropped.  But
+  why is referencing initdata not flagged by compiler ??
+
+root@voyage:~# cat /sys/module/build_asserts/parameters/*
+build_asserts.cbint_get: got bufp:c65cd000='' kp:c8bb967c
+build_asserts.cbint_get: kp-name:cbint
+build_asserts.cbint_get: kp-arg:c8bb9113
+build_asserts.cbint_get: kp-ops:c8bb973c
+build_asserts.cbint_get: kp-ops-set:c8bb9179
+build_asserts.cbint_get: kp-ops-get:c8bb9000
+build_asserts.cbint_get: cbint_get me:c8bb9000
+BUG: unable to handle kernel paging request at c8bbb000
+IP: [<c8bb90d8>] cbint_get+0xd8/0x113 [build_asserts]
+*pde = 06c76067 *pte = 00000000
+Oops: 0000 [#1] PREEMPT
+Modules linked in: build_asserts scx200_gpio scx200_hrt pc8736x_gpio nsc_gpio pc87360 hwmon_vid scx200_acb acx_mac80211(O) arc4 rtl8180 mac80211 eeprom_93cx6 scx200 cfg80211 ohci_hcd pata_sc1200 [last unloaded: build_asserts]
+
+Pid: 1364, comm: cat Tainted: G           O 3.4.0-rc1-ske+ #24
+EIP: 0060:[<c8bb90d8>] EFLAGS: 00010282 CPU: 0
+EIP is at cbint_get+0xd8/0x113 [build_asserts]
+EAX: 00000035 EBX: c65cd000 ECX: c7450000 EDX: 00000003
+ESI: c8bb967c EDI: c65cd000 EBP: c7451efc ESP: c7451ee0
+ DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068
+CR0: 8005003b CR2: c8bbb000 CR3: 066b7000 CR4: 00000000
+DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
+DR6: ffff0ff0 DR7: 00000400
+Process cat (pid: 1364, ti=c7450000 task=c6524f80 task.ti=c7450000)
+Stack:
+ c8bb945e c8bb96bc c8bb9000 c65cd000 c8bb967c ffffffff c675c270 c7451f10
+ c012f536 c675c270 c8bb9790 c012f503 c7451f24 c012f35b c6507740 c60deb40
+ c043eb28 c7451f70 c02010d5 c6753680 00000000 c7451f44 c027fe90 c6753680
+Call Trace:
+ [<c8bb9000>] ? 0xc8bb8fff
+ [<c012f536>] param_attr_show+0x33/0x57
+ [<c012f503>] ? __kernel_param_unlock+0x14/0x14
+ [<c012f35b>] module_attr_show+0x21/0x26
+ [<c02010d5>] sysfs_read_file+0x9e/0x154
+ [<c027fe90>] ? security_file_permission+0x27/0x2b
+ [<c01bb5ee>] ? rw_verify_area+0xd0/0xf2
+ [<c01bb956>] vfs_read+0x8d/0xd3
+ [<c0201037>] ? sysfs_open_file+0x1fa/0x1fa
+ [<c01bb9de>] sys_read+0x42/0x63
+ [<c043592c>] syscall_call+0x7/0xb
+
+ */
+
+int decl_user(void)
+{
+	// BUILD_BUG_DECL(ac, sizeof(a) != sizeof(c));	// errors, properly
+
+	// no complaints testing the union from here.
+	BUILD_BUG_DECL(x, (void*)&pba.a != (void*)&pba.c);
+	// fine for use inside a struct
+	BUILD_BUG_DECL(y, ARRAY_SIZE(clubs[1].names)
+		       != ARRAY_SIZE(clubs[0].member_ids));
+
+	return 0;
+}
+
+// not usable as a mod-param, boot-only I guess
+// also, gives unused warning at compile ?
+static int __init bar_setup(char *str)
+{
+	pr_notice("bar setup: %s\n", str);
+	decl_user();
+	use_cmp_ints();
+	return 1;
+}
+__setup("bar=", bar_setup);
+
+static int cbvar = 0;
+static int cbint_set(const char *val, const struct kernel_param *kp)
+{
+	int i;
+	int err = param_set_int(val, kp);
+
+	pr_notice("err: %d val:%s", err, val);
+        if (err)
+                return err;
+
+	i = BUILD_BUG_DECL_mike[0].BUILD_BUG_DECL_mike[0];
+	pr_notice("access mike? %d, %d\n", i,
+		BUILD_BUG_DECL_mike[0].BUILD_BUG_DECL_mike[0]);
+
+        return 0;
+}
+static int cbint_get(char *buffer, const struct kernel_param *kp)
+{
+	int i;
+
+        pr_notice("got bufp:%p='%s' kp:%p\n", buffer, buffer, kp);
+        pr_notice("kp-name:%s\n", kp->name);
+        pr_notice("kp-arg:%p\n", kp->arg);
+        pr_notice("kp-ops:%p\n", kp->ops);
+        pr_notice("kp-ops-set:%p\n", kp->ops->set);
+        pr_notice("kp-ops-get:%p\n", kp->ops->get);
+        pr_notice("cbint_get me:%p\n", &cbint_get);
+
+	i = BUILD_BUG_DECL_mike[0].BUILD_BUG_DECL_mike[0];
+	pr_notice("access mike? %d, %d\n", i,
+		BUILD_BUG_DECL_mike[0].BUILD_BUG_DECL_mike[0]);
+
+	strcpy(buffer, "hello from cbint_get");
+        return strlen(buffer);
+}
+static struct kernel_param_ops cbint_ops = {
+        .set = cbint_set,
+        .get = cbint_get,  // need one, no default provided
+};
+module_param_cb(cbint, &cbint_ops, cbint_cb, 0644);
+
+static int fooint;
+module_param_named(foo, fooint, int, 0644);
+
+void myexit(void)
+{
+	pr_notice("bye\n");
+}
+
+module_init(use_cmp_ints);
+module_exit(myexit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("jim.cromie@gmail.com");
+MODULE_DESCRIPTION("test BUILD_BUG_DECL");
-- 
1.7.7.6

Re: [00/02] add BUILD_BUG_DECL assertion (for 3.4??)

[Joe Perches]

On Sun, 2012-04-08 at 16:38 -0600, Jim Cromie wrote:

> this patch (0001) adds new bug.h macro, BUILD_BUG_DECL(name, cond),
> which unlike other *BUG* assertions is usable at file scope.  Its
> primary purpose is to enforce identical sizes of 2 separate arrays,
> which but for considerations of packing/padding/section, would be
> together in a struct.
> 
> const char const *names[] = { "bart", "lisa", "homer", "marge" };
> int a[] = {1,2,3,4};
> int b[] = {1,2,3,5};
> long d[] = {1,2};
> 
> BUILD_BUG_DECL(foo, ARRAY_SIZE(a) != ARRAY_SIZE(b));
> BUILD_BUG_DECL(buz, sizeof(a) != sizeof(b));	// good
> BUILD_BUG_DECL(a, sizeof(a) != sizeof(d));	// ok on x32, error x64
> BUILD_BUG_DECL(b, ARRAY_SIZE(a) != ARRAY_SIZE(names));	// good
> 
> macro expands as:
> static __attribute__ ((__section__(".init.data"))) struct {
>        int BUILD_BUG_DECL_buz[1 - 2*!!(sizeof(a) != sizeof(b))];
> } BUILD_BUG_DECL_buz[0] __attribute__((unused));
> 

If possible, it might be better to wrap the
declarations themselves in a macro that ensures
the sizes are the same.

Something like:

declare_same_size_arrays(
	typeof array1[] = {...},
	typeof array2[] = {...}
);

Re: [00/02] add BUILD_BUG_DECL assertion (for 3.4??)

[Jim Cromie]

On Sun, Apr 8, 2012 at 4:52 PM, Joe Perches <joe@perches.com> wrote:
> On Sun, 2012-04-08 at 16:38 -0600, Jim Cromie wrote:
>
>> this patch (0001) adds new bug.h macro, BUILD_BUG_DECL(name, cond),
>> which unlike other *BUG* assertions is usable at file scope.  Its
>> primary purpose is to enforce identical sizes of 2 separate arrays,
>> which but for considerations of packing/padding/section, would be
>> together in a struct.
>>
>> const char const *names[] = { "bart", "lisa", "homer", "marge" };
>> int a[] = {1,2,3,4};
>> int b[] = {1,2,3,5};
>> long d[] = {1,2};
>>
>> BUILD_BUG_DECL(foo, ARRAY_SIZE(a) != ARRAY_SIZE(b));
>> BUILD_BUG_DECL(buz, sizeof(a) != sizeof(b));  // good
>> BUILD_BUG_DECL(a, sizeof(a) != sizeof(d));    // ok on x32, error x64
>> BUILD_BUG_DECL(b, ARRAY_SIZE(a) != ARRAY_SIZE(names));        // good
>>
>> macro expands as:
>> static __attribute__ ((__section__(".init.data"))) struct {
>>        int BUILD_BUG_DECL_buz[1 - 2*!!(sizeof(a) != sizeof(b))];
>> } BUILD_BUG_DECL_buz[0] __attribute__((unused));
>>
>
> If possible, it might be better to wrap the
> declarations themselves in a macro that ensures
> the sizes are the same.
>
> Something like:
>
> declare_same_size_arrays(
>        typeof array1[] = {...},
>        typeof array2[] = {...}
> );
>
>

Unless Im mis-reading you, this has a couple disadvantages:

- bigger patches where its added.
granted these are mostly whitespace, but theyre less trivial to inspect.

- not useful for array definitions which are not contiguous
granted thats a minority case, and the definitions could often be moved,
but not always.

WAG: If *80211 code were to export tables that drivers could use,
(say standardized frequency, channel stuff)
it would be helpful if the drivers could assert that their private tables match
the exported ones.  (I havent looked in *80211, or tried using
BUILD_BUG_DECL this way)

Do you see advantages other than stylistic ones ?

Re: [00/02] add BUILD_BUG_DECL assertion (for 3.4??)

[Joe Perches]

On Sun, 2012-04-08 at 17:59 -0600, Jim Cromie wrote:
> On Sun, Apr 8, 2012 at 4:52 PM, Joe Perches <joe@perches.com> wrote:
> > On Sun, 2012-04-08 at 16:38 -0600, Jim Cromie wrote:
> >
> >> this patch (0001) adds new bug.h macro, BUILD_BUG_DECL(name, cond),
> >> which unlike other *BUG* assertions is usable at file scope.  Its
> >> primary purpose is to enforce identical sizes of 2 separate arrays,
> >> which but for considerations of packing/padding/section, would be
> >> together in a struct.
> >>
> >> const char const *names[] = { "bart", "lisa", "homer", "marge" };
> >> int a[] = {1,2,3,4};
> >> int b[] = {1,2,3,5};
> >> long d[] = {1,2};
> >>
> >> BUILD_BUG_DECL(foo, ARRAY_SIZE(a) != ARRAY_SIZE(b));
> >> BUILD_BUG_DECL(buz, sizeof(a) != sizeof(b));  // good
> >> BUILD_BUG_DECL(a, sizeof(a) != sizeof(d));    // ok on x32, error x64
> >> BUILD_BUG_DECL(b, ARRAY_SIZE(a) != ARRAY_SIZE(names));        // good
> >>
> >> macro expands as:
> >> static __attribute__ ((__section__(".init.data"))) struct {
> >>        int BUILD_BUG_DECL_buz[1 - 2*!!(sizeof(a) != sizeof(b))];
> >> } BUILD_BUG_DECL_buz[0] __attribute__((unused));
> >>
> >
> > If possible, it might be better to wrap the
> > declarations themselves in a macro that ensures
> > the sizes are the same.
> >
> > Something like:
> >
> > declare_same_size_arrays(
> >        typeof array1[] = {...},
> >        typeof array2[] = {...}
> > );
> >
> >
> 
> Unless Im mis-reading you, this has a couple disadvantages:
> 
> - bigger patches where its added.
> granted these are mostly whitespace, but theyre less trivial to inspect.

Not a problem in my view, it moves the
related declarations closer together.

> - not useful for array definitions which are not contiguous
> granted thats a minority case, and the definitions could often be moved,
> but not always.

Discontiguous array definitions must be ugly.

> Do you see advantages other than stylistic ones ?

Not really.

Contiguous declarations.
No need for other markings.

Seems useful enough.

Re: [00/02] add BUILD_BUG_DECL assertion (for 3.4??)

[Jim Cromie]

On Sun, Apr 8, 2012 at 8:37 PM, Joe Perches <joe@perches.com> wrote:
> On Sun, 2012-04-08 at 17:59 -0600, Jim Cromie wrote:
>> On Sun, Apr 8, 2012 at 4:52 PM, Joe Perches <joe@perches.com> wrote:
>> > On Sun, 2012-04-08 at 16:38 -0600, Jim Cromie wrote:
>> >
>> >> this patch (0001) adds new bug.h macro, BUILD_BUG_DECL(name, cond),
>> >> which unlike other *BUG* assertions is usable at file scope.  Its
>> >> primary purpose is to enforce identical sizes of 2 separate arrays,
>> >> which but for considerations of packing/padding/section, would be
>> >> together in a struct.


>> >> BUILD_BUG_DECL(b, ARRAY_SIZE(a) != ARRAY_SIZE(names));        // good

>> >
>> > If possible, it might be better to wrap the
>> > declarations themselves in a macro that ensures
>> > the sizes are the same.
>> >
>> > Something like:
>> >
>> > declare_same_size_arrays(
>> >        typeof array1[] = {...},
>> >        typeof array2[] = {...}
>> > );
>> >
>> >
>>
>> Unless Im mis-reading you, this has a couple disadvantages:
>>
>> - bigger patches where its added.
>> granted these are mostly whitespace, but theyre less trivial to inspect.
>
> Not a problem in my view, it moves the
> related declarations closer together.
>
>> - not useful for array definitions which are not contiguous
>> granted thats a minority case, and the definitions could often be moved,
>> but not always.
>
> Discontiguous array definitions must be ugly.

Ugly ?
too pejorative IMO

each array defn is a single statement.
there may be functions between the 2 defns being compared.

Maybe not ideal,

>
>> Do you see advantages other than stylistic ones ?
>
> Not really.
>
> Contiguous declarations.
> No need for other markings.
>
> Seems useful enough.
>

OK.  Id expect your construct to be built upon mine,
we'd still need to start with something.

Also, mine is usable for things yours isnt.
I dont have a good example, but a simple/silly one is:

BUILD_BUG_DECL( wifi_channels_must_be_14,
                  ARRAY_SIZE(channels_table) == 14 );

Re: [00/02] add BUILD_BUG_DECL assertion (for 3.4??)

[Joe Perches]

On Mon, 2012-04-09 at 13:52 -0600, Jim Cromie wrote:
> On Sun, Apr 8, 2012 at 8:37 PM, Joe Perches <joe@perches.com> wrote:
> > Discontiguous array definitions must be ugly.
> Ugly ?
> too pejorative IMO

Ugly code is not a pejorative, it's more an
artifact of creation and always a beholder
issue.

> each array defn is a single statement.

I thought you meant the array entries not
the arrays themselves.

> there may be functions between the 2 defns being compared.
> 
> Maybe not ideal,
> 
> >
> >> Do you see advantages other than stylistic ones ?
> >
> > Not really.
> >
> > Contiguous declarations.
> > No need for other markings.
> >
> > Seems useful enough.
> >
> 
> OK.  Id expect your construct to be built upon mine,
> we'd still need to start with something.
> 
> Also, mine is usable for things yours isnt.
> I dont have a good example, but a simple/silly one is:
> 
> BUILD_BUG_DECL( wifi_channels_must_be_14,
>                   ARRAY_SIZE(channels_table) == 14 );

Do what you think best.

I think it's a solution for a relatively
minor problem.

The BUILD_BUG_DECL marking might be improved.
BUILD_BUG_DECL might be a bit short or not
descriptive enough.

Maybe BUILD_BUG_DIFF_SIZE or BUILD_BUG_SIZE_NE?