p = kmalloc(sizeof(*p), )

p = kmalloc(sizeof(*p), )

[Russell King]

Hi,

In todays git update, the following text has been added to the coding
style document:

+The preferred form for passing a size of a struct is the following:
+
+       p = kmalloc(sizeof(*p), ...);
+
+The alternative form where struct name is spelled out hurts readability and
+introduces an opportunity for a bug when the pointer variable type is changed
+but the corresponding sizeof that is passed to a memory allocator is not.

I completely disagree with the above assertion for the following
reasons:

1. The above implies that the common case is that we are changing the
   names of structures more frequently than we change the contents of
   structures.  Reality is that we change the contents of structures
   more often than the names of those structures.

   Why is this relevant?  If you change the contents of structures,
   they need checking for initialisation.  How do you find all the
   locations that need initialisation checked?  Via grep.  The problem
   is that:

	p = kmalloc(sizeof(*p), ...)

   is not grep-friendly, and can not be used to identify potential
   initialisation sites.  However:

	p = kmalloc(sizeof(struct foo), ...)

   is grep-friendly, and will lead you to inspect each place where
   such a structure is allocated for correct initialisation.

2. in the rare case that you're changing the name of a structure, you're
   grepping the source for all instances for struct old_name, or doing
   a search and replace for struct old_name.  You will find all instances
   of struct old_name by this method and the bug alluded to will not
   happen.

3. if you are changing the name of a structure, in order to ensure that
   everyone gets fixed up correctly, you do not want to keep an old
   declaration of the structure around, unless you have a very very good
   reason to do so.  This will ensure that any missed old structure
   names (eg, because of merging of independent concurrent threads of
   development) get caught.  As a result, any sizeof(struct) also gets
   caught.

So the assertion above that kmalloc(sizeof(*p) is somehow superiour is
rather flawed, and as such should not be in the Coding Style document.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: p = kmalloc(sizeof(*p), )

[Alan Cox]

On Sul, 2005-09-18 at 11:06 +0100, Russell King wrote:
> I completely disagree with the above assertion for the following
> reasons:

Ditto, but you forgot #4. People are always getting sizeof(*p) wrong, in
particular forgetting that p happens to be a void *, or a struct that is
some generic type not the full space for the more complex object
allocated, so using (*p) everywhere just causes more memory scribbles.

If it bugs people add a kmalloc_object macro that is

#define new_object(foo, gfp) (foo *)kmalloc(sizeof(foo), (gfp))

then you can

	x = new_object(struct frob, GFP_KERNEL)

Other good practice in many cases is a single routine which allocates
and initialises the structure and is used by all allocators of that
object. That removes duplicate initialisers, stops people forgetting to
update all cases, allows better debug and far more.
 
Alan

Re: p = kmalloc(sizeof(*p), )

[Al Viro]

On Sun, Sep 18, 2005 at 12:04:34PM +0100, Alan Cox wrote:
 
> Other good practice in many cases is a single routine which allocates
> and initialises the structure and is used by all allocators of that
> object. That removes duplicate initialisers, stops people forgetting to
> update all cases, allows better debug and far more.

Indeed.  IMO, argument for sizeof(*p) is bullshit - "I've changed a pointer
type and forgot to update the allocation and initialization, but this will
magically save my arse" is missing "except that initialization will remain
bogus" part.

I've seen a lot of bugs around bogus kmalloc+initialization, but I can't
recall a single case when such bug would be prevented by using that form.
If somebody has a different experience, please post pointers to changesets
in question.

Re: p = kmalloc(sizeof(*p), )

[Denis Vlasenko]

On Sunday 18 September 2005 17:39, Al Viro wrote:
> On Sun, Sep 18, 2005 at 12:04:34PM +0100, Alan Cox wrote:
>  
> > Other good practice in many cases is a single routine which allocates
> > and initialises the structure and is used by all allocators of that
> > object. That removes duplicate initialisers, stops people forgetting to
> > update all cases, allows better debug and far more.
> 
> Indeed.  IMO, argument for sizeof(*p) is bullshit - "I've changed a pointer
> type and forgot to update the allocation and initialization, but this will
> magically save my arse" is missing "except that initialization will remain
> bogus" part.
> 
> I've seen a lot of bugs around bogus kmalloc+initialization, but I can't
> recall a single case when such bug would be prevented by using that form.
> If somebody has a different experience, please post pointers to changesets
> in question.

Do these qualify?

http://www.uwsg.iu.edu/hypermail/linux/kernel/0105.1/0579.html
o Fix wrong kmalloc sizes in ixj/emu10k1 (David Chan) 

http://www.mail-archive.com/alsa-cvslog@lists.sourceforge.net/msg02483.html
Update of /cvsroot/alsa/alsa-kernel/isa
In directory sc8-pr-cvs1:/tmp/cvs-serv4034

Modified Files:
        es18xx.c cmi8330.c 
Log Message:
Fixed wrong kmalloc


After looking at output of grep -r -C10 'malloc.*sizeof' .
(epic picture) I think that maybe Alan's typechecking kmalloc
would be useful:

On Sunday 18 September 2005 14:04, Alan Cox wrote:
> If it bugs people add a kmalloc_object macro that is
> 
> #define new_object(foo, gfp) (foo *)kmalloc(sizeof(foo), (gfp))
> 
> then you can
> 
> 	x = new_object(struct frob, GFP_KERNEL)

This will emit a warning if x is not a struct frob*,
which plain kmalloc doesn't do.
--
vda

Re: p = kmalloc(sizeof(*p), )

[Al Viro]

On Sun, Sep 18, 2005 at 07:25:24PM +0300, Denis Vlasenko wrote:
> Do these qualify?
> 
> http://www.uwsg.iu.edu/hypermail/linux/kernel/0105.1/0579.html
> o Fix wrong kmalloc sizes in ixj/emu10k1 (David Chan) 

ixj does, emu10k does not (sizeof(p) instead of sizeof(*p) would be
exact same bug).
 
> http://www.mail-archive.com/alsa-cvslog@lists.sourceforge.net/msg02483.html
> Update of /cvsroot/alsa/alsa-kernel/isa
> In directory sc8-pr-cvs1:/tmp/cvs-serv4034
> 
> Modified Files:
>         es18xx.c cmi8330.c 
> Log Message:
> Fixed wrong kmalloc

Nope.  Wrong order of arguments in kmalloc; nothing to do with what we
intend to pass as size.

Re: p = kmalloc(sizeof(*p), )

[Willy Tarreau]

On Sun, Sep 18, 2005 at 06:30:58PM +0100, Al Viro wrote:
> On Sun, Sep 18, 2005 at 07:25:24PM +0300, Denis Vlasenko wrote:
> > Do these qualify?
> > 
> > http://www.uwsg.iu.edu/hypermail/linux/kernel/0105.1/0579.html
> > o Fix wrong kmalloc sizes in ixj/emu10k1 (David Chan) 
> 
> ixj does, emu10k does not (sizeof(p) instead of sizeof(*p) would be
> exact same bug).

Funny, a few days ago, I found such a bug in a coworker's code. I agree
that finding this at 3am would be close to impossible. OK, you converted
me :-)

