Re: [GIT] Networking

[Alejandro Colomar]

On Thu, Sep 3, 2015 at 11:31 AM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:

>> [-Wsizeof-array-argument]

> Ahh. Google shows that it's an old clang warning that gcc has recently
> picked up.

> But even clang doesn't seem to have any way for a project to say
> "please warn about arrays in function argument declaration". It *is*
> very traditional idiomatic C, it's just that I personally think it's
> one of those bad traditional C things exactly because it's so
> misleading about what actually goes on. But I guess that in practice,
> the only thing that it actually *affects* is "sizeof" (and assignment
> to the variable name - something that would be invalid for a real
> array, but works on argument arrays because they are really just
> pointers).

> The "array as function argument" syntax is occasionally useful
> (particularly for the multi-dimensional array case), so I very much
> understand why it exists, I just think that in the kernel we'd be
> better off with the rule that it's against our coding practices.

>                   Linus


Hi Linus,

First of all, this is my first message to this mailing list, and I'm
trying to reply to a very old thread, so sorry if I don't know how I
should do it.

I have a different approach in my code to avoid that whole class of bugs
relating sizeof and false arrays in function argument declarations.
I do like the sintactic sugar that they provide, so I decided to ban
"sizeof(array)" completely off my code.

I have developed the following macro:

#define ARRAY_BYTES(arr)	(sizeof((arr)[0]) * ARRAY_SIZE(arr))

which compiles to a simple "sizeof(arr)" by undoing the division in
"ARRAY_SIZE()", but with the added benefit that it checks that the
argument is an array (due to "ARRAY_SIZE()"), and if not, compilation
breaks which means that the array is not an array but a pointer.

My rules are:

 - Size of an array (number of elements):
	ARRAY_SIZE(arr)
 - Signed size of an array (normally for loops where I compare against a
 signed variable):
	ARRAY_SSIZE(arr)	defined as: ((ptrdiff_t)ARRAY_SIZE(arr))
 - Size of an array in bytes (normally for buffers):
	ARRAY_BYTES(arr)

No use of "sizeof" is allowed for arrays, which completely rules
out bugs of that class, because I never pass an array to "sizeof", which
is the core of the problem.  I've been using those macros in my code for
8 months, and they work really nice.

I propose to include the macro "ARRAY_BYTES()" in <linux/kernel.h> just
after "ARRAY_SIZE()" and replace every appearance of "sizeof(array)" in
Linux by "ARRAY_BYTES(array)", and modify the coding style guide to ban
"sizeof(array)" completely off the kernel.

Below is a patch with two commits:  one that adds the macro to
<linux/kernel.h>, and another one that serves as an example of usage
for the macro (that one is just as an example).


		Alex.

Please CC me <colomar.6.4.3@xxxxxxxxx> in any response to this thread.

From b5b674d39b28e703300698fa63e4ab4be646df8f Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <colomar.6.4.3@xxxxxxxxx>
Date: Sun, 5 Apr 2020 01:45:35 +0200
Subject: [PATCH 1/2] linux/kernel.h: add ARRAY_BYTES() macro

---
 include/linux/kernel.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 9b7a8d74a9d6..dc806e2a7799 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -46,6 +46,12 @@
  */
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + 
__must_be_array(arr))

+/**
+ * ARRAY_BYTES - get the number of bytes in array @arr
+ * @arr: array to be sized
+ */
+#define ARRAY_BYTES(arr)	(sizeof((arr)[0]) * ARRAY_SIZE(arr))
+
 #define u64_to_user_ptr(x) (		\
 {					\
 	typecheck(u64, (x));		\
--
2.25.1


From 3e7bcf70b708b51a7807c336c5d1b01403989d3b Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <colomar.6.4.3@xxxxxxxxx>
Date: Sun, 5 Apr 2020 01:48:17 +0200
Subject: [PATCH 2/2] block, bfq: Use ARRAY_BYTES() for arrays instead of
 sizeof()

---
 block/bfq-cgroup.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
index 68882b9b8f11..51ba9b9a8855 100644
--- a/block/bfq-cgroup.c
+++ b/block/bfq-cgroup.c
@@ -7,6 +7,7 @@
 #include <linux/blkdev.h>
 #include <linux/cgroup.h>
 #include <linux/elevator.h>
+#include <linux/kernel.h>
 #include <linux/ktime.h>
 #include <linux/rbtree.h>
 #include <linux/ioprio.h>
@@ -794,7 +795,8 @@ void bfq_bic_update_cgroup(struct bfq_io_cq *bic, 
struct bio *bio)
 	 * refcounter for bfqg, to let it disappear only after no
 	 * bfq_queue refers to it any longer.
 	 */
-	blkg_path(bfqg_to_blkg(bfqg), bfqg->blkg_path, sizeof(bfqg->blkg_path));
+	blkg_path(bfqg_to_blkg(bfqg), bfqg->blkg_path,
+						ARRAY_BYTES(bfqg->blkg_path));
 	bic->blkcg_serial_nr = serial_nr;
 out:
 	rcu_read_unlock();
--
2.25.1