From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH 16/16] fix handling of integer constant expressions
Date: Wed, 27 Jun 2007 16:19:53 UTC
Message-ID: <fa.lWGguRa9m8EsaXteluB4CtZJTv0@ifi.uio.no>

On Wed, 27 Jun 2007, Neil Booth wrote:
>
> Here are three independently invalid non-ICEs that sparse doesn't
> diagnose.
>
> extern int f(void);
> enum { cast_to_ptr = (int) (void *) 0 };
> enum { cast_to_float = (int) (double) 1 };

Those two *really* shouldn't fail. I don't care if the C standard says so,
that is *fine*.

In particular, "offsetof()" should be portably able to basically be the
standard #define, which involves an integer cast from a constant pointer.
That had *better* be a valid constant integer expression, because it's
very useful.

And I think standards can go screw themselves, and you can make it an
error with some "--standard-pedantic" switch or similar.

Standards are just random pieces of paper, for crying out loud! They have
zero relevance in the end.

> enum { fncall = 0 ? f(): 3 };

Again, I think that's a deficiency of a standard that tries to be
acceptable to everybody rather than about a "really good language".

So I personally think we should allow it too if at all possible, and
again, use some "--standard-pedantic" to flag it as an error.

Why? Because things like that may not look sensible when written out, but
they are often _very_ sensible when they are the result of a macro that
does some error checking or other thing.

The classic example of this is "__builtin_constant_p()". It is a *great*
way to make a macro that does different things depending on whether
something is a compile-time constant or not, and no, it's not standard,
but dang, it's so useful that a standard that doesn't allow sane use of it
is basically bogus.

So look at the "ntohl()" kind of thing, and realize that it's just "Good
Practice(tm)" to be able to make a ntohl() macro that can be used for
initializers, including very much enum initializers. Ie

	enum { defaultport = htons(9418) };

is actually nice code for something like the kernel, but it turns out that
in order to make this work, you have to do it as

	#define htons(x) (__builtin_constant_p(x) ? constant_htons(x) : __htons(x))

and that in turn generates *exactly* the kind of thing you talk of above.

And when you give _your_ example, it looks insane. When I give _my_
example, it generates exactly the same thing, but suddenly it has a great
reason for doing so, and it's no longer insane.

			Linus

From: Al Viro <viro@ftp.linux.org.uk>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH 16/16] fix handling of integer constant expressions
Date: Wed, 27 Jun 2007 17:26:07 UTC
Message-ID: <fa.Q67zX5n2XDEJlp9QJ5rAbOogmQQ@ifi.uio.no>

On Wed, Jun 27, 2007 at 09:34:55AM -0700, Josh Triplett wrote:

> > is actually nice code for something like the kernel, but it turns out that
> > in order to make this work, you have to do it as
> >
> > 	#define htons(x) (__builtin_constant_p(x) ? constant_htons(x) : __htons(x))

That's not quite right.  In principle, __builtin_choose_expr() could be
used for that kind of stuff and builtins can change the rules.

> Also agreed.  Same goes for other short-circuiting operations like &&,
> ||, and ?: without the center argument; if you can determine at
> compilation time that it does not need to evaluate part of the
> expression at all, go ahead and ignore that part of the expression even
> if it does not constitute an integer constant expression.  If you want
> to optionally check for this case and issue a diagnostic, put it under
> -Wstrict-constant-expressions or similar.

That actually means extra work for evaluate_expression().  Unfortunately.

The thing is, we want to typecheck all branches, even ones not taken.
_However_, we don't want to expand all of them.  Having extra places
where we have to do expansion means extra work.

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH 16/16] fix handling of integer constant expressions
Date: Wed, 27 Jun 2007 17:46:29 UTC
Message-ID: <fa.oFca7jGogrtSVlh4FDdWbYjaqag@ifi.uio.no>

On Wed, 27 Jun 2007, Al Viro wrote:
>
> Eh...  I'd say that my variant for offsetof() is simply better - it usually
> directly turns into EXPR_VALUE, right in place, without rather convoluted
> work.  Aside of "should such cast be a constant integer expression"...

Umm. But sparse is meant to parse C code. Which very much includes *other*
projects.

The kernel, for example, has its own offsetof. And yes, these days we use
"__compiler_offsetof()", but we used to do

	#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)

and I seriously doubt that the kernel is the only one doing things like
that.

		Linus

From: Al Viro <viro@ftp.linux.org.uk>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH 16/16] fix handling of integer constant expressions
Date: Wed, 27 Jun 2007 18:04:25 UTC
Message-ID: <fa.txlV+vaTkEVKGurxU92vUFE/3ks@ifi.uio.no>

On Wed, Jun 27, 2007 at 10:45:55AM -0700, Linus Torvalds wrote:
>
>
> On Wed, 27 Jun 2007, Al Viro wrote:
> >
> > Eh...  I'd say that my variant for offsetof() is simply better - it usually
> > directly turns into EXPR_VALUE, right in place, without rather convoluted
> > work.  Aside of "should such cast be a constant integer expression"...
>
> Umm. But sparse is meant to parse C code. Which very much includes *other*
> projects.
>
> The kernel, for example, has its own offsetof. And yes, these days we use
> "__compiler_offsetof()", but we used to do
>
> 	#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
>
> and I seriously doubt that the kernel is the only one doing things like
> that.

You can't have it both way, really.  If we are talking about annotating
a codebase we _can_ annotate, that one is not a problem at all.  If we
are talking about vanilla C project that never heard about sparse...
We can define whatever extensions we like, but such project has to
cope with whatever C compilers they had been using.

So "sparse believes that this defintion of offsetof can be used as
array size" will mean fsck-all outside of #ifdef __CHECKER__ and
under such ifdef we can always define it to builtin; if anything,
that will be faster and easier on sparse.