Regards,
Willy

Re: p = kmalloc(sizeof(*p), )

[Alan Cox]

On Sul, 2005-09-18 at 19:25 +0300, Denis Vlasenko wrote:
> > #define new_object(foo, gfp) (foo *)kmalloc(sizeof(foo), (gfp))
> > 
> > then you can
> > 
> > 	x = new_object(struct frob, GFP_KERNEL)
> 
> This will emit a warning if x is not a struct frob*,
> which plain kmalloc doesn't do.

In the programs where I use it a lot (eg AberMUD5) I also in debugging
mode pass in __FILE__ and __LINE__ which can be most handy.

Re: p = kmalloc(sizeof(*p), )

[Robert Love]

On Sun, 2005-09-18 at 11:06 +0100, Russell King wrote:

> +The preferred form for passing a size of a struct is the following:
> +
> +       p = kmalloc(sizeof(*p), ...);
> +
> +The alternative form where struct name is spelled out hurts readability and
> +introduces an opportunity for a bug when the pointer variable type is changed
> +but the corresponding sizeof that is passed to a memory allocator is not.

Agreed.

Also, after Alan's #4:

5.  Contrary to the above statement, such coding style does not help,
    but in fact hurts, readability.  How on Earth is sizeof(*p) more
    readable and information-rich than sizeof(struct foo)?  It looks
    like the remains of a 5,000 year old wolverine's spleen and
    conveys no information about the type of the object that is being
    created.

	Robert Love

Re: p = kmalloc(sizeof(*p), )

[Willy Tarreau]

On Sun, Sep 18, 2005 at 12:32:26PM -0400, Robert Love wrote:
> On Sun, 2005-09-18 at 11:06 +0100, Russell King wrote:
> 
> > +The preferred form for passing a size of a struct is the following:
> > +
> > +       p = kmalloc(sizeof(*p), ...);
> > +
> > +The alternative form where struct name is spelled out hurts readability and
> > +introduces an opportunity for a bug when the pointer variable type is changed
> > +but the corresponding sizeof that is passed to a memory allocator is not.
> 
> Agreed.
> 
> Also, after Alan's #4:
> 
> 5.  Contrary to the above statement, such coding style does not help,
>     but in fact hurts, readability.  How on Earth is sizeof(*p) more
>     readable and information-rich than sizeof(struct foo)?  It looks
>     like the remains of a 5,000 year old wolverine's spleen and
>     conveys no information about the type of the object that is being
>     created.
> 
> 	Robert Love

To be honnest, before reading this thread, I would have voted for the
sizeof(*p). However, I completely agree that there is a high risk of
messing up the initialization, and that structures don't change often.
The situations where I think that sizeof(*p) is better than
sizeof(struct foo) is more on functions such as memset() than {,k}malloc() :
forgetting to initialize a struct member is always a high risk, but if the
object is not a struct (eg, a scalar), then it could be tolerated. I don't
know anybody who does kmalloc(sizeof(int)) nor kmalloc(sizeof(char)), but
with memset, it's different. Doing memset(p, 0, sizeof(*p)) seems better
to me than memset(p, 0, sizeof(short)), and represents a smaller risk
when 'p' will silently evolve to a long int.

Last, there's little probability that a scalar will evolve into a struct
without code modifications, while it has happened often that a __u8 or
__u16 was changed to __u32. So perhaps we could accept use of sizeof(*p)
when (*p) is a scalar to protect against silent type changes, and reject
it when (*p) is a structure to avoid incomplete initialization ?

Alan, I like your proposal BTW ;-)

Regards,
Willy

Re: p = kmalloc(sizeof(*p), )

[Al Viro]

On Sun, Sep 18, 2005 at 06:52:19PM +0200, Willy Tarreau wrote:
> know anybody who does kmalloc(sizeof(int)) nor kmalloc(sizeof(char)), but
> with memset, it's different. Doing memset(p, 0, sizeof(*p)) seems better
> to me than memset(p, 0, sizeof(short)), and represents a smaller risk
> when 'p' will silently evolve to a long int.

That's why you do
	*p = (struct foo){....};
instead of
	memset(p, 0, sizeof...);
	p->... =...;

Note that in a lot of cases your memset(p, 0, sizeof(*p)) is actually wrong -
e.g. when you've allocated a struct + some array just past it.

Oh, and in your case above...  *p = 0; is certainly saner than that memset(),
TYVM ;-)

I'm serious about compound literals instead of memset() - they give sane
typechecking for free and give compiler a chance for saner optimizations.
And no, it's not a gcc-ism - it's valid C99.

Re: p = kmalloc(sizeof(*p), )

[Linus Torvalds]

On Sun, 18 Sep 2005, Al Viro wrote:
> 
> That's why you do
> 	*p = (struct foo){....};
> instead of
> 	memset(p, 0, sizeof...);
> 	p->... =...;

Actually, some day that migth be a good idea, but at least historically, 
gcc has really really messed that kind of code up.

Last I looked, depending on what the initializer was, gcc would create a 
temporary struct on the stack first, and then do a "memcpy()" of the 
result. Not only does that obviously generate a lot of extra code, it also 
blows your kernel stack to kingdom come.

So be careful out there, and check what code it generates first. With at 
least a few versions of gcc.

(For _small_ structures it's wonderful. As far as I can tell, gcc does a
pretty good job on structs that are just a single long-word in size).

		Linus

Re: p = kmalloc(sizeof(*p), )

[Al Viro]

On Sun, Sep 18, 2005 at 10:31:36AM -0700, Linus Torvalds wrote:
> 
> 
> On Sun, 18 Sep 2005, Al Viro wrote:
> > 
> > That's why you do
> > 	*p = (struct foo){....};
> > instead of
> > 	memset(p, 0, sizeof...);
> > 	p->... =...;
> 
> Actually, some day that migth be a good idea, but at least historically, 
> gcc has really really messed that kind of code up.
> 
> Last I looked, depending on what the initializer was, gcc would create a 
> temporary struct on the stack first, and then do a "memcpy()" of the 
> result. Not only does that obviously generate a lot of extra code, it also 
> blows your kernel stack to kingdom come.

Ewwwww...  I'd say that it qualifies as one hell of a bug (and yes, at least
3.3 and 4.0.1 are still doing that).  What a mess...
 
> (For _small_ structures it's wonderful. As far as I can tell, gcc does a
> pretty good job on structs that are just a single long-word in size).

Re: p = kmalloc(sizeof(*p), )

[Roman Zippel]

Hi,

On Sun, 18 Sep 2005, Al Viro wrote:

> > On Sun, 18 Sep 2005, Al Viro wrote:
> > > 
> > > That's why you do
> > > 	*p = (struct foo){....};
> > > instead of
> > > 	memset(p, 0, sizeof...);
> > > 	p->... =...;
> > 
> > Actually, some day that migth be a good idea, but at least historically, 
> > gcc has really really messed that kind of code up.
> > 
> > Last I looked, depending on what the initializer was, gcc would create a 
> > temporary struct on the stack first, and then do a "memcpy()" of the 
> > result. Not only does that obviously generate a lot of extra code, it also 
> > blows your kernel stack to kingdom come.
> 
> Ewwwww...  I'd say that it qualifies as one hell of a bug (and yes, at least
> 3.3 and 4.0.1 are still doing that).  What a mess...

