From: Al Viro <viro@ftp.linux.org.uk>
Newsgroups: fa.linux.kernel
Subject: Re: p = kmalloc(sizeof(*p), )
Date: Sun, 18 Sep 2005 14:39:36 UTC
Message-ID: <fa.fka0cqb.k0gt8n@ifi.uio.no>
Original-Message-ID: <20050918143907.GK19626@ftp.linux.org.uk>

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.

From: Al Viro <viro@ftp.linux.org.uk>
Newsgroups: fa.linux.kernel
Subject: Re: p = kmalloc(sizeof(*p), )
Date: Sun, 18 Sep 2005 17:19:09 UTC
Message-ID: <fa.fmq2caa.lgitom@ifi.uio.no>
Original-Message-ID: <20050918171845.GL19626@ftp.linux.org.uk>

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.

From: Linus Torvalds <torvalds@osdl.org>
Newsgroups: fa.linux.kernel
Subject: Re: p = kmalloc(sizeof(*p), )
Date: Sun, 18 Sep 2005 17:32:40 UTC
Message-ID: <fa.ha457cv.n74081@ifi.uio.no>
Original-Message-ID: <Pine.LNX.4.58.0509181028140.26803@g5.osdl.org>

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 might 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

From: Al Viro <viro@ftp.linux.org.uk>
Newsgroups: fa.linux.kernel
Subject: Re: p = kmalloc(sizeof(*p), )
Date: Sun, 18 Sep 2005 17:46:49 UTC
Message-ID: <fa.fnqad27.kgasgr@ifi.uio.no>
Original-Message-ID: <20050918174549.GN19626@ftp.linux.org.uk>

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 might 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).

From: Al Viro <viro@ftp.linux.org.uk>
Newsgroups: fa.linux.kernel
Subject: Re: p = kmalloc(sizeof(*p), )
Date: Sun, 18 Sep 2005 21:12:48 UTC
Message-ID: <fa.fo9mca4.p0utos@ifi.uio.no>
Original-Message-ID: <20050918211225.GP19626@ftp.linux.org.uk>

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.

From: Al Viro <viro@ftp.linux.org.uk>
Newsgroups: fa.linux.kernel
Subject: Re: p = kmalloc(sizeof(*p), )
Date: Sun, 18 Sep 2005 21:53:26 UTC
Message-ID: <fa.fhpuda2.jgusou@ifi.uio.no>
Original-Message-ID: <20050918215257.GA29417@ftp.linux.org.uk>

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.

From: Linus Torvalds <torvalds@osdl.org>
Newsgroups: fa.linux.kernel
Subject: Re: p = kmalloc(sizeof(*p), )
Date: Sun, 18 Sep 2005 22:26:24 UTC
Message-ID: <fa.g5a52jb.h0472p@ifi.uio.no>
Original-Message-ID: <Pine.LNX.4.58.0509181513440.9106@g5.osdl.org>

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

From: Al Viro <viro@ftp.linux.org.uk>
Newsgroups: fa.linux.kernel
Subject: Re: p = kmalloc(sizeof(*p), )
Date: Sun, 18 Sep 2005 23:08:53 UTC
Message-ID: <fa.fj9uc2b.j0etgn@ifi.uio.no>
Original-Message-ID: <20050918230733.GA29869@ftp.linux.org.uk>

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.

From: Al Viro <viro@ftp.linux.org.uk>
Newsgroups: fa.linux.kernel
Subject: Re: p = kmalloc(sizeof(*p), )
Date: Sun, 18 Sep 2005 19:07:51 UTC
Message-ID: <fa.fmq4c29.mgctgp@ifi.uio.no>
Original-Message-ID: <20050918190714.GO19626@ftp.linux.org.uk>

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.

From: Al Viro <viro@ftp.linux.org.uk>
Newsgroups: fa.linux.kernel
Subject: Re: p = kmalloc(sizeof(*p), )
Date: Sun, 18 Sep 2005 21:14:56 UTC
Message-ID: <fa.fopuca6.pg6toq@ifi.uio.no>
Original-Message-ID: <20050918211429.GQ19626@ftp.linux.org.uk>

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...