Re: [PATCH 16/16] fix handling of integer constant expressions
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*.
GCC doesn't guarantee you this, either.
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.
Yes it's useful. That's why GCC gives you __builtin_offsetof()
for this purpose.
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.
Sure, as long as you don't care about compatibility across
compilers, what matters is what the compilers you _do_ use
actually implement. And note that GCC doesn't guarantee
you much over what the C standard does. Almost everything
it allows extra is just an implementation side effect.
Segher
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
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*.
GCC doesn't guarantee you this, either.
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.
Yes it's useful. That's why GCC gives you __builtin_offsetof()
for this purpose.
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.
Sure, as long as you don't care about compatibility across
compilers, what matters is what the compilers you _do_ use
actually implement. And note that GCC doesn't guarantee
you much over what the C standard does. Almost everything
it allows extra is just an implementation side effect.
Segher
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 09:19:17AM -0700, Linus Torvalds wrote: > 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. 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"... - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, 2007-06-27 at 09:19 -0700, Linus Torvalds wrote:
>
> 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*.
[...]
> And I think standards can go screw themselves, and you can make it an
> error with some "--standard-pedantic" switch or similar.
Agreed. Issuing a warning on these kinds of constructs does not seem
useful, but if people really want it, it should go in some warning
option that does not get turned on by default, and probably should not
get turned on by -Wall either.
> > 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.
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.
- Josh Triplett
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 03:06:36PM +0100, Al Viro wrote: > *unprintable* > > Yes, I see... OK, null pointer constants handling (next patch in the > queue) introduces is_zero_constant() (silent evaluation of integer > constant expression, with division by 0/too large shift/- on lowest > value of signed integer type leaving the branch as-is, so that later > expand would generate a proper error on it; then checking if we'd > reduced the sucker to EXPR_VALUE[0]). I'll pull it into a separate > patch, along with is_nonzero_constant(), and change rules for potential > ICE on parser stage to > maybe-ICE && y => maybe-ICE > maybe-ICE || y => maybe-ICE > maybe-ICE ? x : y => maybe-ICE if at least one of x and y is maybe-ICE > maybe-ICE ? : y => maybe-ICE > letting evaluate_expression() on such suckers use them if the first argument > turns out to be ICE after its evaluate_expression()... > > It really stinks, especially since we can't say "oh, parent it known to > be non-ICE, no need to bother" - subexpression might be shared. ... or it could be done simpler, if we keep the current logics for Int_const_expr flag at parse time and add a 'const expression' one with rules as above. Anyway, I'm going to get some sleep before dealing with that crap. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 07:50:18AM -0700, Josh Triplett wrote:
> On Wed, 2007-06-27 at 14:18 +0100, Al Viro wrote:
> > --- a/expand.c
> > +++ b/expand.c
> [...]
> > @@ -488,12 +490,15 @@ static int expand_conditional(struct expression *expr)
> >
> > cond_cost = expand_expression(cond);
> > if (cond->type == EXPR_VALUE) {
> > + unsigned flags = expr->flags;
> > if (!cond->value)
> > true = false;
> > if (!true)
> > true = cond;
> > + cost = expand_expression(*true);
> > *expr = *true;
> > - return expand_expression(expr);
> > + expr->flags = flags;
> > + return cost;
>
> This passes an incorrect type to expand_expression; it wants a struct
> expression *, but *true has type struct expression; did you want to pass
> true rather than *true?
Gah... Yes, of course. Sorry about that...
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, 2007-06-27 at 14:18 +0100, Al Viro wrote:
> --- a/expand.c
> +++ b/expand.c
[...]
> @@ -488,12 +490,15 @@ static int expand_conditional(struct expression *expr)
>
> cond_cost = expand_expression(cond);
> if (cond->type == EXPR_VALUE) {
> + unsigned flags = expr->flags;
> if (!cond->value)
> true = false;
> if (!true)
> true = cond;
> + cost = expand_expression(*true);
> *expr = *true;
> - return expand_expression(expr);
> + expr->flags = flags;
> + return cost;
This passes an incorrect type to expand_expression; it wants a struct
expression *, but *true has type struct expression; did you want to pass
true rather than *true?
- Josh Triplett
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 10:35:46PM +0900, Neil Booth wrote:
> Al Viro wrote:-
>
> >
> > Son of a... expand_comma() cannibalizes the node, should restore ->flags
> > to 0 (same as other similar suckers).
> >
> > > struct c { unsigned int c1: 1 ? 2: a++; };
> >
> > Ditto for expand_conditional, but there we should preserve the original
> > ->flags instead - might be non-zero and we ought to do that after
> > expanding the taken branch...
> >
> > From: Al Viro <[EMAIL PROTECTED]>
> > Date: Wed, 27 Jun 2007 09:10:54 -0400
> > Subject: [PATCH] fix the missed cannibalizing simplifications
> >
> > Signed-off-by: Al Viro <[EMAIL PROTECTED]>
>
> Now I think I only see one class of issues; the following is valid
> C99 (I believe that's what you intend to follow) but being rejected:
>
>struct a { int comma: 1 ? 2: (2, 3); };
*unprintable*
Yes, I see... OK, null pointer constants handling (next patch in the
queue) introduces is_zero_constant() (silent evaluation of integer
constant expression, with division by 0/too large shift/- on lowest
value of signed integer type leaving the branch as-is, so that later
expand would generate a proper error on it; then checking if we'd
reduced the sucker to EXPR_VALUE[0]). I'll pull it into a separate
patch, along with is_nonzero_constant(), and change rules for potential
ICE on parser stage to
maybe-ICE && y => maybe-ICE
maybe-ICE || y => maybe-ICE
maybe-ICE ? x : y => maybe-ICE if at least one of x and y is maybe-ICE
maybe-ICE ? : y => maybe-ICE
letting evaluate_expression() on such suckers use them if the first argument
turns out to be ICE after its evaluate_expression()...
It really stinks, especially since we can't say "oh, parent it known to
be non-ICE, no need to bother" - subexpression might be shared.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote:-
>
> Son of a... expand_comma() cannibalizes the node, should restore ->flags
> to 0 (same as other similar suckers).
>
> > struct c { unsigned int c1: 1 ? 2: a++; };
>
> Ditto for expand_conditional, but there we should preserve the original
> ->flags instead - might be non-zero and we ought to do that after
> expanding the taken branch...
>
> From: Al Viro <[EMAIL PROTECTED]>
> Date: Wed, 27 Jun 2007 09:10:54 -0400
> Subject: [PATCH] fix the missed cannibalizing simplifications
>
> Signed-off-by: Al Viro <[EMAIL PROTECTED]>
Now I think I only see one class of issues; the following is valid
C99 (I believe that's what you intend to follow) but being rejected:
struct a { int comma: 1 ? 2: (2, 3); };
It's invalid C90.
Neil.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 09:59:58PM +0900, Neil Booth wrote:
> Al Viro wrote:-
>
> > If you want to test ICE recognition, right now only the following places
> > are checking for it:
> > * bitfield width
> > * __attribute__((aligned()))
> > * __attribute__((address_space()))
> > * [] in designators within initializer list
> > * [ ... ] - ditto, gccism
> >
> > That's it. We can use it elsewhere too, but that's a separate bunch of
> > patches (trivial to do now).
>
> Bah. You don't escape that easily :)
>
> extern int a;
> struct b { unsigned int b1:(1,2); };
Son of a... expand_comma() cannibalizes the node, should restore ->flags
to 0 (same as other similar suckers).
> struct c { unsigned int c1: 1 ? 2: a++; };
Ditto for expand_conditional, but there we should preserve the original
->flags instead - might be non-zero and we ought to do that after
expanding the taken branch...
From: Al Viro <[EMAIL PROTECTED]>
Date: Wed, 27 Jun 2007 09:10:54 -0400
Subject: [PATCH] fix the missed cannibalizing simplifications
Signed-off-by: Al Viro <[EMAIL PROTECTED]>
---
expand.c |9 +++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/expand.c b/expand.c
index f945518..9a536d5 100644
--- a/expand.c
+++ b/expand.c
@@ -429,8 +429,10 @@ static int expand_comma(struct expression *expr)
cost = expand_expression(expr->left);
cost += expand_expression(expr->right);
- if (expr->left->type == EXPR_VALUE || expr->left->type == EXPR_FVALUE)
+ if (expr->left->type == EXPR_VALUE || expr->left->type == EXPR_FVALUE) {
*expr = *expr->right;
+ expr->flags = 0;
+ }
return cost;
}
@@ -488,12 +490,15 @@ static int expand_conditional(struct expression *expr)
cond_cost = expand_expression(cond);
if (cond->type == EXPR_VALUE) {
+ unsigned flags = expr->flags;
if (!cond->value)
true = false;
if (!true)
true = cond;
+ cost = expand_expression(*true);
*expr = *true;
- return expand_expression(expr);
+ expr->flags = flags;
+ return cost;
}
cost = expand_expression(true);
--
1.5.0-rc2.GIT
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote:-
> If you want to test ICE recognition, right now only the following places
> are checking for it:
> * bitfield width
> * __attribute__((aligned()))
> * __attribute__((address_space()))
> * [] in designators within initializer list
> * [ ... ] - ditto, gccism
>
> That's it. We can use it elsewhere too, but that's a separate bunch of
> patches (trivial to do now).
Bah. You don't escape that easily :)
extern int a;
struct b { unsigned int b1:(1,2); };
struct c { unsigned int c1: 1 ? 2: a++; };
are both invalid yet undiagnosed.
Neil.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 09:26:28PM +0900, Neil Booth wrote: > DR 312 clarified the meaning of "known constant size" > >http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_312.htm > > in the sensible way, thankfully, so your example is actually invalid. Aha. OK... I'd say that 6.7.5.2[4] should simply have "element type is not a variable length array" instead, regardless of clarification of "known constant size", since element type is not allowed to be incomplete in any case. Would be more readable... But yes, clarification makes sense anyway. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 09:10:21PM +0900, Neil Booth wrote:
> Al Viro wrote:-
>
> > Hopefully correct handling of integer constant expressions. Please, review.
>
> 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 };
> enum { fncall = 0 ? f(): 3 };
>
> Hey, I did warn you it was tricky :)
Nope. They are recognized as non-ICE (try with struct { int n :;};
for tester). Again, no check for ICE in enums, and that one is for
a good reason - we might very well want non-integer enums as an extension
at some point. We certainly need to check that they are constant, though.
If you want to test ICE recognition, right now only the following places
are checking for it:
* bitfield width
* __attribute__((aligned()))
* __attribute__((address_space()))
* [] in designators within initializer list
* [ ... ] - ditto, gccism
That's it. We can use it elsewhere too, but that's a separate bunch of
patches (trivial to do now).
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote:-
> Egads... After rereading that... What a mess.
>
> int foo(void)
> {
> static int a[1][0,2];
> }
>
> is, AFAICS, allowed. Reason:
> int a[0,2]
> is a VLA due to 6.7.5.2[4] (0,2 is not an ICE). However, due to the language
> in the same section,
> int a[1][0,2]
> is *not* a VLA, since (a) 2 is an ICE and (b) its element type "has a known
> constant size" (it does - the value of 0,2 is certainly guaranteed to be 2).
> I.e., it's VM type, but not a VLA. I.e. only the first part of 6.7.5.2[2]
> applies and we are actually fine.
>
> So we can have a static single-element array of int [0,2], but
> not a plain static int [0,2]. Lovely, that...
DR 312 clarified the meaning of "known constant size"
http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_312.htm
in the sensible way, thankfully, so your example is actually invalid.
Neil.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 08:52:45PM +0900, Neil Booth wrote:
> Al Viro wrote:-
>
> >
> > sparse simply doesn't check that. We don't have anything resembling
> > support of VLA. Note that check for integer constant expression
> > has nothing to do with that;
> >
> > int x[(int)(0.6 + 0.6)];
> >
> > is valid (if stupid).
>
> It isn't valid; it fails the test twice. Both 0.6 are not "immediate
> operands of casts". Their sum is, but that's irrelevant.
> Therefore the dimension is not an ICE and a diagnostic is required.
Egads... After rereading that... What a mess.
int foo(void)
{
static int a[1][0,2];
}
is, AFAICS, allowed. Reason:
int a[0,2]
is a VLA due to 6.7.5.2[4] (0,2 is not an ICE). However, due to the language
in the same section,
int a[1][0,2]
is *not* a VLA, since (a) 2 is an ICE and (b) its element type "has a known
constant size" (it does - the value of 0,2 is certainly guaranteed to be 2).
I.e., it's VM type, but not a VLA. I.e. only the first part of 6.7.5.2[2]
applies and we are actually fine.
So we can have a static single-element array of int [0,2], but
not a plain static int [0,2]. Lovely, that...
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote:-
> Hopefully correct handling of integer constant expressions. Please, review.
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 };
enum { fncall = 0 ? f(): 3 };
Hey, I did warn you it was tricky :)
Neil.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote:- > > sparse simply doesn't check that. We don't have anything resembling > support of VLA. Note that check for integer constant expression > has nothing to do with that; > > int x[(int)(0.6 + 0.6)]; > > is valid (if stupid). It isn't valid; it fails the test twice. Both 0.6 are not "immediate operands of casts". Their sum is, but that's irrelevant. Therefore the dimension is not an ICE and a diagnostic is required. Neil. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote:- sparse simply doesn't check that. We don't have anything resembling support of VLA. Note that check for integer constant expression has nothing to do with that; int x[(int)(0.6 + 0.6)]; is valid (if stupid). It isn't valid; it fails the test twice. Both 0.6 are not immediate operands of casts. Their sum is, but that's irrelevant. Therefore the dimension is not an ICE and a diagnostic is required. Neil. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote:-
Hopefully correct handling of integer constant expressions. Please, review.
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 };
enum { fncall = 0 ? f(): 3 };
Hey, I did warn you it was tricky :)
Neil.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 08:52:45PM +0900, Neil Booth wrote:
Al Viro wrote:-
sparse simply doesn't check that. We don't have anything resembling
support of VLA. Note that check for integer constant expression
has nothing to do with that;
int x[(int)(0.6 + 0.6)];
is valid (if stupid).
It isn't valid; it fails the test twice. Both 0.6 are not immediate
operands of casts. Their sum is, but that's irrelevant.
Therefore the dimension is not an ICE and a diagnostic is required.
Egads... After rereading that... What a mess.
int foo(void)
{
static int a[1][0,2];
}
is, AFAICS, allowed. Reason:
int a[0,2]
is a VLA due to 6.7.5.2[4] (0,2 is not an ICE). However, due to the language
in the same section,
int a[1][0,2]
is *not* a VLA, since (a) 2 is an ICE and (b) its element type has a known
constant size (it does - the value of 0,2 is certainly guaranteed to be 2).
I.e., it's VM type, but not a VLA. I.e. only the first part of 6.7.5.2[2]
applies and we are actually fine.
So we can have a static single-element array of int [0,2], but
not a plain static int [0,2]. Lovely, that...
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote:-
Egads... After rereading that... What a mess.
int foo(void)
{
static int a[1][0,2];
}
is, AFAICS, allowed. Reason:
int a[0,2]
is a VLA due to 6.7.5.2[4] (0,2 is not an ICE). However, due to the language
in the same section,
int a[1][0,2]
is *not* a VLA, since (a) 2 is an ICE and (b) its element type has a known
constant size (it does - the value of 0,2 is certainly guaranteed to be 2).
I.e., it's VM type, but not a VLA. I.e. only the first part of 6.7.5.2[2]
applies and we are actually fine.
So we can have a static single-element array of int [0,2], but
not a plain static int [0,2]. Lovely, that...
DR 312 clarified the meaning of known constant size
http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_312.htm
in the sensible way, thankfully, so your example is actually invalid.
Neil.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 09:10:21PM +0900, Neil Booth wrote:
Al Viro wrote:-
Hopefully correct handling of integer constant expressions. Please, review.
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 };
enum { fncall = 0 ? f(): 3 };
Hey, I did warn you it was tricky :)
Nope. They are recognized as non-ICE (try with struct { int n :expr;};
for tester). Again, no check for ICE in enums, and that one is for
a good reason - we might very well want non-integer enums as an extension
at some point. We certainly need to check that they are constant, though.
If you want to test ICE recognition, right now only the following places
are checking for it:
* bitfield width
* __attribute__((aligned(number)))
* __attribute__((address_space(number)))
* [index] in designators within initializer list
* [index ... index] - ditto, gccism
That's it. We can use it elsewhere too, but that's a separate bunch of
patches (trivial to do now).
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 09:26:28PM +0900, Neil Booth wrote: DR 312 clarified the meaning of known constant size http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_312.htm in the sensible way, thankfully, so your example is actually invalid. looks Aha. OK... I'd say that 6.7.5.2[4] should simply have element type is not a variable length array instead, regardless of clarification of known constant size, since element type is not allowed to be incomplete in any case. Would be more readable... But yes, clarification makes sense anyway. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote:-
If you want to test ICE recognition, right now only the following places
are checking for it:
* bitfield width
* __attribute__((aligned(number)))
* __attribute__((address_space(number)))
* [index] in designators within initializer list
* [index ... index] - ditto, gccism
That's it. We can use it elsewhere too, but that's a separate bunch of
patches (trivial to do now).
Bah. You don't escape that easily :)
extern int a;
struct b { unsigned int b1:(1,2); };
struct c { unsigned int c1: 1 ? 2: a++; };
are both invalid yet undiagnosed.
Neil.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 09:59:58PM +0900, Neil Booth wrote:
Al Viro wrote:-
If you want to test ICE recognition, right now only the following places
are checking for it:
* bitfield width
* __attribute__((aligned(number)))
* __attribute__((address_space(number)))
* [index] in designators within initializer list
* [index ... index] - ditto, gccism
That's it. We can use it elsewhere too, but that's a separate bunch of
patches (trivial to do now).
Bah. You don't escape that easily :)
extern int a;
struct b { unsigned int b1:(1,2); };
Son of a... expand_comma() cannibalizes the node, should restore -flags
to 0 (same as other similar suckers).
struct c { unsigned int c1: 1 ? 2: a++; };
Ditto for expand_conditional, but there we should preserve the original
-flags instead - might be non-zero and we ought to do that after
expanding the taken branch...
From: Al Viro [EMAIL PROTECTED]
Date: Wed, 27 Jun 2007 09:10:54 -0400
Subject: [PATCH] fix the missed cannibalizing simplifications
Signed-off-by: Al Viro [EMAIL PROTECTED]
---
expand.c |9 +++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/expand.c b/expand.c
index f945518..9a536d5 100644
--- a/expand.c
+++ b/expand.c
@@ -429,8 +429,10 @@ static int expand_comma(struct expression *expr)
cost = expand_expression(expr-left);
cost += expand_expression(expr-right);
- if (expr-left-type == EXPR_VALUE || expr-left-type == EXPR_FVALUE)
+ if (expr-left-type == EXPR_VALUE || expr-left-type == EXPR_FVALUE) {
*expr = *expr-right;
+ expr-flags = 0;
+ }
return cost;
}
@@ -488,12 +490,15 @@ static int expand_conditional(struct expression *expr)
cond_cost = expand_expression(cond);
if (cond-type == EXPR_VALUE) {
+ unsigned flags = expr-flags;
if (!cond-value)
true = false;
if (!true)
true = cond;
+ cost = expand_expression(*true);
*expr = *true;
- return expand_expression(expr);
+ expr-flags = flags;
+ return cost;
}
cost = expand_expression(true);
--
1.5.0-rc2.GIT
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote:-
Son of a... expand_comma() cannibalizes the node, should restore -flags
to 0 (same as other similar suckers).
struct c { unsigned int c1: 1 ? 2: a++; };
Ditto for expand_conditional, but there we should preserve the original
-flags instead - might be non-zero and we ought to do that after
expanding the taken branch...
From: Al Viro [EMAIL PROTECTED]
Date: Wed, 27 Jun 2007 09:10:54 -0400
Subject: [PATCH] fix the missed cannibalizing simplifications
Signed-off-by: Al Viro [EMAIL PROTECTED]
Now I think I only see one class of issues; the following is valid
C99 (I believe that's what you intend to follow) but being rejected:
struct a { int comma: 1 ? 2: (2, 3); };
It's invalid C90.
Neil.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 10:35:46PM +0900, Neil Booth wrote:
Al Viro wrote:-
Son of a... expand_comma() cannibalizes the node, should restore -flags
to 0 (same as other similar suckers).
struct c { unsigned int c1: 1 ? 2: a++; };
Ditto for expand_conditional, but there we should preserve the original
-flags instead - might be non-zero and we ought to do that after
expanding the taken branch...
From: Al Viro [EMAIL PROTECTED]
Date: Wed, 27 Jun 2007 09:10:54 -0400
Subject: [PATCH] fix the missed cannibalizing simplifications
Signed-off-by: Al Viro [EMAIL PROTECTED]
Now I think I only see one class of issues; the following is valid
C99 (I believe that's what you intend to follow) but being rejected:
struct a { int comma: 1 ? 2: (2, 3); };
*unprintable*
Yes, I see... OK, null pointer constants handling (next patch in the
queue) introduces is_zero_constant() (silent evaluation of integer
constant expression, with division by 0/too large shift/- on lowest
value of signed integer type leaving the branch as-is, so that later
expand would generate a proper error on it; then checking if we'd
reduced the sucker to EXPR_VALUE[0]). I'll pull it into a separate
patch, along with is_nonzero_constant(), and change rules for potential
ICE on parser stage to
maybe-ICE y = maybe-ICE
maybe-ICE || y = maybe-ICE
maybe-ICE ? x : y = maybe-ICE if at least one of x and y is maybe-ICE
maybe-ICE ? : y = maybe-ICE
letting evaluate_expression() on such suckers use them if the first argument
turns out to be ICE after its evaluate_expression()...
It really stinks, especially since we can't say oh, parent it known to
be non-ICE, no need to bother - subexpression might be shared.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, 2007-06-27 at 14:18 +0100, Al Viro wrote:
--- a/expand.c
+++ b/expand.c
[...]
@@ -488,12 +490,15 @@ static int expand_conditional(struct expression *expr)
cond_cost = expand_expression(cond);
if (cond-type == EXPR_VALUE) {
+ unsigned flags = expr-flags;
if (!cond-value)
true = false;
if (!true)
true = cond;
+ cost = expand_expression(*true);
*expr = *true;
- return expand_expression(expr);
+ expr-flags = flags;
+ return cost;
This passes an incorrect type to expand_expression; it wants a struct
expression *, but *true has type struct expression; did you want to pass
true rather than *true?
- Josh Triplett
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 07:50:18AM -0700, Josh Triplett wrote:
On Wed, 2007-06-27 at 14:18 +0100, Al Viro wrote:
--- a/expand.c
+++ b/expand.c
[...]
@@ -488,12 +490,15 @@ static int expand_conditional(struct expression *expr)
cond_cost = expand_expression(cond);
if (cond-type == EXPR_VALUE) {
+ unsigned flags = expr-flags;
if (!cond-value)
true = false;
if (!true)
true = cond;
+ cost = expand_expression(*true);
*expr = *true;
- return expand_expression(expr);
+ expr-flags = flags;
+ return cost;
This passes an incorrect type to expand_expression; it wants a struct
expression *, but *true has type struct expression; did you want to pass
true rather than *true?
Gah... Yes, of course. Sorry about that...
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 03:06:36PM +0100, Al Viro wrote: *unprintable* Yes, I see... OK, null pointer constants handling (next patch in the queue) introduces is_zero_constant() (silent evaluation of integer constant expression, with division by 0/too large shift/- on lowest value of signed integer type leaving the branch as-is, so that later expand would generate a proper error on it; then checking if we'd reduced the sucker to EXPR_VALUE[0]). I'll pull it into a separate patch, along with is_nonzero_constant(), and change rules for potential ICE on parser stage to maybe-ICE y = maybe-ICE maybe-ICE || y = maybe-ICE maybe-ICE ? x : y = maybe-ICE if at least one of x and y is maybe-ICE maybe-ICE ? : y = maybe-ICE letting evaluate_expression() on such suckers use them if the first argument turns out to be ICE after its evaluate_expression()... It really stinks, especially since we can't say oh, parent it known to be non-ICE, no need to bother - subexpression might be shared. ... or it could be done simpler, if we keep the current logics for Int_const_expr flag at parse time and add a 'const expression' one with rules as above. Anyway, I'm going to get some sleep before dealing with that crap. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, 2007-06-27 at 09:19 -0700, Linus Torvalds wrote:
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*.
[...]
And I think standards can go screw themselves, and you can make it an
error with some --standard-pedantic switch or similar.
Agreed. Issuing a warning on these kinds of constructs does not seem
useful, but if people really want it, it should go in some warning
option that does not get turned on by default, and probably should not
get turned on by -Wall either.
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.
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.
- Josh Triplett
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 09:19:17AM -0700, Linus Torvalds wrote: 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. 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... - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote: Hopefully correct handling of integer constant expressions. Please, review. Am I invoking sparse wrongly? ./sparse -W -Wall doesn't diagnose the following TU, for example. extern int a; extern int as1[(a = 2)]; sparse simply doesn't check that. We don't have anything resembling support of VLA. If it did support VLAs it would point out that this is a constraint violation. VLAs must have block or function prototype scope. -- Derek M. Jones tel: +44 (0) 1252 520 667 Knowledge Software Ltd mailto:[EMAIL PROTECTED] Applications Standards Conformance Testinghttp://www.knosof.co.uk - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 01:29:59AM +0100, Derek M Jones wrote: > Al Viro wrote: > > >>>Hopefully correct handling of integer constant expressions. Please, > >>>review. > >>Am I invoking sparse wrongly? ./sparse -W -Wall doesn't diagnose > >>the following TU, for example. > >> > >>extern int a; > >>extern int as1[(a = 2)]; > > > >sparse simply doesn't check that. We don't have anything resembling > >support of VLA. > > If it did support VLAs it would point out that this is > a constraint violation. VLAs must have block or function > prototype scope. I know. It's just that "let's do something about array size checks" triggers "yeah, but the poor sod who does that will have to sort VLAs *and* gcc extensions around VLAs out", which is not a nice thought and so far nobody had touched that area at all. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Tue, Jun 26, 2007 at 05:25:06PM -0700, Linus Torvalds wrote:
>
>
> On Wed, 27 Jun 2007, Al Viro wrote:
> >
> > > extern int a;
> > > extern int as1[(a = 2)];
> >
> > sparse simply doesn't check that. We don't have anything resembling
> > support of VLA.
>
> Well, the above has two bugs that sparse could notice _independently_ of
> variable-sized arrays:
> - assignment outside of a function
> - variable size array that isn't an automatic variable
Right; what I'm saying is that we don't do any checks on array sizes at
all, mostly since nobody is brave enough to deal with VLAs (which we'll
have to do if we start doing that).
> (strictly speaking, that's not even a variable size - it's a constant 2,
> just with a non-constant expression - maybe you misread the "=" as an
> "==")
With == it would be a different bug ;-)
BTW, VLA can be not just auto variable - it can be used in derivation of
such (i.e. you can say int (*p)[n], just not for anything not in block
or prototype scope). And $DEITY help us[1] when ({...}) comes into the
game, since it allows leaking types out of the scope they'd been
declared in...
[1] or gcc - it gets an ICE galore in that class of testcases
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, 27 Jun 2007, Al Viro wrote: > > > extern int a; > > extern int as1[(a = 2)]; > > sparse simply doesn't check that. We don't have anything resembling > support of VLA. Well, the above has two bugs that sparse could notice _independently_ of variable-sized arrays: - assignment outside of a function - variable size array that isn't an automatic variable (strictly speaking, that's not even a variable size - it's a constant 2, just with a non-constant expression - maybe you misread the "=" as an "==") Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 08:32:26AM +0900, Neil Booth wrote:
> Al Viro wrote:-
>
> > Hopefully correct handling of integer constant expressions. Please, review.
>
> Am I invoking sparse wrongly? ./sparse -W -Wall doesn't diagnose
> the following TU, for example.
>
> extern int a;
> extern int as1[(a = 2)];
sparse simply doesn't check that. We don't have anything resembling
support of VLA. Note that check for integer constant expression
has nothing to do with that;
int x[(int)(0.6 + 0.6)];
is valid (if stupid). And yes, footnote in 6.6 contradicts 6.7.5.2(1);
too bad...
We certainly need to do checks on array sizes; however, that part
("if it has static storage duration, it should not be a VLA") is minor.
And then there are gccisms:
size_t foo(int n)
{
struct {
int a[n];
char b;
} x;
return offsetof(typeof(x), b);
}
Yes, it's eaten up just fine. And yes, such structures are silently
accepted even with -pedantic -std=c99, which is a bug. Sigh...
We'll need to tackle VLAs at some point, but it certainly won't be fun ;-/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote:- > Hopefully correct handling of integer constant expressions. Please, review. Am I invoking sparse wrongly? ./sparse -W -Wall doesn't diagnose the following TU, for example. extern int a; extern int as1[(a = 2)]; Neil. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Tue, 2007-06-26 at 23:10 +0100, Al Viro wrote: > On Sun, Jun 24, 2007 at 10:31:06PM -0700, Josh Triplett wrote: > > Al Viro wrote: > > > Joy. OK, folks, disregard 16/16 in the current form; everything prior > > > to it stands on its own. > > > > Acknowledged. The rest of the patches look good to me, so I'll merge 1-15 > > soon, and ignore 16. > > OK, here's the replacement. First the handling of __builtin_offsetof() > (below in this mail), then integer constant expressions (in followup). > They can be pulled from > git://git.kernel.org/pub/scm/linux/kernel/git/viro/sparse.git/ > integer-constant Both patches looked good to me in review, and neither changes the validation output. Merged. Thanks! - Josh Triplett - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
> OK, here's the replacement. First the handling of __builtin_offsetof()
> (below in this mail), then integer constant expressions (in followup).
>From a24a3adf3f0e3c22b0d98837090c55307f6fec84 Mon Sep 17 00:00:00 2001
From: Al Viro <[EMAIL PROTECTED]>
Date: Sun, 24 Jun 2007 03:11:14 -0400
Subject: [PATCH] fix handling of integer constant expressions
Hopefully correct handling of integer constant expressions. Please, review.
Rules:
* two new flags for expression: int_const_expr and float_literal.
* parser sets them by the following rules:
* EXPR_FVALUE gets float_literal
* EXPR_VALUE gets int_const_expr
* EXPR_PREOP[(] inherits from argument
* EXPR_SIZEOF, EXPR_PTRSIZEOF, EXPR_ALIGNOF get int_const_expr
* EXPR_BINOP, EXPR_COMPARE, EXPR_LOGICAL, EXPR_CONDITIONAL,
EXPR_PREOP[+,-,!,~]: get marked int_const_expr if all their
arguments are marked that way
* EXPR_CAST gets marked int_const_expr if argument is marked
that way; if argument is marked float_literal but not
int_const_expr, we get both flags set.
* EXPR_TYPE also gets marked int_const_expr (to make it DTRT
on the builtin_same_type_p() et.al.)
* EXPR_OFFSETOF gets marked int_const_expr
When we get an expression from parser, we know that having int_const_expr on
it is almost equivalent to "it's an integer constant expression". Indeed,
the only checks we still have not done are that all casts present in there
are to integer types, that expression is correctly typed and that all indices
in offsetof are integer constant expressions. That belongs to
evaluate_expression()
and is easily done there.
* evaluate_expression() removes int_const_expr from some nodes:
* EXPR_BINOP, EXPR_COMPARE, EXPR_LOGICAL, EXPR_CONDITIONAL,
EXPR_PREOP: if the node is marked int_const_expr and some
of its arguments are not marked that way once we have
done evaluate_expression() on them, unmark our node.
* EXPR_IMLICIT_CAST: inherit flags from argument.
* cannibalizing nodes in *& and &* simplifications: unmark
the result.
* EXPR_CAST: unmark if we are casting not to an integer type.
Unmark if argument is not marked with int_const_expr after
evaluate_expression() on it *and* our node is not marked
float_literal (i.e. (int)0.0 is fine with us).
* EXPR_BINOP created (or cannibalizing EXPR_OFFSETOF) by
evaluation of evaluate_offsetof() get int_const_expr
if both arguments (already typechecked) have int_const_expr.
* unmark node when we declare it mistyped.
That does it - after evaluate_expression() we keep int_const_expr only if
expression was a valid integer constant expression.
Remaining issue: VLA handling. Right now sparse doesn't deal with those in
any sane way, but once we start handling their sizeof, we'll need to check
that type is constant-sized before marking EXPR_SIZEOF int_const_expr.
Signed-off-by: Al Viro <[EMAIL PROTECTED]>
---
evaluate.c | 43
expand.c | 16 -
expression.c | 68 -
expression.h |9 ++-
parse.c | 10
5 files changed, 133 insertions(+), 13 deletions(-)
diff --git a/evaluate.c b/evaluate.c
index c702ac4..bcac1d2 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -311,6 +311,7 @@ static struct expression * cast_to(struct expression *old,
struct symbol *type)
}
expr = alloc_expression(old->pos, EXPR_IMPLIED_CAST);
+ expr->flags = old->flags;
expr->ctype = type;
expr->cast_type = type;
expr->cast_expression = old;
@@ -403,6 +404,7 @@ static struct symbol *bad_expr_type(struct expression *expr)
break;
}
+ expr->flags = 0;
return expr->ctype = _ctype;
}
@@ -880,6 +882,10 @@ static struct symbol *evaluate_logical(struct expression
*expr)
return NULL;
expr->ctype = _ctype;
+ if (expr->flags) {
+ if (!(expr->left->flags & expr->right->flags & Int_const_expr))
+ expr->flags = 0;
+ }
return _ctype;
}
@@ -890,6 +896,11 @@ static struct symbol *evaluate_binop(struct expression
*expr)
int rclass = classify_type(expr->right->ctype, );
int op = expr->op;
+ if (expr->flags) {
+ if (!(expr->left->flags & expr->right->flags & Int_const_expr))
+ expr->flags = 0;
+ }
+
/* number op number */
if (lclass & rclass & TYPE_NUM) {
if ((lclass | rclass) & TYPE_FLOAT) {
@@
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, Jun 24, 2007 at 10:31:06PM -0700, Josh Triplett wrote:
> Al Viro wrote:
> > Joy. OK, folks, disregard 16/16 in the current form; everything prior
> > to it stands on its own.
>
> Acknowledged. The rest of the patches look good to me, so I'll merge 1-15
> soon, and ignore 16.
OK, here's the replacement. First the handling of __builtin_offsetof()
(below in this mail), then integer constant expressions (in followup).
They can be pulled from
git://git.kernel.org/pub/scm/linux/kernel/git/viro/sparse.git/ integer-constant
>From a194f3e092f7b1c31502564b3fd7fcfce0910aff Mon Sep 17 00:00:00 2001
From: Al Viro <[EMAIL PROTECTED]>
Date: Tue, 26 Jun 2007 16:06:32 -0400
Subject: [PATCH] implement __builtin_offsetof()
Signed-off-by: Al Viro <[EMAIL PROTECTED]>
---
evaluate.c | 84 ++
expand.c |1 +
expression.c | 67 ++
expression.h | 10 +++
ident-list.h |1 +
inline.c | 18 +++-
lib.c|1 -
show-parse.c |1 +
8 files changed, 181 insertions(+), 2 deletions(-)
diff --git a/evaluate.c b/evaluate.c
index 07ebf0c..c702ac4 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2608,6 +2608,87 @@ static struct symbol *evaluate_call(struct expression
*expr)
return expr->ctype;
}
+static struct symbol *evaluate_offsetof(struct expression *expr)
+{
+ struct expression *e = expr->down;
+ struct symbol *ctype = expr->in;
+ int class;
+
+ if (expr->op == '.') {
+ struct symbol *field;
+ int offset = 0;
+ if (!ctype) {
+ expression_error(expr, "expected structure or union");
+ return NULL;
+ }
+ examine_symbol_type(ctype);
+ class = classify_type(ctype, );
+ if (class != TYPE_COMPOUND) {
+ expression_error(expr, "expected structure or union");
+ return NULL;
+ }
+
+ field = find_identifier(expr->ident, ctype->symbol_list,
);
+ if (!field) {
+ expression_error(expr, "unknown member");
+ return NULL;
+ }
+ ctype = field;
+ expr->type = EXPR_VALUE;
+ expr->value = offset;
+ expr->ctype = size_t_ctype;
+ } else {
+ if (!ctype) {
+ expression_error(expr, "expected structure or union");
+ return NULL;
+ }
+ examine_symbol_type(ctype);
+ class = classify_type(ctype, );
+ if (class != (TYPE_COMPOUND | TYPE_PTR)) {
+ expression_error(expr, "expected array");
+ return NULL;
+ }
+ ctype = ctype->ctype.base_type;
+ if (!expr->index) {
+ expr->type = EXPR_VALUE;
+ expr->value = 0;
+ expr->ctype = size_t_ctype;
+ } else {
+ struct expression *idx = expr->index, *m;
+ struct symbol *i_type = evaluate_expression(idx);
+ int i_class = classify_type(i_type, _type);
+ if (!is_int(i_class)) {
+ expression_error(expr, "non-integer index");
+ return NULL;
+ }
+ unrestrict(idx, i_class, _type);
+ idx = cast_to(idx, size_t_ctype);
+ m = alloc_const_expression(expr->pos,
+ ctype->bit_size >> 3);
+ m->ctype = size_t_ctype;
+ expr->type = EXPR_BINOP;
+ expr->left = idx;
+ expr->right = m;
+ expr->op = '*';
+ expr->ctype = size_t_ctype;
+ }
+ }
+ if (e) {
+ struct expression *copy = __alloc_expression(0);
+ *copy = *expr;
+ if (e->type == EXPR_OFFSETOF)
+ e->in = ctype;
+ if (!evaluate_expression(e))
+ return NULL;
+ expr->type = EXPR_BINOP;
+ expr->op = '+';
+ expr->ctype = size_t_ctype;
+ expr->left = copy;
+ expr->right = e;
+ }
+ return size_t_ctype;
+}
+
struct symbol *evaluate_expression(struct expression *expr)
{
if (!expr)
@@ -2688,6 +2769,9 @@ struct symbol *evaluate_expression(struct expression
*expr)
expr->ctype = _ctype;
return _ctype;
+ case EXPR_OFFSETOF:
+ return evaluate_offsetof(expr);
+
/* These can not exist as stand-alone expressions */
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, Jun 24, 2007 at 10:31:06PM -0700, Josh Triplett wrote:
Al Viro wrote:
Joy. OK, folks, disregard 16/16 in the current form; everything prior
to it stands on its own.
Acknowledged. The rest of the patches look good to me, so I'll merge 1-15
soon, and ignore 16.
OK, here's the replacement. First the handling of __builtin_offsetof()
(below in this mail), then integer constant expressions (in followup).
They can be pulled from
git://git.kernel.org/pub/scm/linux/kernel/git/viro/sparse.git/ integer-constant
From a194f3e092f7b1c31502564b3fd7fcfce0910aff Mon Sep 17 00:00:00 2001
From: Al Viro [EMAIL PROTECTED]
Date: Tue, 26 Jun 2007 16:06:32 -0400
Subject: [PATCH] implement __builtin_offsetof()
Signed-off-by: Al Viro [EMAIL PROTECTED]
---
evaluate.c | 84 ++
expand.c |1 +
expression.c | 67 ++
expression.h | 10 +++
ident-list.h |1 +
inline.c | 18 +++-
lib.c|1 -
show-parse.c |1 +
8 files changed, 181 insertions(+), 2 deletions(-)
diff --git a/evaluate.c b/evaluate.c
index 07ebf0c..c702ac4 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2608,6 +2608,87 @@ static struct symbol *evaluate_call(struct expression
*expr)
return expr-ctype;
}
+static struct symbol *evaluate_offsetof(struct expression *expr)
+{
+ struct expression *e = expr-down;
+ struct symbol *ctype = expr-in;
+ int class;
+
+ if (expr-op == '.') {
+ struct symbol *field;
+ int offset = 0;
+ if (!ctype) {
+ expression_error(expr, expected structure or union);
+ return NULL;
+ }
+ examine_symbol_type(ctype);
+ class = classify_type(ctype, ctype);
+ if (class != TYPE_COMPOUND) {
+ expression_error(expr, expected structure or union);
+ return NULL;
+ }
+
+ field = find_identifier(expr-ident, ctype-symbol_list,
offset);
+ if (!field) {
+ expression_error(expr, unknown member);
+ return NULL;
+ }
+ ctype = field;
+ expr-type = EXPR_VALUE;
+ expr-value = offset;
+ expr-ctype = size_t_ctype;
+ } else {
+ if (!ctype) {
+ expression_error(expr, expected structure or union);
+ return NULL;
+ }
+ examine_symbol_type(ctype);
+ class = classify_type(ctype, ctype);
+ if (class != (TYPE_COMPOUND | TYPE_PTR)) {
+ expression_error(expr, expected array);
+ return NULL;
+ }
+ ctype = ctype-ctype.base_type;
+ if (!expr-index) {
+ expr-type = EXPR_VALUE;
+ expr-value = 0;
+ expr-ctype = size_t_ctype;
+ } else {
+ struct expression *idx = expr-index, *m;
+ struct symbol *i_type = evaluate_expression(idx);
+ int i_class = classify_type(i_type, i_type);
+ if (!is_int(i_class)) {
+ expression_error(expr, non-integer index);
+ return NULL;
+ }
+ unrestrict(idx, i_class, i_type);
+ idx = cast_to(idx, size_t_ctype);
+ m = alloc_const_expression(expr-pos,
+ ctype-bit_size 3);
+ m-ctype = size_t_ctype;
+ expr-type = EXPR_BINOP;
+ expr-left = idx;
+ expr-right = m;
+ expr-op = '*';
+ expr-ctype = size_t_ctype;
+ }
+ }
+ if (e) {
+ struct expression *copy = __alloc_expression(0);
+ *copy = *expr;
+ if (e-type == EXPR_OFFSETOF)
+ e-in = ctype;
+ if (!evaluate_expression(e))
+ return NULL;
+ expr-type = EXPR_BINOP;
+ expr-op = '+';
+ expr-ctype = size_t_ctype;
+ expr-left = copy;
+ expr-right = e;
+ }
+ return size_t_ctype;
+}
+
struct symbol *evaluate_expression(struct expression *expr)
{
if (!expr)
@@ -2688,6 +2769,9 @@ struct symbol *evaluate_expression(struct expression
*expr)
expr-ctype = type_ctype;
return type_ctype;
+ case EXPR_OFFSETOF:
+ return evaluate_offsetof(expr);
+
/* These can not exist as stand-alone expressions */
case EXPR_INITIALIZER:
Re: [PATCH 16/16] fix handling of integer constant expressions
OK, here's the replacement. First the handling of __builtin_offsetof()
(below in this mail), then integer constant expressions (in followup).
From a24a3adf3f0e3c22b0d98837090c55307f6fec84 Mon Sep 17 00:00:00 2001
From: Al Viro [EMAIL PROTECTED]
Date: Sun, 24 Jun 2007 03:11:14 -0400
Subject: [PATCH] fix handling of integer constant expressions
Hopefully correct handling of integer constant expressions. Please, review.
Rules:
* two new flags for expression: int_const_expr and float_literal.
* parser sets them by the following rules:
* EXPR_FVALUE gets float_literal
* EXPR_VALUE gets int_const_expr
* EXPR_PREOP[(] inherits from argument
* EXPR_SIZEOF, EXPR_PTRSIZEOF, EXPR_ALIGNOF get int_const_expr
* EXPR_BINOP, EXPR_COMPARE, EXPR_LOGICAL, EXPR_CONDITIONAL,
EXPR_PREOP[+,-,!,~]: get marked int_const_expr if all their
arguments are marked that way
* EXPR_CAST gets marked int_const_expr if argument is marked
that way; if argument is marked float_literal but not
int_const_expr, we get both flags set.
* EXPR_TYPE also gets marked int_const_expr (to make it DTRT
on the builtin_same_type_p() et.al.)
* EXPR_OFFSETOF gets marked int_const_expr
When we get an expression from parser, we know that having int_const_expr on
it is almost equivalent to it's an integer constant expression. Indeed,
the only checks we still have not done are that all casts present in there
are to integer types, that expression is correctly typed and that all indices
in offsetof are integer constant expressions. That belongs to
evaluate_expression()
and is easily done there.
* evaluate_expression() removes int_const_expr from some nodes:
* EXPR_BINOP, EXPR_COMPARE, EXPR_LOGICAL, EXPR_CONDITIONAL,
EXPR_PREOP: if the node is marked int_const_expr and some
of its arguments are not marked that way once we have
done evaluate_expression() on them, unmark our node.
* EXPR_IMLICIT_CAST: inherit flags from argument.
* cannibalizing nodes in * and * simplifications: unmark
the result.
* EXPR_CAST: unmark if we are casting not to an integer type.
Unmark if argument is not marked with int_const_expr after
evaluate_expression() on it *and* our node is not marked
float_literal (i.e. (int)0.0 is fine with us).
* EXPR_BINOP created (or cannibalizing EXPR_OFFSETOF) by
evaluation of evaluate_offsetof() get int_const_expr
if both arguments (already typechecked) have int_const_expr.
* unmark node when we declare it mistyped.
That does it - after evaluate_expression() we keep int_const_expr only if
expression was a valid integer constant expression.
Remaining issue: VLA handling. Right now sparse doesn't deal with those in
any sane way, but once we start handling their sizeof, we'll need to check
that type is constant-sized before marking EXPR_SIZEOF int_const_expr.
Signed-off-by: Al Viro [EMAIL PROTECTED]
---
evaluate.c | 43
expand.c | 16 -
expression.c | 68 -
expression.h |9 ++-
parse.c | 10
5 files changed, 133 insertions(+), 13 deletions(-)
diff --git a/evaluate.c b/evaluate.c
index c702ac4..bcac1d2 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -311,6 +311,7 @@ static struct expression * cast_to(struct expression *old,
struct symbol *type)
}
expr = alloc_expression(old-pos, EXPR_IMPLIED_CAST);
+ expr-flags = old-flags;
expr-ctype = type;
expr-cast_type = type;
expr-cast_expression = old;
@@ -403,6 +404,7 @@ static struct symbol *bad_expr_type(struct expression *expr)
break;
}
+ expr-flags = 0;
return expr-ctype = bad_ctype;
}
@@ -880,6 +882,10 @@ static struct symbol *evaluate_logical(struct expression
*expr)
return NULL;
expr-ctype = bool_ctype;
+ if (expr-flags) {
+ if (!(expr-left-flags expr-right-flags Int_const_expr))
+ expr-flags = 0;
+ }
return bool_ctype;
}
@@ -890,6 +896,11 @@ static struct symbol *evaluate_binop(struct expression
*expr)
int rclass = classify_type(expr-right-ctype, rtype);
int op = expr-op;
+ if (expr-flags) {
+ if (!(expr-left-flags expr-right-flags Int_const_expr))
+ expr-flags = 0;
+ }
+
/* number op number */
if (lclass rclass TYPE_NUM) {
if ((lclass | rclass) TYPE_FLOAT) {
@@ -968,6 +979,11 @@ static
Re: [PATCH 16/16] fix handling of integer constant expressions
On Tue, 2007-06-26 at 23:10 +0100, Al Viro wrote: On Sun, Jun 24, 2007 at 10:31:06PM -0700, Josh Triplett wrote: Al Viro wrote: Joy. OK, folks, disregard 16/16 in the current form; everything prior to it stands on its own. Acknowledged. The rest of the patches look good to me, so I'll merge 1-15 soon, and ignore 16. OK, here's the replacement. First the handling of __builtin_offsetof() (below in this mail), then integer constant expressions (in followup). They can be pulled from git://git.kernel.org/pub/scm/linux/kernel/git/viro/sparse.git/ integer-constant Both patches looked good to me in review, and neither changes the validation output. Merged. Thanks! - Josh Triplett - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote:- Hopefully correct handling of integer constant expressions. Please, review. Am I invoking sparse wrongly? ./sparse -W -Wall doesn't diagnose the following TU, for example. extern int a; extern int as1[(a = 2)]; Neil. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 08:32:26AM +0900, Neil Booth wrote:
Al Viro wrote:-
Hopefully correct handling of integer constant expressions. Please, review.
Am I invoking sparse wrongly? ./sparse -W -Wall doesn't diagnose
the following TU, for example.
extern int a;
extern int as1[(a = 2)];
sparse simply doesn't check that. We don't have anything resembling
support of VLA. Note that check for integer constant expression
has nothing to do with that;
int x[(int)(0.6 + 0.6)];
is valid (if stupid). And yes, footnote in 6.6 contradicts 6.7.5.2(1);
too bad...
We certainly need to do checks on array sizes; however, that part
(if it has static storage duration, it should not be a VLA) is minor.
And then there are gccisms:
size_t foo(int n)
{
struct {
int a[n];
char b;
} x;
return offsetof(typeof(x), b);
}
Yes, it's eaten up just fine. And yes, such structures are silently
accepted even with -pedantic -std=c99, which is a bug. Sigh...
We'll need to tackle VLAs at some point, but it certainly won't be fun ;-/
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, 27 Jun 2007, Al Viro wrote: extern int a; extern int as1[(a = 2)]; sparse simply doesn't check that. We don't have anything resembling support of VLA. Well, the above has two bugs that sparse could notice _independently_ of variable-sized arrays: - assignment outside of a function - variable size array that isn't an automatic variable (strictly speaking, that's not even a variable size - it's a constant 2, just with a non-constant expression - maybe you misread the = as an ==) Linus - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Tue, Jun 26, 2007 at 05:25:06PM -0700, Linus Torvalds wrote:
On Wed, 27 Jun 2007, Al Viro wrote:
extern int a;
extern int as1[(a = 2)];
sparse simply doesn't check that. We don't have anything resembling
support of VLA.
Well, the above has two bugs that sparse could notice _independently_ of
variable-sized arrays:
- assignment outside of a function
- variable size array that isn't an automatic variable
Right; what I'm saying is that we don't do any checks on array sizes at
all, mostly since nobody is brave enough to deal with VLAs (which we'll
have to do if we start doing that).
(strictly speaking, that's not even a variable size - it's a constant 2,
just with a non-constant expression - maybe you misread the = as an
==)
With == it would be a different bug ;-)
BTW, VLA can be not just auto variable - it can be used in derivation of
such (i.e. you can say int (*p)[n], just not for anything not in block
or prototype scope). And $DEITY help us[1] when ({...}) comes into the
game, since it allows leaking types out of the scope they'd been
declared in...
[1] or gcc - it gets an ICE galore in that class of testcases
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Wed, Jun 27, 2007 at 01:29:59AM +0100, Derek M Jones wrote: Al Viro wrote: Hopefully correct handling of integer constant expressions. Please, review. Am I invoking sparse wrongly? ./sparse -W -Wall doesn't diagnose the following TU, for example. extern int a; extern int as1[(a = 2)]; sparse simply doesn't check that. We don't have anything resembling support of VLA. If it did support VLAs it would point out that this is a constraint violation. VLAs must have block or function prototype scope. I know. It's just that let's do something about array size checks triggers yeah, but the poor sod who does that will have to sort VLAs *and* gcc extensions around VLAs out, which is not a nice thought and so far nobody had touched that area at all. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote: Hopefully correct handling of integer constant expressions. Please, review. Am I invoking sparse wrongly? ./sparse -W -Wall doesn't diagnose the following TU, for example. extern int a; extern int as1[(a = 2)]; sparse simply doesn't check that. We don't have anything resembling support of VLA. If it did support VLAs it would point out that this is a constraint violation. VLAs must have block or function prototype scope. -- Derek M. Jones tel: +44 (0) 1252 520 667 Knowledge Software Ltd mailto:[EMAIL PROTECTED] Applications Standards Conformance Testinghttp://www.knosof.co.uk - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote: > On Sun, Jun 24, 2007 at 10:31:06PM -0700, Josh Triplett wrote: >> Al Viro wrote: >>> Joy. OK, folks, disregard 16/16 in the current form; everything prior >>> to it stands on its own. >> Acknowledged. The rest of the patches look good to me, so I'll merge 1-15 >> soon, and ignore 16. >> >> Do you have those patches in public git somewhere, or would you like me to >> just apply the patches from mail? > > git://git.kernel.org/pub/scm/linux/kernel/git/viro/sparse.git/ misc No changes to validation output at any point in the series. Merged. Thanks! - Josh Triplett signature.asc Description: OpenPGP digital signature
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, Jun 24, 2007 at 10:31:06PM -0700, Josh Triplett wrote: > Al Viro wrote: > > Joy. OK, folks, disregard 16/16 in the current form; everything prior > > to it stands on its own. > > Acknowledged. The rest of the patches look good to me, so I'll merge 1-15 > soon, and ignore 16. > > Do you have those patches in public git somewhere, or would you like me to > just apply the patches from mail? git://git.kernel.org/pub/scm/linux/kernel/git/viro/sparse.git/ misc (or directly from hera) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
don't forget -> if you're going to accept extra stuff. GCC forgot -> with the parser rewrite, yes I filed a PR. -> is not allowed within the second arg to __builtin_offsetof(). Or do you mean something else? What's the PR #, and did it ever get fixed? Segher - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Humm... Right, so __builtin_offsetof() needs special treatment too. Oh, bugger. Is offsetof(struct foo, a.x[n]) a documented extension? It is. See info gcc -> C Extensions -> Offsetof Segher - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Humm... Right, so __builtin_offsetof() needs special treatment too. Oh, bugger. Is offsetof(struct foo, a.x[n]) a documented extension? It is. See info gcc - C Extensions - Offsetof Segher - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
don't forget - if you're going to accept extra stuff. GCC forgot - with the parser rewrite, yes I filed a PR. - is not allowed within the second arg to __builtin_offsetof(). Or do you mean something else? What's the PR #, and did it ever get fixed? Segher - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, Jun 24, 2007 at 10:31:06PM -0700, Josh Triplett wrote: Al Viro wrote: Joy. OK, folks, disregard 16/16 in the current form; everything prior to it stands on its own. Acknowledged. The rest of the patches look good to me, so I'll merge 1-15 soon, and ignore 16. Do you have those patches in public git somewhere, or would you like me to just apply the patches from mail? git://git.kernel.org/pub/scm/linux/kernel/git/viro/sparse.git/ misc (or directly from hera) - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote: On Sun, Jun 24, 2007 at 10:31:06PM -0700, Josh Triplett wrote: Al Viro wrote: Joy. OK, folks, disregard 16/16 in the current form; everything prior to it stands on its own. Acknowledged. The rest of the patches look good to me, so I'll merge 1-15 soon, and ignore 16. Do you have those patches in public git somewhere, or would you like me to just apply the patches from mail? git://git.kernel.org/pub/scm/linux/kernel/git/viro/sparse.git/ misc No changes to validation output at any point in the series. Merged. Thanks! - Josh Triplett signature.asc Description: OpenPGP digital signature
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote: > Joy. OK, folks, disregard 16/16 in the current form; everything prior > to it stands on its own. Acknowledged. The rest of the patches look good to me, so I'll merge 1-15 soon, and ignore 16. Do you have those patches in public git somewhere, or would you like me to just apply the patches from mail? - Josh Triplett signature.asc Description: OpenPGP digital signature
Re: [PATCH 16/16] fix handling of integer constant expressions
On Mon, Jun 25, 2007 at 06:42:53AM +0900, Neil Booth wrote:
> > such a big deal... Parser would need to accept
> > ident ( \[ expr \] | . ident )*
>
> don't forget -> if you're going to accept extra stuff. GCC forgot ->
> with the parser rewrite, yes I filed a PR.
In offsetof() second argument??? Ah, hell... You mean the situations
like offsetof(struct foo, x->a) in
struct foo {
struct {
int a;
} x[10];
};
OK...
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote:- > > See gcc.gnu.org/PR456 for more discussion. Yes it's an old > > bug... > > Humm... Right, so __builtin_offsetof() needs special treatment too. > Oh, bugger. Is > offsetof(struct foo, a.x[n]) > a documented extension? I _know_ that it's not promised by 7.17, > but gcc eats it (and obviously that sucker requires extra treatment > in that case). I asked on comp.std.c about this; the feeling was that only identifiers are intended to be permitted as the 2nd argument to offsetof. If true the standard has a very obscure way of stating something that could be said much more simply. > Parsing __builtin_offsetof() arguments is going to be fun ;-/ Right > now sparse has it as a predefined macro, but if we want to do that > kind of analysis, we need to really parse it. OTOH, that's not > such a big deal... Parser would need to accept > ident ( \[ expr \] | . ident )* don't forget -> if you're going to accept extra stuff. GCC forgot -> with the parser rewrite, yes I filed a PR. Neil. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, Jun 24, 2007 at 09:40:06PM +0200, Segher Boessenkool wrote: > >>Why? I'd say it's not better than BUILD_BUG_ON_ZERO() use > >>instead of that ?: > > > >Oh, _that_ part I have no problem with. It's more that it seems that > >the > >gcc optimization is ok at least as an extension. > > Sure, but it's not an extension (yet), but an implementation > side-effect; it would have to be (semi-formally) defined in > the manual to be an extension. Until that happens, anyone > using this "feature" risks haven his code broken at any time > (or, rather, his code already was broken but he didn't know > it). > > See gcc.gnu.org/PR456 for more discussion. Yes it's an old > bug... Humm... Right, so __builtin_offsetof() needs special treatment too. Oh, bugger. Is offsetof(struct foo, a.x[n]) a documented extension? I _know_ that it's not promised by 7.17, but gcc eats it (and obviously that sucker requires extra treatment in that case). Parsing __builtin_offsetof() arguments is going to be fun ;-/ Right now sparse has it as a predefined macro, but if we want to do that kind of analysis, we need to really parse it. OTOH, that's not such a big deal... Parser would need to accept ident ( \[ expr \] | . ident )* there, typecheck would walk down, check types and find offsets of struct members on the way down and build expression on the way back (e.g. in form of constant + sum of stuff from [...]), doing evaluate_expression() on each index and slapping Int_const_expr on created nodes accordingly. In the end, slap the Int_const_expr on resulting expression in obvious way. expand would work as usual, no interesting nodes surviving by that point... OK, that's doable and probably should be split into implementation of __builtin_offsetof() (sans Int_const_expr logics) and Int_const_expr parts merged into the patch that does all handling of integer constant expressions. Joy. OK, folks, disregard 16/16 in the current form; everything prior to it stands on its own. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, Jun 24, 2007 at 12:04:44PM -0700, Linus Torvalds wrote: > > > On Sun, 24 Jun 2007, Al Viro wrote: > > > > Why? I'd say it's not better than BUILD_BUG_ON_ZERO() use > > instead of that ?: > > Oh, _that_ part I have no problem with. It's more that it seems that the > gcc optimization is ok at least as an extension. gcc logs: * expressions of form <> can be reduced to cheaper form by <>. Tested and merged. gcc logs a year later: * revert commit <...>, it causes subtle problems (see PR<>, <> and <>). Proposed replacement is too intrusive for stable branch. gcc logs a month later: * tested and merged the real fix for PR<>; will go into the next release. foobar logs a year later: * gcc versions between <...> and <...> refuse to compile baz.c, complain about non-constant index in initializer. Waded through the sewers of macros we have in barf.h and blah.h, found what had been causing that. Fixed. Ain't fun... - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Why? I'd say it's not better than BUILD_BUG_ON_ZERO() use instead of that ?: Oh, _that_ part I have no problem with. It's more that it seems that the gcc optimization is ok at least as an extension. Sure, but it's not an extension (yet), but an implementation side-effect; it would have to be (semi-formally) defined in the manual to be an extension. Until that happens, anyone using this "feature" risks haven his code broken at any time (or, rather, his code already was broken but he didn't know it). See gcc.gnu.org/PR456 for more discussion. Yes it's an old bug... Segher - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, Jun 24, 2007 at 08:07:10PM +0200, Arnd Bergmann wrote: > One minor issue though: > While BUILD_BUG_ON and a few other macros in linux/kernel.h are currently > exported to user space, I would think that they should really be hidden > in #ifdef __KERNEL__, which means that we also need something like > > #ifdef __KERNEL__ > #define _IOC_TYPECHECK(t) \ > (sizeof(t) + BUILD_BUG_ON_ZERO(sizeof(t) == sizeof(t[1]) && \ > sizeof(t) < (1 << _IOC_SIZEBITS))) > #else > #define _IOC_TYPECHECK(t) sizeof(t) > #endif Point, but then we might want to export the damn thing in properly named version (i.e. put it into __ reserved namespace). It doesn't depend on anything kernel-specific... FWIW, the current version suffers from similar problem - it's outside of __KERNEL__ in userland-exported header and it can lead to breakage when included in build by something other than gcc... - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
If I understand correctly what bugs you are talking about,
most (all?) of those were solved in the dark ages already
(i.e., the 3.x series).
Alas, no. gcc is amazingly (and inconsistently) sloppy about the
things it accepts as integer constant expressions.
Ah yes, now I see what you were talking about. Most of this
is well-known, but feel free to file more PRs :-)
It certainly is not a valid C
Why not? Nothing in the C standard says all your externs
have to be defined in some other translation unit you link
with AFAIK.
It's not about externs. It's about things like
unsigned n;
int a[] = {[n - n + n - n] = 1};
And yes, gcc does eat that.
Yeah.
With -pedantic -std=c99, at that.
However,
unsigned n;
int a[] = {[n + n - n - n] = 1};
gets you error: nonconstant array index in initializer
And that's 4.1, not 3.x...
Why are you using such an ancient compiler? :-)
(Not that it is fixed in the current release though).
Segher
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, 24 Jun 2007, Al Viro wrote: > > Why? I'd say it's not better than BUILD_BUG_ON_ZERO() use > instead of that ?: Oh, _that_ part I have no problem with. It's more that it seems that the gcc optimization is ok at least as an extension. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, Jun 24, 2007 at 08:18:52PM +0200, Segher Boessenkool wrote:
> >#define _IOC_TYPECHECK(t) \
> >((sizeof(t) == sizeof(t[1]) && \
> > sizeof(t) < (1 << _IOC_SIZEBITS)) ? \
> > sizeof(t) : __invalid_size_argument_for_IOC)
> >poisoning _IOW() et.al., so those who do something like
> >
> >static const char *v4l1_ioctls[] = {
> >[_IOC_NR(VIDIOCGCAP)] = "VIDIOCGCAP",
> >
> >run into trouble.
>
> >The only reason that doesn't break gcc to hell and back is
> >that gcc has unfixed bugs in that area.
>
> If I understand correctly what bugs you are talking about,
> most (all?) of those were solved in the dark ages already
> (i.e., the 3.x series).
Alas, no. gcc is amazingly (and inconsistently) sloppy about the
things it accepts as integer constant expressions.
> >It certainly is not a valid C
>
> Why not? Nothing in the C standard says all your externs
> have to be defined in some other translation unit you link
> with AFAIK.
It's not about externs. It's about things like
unsigned n;
int a[] = {[n - n + n - n] = 1};
And yes, gcc does eat that. With -pedantic -std=c99, at that.
However,
unsigned n;
int a[] = {[n + n - n - n] = 1};
gets you error: nonconstant array index in initializer
And that's 4.1, not 3.x...
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, Jun 24, 2007 at 11:04:57AM -0700, Linus Torvalds wrote:
>
>
> On Sun, 24 Jun 2007, Al Viro wrote:
> >
> > Heh... The first catches are lovely:
> > struct fxsrAlignAssert {
> > int _:!(offsetof(struct task_struct,
> > thread.i387.fxsave) & 15);
>
> Ok, that's a bit odd.
>
> > as an idiotic way to do BUILD_BUG() and
> > #define _IOC_TYPECHECK(t) \
> > ((sizeof(t) == sizeof(t[1]) && \
> > sizeof(t) < (1 << _IOC_SIZEBITS)) ? \
> > sizeof(t) : __invalid_size_argument_for_IOC)
> > poisoning _IOW() et.al., so those who do something like
> >
> > static const char *v4l1_ioctls[] = {
> > [_IOC_NR(VIDIOCGCAP)] = "VIDIOCGCAP",
>
> On the other hand, this one really does seem to be "nice".
Why? I'd say it's not better than BUILD_BUG_ON_ZERO() use
instead of that ?:. This code would remain unchanged; the
entire reason why we run into problems is that a way to
force an error at build time had produced something that
was not a constant expression. It's not a matter of having
to change something in the users of that sucker (or _IOC_NR(), or
_IOR(), or _IO()). The code in question is there for one thing -
to give (constant) sizeof(t) or an error if t doesn't look good for
us. That's all. And we have a perfectly good way to do that...
> I don't think it's a misfeature to be able to do "obvious compile-time
> constant optimizations" on initializer indexes. The bitfield size thing in
> some ways does do the same thing - it's clearly _odd_, but if I had my
> choice, I'd prefer a language that allows it over one that doesn't.
Er... That's nice, assuming you don't suddenly run into "code with
convoluted bunch of macros breaks on a different version of compiler
and/or different CFLAGS".
IOW, having the optimizer strength define the boundary between the programs
that compile and ones that give an error is not a good idea, IMO.
Where do you place that boundary? Is 0 && n good? How about 0 & n?
Or 0 * n? Or n - n? Or (n+1)-(n+1) from macro expansion? Note that
gcc does _not_ take the last one as integer constant expression, but
happens to deal with the earlier ones. OTOH, it does deal with n * m - n * m.
And I would not bet a dime on gcc versions being consistent with each other
in that area.
Now, there is one possible answer that makes some sense - allow removal
of unevaluated parts in &&, || and ?:. I can do that, but I'm not
sure if it's actually worth doing. The only case in the kernel tree
become more readable with use of BUILD_BUG_ON_ZERO() anyway and I would
be very surprised to find any real code where something of that kind
would be a problem.
> > Objections? The only reason that doesn't break gcc to hell and back is
> > that gcc has unfixed bugs in that area. It certainly is not a valid C
> > or even a remotely sane one.
>
> I agree that it's obviously not "valid C", but I don't agree that it's not
> remotely sane. Why not allow that extension?
Because it's not well-defined and because having "let me check if this
version of compiler will eat that" as the only way to tell if construct
would be OK is not a good thing...
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Hopefully correct handling of integer constant expressions. Please,
review.
Heh... The first catches are lovely:
struct fxsrAlignAssert {
int _:!(offsetof(struct task_struct,
thread.i387.fxsave) & 15);
};
That's... wow?
#define _IOC_TYPECHECK(t) \
((sizeof(t) == sizeof(t[1]) && \
sizeof(t) < (1 << _IOC_SIZEBITS)) ? \
sizeof(t) : __invalid_size_argument_for_IOC)
poisoning _IOW() et.al., so those who do something like
static const char *v4l1_ioctls[] = {
[_IOC_NR(VIDIOCGCAP)] = "VIDIOCGCAP",
run into trouble.
The only reason that doesn't break gcc to hell and back is
that gcc has unfixed bugs in that area.
If I understand correctly what bugs you are talking about,
most (all?) of those were solved in the dark ages already
(i.e., the 3.x series).
It certainly is not a valid C
Why not? Nothing in the C standard says all your externs
have to be defined in some other translation unit you link
with AFAIK.
or even a remotely sane one.
It's unusual at least :-)
Segher
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sunday 24 June 2007, Al Viro wrote: > but the latter... Probably ought to be > #define _IOC_TYPECHECK(t) \ > (sizeof(t) + BUILD_BUG_ON_ZERO(sizeof(t) == sizeof(t[1]) && \ > sizeof(t) < (1 << _IOC_SIZEBITS))) > > Objections? The only reason that doesn't break gcc to hell and back is > that gcc has unfixed bugs in that area. It certainly is not a valid C > or even a remotely sane one. Yes, looks good. I originally came up with _IOC_TYPECHECK before we had the generic BUILD_BUG_ON(). One minor issue though: While BUILD_BUG_ON and a few other macros in linux/kernel.h are currently exported to user space, I would think that they should really be hidden in #ifdef __KERNEL__, which means that we also need something like #ifdef __KERNEL__ #define _IOC_TYPECHECK(t) \ (sizeof(t) + BUILD_BUG_ON_ZERO(sizeof(t) == sizeof(t[1]) && \ sizeof(t) < (1 << _IOC_SIZEBITS))) #else #define _IOC_TYPECHECK(t) sizeof(t) #endif Arnd <>< - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, 24 Jun 2007, Al Viro wrote:
>
> Heh... The first catches are lovely:
> struct fxsrAlignAssert {
> int _:!(offsetof(struct task_struct,
> thread.i387.fxsave) & 15);
Ok, that's a bit odd.
> as an idiotic way to do BUILD_BUG() and
> #define _IOC_TYPECHECK(t) \
> ((sizeof(t) == sizeof(t[1]) && \
> sizeof(t) < (1 << _IOC_SIZEBITS)) ? \
> sizeof(t) : __invalid_size_argument_for_IOC)
> poisoning _IOW() et.al., so those who do something like
>
> static const char *v4l1_ioctls[] = {
> [_IOC_NR(VIDIOCGCAP)] = "VIDIOCGCAP",
On the other hand, this one really does seem to be "nice".
I don't think it's a misfeature to be able to do "obvious compile-time
constant optimizations" on initializer indexes. The bitfield size thing in
some ways does do the same thing - it's clearly _odd_, but if I had my
choice, I'd prefer a language that allows it over one that doesn't.
> Objections? The only reason that doesn't break gcc to hell and back is
> that gcc has unfixed bugs in that area. It certainly is not a valid C
> or even a remotely sane one.
I agree that it's obviously not "valid C", but I don't agree that it's not
remotely sane. Why not allow that extension?
Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, Jun 24, 2007 at 09:05:51AM +0100, Al Viro wrote:
>
> Hopefully correct handling of integer constant expressions. Please, review.
Heh... The first catches are lovely:
struct fxsrAlignAssert {
int _:!(offsetof(struct task_struct,
thread.i387.fxsave) & 15);
};
as an idiotic way to do BUILD_BUG() and
#define _IOC_TYPECHECK(t) \
((sizeof(t) == sizeof(t[1]) && \
sizeof(t) < (1 << _IOC_SIZEBITS)) ? \
sizeof(t) : __invalid_size_argument_for_IOC)
poisoning _IOW() et.al., so those who do something like
static const char *v4l1_ioctls[] = {
[_IOC_NR(VIDIOCGCAP)] = "VIDIOCGCAP",
run into trouble. The former is "tell jbeulich to cut down on crack",
but the latter... Probably ought to be
#define _IOC_TYPECHECK(t) \
(sizeof(t) + BUILD_BUG_ON_ZERO(sizeof(t) == sizeof(t[1]) && \
sizeof(t) < (1 << _IOC_SIZEBITS)))
Objections? The only reason that doesn't break gcc to hell and back is
that gcc has unfixed bugs in that area. It certainly is not a valid C
or even a remotely sane one.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, Jun 24, 2007 at 09:05:51AM +0100, Al Viro wrote:
Hopefully correct handling of integer constant expressions. Please, review.
Heh... The first catches are lovely:
struct fxsrAlignAssert {
int _:!(offsetof(struct task_struct,
thread.i387.fxsave) 15);
};
as an idiotic way to do BUILD_BUG() and
#define _IOC_TYPECHECK(t) \
((sizeof(t) == sizeof(t[1]) \
sizeof(t) (1 _IOC_SIZEBITS)) ? \
sizeof(t) : __invalid_size_argument_for_IOC)
poisoning _IOW() et.al., so those who do something like
static const char *v4l1_ioctls[] = {
[_IOC_NR(VIDIOCGCAP)] = VIDIOCGCAP,
run into trouble. The former is tell jbeulich to cut down on crack,
but the latter... Probably ought to be
#define _IOC_TYPECHECK(t) \
(sizeof(t) + BUILD_BUG_ON_ZERO(sizeof(t) == sizeof(t[1]) \
sizeof(t) (1 _IOC_SIZEBITS)))
Objections? The only reason that doesn't break gcc to hell and back is
that gcc has unfixed bugs in that area. It certainly is not a valid C
or even a remotely sane one.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, 24 Jun 2007, Al Viro wrote:
Heh... The first catches are lovely:
struct fxsrAlignAssert {
int _:!(offsetof(struct task_struct,
thread.i387.fxsave) 15);
Ok, that's a bit odd.
as an idiotic way to do BUILD_BUG() and
#define _IOC_TYPECHECK(t) \
((sizeof(t) == sizeof(t[1]) \
sizeof(t) (1 _IOC_SIZEBITS)) ? \
sizeof(t) : __invalid_size_argument_for_IOC)
poisoning _IOW() et.al., so those who do something like
static const char *v4l1_ioctls[] = {
[_IOC_NR(VIDIOCGCAP)] = VIDIOCGCAP,
On the other hand, this one really does seem to be nice.
I don't think it's a misfeature to be able to do obvious compile-time
constant optimizations on initializer indexes. The bitfield size thing in
some ways does do the same thing - it's clearly _odd_, but if I had my
choice, I'd prefer a language that allows it over one that doesn't.
Objections? The only reason that doesn't break gcc to hell and back is
that gcc has unfixed bugs in that area. It certainly is not a valid C
or even a remotely sane one.
I agree that it's obviously not valid C, but I don't agree that it's not
remotely sane. Why not allow that extension?
Linus
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sunday 24 June 2007, Al Viro wrote: but the latter... Probably ought to be #define _IOC_TYPECHECK(t) \ (sizeof(t) + BUILD_BUG_ON_ZERO(sizeof(t) == sizeof(t[1]) \ sizeof(t) (1 _IOC_SIZEBITS))) Objections? The only reason that doesn't break gcc to hell and back is that gcc has unfixed bugs in that area. It certainly is not a valid C or even a remotely sane one. Yes, looks good. I originally came up with _IOC_TYPECHECK before we had the generic BUILD_BUG_ON(). One minor issue though: While BUILD_BUG_ON and a few other macros in linux/kernel.h are currently exported to user space, I would think that they should really be hidden in #ifdef __KERNEL__, which means that we also need something like #ifdef __KERNEL__ #define _IOC_TYPECHECK(t) \ (sizeof(t) + BUILD_BUG_ON_ZERO(sizeof(t) == sizeof(t[1]) \ sizeof(t) (1 _IOC_SIZEBITS))) #else #define _IOC_TYPECHECK(t) sizeof(t) #endif Arnd - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Hopefully correct handling of integer constant expressions. Please,
review.
Heh... The first catches are lovely:
struct fxsrAlignAssert {
int _:!(offsetof(struct task_struct,
thread.i387.fxsave) 15);
};
That's... wow?
#define _IOC_TYPECHECK(t) \
((sizeof(t) == sizeof(t[1]) \
sizeof(t) (1 _IOC_SIZEBITS)) ? \
sizeof(t) : __invalid_size_argument_for_IOC)
poisoning _IOW() et.al., so those who do something like
static const char *v4l1_ioctls[] = {
[_IOC_NR(VIDIOCGCAP)] = VIDIOCGCAP,
run into trouble.
The only reason that doesn't break gcc to hell and back is
that gcc has unfixed bugs in that area.
If I understand correctly what bugs you are talking about,
most (all?) of those were solved in the dark ages already
(i.e., the 3.x series).
It certainly is not a valid C
Why not? Nothing in the C standard says all your externs
have to be defined in some other translation unit you link
with AFAIK.
or even a remotely sane one.
It's unusual at least :-)
Segher
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, Jun 24, 2007 at 11:04:57AM -0700, Linus Torvalds wrote:
On Sun, 24 Jun 2007, Al Viro wrote:
Heh... The first catches are lovely:
struct fxsrAlignAssert {
int _:!(offsetof(struct task_struct,
thread.i387.fxsave) 15);
Ok, that's a bit odd.
as an idiotic way to do BUILD_BUG() and
#define _IOC_TYPECHECK(t) \
((sizeof(t) == sizeof(t[1]) \
sizeof(t) (1 _IOC_SIZEBITS)) ? \
sizeof(t) : __invalid_size_argument_for_IOC)
poisoning _IOW() et.al., so those who do something like
static const char *v4l1_ioctls[] = {
[_IOC_NR(VIDIOCGCAP)] = VIDIOCGCAP,
On the other hand, this one really does seem to be nice.
Why? I'd say it's not better than BUILD_BUG_ON_ZERO() use
instead of that ?:. This code would remain unchanged; the
entire reason why we run into problems is that a way to
force an error at build time had produced something that
was not a constant expression. It's not a matter of having
to change something in the users of that sucker (or _IOC_NR(), or
_IOR(), or _IO()). The code in question is there for one thing -
to give (constant) sizeof(t) or an error if t doesn't look good for
us. That's all. And we have a perfectly good way to do that...
I don't think it's a misfeature to be able to do obvious compile-time
constant optimizations on initializer indexes. The bitfield size thing in
some ways does do the same thing - it's clearly _odd_, but if I had my
choice, I'd prefer a language that allows it over one that doesn't.
Er... That's nice, assuming you don't suddenly run into code with
convoluted bunch of macros breaks on a different version of compiler
and/or different CFLAGS.
IOW, having the optimizer strength define the boundary between the programs
that compile and ones that give an error is not a good idea, IMO.
Where do you place that boundary? Is 0 n good? How about 0 n?
Or 0 * n? Or n - n? Or (n+1)-(n+1) from macro expansion? Note that
gcc does _not_ take the last one as integer constant expression, but
happens to deal with the earlier ones. OTOH, it does deal with n * m - n * m.
And I would not bet a dime on gcc versions being consistent with each other
in that area.
Now, there is one possible answer that makes some sense - allow removal
of unevaluated parts in , || and ?:. I can do that, but I'm not
sure if it's actually worth doing. The only case in the kernel tree
become more readable with use of BUILD_BUG_ON_ZERO() anyway and I would
be very surprised to find any real code where something of that kind
would be a problem.
Objections? The only reason that doesn't break gcc to hell and back is
that gcc has unfixed bugs in that area. It certainly is not a valid C
or even a remotely sane one.
I agree that it's obviously not valid C, but I don't agree that it's not
remotely sane. Why not allow that extension?
Because it's not well-defined and because having let me check if this
version of compiler will eat that as the only way to tell if construct
would be OK is not a good thing...
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, Jun 24, 2007 at 08:18:52PM +0200, Segher Boessenkool wrote:
#define _IOC_TYPECHECK(t) \
((sizeof(t) == sizeof(t[1]) \
sizeof(t) (1 _IOC_SIZEBITS)) ? \
sizeof(t) : __invalid_size_argument_for_IOC)
poisoning _IOW() et.al., so those who do something like
static const char *v4l1_ioctls[] = {
[_IOC_NR(VIDIOCGCAP)] = VIDIOCGCAP,
run into trouble.
The only reason that doesn't break gcc to hell and back is
that gcc has unfixed bugs in that area.
If I understand correctly what bugs you are talking about,
most (all?) of those were solved in the dark ages already
(i.e., the 3.x series).
Alas, no. gcc is amazingly (and inconsistently) sloppy about the
things it accepts as integer constant expressions.
It certainly is not a valid C
Why not? Nothing in the C standard says all your externs
have to be defined in some other translation unit you link
with AFAIK.
It's not about externs. It's about things like
unsigned n;
int a[] = {[n - n + n - n] = 1};
And yes, gcc does eat that. With -pedantic -std=c99, at that.
However,
unsigned n;
int a[] = {[n + n - n - n] = 1};
gets you error: nonconstant array index in initializer
And that's 4.1, not 3.x...
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
If I understand correctly what bugs you are talking about,
most (all?) of those were solved in the dark ages already
(i.e., the 3.x series).
Alas, no. gcc is amazingly (and inconsistently) sloppy about the
things it accepts as integer constant expressions.
Ah yes, now I see what you were talking about. Most of this
is well-known, but feel free to file more PRs :-)
It certainly is not a valid C
Why not? Nothing in the C standard says all your externs
have to be defined in some other translation unit you link
with AFAIK.
It's not about externs. It's about things like
unsigned n;
int a[] = {[n - n + n - n] = 1};
And yes, gcc does eat that.
Yeah.
With -pedantic -std=c99, at that.
However,
unsigned n;
int a[] = {[n + n - n - n] = 1};
gets you error: nonconstant array index in initializer
And that's 4.1, not 3.x...
Why are you using such an ancient compiler? :-)
(Not that it is fixed in the current release though).
Segher
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, Jun 24, 2007 at 08:07:10PM +0200, Arnd Bergmann wrote: One minor issue though: While BUILD_BUG_ON and a few other macros in linux/kernel.h are currently exported to user space, I would think that they should really be hidden in #ifdef __KERNEL__, which means that we also need something like #ifdef __KERNEL__ #define _IOC_TYPECHECK(t) \ (sizeof(t) + BUILD_BUG_ON_ZERO(sizeof(t) == sizeof(t[1]) \ sizeof(t) (1 _IOC_SIZEBITS))) #else #define _IOC_TYPECHECK(t) sizeof(t) #endif Point, but then we might want to export the damn thing in properly named version (i.e. put it into __ reserved namespace). It doesn't depend on anything kernel-specific... FWIW, the current version suffers from similar problem - it's outside of __KERNEL__ in userland-exported header and it can lead to breakage when included in build by something other than gcc... - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, 24 Jun 2007, Al Viro wrote: Why? I'd say it's not better than BUILD_BUG_ON_ZERO() use instead of that ?: Oh, _that_ part I have no problem with. It's more that it seems that the gcc optimization is ok at least as an extension. Linus - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Why? I'd say it's not better than BUILD_BUG_ON_ZERO() use instead of that ?: Oh, _that_ part I have no problem with. It's more that it seems that the gcc optimization is ok at least as an extension. Sure, but it's not an extension (yet), but an implementation side-effect; it would have to be (semi-formally) defined in the manual to be an extension. Until that happens, anyone using this feature risks haven his code broken at any time (or, rather, his code already was broken but he didn't know it). See gcc.gnu.org/PR456 for more discussion. Yes it's an old bug... Segher - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, Jun 24, 2007 at 12:04:44PM -0700, Linus Torvalds wrote: On Sun, 24 Jun 2007, Al Viro wrote: Why? I'd say it's not better than BUILD_BUG_ON_ZERO() use instead of that ?: Oh, _that_ part I have no problem with. It's more that it seems that the gcc optimization is ok at least as an extension. gcc logs: * expressions of form can be reduced to cheaper form by . Tested and merged. gcc logs a year later: * revert commit ..., it causes subtle problems (see PR, and ). Proposed replacement is too intrusive for stable branch. gcc logs a month later: * tested and merged the real fix for PR; will go into the next release. foobar logs a year later: * gcc versions between ... and ... refuse to compile baz.c, complain about non-constant index in initializer. Waded through the sewers of macros we have in barf.h and blah.h, found what had been causing that. Fixed. Ain't fun... - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Sun, Jun 24, 2007 at 09:40:06PM +0200, Segher Boessenkool wrote: Why? I'd say it's not better than BUILD_BUG_ON_ZERO() use instead of that ?: Oh, _that_ part I have no problem with. It's more that it seems that the gcc optimization is ok at least as an extension. Sure, but it's not an extension (yet), but an implementation side-effect; it would have to be (semi-formally) defined in the manual to be an extension. Until that happens, anyone using this feature risks haven his code broken at any time (or, rather, his code already was broken but he didn't know it). See gcc.gnu.org/PR456 for more discussion. Yes it's an old bug... Humm... Right, so __builtin_offsetof() needs special treatment too. Oh, bugger. Is offsetof(struct foo, a.x[n]) a documented extension? I _know_ that it's not promised by 7.17, but gcc eats it (and obviously that sucker requires extra treatment in that case). Parsing __builtin_offsetof() arguments is going to be fun ;-/ Right now sparse has it as a predefined macro, but if we want to do that kind of analysis, we need to really parse it. OTOH, that's not such a big deal... Parser would need to accept ident ( \[ expr \] | . ident )* there, typecheck would walk down, check types and find offsets of struct members on the way down and build expression on the way back (e.g. in form of constant + sum of stuff from [...]), doing evaluate_expression() on each index and slapping Int_const_expr on created nodes accordingly. In the end, slap the Int_const_expr on resulting expression in obvious way. expand would work as usual, no interesting nodes surviving by that point... OK, that's doable and probably should be split into implementation of __builtin_offsetof() (sans Int_const_expr logics) and Int_const_expr parts merged into the patch that does all handling of integer constant expressions. Joy. OK, folks, disregard 16/16 in the current form; everything prior to it stands on its own. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote:- See gcc.gnu.org/PR456 for more discussion. Yes it's an old bug... Humm... Right, so __builtin_offsetof() needs special treatment too. Oh, bugger. Is offsetof(struct foo, a.x[n]) a documented extension? I _know_ that it's not promised by 7.17, but gcc eats it (and obviously that sucker requires extra treatment in that case). I asked on comp.std.c about this; the feeling was that only identifiers are intended to be permitted as the 2nd argument to offsetof. If true the standard has a very obscure way of stating something that could be said much more simply. Parsing __builtin_offsetof() arguments is going to be fun ;-/ Right now sparse has it as a predefined macro, but if we want to do that kind of analysis, we need to really parse it. OTOH, that's not such a big deal... Parser would need to accept ident ( \[ expr \] | . ident )* don't forget - if you're going to accept extra stuff. GCC forgot - with the parser rewrite, yes I filed a PR. Neil. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
On Mon, Jun 25, 2007 at 06:42:53AM +0900, Neil Booth wrote:
such a big deal... Parser would need to accept
ident ( \[ expr \] | . ident )*
don't forget - if you're going to accept extra stuff. GCC forgot -
with the parser rewrite, yes I filed a PR.
In offsetof() second argument??? Ah, hell... You mean the situations
like offsetof(struct foo, x-a) in
struct foo {
struct {
int a;
} x[10];
};
OK...
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] fix handling of integer constant expressions
Al Viro wrote: Joy. OK, folks, disregard 16/16 in the current form; everything prior to it stands on its own. Acknowledged. The rest of the patches look good to me, so I'll merge 1-15 soon, and ignore 16. Do you have those patches in public git somewhere, or would you like me to just apply the patches from mail? - Josh Triplett signature.asc Description: OpenPGP digital signature