It's not a bug, it's exactly what you're asking for, e.g. "*p1 = *p2" 
translates to memcpy. gcc also can't simply initialize that structure in 
place, e.g. you could do something like this (not necessarily useful but 
still valid): "*p = (struct foo){...,  bar(p),...};".
In the end it all depends on how good gcc can optimize away the memcpy, 
but initially there is always a memcpy.

bye, Roman

Re: p = kmalloc(sizeof(*p), )

[Al Viro]

On Sun, Sep 18, 2005 at 10:34:16PM +0200, Roman Zippel wrote:
> > Ewwwww...  I'd say that it qualifies as one hell of a bug (and yes, at least
> > 3.3 and 4.0.1 are still doing that).  What a mess...
> 
> It's not a bug, it's exactly what you're asking for, e.g. "*p1 = *p2" 
> translates to memcpy. gcc also can't simply initialize that structure in 
> place, e.g. you could do something like this (not necessarily useful but 
> still valid): "*p = (struct foo){...,  bar(p),...};".
> In the end it all depends on how good gcc can optimize away the memcpy, 
> but initially there is always a memcpy.

No.  Assignment is _not_ defined via memcpy(); it's a primitive that could
be implemented that way.  Choosing such (pretty much worst-case) implementation
in every case is a major bug in code generator.

You _can_ introduce a new local variable for each arithmetic operation in
your function and store result of operation in the corresponding variable.
As the matter of fact, this is a fairly common intermediate form.  However,
if compiler ends up leaving all these suckers intact in the final code,
it has a serious problem.

Compound literal _is_ an object, all right.  However, decision to allocate
storage for given object is up to compiler and it's hardly something unusual.
"Value of right-hand side is not needed to finish calculating left-hand side,
so its storage is fair game from that point on" is absolutely normal.

Re: p = kmalloc(sizeof(*p), )

[Al Viro]

On Sun, Sep 18, 2005 at 10:12:25PM +0100, Al Viro wrote:
 
> Compound literal _is_ an object, all right.  However, decision to allocate
> storage for given object is up to compiler and it's hardly something unusual.
> "Value of right-hand side is not needed to finish calculating left-hand side,
> so its storage is fair game from that point on" is absolutely normal.

BTW, for some idea of how hard does it actually blow, with
struct foo {
	int a, b[100];
};
we get
void f(struct foo *p)
{
	*p = (struct foo){.b[1] = 2};
}
compiled essentially to
void f(struct foo *p)
{
	static struct foo hidden_const = {.b[1] = 2};
	auto struct foo hidden_var;
	memcpy(&hidden_var, &hidden_const, sizeof(struct foo));
	memcpy(p, &hidden_var, sizeof(struct foo));
}

That's right - two memcpy back-to-back, both inserted by gcc, intermediate
object lost immediately after the second one, both source and intermediate
introduced by gcc, so it knows that there is no aliasing problems to be
dealt with.  And yes, that's with optimizations turned on - -O2 and -Os
generate the same.

Re: p = kmalloc(sizeof(*p), )

[Linus Torvalds]

On Sun, 18 Sep 2005, Al Viro wrote:
> 
> BTW, for some idea of how hard does it actually blow

Well, to be slightly more positive: it's not a very easy feature to do 
properly.

The thing about "(cast) { .. }" initializers is that they aren't just 
initializers: they really are local objects that can be used any way you 
want to. So in the _generic_ case, gcc does exactly the right thing: it 
introduces a local object that is filled in with the initializer.

So in the generic case, you could have

	x = (cast) { ... }.member + 2;

instead of just a straight assignment.

The problem is just that the generic case is semantically pretty damn far 
away from the case we actually want to use, ie the special case of an 
assignment. So some generic top-level code has created the generic code, 
and now the lower levels of the compiler need to "undo" that generic code, 
and see what it actually boils down to. And that's quite hard.

The sane thing to do for good code generation would be to special-case the 
assignment of this kind of thing, and just make it very obvious that an 
assignment of a (cast) {...} is very different from the generic use of 
same. But that would introduce two totally different cases for the thing.

So considering that almost nobody does this (certainly not SpecInt), and 
it would probably require re-organizations at many levels, I'm not 
surprised it hasn't gotten a lot of attention.

		Linus

Re: p = kmalloc(sizeof(*p), )

[Al Viro]

On Sun, Sep 18, 2005 at 03:25:39PM -0700, Linus Torvalds wrote:
> 
> 
> On Sun, 18 Sep 2005, Al Viro wrote:
> > 
> > BTW, for some idea of how hard does it actually blow
> 
> Well, to be slightly more positive: it's not a very easy feature to do 
> properly.
> 
> The thing about "(cast) { .. }" initializers is that they aren't just 
> initializers: they really are local objects that can be used any way you 
> want to. So in the _generic_ case, gcc does exactly the right thing: it 
> introduces a local object that is filled in with the initializer.

Of course.  It's not a question of local object being semantically correct.

Forget for a minute about compound literals; consider

{
	some_type foo = expression; /* doesn't use bar */
	bar = foo;
}

That's exactly what happens here.  What does _not_ happen is elimination
of foo.  Note that all tricky cases happen when we take an address of
our object or of some part of it.  E.g. (struct foo){...}.scalar_field is
happily optimized to <evaluate all members of initializer, memorizing the
result for initializer if our field><use the memorized result>.

What's happening might be (and no, I haven't looked into the gcc codegenerator
yet) as simple as too early conversion of assignment to memcpy() call, losing
the "we don't really use the address of this sucker after initialization"
in process.

Re: p = kmalloc(sizeof(*p), )

[Richard Henderson]

On Mon, Sep 19, 2005 at 12:07:33AM +0100, Al Viro wrote:
> What's happening might be (and no, I haven't looked into the gcc codegenerator
> yet) as simple as too early conversion of assignment to memcpy() call, losing
> the "we don't really use the address of this sucker after initialization"
> in process.

Not quite.  But failure to copy-propagate structures is a known problem.
It's on the to-do list.  Hopefully the improved alias analysis to be done
for gcc 4.2 will make this task not suck.

*shrug*


r~

Re: p = kmalloc(sizeof(*p), )

[Matthias Urlichs]

Hi, Linus Torvalds wrote:

> Well, to be slightly more positive: it's not a very easy feature to do
> properly.

It's apparently an easy feature to do decidedly suboptimally.

This sample code

#include <malloc.h>
struct foo {
    char fill1[1000];
    int bar;
    char fill2[1000];
    int baz;
    char fill3[1000];
};
struct foo *get_foo() {
    struct foo *res = malloc(sizeof(struct foo));
    *res = (struct foo){.bar=5, .baz=7};
    return res;
}

calls memcpy _twice_ -- it initializes the object on the stack from a
couple of mostly-zero bytes in .rodata, and then memcpy's the thing to the
heap.
 
