All of lore.kernel.org
 help / color / mirror / Atom feed
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Josh Triplett <josh@freedesktop.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Harvey Harrison <harvey.harrison@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-sparse@vger.kernel.org
Subject: Re: [PATCH 7/7] asm-generic: suppress sparse warning in ioctl.h
Date: Fri, 4 Apr 2008 02:19:51 +0100	[thread overview]
Message-ID: <20080404011951.GC9785@ZenIV.linux.org.uk> (raw)
In-Reply-To: <47F53E84.8080607@freedesktop.org>

On Thu, Apr 03, 2008 at 01:31:00PM -0700, Josh Triplett wrote:
> I mostly kept up with Sparse email, hence the recent mails, but built
> up a backlog of patches to deal with.  (Sparse has a fairly low patch
> rate, so that backlog constitutes less than 10 patches, counting the
> ones that still need further work or discussion before they can
> merge.)  Also, in switching my email to an IMAP server, I managed to
> lose the read/unread status on some of my list mail, including
> linux-sparse, which slowed me down a fair bit when trying to process
> those mails.
> 
> I have a small handful of pending patches to work through on
> linux-sparse, with yours at the top of that list; I also have some
> local patches that I need to test and push.  Apologies for the
> processing delay.
> 
> I should have thought to send a "still alive" note with an explanation
> to linux-sparse.  I'll make sure to do so in the future.  (Some time
> in the next couple of years I have a thesis to write. :) )
> 
> I'll catch the backlog up and then send out an "all patches merged"
> mail, so that anyone with additional patches or anyone who thinks
> their patch got lost can resend.

OK...  FWIW, I'll probably do relaxed integer constant expressions on
top of whatever shows up in your tree + patches I've sent to the list
at some point in the next couple of weeks.  For now I'd say that we
can keep ignoring the errors from _IO...() uses in context that expect
an i-c-e (mostly - indices in array initializers); it's not widespread
and we'd lived with that for quite a while.

As far as annoyances go, lack of VLA support is responsible for far more
lost warnings.  I don't have anything in that direction beyond rather
vague plans (*and* a monstrous backlog of my own, both kernel-side and
sparse-side, so it's unlikely to drift up the list in the next couple
of months).  Does anybody have any pending work in that direction?  It's
not _that_ hard to do, but for pity sake, go with C99 standard when doing
it; gcc is choke-full of broken and barely documented extensions in that
area, so reverse-engineering them is going to result in utter mess.

C99 VLAs are fairly dumb and straightforward - the underlying idea is
"for each variable of type that involves a VLA, compiler should be able
to slap a shadow variable containing the array size in scope nearby".
Thus
	* no variably-modified members in structs or unions (not only
no VLA, but no pointers to VLA, etc.); we would need shadow variables
for each struct instance and that obviously wouldn't work in that model).
	* no functions returning variably-modified type.
	* no variably-modified objects in global scope.
	* no VLA static in function.
	* typedef *can* be variably-modified, but only in block scope.
Warning: this can get sticky for us - all sizes are evaluated when
typedef is reached.  IOW,
	typedef int a[n];
	a x;
	if (n++ == 5) {
		a y;
		int z[n];
	}
will have size of y equal to that of x, but *not* equal to that of z.
	* do *NOT* jump inside the scope of anything variably modified;
not with goto, not with switch (again, you'll miss initialization of
shadows).

Passing a VLA (let alone pointer to such, etc.) to a function is done
exactly as we would for normal arrays; $DEITY help you if the function's
idea of sizes doesn't match that of caller - shadow stuff is _NOT_ passed
at all.  No match => undefined behaviour.

Mixing VLA with normal arrays: compatible if the elements are compatible,
but if you have actual sizes that do not match, you are in nasal daemon
country.  So composite type of two VLA ones is *either*, and if actual
sizes differ, well, too bad for you.

Basically, everything works as if you had SYM_VLA(element, symbol) that
behaved almost as SYM_ARRAY.  With initializers for symbols created at
the point where we declare the object in question (or type, in case of
typedef).  Passing such sucker to function converts to SYM_VLA with *new*
symbol - one in function scope and initialized there.  Note that this
is where the things get extremely ugly for gcc - there you can pass
an object out of its scope and _that_ is what leads to lovely internal
compiler errors for things like
	sizeof *({int n = f(); int a[n]; &a;})
since we get shadow symbol dead and gone with the scope while the type
(SYM_PTR(SYM_VLA(int, n)) outlives the scope, leaving a landmine for
sizeof.

Another thing to keep in mind is that sizeof(VLA) is not a constant
expression *and* that sizeof argument is evaluated.  IOW, not only
	sizeof(int [n = f()])
has side effects, but in
	int (*p)[n];
	...
	sizeof(*(g(), p))
will have g() evaluated.  Note that if p had been declared as
	int (*p)[4];
the same sizeof() would *NOT* have called anything.  This, BTW, is where
the rules for what an integer constant expression is are getting bloody
important: 

int n = 1;
int f(void)
{
	int (*p)[1 + (0 && n)];
	return sizeof(*(g(), p));
}

_must_ call g() according to C99, while the same with
enum {n = 1;} must not, even though n won't be even looked at in either
case *and* any sane compiler will find return value of f() at compile
time, turning it to
inf f(void)
{
	g();
	return sizeof(int);
}
in the first case and
int f(void)
{
	return sizeof(int);
}
in the second.

One more thing: use of sizeof(variably-modified type) may or may not
evaluate size expressions in there if they do not affect the result.
IOW, it's unspecified whether
	sizeof(int (*)[n++])
increments n; different compilers are broken in different ways and it
might be worth generating a warning on such.  Note that
	sizeof(int [n++])
*is* guaranteed to increment n - the unspecified part is for size expressions
in such sizeof(type) buried behind a pointer.

  reply	other threads:[~2008-04-04  1:19 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-04-03  0:33 [PATCH 7/7] asm-generic: suppress sparse warning in ioctl.h Harvey Harrison
2008-04-03  0:57 ` Linus Torvalds
2008-04-03  1:34   ` Al Viro
2008-04-03  1:50     ` Linus Torvalds
2008-04-03  1:59       ` Al Viro
2008-04-03  2:51         ` Linus Torvalds
2008-04-03  2:55           ` Al Viro
2008-04-04  9:19             ` Al Viro
2008-04-03  2:41       ` Al Viro
2008-04-03 20:31         ` Josh Triplett
2008-04-04  1:19           ` Al Viro [this message]
2008-04-04  2:00             ` Al Viro
2008-04-04 14:08             ` Derek M Jones
2008-04-04 18:42               ` Al Viro

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080404011951.GC9785@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=akpm@linux-foundation.org \
    --cc=harvey.harrison@gmail.com \
    --cc=josh@freedesktop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sparse@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.