there are numerous places throughout the source tree that apparently
calculate the size of an array using the construct
"sizeof(fubar)/sizeof(fubar[0])". see for yourself:
$ grep -Er "sizeof\((.*)\) ?/ ?sizeof\(\1\[0\]\)" *
but we already have, from "include/linux/kernel.h":
#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
a simple script to make the appropriate cleanup, given the directory
name as an argument:
================== cut ================
#!/bin/sh
DIR=$1
for f in $(grep -Erl "sizeof\((.*)\) ?/ ?sizeof\(\1\[0\]\)" ${DIR}) ;
do
echo "Fixing $f ..."
perl -pi -e "s|sizeof\((.*)\) ?/ ?sizeof\(\1\[0\]\)|ARRAY_SIZE\(\1\)|" $f
done
=======================================
of course, the file must (eventually) include linux/kernel.h but one
would think that applies to the majority of the source tree, no?
just a thought.
rday
On Thu, 14 Dec 2006, Zach Brown wrote:
> > there are numerous places throughout the source tree that
> > apparently calculate the size of an array using the construct
> > "sizeof(fubar)/sizeof(fubar[0])". see for yourself:
> > $ grep -Er "sizeof\((.*)\) ?/ ?sizeof\(\1\[0\]\)" *
>
> Indeed, there seems to be lots of potential clean-up there.
> Including duplicate macros like:
>
> ./drivers/ide/ide-cd.h:#define ARY_LEN(a) ((sizeof(a) / sizeof(a[0])))
not surprisingly, i have a script "arraysize.sh":
============================================================
#!/bin/sh
DIR=$1
# grep -Er "sizeof\((.*)\) ?/ ?sizeof\(\1\[0\]\)" ${DIR}
# grep -Erl "sizeof\((.*)\) ?/ ?sizeof\(\1\[0\]\)" ${DIR}
for f in $(grep -Erl "sizeof\((.*)\) ?/ ?sizeof\(\1\[0\]\)" ${DIR}) ; do
echo "ARRAY_SIZE()ing $f ..."
perl -pi -e "s|sizeof\((.*)\) ?/ ?sizeof\(\1\[0\]\)|ARRAY_SIZE\(\1\)|" $f
done
===========================================================
anyone who's interested can run it with a single argument of the
directory to process, eg.:
$ arraysize.sh fs
$ arraysize.sh drivers
$ arraysize.sh . # entire tree, of course
it's just a first pass, but it seems to produce reasonable code.
rday
Robert P. J. Day wrote:
> On Thu, 14 Dec 2006, Zach Brown wrote:
...
>> Indeed, there seems to be lots of potential clean-up there.
>> Including duplicate macros like:
>>
>> ./drivers/ide/ide-cd.h:#define ARY_LEN(a) ((sizeof(a) / sizeof(a[0])))
>
> not surprisingly, i have a script "arraysize.sh":
...
This could also come in the flavor "sizeof(a) / sizeof(*a)".
I haven't checked if there are actual instances.
--
Stefan Richter
-=====-=-==- ==-- -====
http://arcgraph.de/sr/
On 12/13/06, Robert P. J. Day <[email protected]> wrote:
>
> there are numerous places throughout the source tree that apparently
> calculate the size of an array using the construct
> "sizeof(fubar)/sizeof(fubar[0])". see for yourself:
>
> $ grep -Er "sizeof\((.*)\) ?/ ?sizeof\(\1\[0\]\)" *
>
> but we already have, from "include/linux/kernel.h":
>
> #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
Maybe *(x) instead of (x)[0]?
--
Miguel Ojeda
http://maxextreme.googlepages.com/index.htm
On Fri, 15 Dec 2006, Miguel Ojeda wrote:
> On 12/13/06, Robert P. J. Day <[email protected]> wrote:
> >
> > there are numerous places throughout the source tree that apparently
> > calculate the size of an array using the construct
> > "sizeof(fubar)/sizeof(fubar[0])". see for yourself:
> >
> > $ grep -Er "sizeof\((.*)\) ?/ ?sizeof\(\1\[0\]\)" *
> >
> > but we already have, from "include/linux/kernel.h":
> >
> > #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>
> Maybe *(x) instead of (x)[0]?
yeah, there's a bunch of that, too:
$ grep -Er "sizeof\((.*)\) ?/ ?sizeof\(\*\1\)" .
easy enough to catch using the same technique.
rday
>>> Indeed, there seems to be lots of potential clean-up there.
>>> Including duplicate macros like:
>>>
>>> ./drivers/ide/ide-cd.h:#define ARY_LEN(a) ((sizeof(a) / sizeof(a[0])))
>>
>> not surprisingly, i have a script "arraysize.sh":
>...
>
>This could also come in the flavor "sizeof(a) / sizeof(*a)".
>I haven't checked if there are actual instances.
Even sizeof a / sizeof *a
may happen.
-`J'
--
On Fri, 15 Dec 2006, Jan Engelhardt wrote:
>
> >>> Indeed, there seems to be lots of potential clean-up there.
> >>> Including duplicate macros like:
> >>>
> >>> ./drivers/ide/ide-cd.h:#define ARY_LEN(a) ((sizeof(a) / sizeof(a[0])))
> >>
> >> not surprisingly, i have a script "arraysize.sh":
> >...
> >
> >This could also come in the flavor "sizeof(a) / sizeof(*a)".
> >I haven't checked if there are actual instances.
>
> Even sizeof a / sizeof *a
>
> may happen.
yes, sadly, there are a number of those as well. back to the drawing
board.
rday
p.s. you know, once i nail this script, somebody better apply the end
result. :-)
On Fri, 15 Dec 2006, Robert P. J. Day wrote:
> On Fri, 15 Dec 2006, Jan Engelhardt wrote:
> > Even sizeof a / sizeof *a
> >
> > may happen.
>
> yes, sadly, there are a number of those as well. back to the drawing
> board.
It might be interesting to grep for anything that divides two sizeofs and
eyeball the result for possible mistakes. This would provide some real
benefit beyond the cosmetical changes.
Tim
Hi!
> there are numerous places throughout the source tree that apparently
> calculate the size of an array using the construct
> "sizeof(fubar)/sizeof(fubar[0])". see for yourself:
>
> $ grep -Er "sizeof\((.*)\) ?/ ?sizeof\(\1\[0\]\)" *
>
> but we already have, from "include/linux/kernel.h":
>
> #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
Hmmm. quite misleading name :-(. ARRAY_LEN would be better.
--
Thanks for all the (sleeping) penguins.
On Fri, 15 Dec 2006, Tim Schmielau wrote:
> On Fri, 15 Dec 2006, Robert P. J. Day wrote:
> > On Fri, 15 Dec 2006, Jan Engelhardt wrote:
> > > Even sizeof a / sizeof *a
> > >
> > > may happen.
> >
> > yes, sadly, there are a number of those as well. back to the drawing
> > board.
>
> It might be interesting to grep for anything that divides two
> sizeofs and eyeball the result for possible mistakes. This would
> provide some real benefit beyond the cosmetical changes.
i did that a while ago and it's amazing the variation that you find
beyond the obvious:
$ grep -Er "sizeof.*/.*sizeof" . | less
...
./net/key/af_key.c: sa->sadb_sa_len = sizeof(struct sadb_sa)/sizeof(uint64_t);
./net/xfrm/xfrm_policy.c: int len = sizeof(struct xfrm_selector) / sizeof(u32);
./net/core/flow.c: const int n_elem = sizeof(struct flowi) / sizeof(flow_compare_t);
./net/ipv4/netfilter/arp_tables.c: for (i = 0; i < sizeof(*arp)/sizeof(__u32); i++)
./net/ipv4/af_inet.c:#define INETSW_ARRAY_LEN (sizeof(inetsw_array) / sizeof(struct inet_protosw))
./drivers/net/wireless/ray_cs.c: .num_standard = sizeof(ray_handler)/sizeof(iw_handler),
and on and on. there's no way a cute little perl script is
going to clean up all of *that*.
so what to do?
rday
On Sat, 16 Dec 2006, Pavel Machek wrote:
> Hi!
>
> > there are numerous places throughout the source tree that apparently
> > calculate the size of an array using the construct
> > "sizeof(fubar)/sizeof(fubar[0])". see for yourself:
> >
> > $ grep -Er "sizeof\((.*)\) ?/ ?sizeof\(\1\[0\]\)" *
> >
> > but we already have, from "include/linux/kernel.h":
> >
> > #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>
> Hmmm. quite misleading name :-(. ARRAY_LEN would be better.
i suspect it's *way* too late to make that kind of change, given that
"ARRAY_SIZE" is firmly ensconced in countless places in the source
tree and that would be a major, disruptive change.
even *i* wouldn't try to promote that idea. :-)
rday
On Sat, 16 Dec 2006, Robert P. J. Day wrote:
> On Fri, 15 Dec 2006, Tim Schmielau wrote:
> >
> > It might be interesting to grep for anything that divides two
> > sizeofs and eyeball the result for possible mistakes. This would
> > provide some real benefit beyond the cosmetical changes.
>
> i did that a while ago and it's amazing the variation that you find
> beyond the obvious:
>
> $ grep -Er "sizeof.*/.*sizeof" . | less
>
> ...
> ./net/key/af_key.c: sa->sadb_sa_len = sizeof(struct sadb_sa)/sizeof(uint64_t);
> ./net/xfrm/xfrm_policy.c: int len = sizeof(struct xfrm_selector) / sizeof(u32);
> ./net/core/flow.c: const int n_elem = sizeof(struct flowi) / sizeof(flow_compare_t);
> ./net/ipv4/netfilter/arp_tables.c: for (i = 0; i < sizeof(*arp)/sizeof(__u32); i++)
> ./net/ipv4/af_inet.c:#define INETSW_ARRAY_LEN (sizeof(inetsw_array) / sizeof(struct inet_protosw))
> ./drivers/net/wireless/ray_cs.c: .num_standard = sizeof(ray_handler)/sizeof(iw_handler),
>
Of the above, af_inet.c and ray_cs.c seem to be good candidates for
ARRAY_SIZE. You might even remove the INETSW_ARRAY_LEN #define in
af_inet.c altogether, since ARRAY_SIZE(inetsw_array) is quite readable.
xfrm_policy.c, flow.c and arp_tables.c seem to be reasonably readable
trickery that can be left as-is.
>From a first glance, af_key.c is ok but might profit from a comment
in include/linux/pfkeyv2.h saying that sadb_msg_len is measured in 64-bit
words per RFC 2367. Though documenting the structs in pfkeyv2.h would be
quite a bit different from what you initially intended...
So, if you have some time to spend on this, manual inspection would
probably be the most useful thing, since any automatic sed tricks will
only replace what a human ready would easily understand as well.
If you manually generate cleanup patches, it would be very good to check
that compilation with allyesconfig generates identical code before and
after before feeding them through the respective maintainers.
If you want to find genuine bugs by this, it might be more worthwhile to
start with drivers/, as davem is just too clever to make any mistakes.
Not that I want to make you spend you time on this. It's just beause you
asked.
Tim
(i'm not *trying* to belabour this issue ... i am merely succeeding)
On Sat, 16 Dec 2006, Tim Schmielau wrote:
> On Sat, 16 Dec 2006, Robert P. J. Day wrote:
...
> > ... it's amazing the variation that you find beyond the obvious:
> >
> > $ grep -Er "sizeof.*/.*sizeof" . | less
> >
> > ...
> > ./net/key/af_key.c: sa->sadb_sa_len = sizeof(struct sadb_sa)/sizeof(uint64_t);
> > ./net/xfrm/xfrm_policy.c: int len = sizeof(struct xfrm_selector) / sizeof(u32);
> > ./net/core/flow.c: const int n_elem = sizeof(struct flowi) / sizeof(flow_compare_t);
> > ./net/ipv4/netfilter/arp_tables.c: for (i = 0; i < sizeof(*arp)/sizeof(__u32); i++)
> > ./net/ipv4/af_inet.c:#define INETSW_ARRAY_LEN (sizeof(inetsw_array) / sizeof(struct inet_protosw))
> > ./drivers/net/wireless/ray_cs.c: .num_standard = sizeof(ray_handler)/sizeof(iw_handler),
> >
>
> Of the above, af_inet.c and ray_cs.c seem to be good candidates for
> ARRAY_SIZE. You might even remove the INETSW_ARRAY_LEN #define in
> af_inet.c altogether, since ARRAY_SIZE(inetsw_array) is quite
> readable.
note that the above examples i listed were just a *few* of the
examples that didn't match the most common variants:
sizeof(fubar) / sizeof(fubar[0])
sizeof(fubar) / sizeof(*fubar)
i just did that to show that, even if i can run a script to handle the
most common variants, there would be lots of manual cleanup left.
> From a first glance, af_key.c is ok but might profit from a comment
> in include/linux/pfkeyv2.h saying that sadb_msg_len is measured in
> 64-bit words per RFC 2367. Though documenting the structs in
> pfkeyv2.h would be quite a bit different from what you initially
> intended...
in fact, i just emailed a short CodingStyle note to randy dunlap
(since he seemed to be heavily into the coding style stuff),
suggesting that a short note be added strongly recommending that one
should use ARRAY_SIZE wherever possible and, if not possible, a
comment should be added explaining why not, if it seems to be useful.
> So, if you have some time to spend on this, manual inspection would
> probably be the most useful thing, since any automatic sed tricks
> will only replace what a human ready would easily understand as
> well.
true enough, but if the most common variants can be handled
automatically, then the remainder would stand out more obviously and
could be manually handled from there.
> If you manually generate cleanup patches, it would be very good to
> check that compilation with allyesconfig generates identical code
> before and after before feeding them through the respective
> maintainers.
i'm actually in the process of trying that as we speak, at least with
the automatic cleanup. there's no way i'm going to try to get into
manual cleanup with all of those weird variants. life's too short for
that. :-)
rday
On Dec 16 2006 08:09, Robert P. J. Day wrote:
>On Sat, 16 Dec 2006, Pavel Machek wrote:
>> > but we already have, from "include/linux/kernel.h":
>> >
>> > #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>>
>> Hmmm. quite misleading name :-(. ARRAY_LEN would be better.
>
>i suspect it's *way* too late to make that kind of change, given that
>"ARRAY_SIZE" is firmly ensconced in countless places in the source
>tree and that would be a major, disruptive change.
>
>even *i* wouldn't try to promote that idea. :-)
You know, you could always make it compat for a while, but that requires
approval from Linus I suppose /* heh, heh */
I don't even know if this will compile everywhere,
but I hope you can figure out the idea...
#define ARRAY_SIZE(x) (print_warning(), sizeof(x) / sizeof(*x))
#define ARRAY_LEN(x) (sizeof(x) / sizeof(*x))
extern ...
void print_warning(void) {
printk("Don't use ARRAY_SIZE anymore, it will go away\n");
}
-`J'
--
Jan Engelhardt wrote:
> On Dec 16 2006 08:09, Robert P. J. Day wrote:
>> On Sat, 16 Dec 2006, Pavel Machek wrote:
>>>> but we already have, from "include/linux/kernel.h":
>>>>
>>>> #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>>> Hmmm. quite misleading name :-(. ARRAY_LEN would be better.
"Size", "length", "width", "depth" is all the same in that one needs to
know the unit of measurement. The unit of measurement of ARRAY_SIZE is
one array member. This makes it useful as a bound for [ ] pointer
arithmetic which uses the same unit.
If you want to look at it from a slightly higher level of abstraction
and want to avoid the ambiguity WRT units of measurement (C programs
most often use Byte as unit for data size), consider the unitless
(cardinal) ARRAY_INDEX_BOUND or ARRAY_CARDINALITY. (In a language which
starts array indexes at 1 instead of 0, it could also be called
ARRAY_HIINDEX.)
But fortunately...
>> i suspect it's *way* too late to make that kind of change, given that
>> "ARRAY_SIZE" is firmly ensconced in countless places in the source
>> tree and that would be a major, disruptive change.
>>
>> even *i* wouldn't try to promote that idea. :-)
>
> You know, you could always make it compat for a while, but that requires
> approval from Linus I suppose /* heh, heh */
>
> I don't even know if this will compile everywhere,
> but I hope you can figure out the idea...
>
> #define ARRAY_SIZE(x) (print_warning(), sizeof(x) / sizeof(*x))
> #define ARRAY_LEN(x) (sizeof(x) / sizeof(*x))
> extern ...
> void print_warning(void) {
> printk("Don't use ARRAY_SIZE anymore, it will go away\n");
> }
...those who know that the ARRAY_SIZE macro is available also know what
it means and how to use it. Therefore there is no need to rename this macro.
--
Stefan Richter
-=====-=-==- ==-- =---=
http://arcgraph.de/sr/
so here's the end result of my experiment to replace unnecessary
code snippets with an invocation of the ARRAY_SIZE() macro from
include/linux/kernel.h. i've attached the script that i ran on the
entire tree, then (after adding al viro's connector patch), did:
$ make allyesconfig # for the stress factor
$ make
to see what would happen.
amazingly, the compile worked all the way down to:
AS arch/i386/boot/bootsect.o
LD arch/i386/boot/bootsect
AS arch/i386/boot/setup.o
LD arch/i386/boot/setup
AS arch/i386/boot/compressed/head.o
CC arch/i386/boot/compressed/misc.o
OBJCOPY arch/i386/boot/compressed/vmlinux.bin
HOSTCC arch/i386/boot/compressed/relocs
arch/i386/boot/compressed/relocs.c: In function 'sym_type':
arch/i386/boot/compressed/relocs.c:72: warning: implicit declaration of function 'ARRAY_SIZE'
/tmp/ccRTpFxM.o: In function `main':
relocs.c:(.text+0xb13): undefined reference to `ARRAY_SIZE'
relocs.c:(.text+0xddb): undefined reference to `ARRAY_SIZE'
relocs.c:(.text+0xe10): undefined reference to `ARRAY_SIZE'
relocs.c:(.text+0xe2b): undefined reference to `ARRAY_SIZE'
collect2: ld returned 1 exit status
make[2]: *** [arch/i386/boot/compressed/relocs] Error 1
make[1]: *** [arch/i386/boot/compressed/vmlinux] Error 2
make: *** [bzImage] Error 2
not surprisingly, the diff is fairly sizable (2408 lines), and
that's *way* outside of my comfort zone in terms of what i would
submit as a patch. others higher up the food chain can decide if they
want to do anything with this. (for the information value, i also
attached the "diffstat" output.)
at this point, i think it's someone else's call.
rday
p.s. clearly, this didn't even hit all of the possible
transformations, such as the ones based on typedefs, or even the ones
that are broken over two lines. you can see from the script that i
just went after the low-hanging fruit.
p.p.s. i didn't bother fixing relocs.c yet to see if the build would
actually finish. i thought i'd better stop here and wait to hear what
others think.
On Sun, 17 Dec 2006 13:13:59 -0500 (EST) Robert P. J. Day wrote:
>
> so here's the end result of my experiment to replace unnecessary
> code snippets with an invocation of the ARRAY_SIZE() macro from
> include/linux/kernel.h. i've attached the script that i ran on the
> entire tree, then (after adding al viro's connector patch), did:
>
> $ make allyesconfig # for the stress factor
> $ make
>
> to see what would happen.
>
> amazingly, the compile worked all the way down to:
>
> AS arch/i386/boot/bootsect.o
> LD arch/i386/boot/bootsect
> AS arch/i386/boot/setup.o
> LD arch/i386/boot/setup
> AS arch/i386/boot/compressed/head.o
> CC arch/i386/boot/compressed/misc.o
> OBJCOPY arch/i386/boot/compressed/vmlinux.bin
> HOSTCC arch/i386/boot/compressed/relocs
> arch/i386/boot/compressed/relocs.c: In function 'sym_type':
> arch/i386/boot/compressed/relocs.c:72: warning: implicit declaration of function 'ARRAY_SIZE'
That's a userspace program and shouldn't use kernel.h.
> /tmp/ccRTpFxM.o: In function `main':
> relocs.c:(.text+0xb13): undefined reference to `ARRAY_SIZE'
> relocs.c:(.text+0xddb): undefined reference to `ARRAY_SIZE'
> relocs.c:(.text+0xe10): undefined reference to `ARRAY_SIZE'
> relocs.c:(.text+0xe2b): undefined reference to `ARRAY_SIZE'
> collect2: ld returned 1 exit status
> make[2]: *** [arch/i386/boot/compressed/relocs] Error 1
> make[1]: *** [arch/i386/boot/compressed/vmlinux] Error 2
> make: *** [bzImage] Error 2
---
~Randy
On Sun, 17 Dec 2006, Randy Dunlap wrote:
> On Sun, 17 Dec 2006 13:13:59 -0500 (EST) Robert P. J. Day wrote:
>
> >
> > so here's the end result of my experiment to replace unnecessary
> > code snippets with an invocation of the ARRAY_SIZE() macro from
> > include/linux/kernel.h. i've attached the script that i ran on the
> > entire tree, then (after adding al viro's connector patch), did:
> >
> > $ make allyesconfig # for the stress factor
> > $ make
> >
> > to see what would happen.
> >
> > amazingly, the compile worked all the way down to:
> >
> > AS arch/i386/boot/bootsect.o
> > LD arch/i386/boot/bootsect
> > AS arch/i386/boot/setup.o
> > LD arch/i386/boot/setup
> > AS arch/i386/boot/compressed/head.o
> > CC arch/i386/boot/compressed/misc.o
> > OBJCOPY arch/i386/boot/compressed/vmlinux.bin
> > HOSTCC arch/i386/boot/compressed/relocs
> > arch/i386/boot/compressed/relocs.c: In function 'sym_type':
> > arch/i386/boot/compressed/relocs.c:72: warning: implicit declaration of function 'ARRAY_SIZE'
>
> That's a userspace program and shouldn't use kernel.h.
ah, quite right, my bad. eggnog hangover.
rday