> So considering that almost nobody does this (certainly not SpecInt), and
> it would probably require re-organizations at many levels, I'm not
> surprised it hasn't gotten a lot of attention.
> 
This is gcc 4.0. Optimization levels (I tried 0, 3, and s) don't affect
this -- which surprised me; I'd have thought that gcc would decide on
the proper trade-off between programmed and static initialization a bit
later.

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
O that my tongue were in the thunder's mouth! Then with a passion would I
shake the world.
					-- Shakespeare

Re: p = kmalloc(sizeof(*p), )

[Matthias Urlichs]

Umm...

> It's apparently an easy feature to do decidedly suboptimally.

Bah. Ignore me while I beat the "read the thread before replying" idea
into my head.  :-/

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
Button: Punned-it

Re: p = kmalloc(sizeof(*p), )

[Al Viro]

On Sun, Sep 18, 2005 at 10:31:36AM -0700, Linus Torvalds wrote:
> Last I looked, depending on what the initializer was, gcc would create a 
> temporary struct on the stack first, and then do a "memcpy()" of the 
> result. Not only does that obviously generate a lot of extra code, it also 
> blows your kernel stack to kingdom come.
> 
> So be careful out there, and check what code it generates first. With at 
> least a few versions of gcc.

BTW, one very useful thing we could do in sparse would be an attribute for
a struct that would generate a warning whenever sizeof is calculated - i.e.
catch the
	pointer arithmetics on pointers to these suckers
	sizeof(expression having such type)
	variable of such type (as opposed to pointers to such type)
	composite types containing elements of such type
with new primitive (#defined to sizeof if __CHECKER__ is not defined)
that would act as sizeof, but without a warning.  Optionally, we might
want assignments, passing as argument and conversion of pointers to
such beasts down to void * generate the same warnings.

It would be very useful when e.g. tracking down improper uses of
struct file, struct dentry, etc. - stuff that should always be
allocated by one helper function.  Same goes for e.g. net_device -
conversion to dynamic allocation involved doing what I've described
above manually (i.e. creative uses of grep).  If we had sparse
working on the entire tree back then and could do that sort of
checks, it would have saved a lot of PITA...

It shouldn't be hard to implement, AFAICS; I'll try to put together
something of that kind.

Re: p = kmalloc(sizeof(*p), )

[Alan Cox]

> It would be very useful when e.g. tracking down improper uses of
> struct file, struct dentry, etc. - stuff that should always be
> allocated by one helper function.  Same goes for e.g. net_device -

Another useful trick here btw is to make such objects contain (when
debugging)

	void *magic_ptr;

which is initialised as foo->magic_ptr = foo;

That catches anyone copying them and tells you what got copied

Re: p = kmalloc(sizeof(*p), )

[Al Viro]

On Sun, Sep 18, 2005 at 10:30:26PM +0100, Alan Cox wrote:
> > It would be very useful when e.g. tracking down improper uses of
> > struct file, struct dentry, etc. - stuff that should always be
> > allocated by one helper function.  Same goes for e.g. net_device -
> 
> Another useful trick here btw is to make such objects contain (when
> debugging)
> 
> 	void *magic_ptr;
> 
> which is initialised as foo->magic_ptr = foo;
> 
> That catches anyone copying them and tells you what got copied

At runtime, _if_ we do not forget to initialize it.  Which is what
we are trying to catch...

Re: p = kmalloc(sizeof(*p), )

[Coywolf Qi Hunt]

On 9/19/05, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > It would be very useful when e.g. tracking down improper uses of
> > struct file, struct dentry, etc. - stuff that should always be
> > allocated by one helper function.  Same goes for e.g. net_device -
> 
> Another useful trick here btw is to make such objects contain (when
> debugging)
> 
>         void *magic_ptr;
> 
> which is initialised as foo->magic_ptr = foo;
> 
> That catches anyone copying them and tells you what got copied

seems like C++ RTTI
-- 
Coywolf Qi Hunt
http://sosdg.org/~coywolf/

Re: p = kmalloc(sizeof(*p), )

[Miles Bader]

Linus Torvalds <torvalds@osdl.org> writes:
> Actually, some day that migth be a good idea, but at least historically, 
> gcc has really really messed that kind of code up.
>
> Last I looked, depending on what the initializer was, gcc would create a 
> temporary struct on the stack first, and then do a "memcpy()" of the 
> result.

A little test shows:

gcc-3.4.4 seems to still do what you describe.

gcc-4.0.1 seems to  it the "right" way (writing each field directly to
the destination structure).

Someday...

-miles
-- 
Yo mama's so fat when she gets on an elevator it HAS to go down.

Re: p = kmalloc(sizeof(*p), )

[Randy.Dunlap]

On Sun, 18 Sep 2005 12:32:26 -0400 Robert Love wrote:

> On Sun, 2005-09-18 at 11:06 +0100, Russell King wrote:
> 
> > +The preferred form for passing a size of a struct is the following:
> > +
> > +       p = kmalloc(sizeof(*p), ...);
> > +
> > +The alternative form where struct name is spelled out hurts readability and
> > +introduces an opportunity for a bug when the pointer variable type is changed
> > +but the corresponding sizeof that is passed to a memory allocator is not.
> 
> Agreed.
> 
> Also, after Alan's #4:
> 
> 5.  Contrary to the above statement, such coding style does not help,
>     but in fact hurts, readability.  How on Earth is sizeof(*p) more
>     readable and information-rich than sizeof(struct foo)?  It looks
>     like the remains of a 5,000 year old wolverine's spleen and
>     conveys no information about the type of the object that is being
>     created.

I also dislike & disagree with the CodingStyle addition....


---
~Randy
You can't do anything without having to do something else first.
-- Belefant's Law

Re: p = kmalloc(sizeof(*p), )

[Coywolf Qi Hunt]

On 9/19/05, Robert Love <rml@novell.com> wrote:
> On Sun, 2005-09-18 at 11:06 +0100, Russell King wrote:
> 
> > +The preferred form for passing a size of a struct is the following:
> > +
> > +       p = kmalloc(sizeof(*p), ...);
> > +
> > +The alternative form where struct name is spelled out hurts readability and
> > +introduces an opportunity for a bug when the pointer variable type is changed
> > +but the corresponding sizeof that is passed to a memory allocator is not.
> 
> Agreed.
> 
> Also, after Alan's #4:
> 
> 5.  Contrary to the above statement, such coding style does not help,
>     but in fact hurts, readability.  How on Earth is sizeof(*p) more
>     readable and information-rich than sizeof(struct foo)?  It looks
>     like the remains of a 5,000 year old wolverine's spleen and
>     conveys no information about the type of the object that is being
>     created.
 
6. The attribute size is _firstly_ an attribute of the data type, not
of the variable. So sizeof(type) is a bit saner than sizeof(var).
While allocating, we are allocating an instance of the type and we
don't care which instance it would be but we care what data type it
is.

-- 
Coywolf Qi Hunt
http://sosdg.org/~coywolf/

Re: p = kmalloc(sizeof(*p), )

[Pekka Enberg]

On 9/18/05, Robert Love <rml@novell.com> wrote:
> 5.  Contrary to the above statement, such coding style does not help,
>     but in fact hurts, readability.  How on Earth is sizeof(*p) more
>     readable and information-rich than sizeof(struct foo)?  It looks
>     like the remains of a 5,000 year old wolverine's spleen and
>     conveys no information about the type of the object that is being
>     created.

Yes it does. The semantics are clearly "I want enough memory to hold
the type this pointer points to." While sizeof(struct foo) might seem
more readable, it is in fact not as you have no way of knowing whether
the allocation is correct or not by looking at the line. So for
spotting allocation errors with grep, the shorter form is better (and
arguably less error-prone).

                                Pekka

Re: p = kmalloc(sizeof(*p), )

[Al Viro]

On Tue, Sep 20, 2005 at 11:53:42AM +0300, Pekka Enberg wrote:
> On 9/18/05, Robert Love <rml@novell.com> wrote:
> > 5.  Contrary to the above statement, such coding style does not help,
> >     but in fact hurts, readability.  How on Earth is sizeof(*p) more
> >     readable and information-rich than sizeof(struct foo)?  It looks
> >     like the remains of a 5,000 year old wolverine's spleen and
> >     conveys no information about the type of the object that is being
> >     created.
> 
> Yes it does. The semantics are clearly "I want enough memory to hold
> the type this pointer points to." While sizeof(struct foo) might seem
> more readable, it is in fact not as you have no way of knowing whether
> the allocation is correct or not by looking at the line. So for
> spotting allocation errors with grep, the shorter form is better (and
> arguably less error-prone).

Huh???   How do you use grep to find something of that sort?

Re: p = kmalloc(sizeof(*p), )

[Pekka J Enberg]

On Tue, 20 Sep 2005, Al Viro wrote:

> On Tue, Sep 20, 2005 at 11:53:42AM +0300, Pekka Enberg wrote:
> > On 9/18/05, Robert Love <rml@novell.com> wrote:
> > > 5.  Contrary to the above statement, such coding style does not help,
> > >     but in fact hurts, readability.  How on Earth is sizeof(*p) more
> > >     readable and information-rich than sizeof(struct foo)?  It looks
> > >     like the remains of a 5,000 year old wolverine's spleen and
> > >     conveys no information about the type of the object that is being
> > >     created.
> > 
> > Yes it does. The semantics are clearly "I want enough memory to hold
> > the type this pointer points to." While sizeof(struct foo) might seem
> > more readable, it is in fact not as you have no way of knowing whether
> > the allocation is correct or not by looking at the line. So for
> > spotting allocation errors with grep, the shorter form is better (and
> > arguably less error-prone).
> 
> Huh???   How do you use grep to find something of that sort?

To find candidates, something like:

grep "kmalloc(sizeof([^*]" -r drivers/ | grep -v "sizeof(struct"

And then use my eyes to find real bugs.

			Pekka

Re: p = kmalloc(sizeof(*p), )

[Al Viro]

On Tue, Sep 20, 2005 at 12:47:32PM +0300, Pekka J Enberg wrote:
> On Tue, 20 Sep 2005, Al Viro wrote:
> 
> > On Tue, Sep 20, 2005 at 11:53:42AM +0300, Pekka Enberg wrote:
> > > On 9/18/05, Robert Love <rml@novell.com> wrote:
> > > > 5.  Contrary to the above statement, such coding style does not help,
> > > >     but in fact hurts, readability.  How on Earth is sizeof(*p) more
> > > >     readable and information-rich than sizeof(struct foo)?  It looks
> > > >     like the remains of a 5,000 year old wolverine's spleen and
> > > >     conveys no information about the type of the object that is being
> > > >     created.
> > > 
> > > Yes it does. The semantics are clearly "I want enough memory to hold
> > > the type this pointer points to." While sizeof(struct foo) might seem
> > > more readable, it is in fact not as you have no way of knowing whether
> > > the allocation is correct or not by looking at the line. So for
> > > spotting allocation errors with grep, the shorter form is better (and
> > > arguably less error-prone).
> > 
> > Huh???   How do you use grep to find something of that sort?
> 
> To find candidates, something like:
> 
> grep "kmalloc(sizeof([^*]" -r drivers/ | grep -v "sizeof(struct"
> 
> And then use my eyes to find real bugs.

"grep for kmallocs that do not have _either_ form and look for bugs among
them" is hardly usable as an argument in favour of one of them...

Re: p = kmalloc(sizeof(*p), )

[Pekka J Enberg]

On Tue, Sep 20, 2005 at 12:47:32PM +0300, Pekka J Enberg wrote:
> > To find candidates, something like:
> > 
> > grep "kmalloc(sizeof([^*]" -r drivers/ | grep -v "sizeof(struct"
> > 
> > And then use my eyes to find real bugs.

On Tue, 20 Sep 2005, Al Viro wrote:
> "grep for kmallocs that do not have _either_ form and look for bugs among
> them" is hardly usable as an argument in favour of one of them...

I would disagree with that. The _common case_ for allocation is:

	p = kmalloc(sizeof(*p), ...);

For which you know that you are allocating enough memory for the struct. 
Now the only way to screw it up is to write:

	p = kmalloc(sizeof(p), ...);

That is trivial to grep for.

Yes, currently, typedefs and open-coded kcalloc's give false positives but
that's what kernel janitors are for...

			Pekka

Re: p = kmalloc(sizeof(*p), )

[Randy.Dunlap]

On Tue, 20 Sep 2005, Pekka J Enberg wrote:

> On Tue, 20 Sep 2005, Al Viro wrote:
>
> > On Tue, Sep 20, 2005 at 11:53:42AM +0300, Pekka Enberg wrote:
> > > On 9/18/05, Robert Love <rml@novell.com> wrote:
> > > > 5.  Contrary to the above statement, such coding style does not help,
> > > >     but in fact hurts, readability.  How on Earth is sizeof(*p) more
> > > >     readable and information-rich than sizeof(struct foo)?  It looks
> > > >     like the remains of a 5,000 year old wolverine's spleen and
> > > >     conveys no information about the type of the object that is being
> > > >     created.
> > >
> > > Yes it does. The semantics are clearly "I want enough memory to hold
> > > the type this pointer points to." While sizeof(struct foo) might seem
> > > more readable, it is in fact not as you have no way of knowing whether
> > > the allocation is correct or not by looking at the line. So for
> > > spotting allocation errors with grep, the shorter form is better (and
> > > arguably less error-prone).
> >
> > Huh???   How do you use grep to find something of that sort?
>
> To find candidates, something like:
>
> grep "kmalloc(sizeof([^*]" -r drivers/ | grep -v "sizeof(struct"
>
> And then use my eyes to find real bugs.

ugh.

I guess I'm old-fashioned:  I think that the real answer is
to do the due diligence when making a patch.  Don't assume
that you/I/we can make a one-line change without checking
around/above/below it, where it's declared, allocated, etc.

I once worked with someone whose attitude was to "let the
compiler find all of the problems," so he threw quite the
garbage at it and iterated until the compiler no longer
complained.  I think that's the wrong way to do it, but
maybe it was his version of release early, release often.

-- 
~Randy

Re: p = kmalloc(sizeof(*p), )

[Pekka Enberg]

Hi,

On 9/18/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> 1. The above implies that the common case is that we are changing the
>    names of structures more frequently than we change the contents of
>    structures.  Reality is that we change the contents of structures
>    more often than the names of those structures.
> 
>    Why is this relevant?  If you change the contents of structures,
>    they need checking for initialisation.  How do you find all the
>    locations that need initialisation checked?  Via grep.  The problem
>    is that:
> 
>         p = kmalloc(sizeof(*p), ...)
> 
>    is not grep-friendly, and can not be used to identify potential
>    initialisation sites.  However:
> 
>         p = kmalloc(sizeof(struct foo), ...)
> 
>    is grep-friendly, and will lead you to inspect each place where
>    such a structure is allocated for correct initialisation.

I would disagree on this one. You can still grep all the places where
the local variable is declared in. Furthermore, structs are not always
initialized where they're kmalloc'd so you need to manually inspect
anyway.

On 9/18/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> 2. in the rare case that you're changing the name of a structure, you're
>    grepping the source for all instances for struct old_name, or doing
>    a search and replace for struct old_name.  You will find all instances
>    of struct old_name by this method and the bug alluded to will not
>    happen.

Perhaps it has poor wording but I was more thinking about a case where
you shuffle code around and forget that you changed a struct to
something else (not necessarily removing the old one).

On 9/18/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> So the assertion above that kmalloc(sizeof(*p) is somehow superiour is
> rather flawed, and as such should not be in the Coding Style document.

I think it is better because:

  - It is easier to get right.
  - It is easier to audit with a script.
  - It is shorter.

I am not saying you can use sizeof(*p) everywhere but it is the common
case and as such the preferred form.

                            Pekka

Re: p = kmalloc(sizeof(*p), )

[Russell King]

On Tue, Sep 20, 2005 at 02:18:42PM +0300, Pekka Enberg wrote:
> On 9/18/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> > 1. The above implies that the common case is that we are changing the
> >    names of structures more frequently than we change the contents of
> >    structures.  Reality is that we change the contents of structures
> >    more often than the names of those structures.
> > 
> >    Why is this relevant?  If you change the contents of structures,
> >    they need checking for initialisation.  How do you find all the
> >    locations that need initialisation checked?  Via grep.  The problem
> >    is that:
> > 
> >         p = kmalloc(sizeof(*p), ...)
> > 
> >    is not grep-friendly, and can not be used to identify potential
> >    initialisation sites.  However:
> > 
> >         p = kmalloc(sizeof(struct foo), ...)
> > 
> >    is grep-friendly, and will lead you to inspect each place where
> >    such a structure is allocated for correct initialisation.
> 
> I would disagree on this one. You can still grep all the places where
> the local variable is declared in. Furthermore, structs are not always
> initialized where they're kmalloc'd so you need to manually inspect
> anyway.

Think about it some more.  You've added a new member to struct foo.
You want to fix up all the places which allocate struct foo to
initialise this new member.  Grepping for 'struct foo' returns 100
files.  Grepping for kmalloc in those 100 files returns 100 files.

Do you open all 100 in an editor and manually try and locate the five
kmalloc instances of this structure, and end up missing some.

Or do you do the sane thing and use kmalloc(sizeof(struct foo), ...)
and grep for "kmalloc[[:space:]]*(sizeof[[:space:]]*(struct foo)"
which returns only the five files and fix those up with knowledge
that you've found all the instances?

> On 9/18/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> > 2. in the rare case that you're changing the name of a structure, you're
> >    grepping the source for all instances for struct old_name, or doing
> >    a search and replace for struct old_name.  You will find all instances
> >    of struct old_name by this method and the bug alluded to will not
> >    happen.
> 
> Perhaps it has poor wording but I was more thinking about a case where
> you shuffle code around and forget that you changed a struct to
> something else (not necessarily removing the old one).

Such shuffling around should be done in easy to review stages so that
bugs can be found, and not a mega patch.  This inherently means that
for a structure name change, you don't end up with a new structure
named the same as an old structure.  And if you compile-test the
stages, you find out if you missed the problem.

That's the only real sane way of doing such changes to ensure
correctness.  Anything less and the patch should not be accepted.

> On 9/18/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> > So the assertion above that kmalloc(sizeof(*p) is somehow superiour is
> > rather flawed, and as such should not be in the Coding Style document.
> 
> I think it is better because:
> 
>   - It is easier to get right.
>   - It is easier to audit with a script.
>   - It is shorter.
> 
> I am not saying you can use sizeof(*p) everywhere but it is the common
> case and as such the preferred form.

Your solution is better if the only thing you're concerned about is
"are we allocating enough memory for this fixed size structure".
It completely breaks if you are also concerned about "are we doing
correct initialisation" or "are we allocating enough memory for this
variable sized structure" both of which are far more important
questions.

*especially* when you consider that kmalloc is redzoned and therefore
will catch the kinds of bugs you're suggesting.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: p = kmalloc(sizeof(*p), )

[Denis Vlasenko]

> > >         p = kmalloc(sizeof(*p), ...)
> > > 
> > >    is not grep-friendly, and can not be used to identify potential
> > >    initialisation sites.  However:
> > > 
> > >         p = kmalloc(sizeof(struct foo), ...)
> > > 
> > >    is grep-friendly, and will lead you to inspect each place where
> > >    such a structure is allocated for correct initialisation.
> > 
> > I would disagree on this one. You can still grep all the places where
> > the local variable is declared in. Furthermore, structs are not always
> > initialized where they're kmalloc'd so you need to manually inspect
> > anyway.
> 
> Think about it some more.  You've added a new member to struct foo.
> You want to fix up all the places which allocate struct foo to
> initialise this new member.  Grepping for 'struct foo' returns 100
> files.  Grepping for kmalloc in those 100 files returns 100 files.
> 
> Do you open all 100 in an editor and manually try and locate the five
> kmalloc instances of this structure, and end up missing some.
> 
> Or do you do the sane thing and use kmalloc(sizeof(struct foo), ...)
> and grep for "kmalloc[[:space:]]*(sizeof[[:space:]]*(struct foo)"
> which returns only the five files and fix those up with knowledge
> that you've found all the instances?

Both are inferior to Alans macro

p = typed_kmalloc(struct foo, ...);

which has greppable struct name, saves typing sizeof() and also
gives you typechecking (fails with "pointers to different types"
if p is not struct foo*).
--
vda

Re: p = kmalloc(sizeof(*p), )

[Pekka J Enberg]

Hi Russell,

On Tue, 20 Sep 2005, Russell King wrote:
> Think about it some more.  You've added a new member to struct foo.
> You want to fix up all the places which allocate struct foo to
> initialise this new member.  Grepping for 'struct foo' returns 100
> files.  Grepping for kmalloc in those 100 files returns 100 files.
> 
> Do you open all 100 in an editor and manually try and locate the five
> kmalloc instances of this structure, and end up missing some.

Nope. I grep for assignments to other members of that struct.

On Tue, 20 Sep 2005, Russell King wrote:
> Or do you do the sane thing and use kmalloc(sizeof(struct foo), ...)
> and grep for "kmalloc[[:space:]]*(sizeof[[:space:]]*(struct foo)"
> which returns only the five files and fix those up with knowledge
> that you've found all the instances?

There are still statically allocated structs left. So neither heuristic 
for figuring out initialization points is perfect.

On 9/18/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> Such shuffling around should be done in easy to review stages so that
> bugs can be found, and not a mega patch.  This inherently means that
> for a structure name change, you don't end up with a new structure
> named the same as an old structure.  And if you compile-test the
> stages, you find out if you missed the problem.

No disagreement here.

On 9/18/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> Your solution is better if the only thing you're concerned about is
> "are we allocating enough memory for this fixed size structure".
> It completely breaks if you are also concerned about "are we doing
> correct initialisation" or "are we allocating enough memory for this
> variable sized structure" both of which are far more important
> questions.
> 
> *especially* when you consider that kmalloc is redzoned and therefore
> will catch the kinds of bugs you're suggesting.

Well, yes, but for initialization, I would prefer something like what Al 
Viro suggested. To me, initialization is a separate issue from kmalloc. I 
do get your point but I just don't think sizeof(struct foo) is the answer.

In all completeness, I would personally prefer the following form for 
allocation and initialization which is readable, easy to get right, and 
highly greppable:

	struct foo *p = kmalloc(sizeof *p, ...);

	*p = (struct foo) {
		.bar = ...;
	};

Unfortunately it doesn't seem like gcc is doing such a good job with it.

			Pekka

Re: p = kmalloc(sizeof(*p), )

[Russell King]

On Tue, Sep 20, 2005 at 03:20:18PM +0300, Pekka J Enberg wrote:
> Well, yes, but for initialization, I would prefer something like what Al 
> Viro suggested. To me, initialization is a separate issue from kmalloc. I 
> do get your point but I just don't think sizeof(struct foo) is the answer.

No matter, and no matter what CodingStyle says, I won't be changing
my style of kmalloc for something which I disagree with.

Since some of the other major contributors to the kernel appear to
also disagree with the statement, I think that the entry in
CodingStyle must be removed.

Plus, this means that kernel janitors should _not_ fix up code to
follow the sizeof(*p) style.

---

It isn't clear that the use of p = kmalloc(sizeof(*p), ...) is
preferred over p = kmalloc(sizeof(struct foo), ...) - in fact,
there are some good reasons to use the latter form.

Therefore, the choice of which to use should be left up to the
developer concerned, and not written in to the coding style.

For discussion, please see the thread:
      http://lkml.org/lkml/2005/9/18/29

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
--- a/Documentation/CodingStyle
+++ b/Documentation/CodingStyle
@@ -410,26 +410,7 @@ Kernel messages do not have to be termin
 Printing numbers in parentheses (%d) adds no value and should be avoided.
 
 
-		Chapter 13: Allocating memory
-
-The kernel provides the following general purpose memory allocators:
-kmalloc(), kzalloc(), kcalloc(), and vmalloc().  Please refer to the API
-documentation for further information about them.
-
-The preferred form for passing a size of a struct is the following:
-
-	p = kmalloc(sizeof(*p), ...);
-
-The alternative form where struct name is spelled out hurts readability and
-introduces an opportunity for a bug when the pointer variable type is changed
-but the corresponding sizeof that is passed to a memory allocator is not.
-
-Casting the return value which is a void pointer is redundant. The conversion
-from void pointer to any other pointer type is guaranteed by the C programming
-language.
-
-
-		Chapter 14: References
+		Chapter 13: References
 
 The C Programming Language, Second Edition
 by Brian W. Kernighan and Dennis M. Ritchie.



-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: p = kmalloc(sizeof(*p), )

[Pekka J Enberg]

On Tue, 20 Sep 2005, Russell King wrote:
> No matter, and no matter what CodingStyle says, I won't be changing
> my style of kmalloc for something which I disagree with.

Ack. No need to clutter Coding Style on things that people won't follow.

			Pekka

Re: p = kmalloc(sizeof(*p), )

[Randy.Dunlap]

On Tue, 20 Sep 2005, Pekka J Enberg wrote:

> On Tue, 20 Sep 2005, Russell King wrote:
> > No matter, and no matter what CodingStyle says, I won't be changing
> > my style of kmalloc for something which I disagree with.
>
> Ack. No need to clutter Coding Style on things that people won't follow.

Ack.  or don't have concensus on (unless someone wants for force it)

-- 
~Randy

Re: p = kmalloc(sizeof(*p), )

[Pekka J Enberg]

Btw, I would prefer this one to be applied instead. The other parts should 
be okay, right?

[PATCH] CodingStyle remove sizeof preferred form

It isn't clear that the use of p = kmalloc(sizeof(*p), ...) is
preferred over p = kmalloc(sizeof(struct foo), ...) - in fact,
there are some good reasons to use the latter form.

Therefore, the choice of which to use should be left up to the
developer concerned, and not written in to the coding style.

For discussion, please see the thread:
     http://lkml.org/lkml/2005/9/18/29

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>

Index: 2.6/Documentation/CodingStyle
===================================================================
--- 2.6.orig/Documentation/CodingStyle
+++ 2.6/Documentation/CodingStyle
@@ -416,14 +416,6 @@ The kernel provides the following genera
 kmalloc(), kzalloc(), kcalloc(), and vmalloc().  Please refer to the API
 documentation for further information about them.
 
-The preferred form for passing a size of a struct is the following:
-
-	p = kmalloc(sizeof(*p), ...);
-
-The alternative form where struct name is spelled out hurts readability and
-introduces an opportunity for a bug when the pointer variable type is changed
-but the corresponding sizeof that is passed to a memory allocator is not.
-
 Casting the return value which is a void pointer is redundant. The conversion
 from void pointer to any other pointer type is guaranteed by the C programming
 language.

Re: p = kmalloc(sizeof(*p), )

[Andrew Morton]

Russell King <rmk+lkml@arm.linux.org.uk> wrote:
>
>  Since some of the other major contributors to the kernel appear to
>  also disagree with the statement, I think that the entry in
>  CodingStyle must be removed.

Nobody has put forward a decent reason for doing so.  "I want to grep for
initialisations" is pretty pointless because a) it won't catch everything
anyway and b) most structures are allocated and initialised at a single
place and many of those which aren't should probably be converted to do
that anyway.

The broader point is that you're trying to optimise for the wrong thing. 
We should optimise for those who read code, not for those who write it.

Every time I see such a type-unsafe allocation in a patch I have to go hunt
down the definition of the lhs.  Which is sometimes in a header file, often
one which hasn't been indexed yet.  Is a pita.

Re: p = kmalloc(sizeof(*p), )

[Russell King]

On Tue, Sep 20, 2005 at 10:11:28AM -0700, Andrew Morton wrote:
> Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> >
> >  Since some of the other major contributors to the kernel appear to
> >  also disagree with the statement, I think that the entry in
> >  CodingStyle must be removed.
> 
> Nobody has put forward a decent reason for doing so.  "I want to grep for
> initialisations" is pretty pointless because a) it won't catch everything
> anyway and b) most structures are allocated and initialised at a single
> place and many of those which aren't should probably be converted to do
> that anyway.
> 
> The broader point is that you're trying to optimise for the wrong thing. 
> We should optimise for those who read code, not for those who write it.
> 
> Every time I see such a type-unsafe allocation in a patch I have to go hunt
> down the definition of the lhs.  Which is sometimes in a header file, often
> one which hasn't been indexed yet.  Is a pita.

Well, as I've said, don't expect folk to change their style just
because something has been decided privately amongst a small select
group of folk (which is exactly what seems to have happened - maybe
not intentionally.)

And don't expect subsystem maintainers to accept the new "style"
guidelines without a fight.

However, if we really are concerned about type-unsafe allocation,
we should be using something like Alan's suggestion, where the
return type from the *alloc function is appropriately typed and
not void *.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: p = kmalloc(sizeof(*p), )

[Alan Cox]

On Maw, 2005-09-20 at 10:11 -0700, Andrew Morton wrote:
> Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> >
> >  Since some of the other major contributors to the kernel appear to
> >  also disagree with the statement, I think that the entry in
> >  CodingStyle must be removed.
> 
> Nobody has put forward a decent reason for doing so.  

I've seen five decent reasons so far. Which of the reasons on the thread
do you disagree with and why ?

Re: p = kmalloc(sizeof(*p), )

[Andrew Morton]

Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>
> On Maw, 2005-09-20 at 10:11 -0700, Andrew Morton wrote:
> > Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> > >
> > >  Since some of the other major contributors to the kernel appear to
> > >  also disagree with the statement, I think that the entry in
> > >  CodingStyle must be removed.
> > 
> > Nobody has put forward a decent reason for doing so.  
> 
> I've seen five decent reasons so far. Which of the reasons on the thread
> do you disagree with and why ?
> 

umm, the three reasons which you deleted from the mail to which you're
replying?

> "I want to grep for
> initialisations" is pretty pointless because a) it won't catch everything
> anyway and b) most structures are allocated and initialised at a single
> place and many of those which aren't should probably be converted to do
> that anyway.
>
> The broader point is that you're trying to optimise for the wrong thing. 
> We should optimise for those who read code, not for those who write it.
>

If you look back, your five reasons tend to address modifiability, not
readability.

Re: p = kmalloc(sizeof(*p), )

[Russell King]

On Tue, Sep 20, 2005 at 10:59:39AM -0700, Andrew Morton wrote:
> > "I want to grep for
> > initialisations" is pretty pointless because a) it won't catch everything
> > anyway and b) most structures are allocated and initialised at a single
> > place and many of those which aren't should probably be converted to do
> > that anyway.
> >
> > The broader point is that you're trying to optimise for the wrong thing. 
> > We should optimise for those who read code, not for those who write it.
> >
> 
> If you look back, your five reasons tend to address modifiability, not
> readability.

So what you're saying is that we should optimise the code so that
we make mistakes when we modify it.  Umm, yes, of course.

This is a contentious issue, and I don't think anyone should be
stipulating which way is the right way - which is exactly what
has been done by placing it in Coding Style.  Remember, we have
kernel janitors who _will_ change code to match that.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: p = kmalloc(sizeof(*p), )

[Jeff Garzik]

Russell King wrote:
> On Tue, Sep 20, 2005 at 10:59:39AM -0700, Andrew Morton wrote:
> 
>>>"I want to grep for
>>>initialisations" is pretty pointless because a) it won't catch everything
>>>anyway and b) most structures are allocated and initialised at a single
>>>place and many of those which aren't should probably be converted to do
>>>that anyway.
>>>
>>>The broader point is that you're trying to optimise for the wrong thing. 
>>>We should optimise for those who read code, not for those who write it.
>>>
>>
>>If you look back, your five reasons tend to address modifiability, not
>>readability.
> 
> 
> So what you're saying is that we should optimise the code so that
> we make mistakes when we modify it.  Umm, yes, of course.
> 
> This is a contentious issue, and I don't think anyone should be
> stipulating which way is the right way - which is exactly what
> has been done by placing it in Coding Style.  Remember, we have
> kernel janitors who _will_ change code to match that.

Precisely.  Since there is enough disagreement, it lacks consensus to be 
in CodingStyle.

	Jeff

Re: p = kmalloc(sizeof(*p), )

[Alan Cox]

On Maw, 2005-09-20 at 10:59 -0700, Andrew Morton wrote:
> umm, the three reasons which you deleted from the mail to which you're
> replying?

There were no reasons given in the mail I replied to. Perhaps I missed
another mail from you earlier.

I'm also puzzled by the one comemnt you made. You seem to imply that
seeing

foo = malloc(sizeof(*foo))

means it doesn't need checking. That is false on various grounds

1.	A lot of stuff is using void *, char * etc
2.	You've no idea that foo is the full object not a generic object with
stuff tacked on.

So thats very much false. You have to know what is really being
allocated in both cases. In the sizeof(*foo) case you also have to go
back, work out wtf foo really is and check that its not an array pointer
because sizeof char[40] is not the same as sizeof(* char *).

Thankfully Linus hates typedefs so that removes many of those

Re: p = kmalloc(sizeof(*p), )

[Horst von Brand]

Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Maw, 2005-09-20 at 10:11 -0700, Andrew Morton wrote:
> > Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> > >  Since some of the other major contributors to the kernel appear to
> > >  also disagree with the statement, I think that the entry in
> > >  CodingStyle must be removed.

> > Nobody has put forward a decent reason for doing so.  

> I've seen five decent reasons so far. Which of the reasons on the thread
> do you disagree with and why ?

Not sure that I'm following the logic here, so...

For one, I leaned towards "p = malloc(sizeof(*p))" before, but the reasons
given for "p = malloc(sizeof struct foo))" (or even "p = (struct foo *)
malloc(sizeof(struct foo))", wrapped in a macro) did convince me.

The gains for a reader/maintainer/code auditor I see:

- It is easier to find it later
- Initialization of *p should be nearby, finding it by type name is useful
  for checking/updating
- It forces you to think a bit before typing it in, this should make making
  mistakes somewhat harder

The loss for a code writer are:

- (Marginally) more typing
- Have to know the type of *p [but if you don't, better don't touch it...]

If the writer has got the type wrong, she will initialize wrongly (and the
compile will blow up), so I don't see any advantage. The only other case
would be something like:

   p = malloc(sizeof(...));
   memset(p, v, sizeof(...));

As v is more often than not 0, this should really be:

   p = calloc(1, sizeof(...));

and perhaps in this case (with /no/ further initialization) it could be
called a tie. For uniformity's sake I'd prefer "sizeof(struct foo)"
everywhere.

In any case, give me help in finding bugs and updating code over (minor)
initial coding convenience everyday.

In any case, as the parallel flamewars conclusively demonstrate, writing it
down in CodingStyle won't make everybody agree on using it anyway, so I'd
vote for including the "sizeof(struct foo)" version there as recommended
practice.
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513