Re: [PATCH 16/16] fix handling of integer constant expressions

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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.

Re: [PATCH 16/16] fix handling of integer constant expressions

[Linus Torvalds]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Linus Torvalds]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Segher Boessenkool]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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.

Re: [PATCH 16/16] fix handling of integer constant expressions

[Neil Booth]

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.

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Segher Boessenkool]

> 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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Josh Triplett]

[-- Attachment #1: Type: text/plain, Size: 354 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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)

Re: [PATCH 16/16] fix handling of integer constant expressions

[Josh Triplett]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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 <viro@zeniv.linux.org.uk>
Date: Tue, 26 Jun 2007 16:06:32 -0400
Subject: [PATCH] implement __builtin_offsetof()

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 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:
 	case EXPR_IDENTIFIER:
diff --git a/expand.c b/expand.c
index a46f1c2..c6d188e 100644
--- a/expand.c
+++ b/expand.c
@@ -968,6 +968,7 @@ static int expand_expression(struct expression *expr)
 	case EXPR_SIZEOF:
 	case EXPR_PTRSIZEOF:
 	case EXPR_ALIGNOF:
+	case EXPR_OFFSETOF:
 		expression_error(expr, "internal front-end error: sizeof in expansion?");
 		return UNSAFE;
 	}
diff --git a/expression.c b/expression.c
index a40ab2b..0108224 100644
--- a/expression.c
+++ b/expression.c
@@ -152,6 +152,69 @@ static struct token *builtin_types_compatible_p_expr(struct token *token,
 	return token;
 }
 
+static struct token *builtin_offsetof_expr(struct token *token,
+					   struct expression **tree)
+{
+	struct expression *expr = NULL;
+	struct expression **p = &expr;
+	struct symbol *sym;
+	int op = '.';
+
+	token = token->next;
+	if (!match_op(token, '('))
+		return expect(token, '(', "after __builtin_offset");
+
+	token = token->next;
+	token = typename(token, &sym);
+	if (sym->ident)
+		sparse_error(token->pos,
+			     "type expression should not include identifier "
+			     "\"%s\"", sym->ident->name);
+
+	if (!match_op(token, ','))
+		return expect(token, ',', "in __builtin_offset");
+
+	while (1) {
+		struct expression *e;
+		switch (op) {
+		case ')':
+			expr->in = sym;
+			*tree = expr;
+		default:
+			return expect(token, ')', "at end of __builtin_offset");
+		case SPECIAL_DEREFERENCE:
+			e = alloc_expression(token->pos, EXPR_OFFSETOF);
+			e->op = '[';
+			*p = e;
+			p = &e->down;
+			/* fall through */
+		case '.':
+			token = token->next;
+			e = alloc_expression(token->pos, EXPR_OFFSETOF);
+			e->op = '.';
+			if (token_type(token) != TOKEN_IDENT) {
+				sparse_error(token->pos, "Expected member name");
+				return token;
+			}
+			e->ident = token->ident;
+			token = token->next;
+			break;
+		case '[':
+			token = token->next;
+			e = alloc_expression(token->pos, EXPR_OFFSETOF);
+			e->op = '[';
+			token = parse_expression(token, &e->index);
+			token = expect(token, ']',
+					"at end of array dereference");
+			if (!e->index)
+				return token;
+		}
+		*p = e;
+		p = &e->down;
+		op = token_type(token) == TOKEN_SPECIAL ? token->special : 0;
+	}
+}
+
 static struct token *string_expression(struct token *token, struct expression *expr)
 {
 	struct string *string = token->string;
@@ -359,6 +422,10 @@ struct token *primary_expression(struct token *token, struct expression **tree)
 				token = builtin_types_compatible_p_expr(token, &expr);
 				break;
 			}
+			if (token->ident == &__builtin_offsetof_ident) {
+				token = builtin_offsetof_expr(token, &expr);
+				break;
+			}
 		} else if (sym->enum_member) {
 			expr = alloc_expression(token->pos, EXPR_VALUE);
 			*expr = *sym->initializer;
diff --git a/expression.h b/expression.h
index 1bf02c4..19e0302 100644
--- a/expression.h
+++ b/expression.h
@@ -44,6 +44,7 @@ enum expression_type {
 	EXPR_POS,		// position in initializer
 	EXPR_FVALUE,
 	EXPR_SLICE,
+	EXPR_OFFSETOF,
 };
 
 struct expression {
@@ -127,6 +128,15 @@ struct expression {
 			unsigned int init_offset, init_nr;
 			struct expression *init_expr;
 		};
+		// EXPR_OFFSETOF
+		struct {
+			struct symbol *in;
+			struct expression *down;
+			union {
+				struct ident *ident;
+				struct expression *index;
+			};
+		};
 	};
 };
 
diff --git a/ident-list.h b/ident-list.h
index 0f654bf..b183eac 100644
--- a/ident-list.h
+++ b/ident-list.h
@@ -29,6 +29,7 @@ IDENT(asm); IDENT_RESERVED(__asm); IDENT_RESERVED(__asm__);
 IDENT(alignof); IDENT_RESERVED(__alignof); IDENT_RESERVED(__alignof__); 
 IDENT_RESERVED(__sizeof_ptr__);
 IDENT_RESERVED(__builtin_types_compatible_p);
+IDENT_RESERVED(__builtin_offsetof);
 
 /* Attribute names */
 IDENT(packed); IDENT(__packed__);
diff --git a/inline.c b/inline.c
index f80403c..061261a 100644
--- a/inline.c
+++ b/inline.c
@@ -237,7 +237,23 @@ static struct expression * copy_expression(struct expression *expr)
 		expr->init_expr = val;
 		break;
 	}
-
+	case EXPR_OFFSETOF: {
+		struct expression *val = copy_expression(expr->down);
+		if (expr->op == '.') {
+			if (expr->down != val) {
+				expr = dup_expression(expr);
+				expr->down = val;
+			}
+		} else {
+			struct expression *idx = copy_expression(expr->index);
+			if (expr->down != val || expr->index != idx) {
+				expr = dup_expression(expr);
+				expr->down = val;
+				expr->index = idx;
+			}
+		}
+		break;
+	}
 	default:
 		warning(expr->pos, "trying to copy expression type %d", expr->type);
 	}
diff --git a/lib.c b/lib.c
index 7f6334c..7fea474 100644
--- a/lib.c
+++ b/lib.c
@@ -609,7 +609,6 @@ void create_builtin_stream(void)
 	add_pre_buffer("#define __builtin_va_arg_incr(x) ((x) + 1)\n");
 	add_pre_buffer("#define __builtin_va_copy(dest, src) ({ dest = src; (void)0; })\n");
 	add_pre_buffer("#define __builtin_va_end(arg)\n");
-	add_pre_buffer("#define __builtin_offsetof(type, name) ((__SIZE_TYPE__)&((type *)(0ul))->name)\n");
 
 	/* FIXME! We need to do these as special magic macros at expansion time! */
 	add_pre_buffer("#define __BASE_FILE__ \"base_file.c\"\n");
diff --git a/show-parse.c b/show-parse.c
index 0bef01c..07ee763 100644
--- a/show-parse.c
+++ b/show-parse.c
@@ -1049,6 +1049,7 @@ int show_expression(struct expression *expr)
 	case EXPR_SIZEOF:
 	case EXPR_PTRSIZEOF:
 	case EXPR_ALIGNOF:
+	case EXPR_OFFSETOF:
 		warning(expr->pos, "invalid expression after evaluation");
 		return 0;
 	case EXPR_CAST:
-- 
1.5.0-rc2.GIT

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

> 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 <viro@zeniv.linux.org.uk>
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 <viro@zeniv.linux.org.uk>
---
 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 struct symbol *evaluate_compare(struct expression *expr)
 	struct symbol *ctype;
 	int lclass, rclass;
 
+	if (expr->flags) {
+		if (!(expr->left->flags & expr->right->flags & Int_const_expr))
+			expr->flags = 0;
+	}
+
 	/* Type types? */
 	if (is_type_type(ltype) && is_type_type(rtype))
 		goto OK;
@@ -1057,6 +1073,13 @@ static struct symbol *evaluate_conditional_expression(struct expression *expr)
 		true = &expr->cond_true;
 	}
 
+	if (expr->flags) {
+		int flags = expr->conditional->flags & Int_const_expr;
+		flags &= (*true)->flags & expr->cond_false->flags;
+		if (!flags)
+			expr->flags = 0;
+	}
+
 	lclass = classify_type(ltype, &ltype);
 	rclass = classify_type(rtype, &rtype);
 	if (lclass & rclass & TYPE_NUM) {
@@ -1436,6 +1459,7 @@ static struct symbol *evaluate_addressof(struct expression *expr)
 	}
 	ctype = op->ctype;
 	*expr = *op->unop;
+	expr->flags = 0;
 
 	if (expr->type == EXPR_SYMBOL) {
 		struct symbol *sym = expr->symbol;
@@ -1463,6 +1487,7 @@ static struct symbol *evaluate_dereference(struct expression *expr)
 	/* Simplify: *&(expr) => (expr) */
 	if (op->type == EXPR_PREOP && op->op == '&') {
 		*expr = *op->unop;
+		expr->flags = 0;
 		return expr->ctype;
 	}
 
@@ -1540,6 +1565,8 @@ static struct symbol *evaluate_postop(struct expression *expr)
 static struct symbol *evaluate_sign(struct expression *expr)
 {
 	struct symbol *ctype = expr->unop->ctype;
+	if (expr->flags && !(expr->unop->flags & Int_const_expr))
+		expr->flags = 0;
 	if (is_int_type(ctype)) {
 		struct symbol *rtype = rtype = integer_promotion(ctype);
 		expr->unop = cast_to(expr->unop, rtype);
@@ -1588,6 +1615,8 @@ static struct symbol *evaluate_preop(struct expression *expr)
 		return evaluate_postop(expr);
 
 	case '!':
+		if (expr->flags && !(expr->unop->flags & Int_const_expr))
+			expr->flags = 0;
 		if (is_safe_type(ctype))
 			warning(expr->pos, "testing a 'safe expression'");
 		if (is_float_type(ctype)) {
@@ -2477,6 +2506,15 @@ static struct symbol *evaluate_cast(struct expression *expr)
 	degenerate(target);
 
 	class1 = classify_type(ctype, &t1);
+
+	/* cast to non-integer type -> not an integer constant expression */
+	if (!is_int(class1))
+		expr->flags = 0;
+	/* if argument turns out to be not an integer constant expression *and*
+	   it was not a floating literal to start with -> too bad */
+	else if (expr->flags == Int_const_expr &&
+		!(target->flags & Int_const_expr))
+		expr->flags = 0;
 	/*
 	 * You can always throw a value away by casting to
 	 * "void" - that's an implicit "force". Note that
@@ -2635,6 +2673,7 @@ static struct symbol *evaluate_offsetof(struct expression *expr)
 		}
 		ctype = field;
 		expr->type = EXPR_VALUE;
+		expr->flags = Int_const_expr;
 		expr->value = offset;
 		expr->ctype = size_t_ctype;
 	} else {
@@ -2651,6 +2690,7 @@ static struct symbol *evaluate_offsetof(struct expression *expr)
 		ctype = ctype->ctype.base_type;
 		if (!expr->index) {
 			expr->type = EXPR_VALUE;
+			expr->flags = Int_const_expr;
 			expr->value = 0;
 			expr->ctype = size_t_ctype;
 		} else {
@@ -2666,11 +2706,13 @@ static struct symbol *evaluate_offsetof(struct expression *expr)
 			m = alloc_const_expression(expr->pos,
 						   ctype->bit_size >> 3);
 			m->ctype = size_t_ctype;
+			m->flags = Int_const_expr;
 			expr->type = EXPR_BINOP;
 			expr->left = idx;
 			expr->right = m;
 			expr->op = '*';
 			expr->ctype = size_t_ctype;
+			expr->flags = m->flags & idx->flags & Int_const_expr;
 		}
 	}
 	if (e) {
@@ -2681,6 +2723,7 @@ static struct symbol *evaluate_offsetof(struct expression *expr)
 		if (!evaluate_expression(e))
 			return NULL;
 		expr->type = EXPR_BINOP;
+		expr->flags = e->flags & copy->flags & Int_const_expr;
 		expr->op = '+';
 		expr->ctype = size_t_ctype;
 		expr->left = copy;
diff --git a/expand.c b/expand.c
index c6d188e..f945518 100644
--- a/expand.c
+++ b/expand.c
@@ -1144,7 +1144,7 @@ static int expand_statement(struct statement *stmt)
 	return SIDE_EFFECTS;
 }
 
-long long get_expression_value(struct expression *expr)
+static long long __get_expression_value(struct expression *expr, int strict)
 {
 	long long value, mask;
 	struct symbol *ctype;
@@ -1161,6 +1161,10 @@ long long get_expression_value(struct expression *expr)
 		expression_error(expr, "bad constant expression");
 		return 0;
 	}
+	if (strict && !(expr->flags & Int_const_expr)) {
+		expression_error(expr, "bad integer constant expression");
+		return 0;
+	}
 
 	value = expr->value;
 	mask = 1ULL << (ctype->bit_size-1);
@@ -1173,3 +1177,13 @@ long long get_expression_value(struct expression *expr)
 	}
 	return value;
 }
+
+long long get_expression_value(struct expression *expr)
+{
+	return __get_expression_value(expr, 0);
+}
+
+long long const_expression_value(struct expression *expr)
+{
+	return __get_expression_value(expr, 1);
+}
diff --git a/expression.c b/expression.c
index 0108224..d36c3d2 100644
--- a/expression.c
+++ b/expression.c
@@ -117,6 +117,7 @@ static struct token *parse_type(struct token *token, struct expression **tree)
 {
 	struct symbol *sym;
 	*tree = alloc_expression(token->pos, EXPR_TYPE);
+	(*tree)->flags = Int_const_expr; /* sic */
 	token = typename(token, &sym);
 	if (sym->ident)
 		sparse_error(token->pos,
@@ -131,6 +132,7 @@ static struct token *builtin_types_compatible_p_expr(struct token *token,
 {
 	struct expression *expr = alloc_expression(
 		token->pos, EXPR_COMPARE);
+	expr->flags = Int_const_expr;
 	expr->op = SPECIAL_EQUAL;
 	token = token->next;
 	if (!match_op(token, '('))
@@ -184,6 +186,7 @@ static struct token *builtin_offsetof_expr(struct token *token,
 			return expect(token, ')', "at end of __builtin_offset");
 		case SPECIAL_DEREFERENCE:
 			e = alloc_expression(token->pos, EXPR_OFFSETOF);
+			e->flags = Int_const_expr;
 			e->op = '[';
 			*p = e;
 			p = &e->down;
@@ -191,6 +194,7 @@ static struct token *builtin_offsetof_expr(struct token *token,
 		case '.':
 			token = token->next;
 			e = alloc_expression(token->pos, EXPR_OFFSETOF);
+			e->flags = Int_const_expr;
 			e->op = '.';
 			if (token_type(token) != TOKEN_IDENT) {
 				sparse_error(token->pos, "Expected member name");
@@ -202,6 +206,7 @@ static struct token *builtin_offsetof_expr(struct token *token,
 		case '[':
 			token = token->next;
 			e = alloc_expression(token->pos, EXPR_OFFSETOF);
+			e->flags = Int_const_expr;
 			e->op = '[';
 			token = parse_expression(token, &e->index);
 			token = expect(token, ']',
@@ -353,6 +358,7 @@ got_it:
 			"likely to produce unsigned long (and a warning) here",
 			show_token(token));
         expr->type = EXPR_VALUE;
+	expr->flags = Int_const_expr;
         expr->ctype = ctype_integer(modifiers);
         expr->value = value;
 	return;
@@ -377,6 +383,7 @@ Float:
 	else
 		goto Enoint;
 
+	expr->flags = Float_literal;
 	expr->type = EXPR_FVALUE;
 	return;
 
@@ -391,6 +398,7 @@ struct token *primary_expression(struct token *token, struct expression **tree)
 	switch (token_type(token)) {
 	case TOKEN_CHAR:
 		expr = alloc_expression(token->pos, EXPR_VALUE);   
+		expr->flags = Int_const_expr;
 		expr->ctype = &int_ctype; 
 		expr->value = (unsigned char) token->character;
 		token = token->next;
@@ -398,12 +406,13 @@ struct token *primary_expression(struct token *token, struct expression **tree)
 
 	case TOKEN_NUMBER:
 		expr = alloc_expression(token->pos, EXPR_VALUE);
-		get_number_value(expr, token);
+		get_number_value(expr, token); /* will see if it's an integer */
 		token = token->next;
 		break;
 
 	case TOKEN_ZERO_IDENT: {
 		expr = alloc_expression(token->pos, EXPR_SYMBOL);
+		expr->flags = Int_const_expr;
 		expr->ctype = &int_ctype;
 		expr->symbol = &zero_int;
 		expr->symbol_name = token->ident;
@@ -431,6 +440,7 @@ struct token *primary_expression(struct token *token, struct expression **tree)
 			*expr = *sym->initializer;
 			/* we want the right position reported, thus the copy */
 			expr->pos = token->pos;
+			expr->flags = Int_const_expr;
 			token = next;
 			break;
 		}
@@ -465,10 +475,13 @@ struct token *primary_expression(struct token *token, struct expression **tree)
 			expr = alloc_expression(token->pos, EXPR_PREOP);
 			expr->op = '(';
 			token = parens_expression(token, &expr->unop, "in expression");
+			if (expr->unop)
+				expr->flags = expr->unop->flags;
 			break;
 		}
 		if (token->special == '[' && lookup_type(token->next)) {
 			expr = alloc_expression(token->pos, EXPR_TYPE);
+			expr->flags = Int_const_expr; /* sic */
 			token = typename(token->next, &expr->symbol);
 			token = expect(token, ']', "in type expression");
 			break;
@@ -583,6 +596,7 @@ static struct token *type_info_expression(struct token *token,
 	struct expression *expr = alloc_expression(token->pos, type);
 
 	*tree = expr;
+	expr->flags = Int_const_expr; /* XXX: VLA support will need that changed */
 	token = token->next;
 	if (!match_op(token, '(') || !lookup_type(token->next))
 		return unary_expression(token, &expr->cast_expression);
@@ -632,7 +646,7 @@ static struct token *unary_expression(struct token *token, struct expression **t
 	if (token_type(token) == TOKEN_SPECIAL) {
 		if (match_oplist(token->special,
 		    SPECIAL_INCREMENT, SPECIAL_DECREMENT,
-		    '&', '*', '+', '-', '~', '!', 0)) {
+		    '&', '*', 0)) {
 		    	struct expression *unop;
 			struct expression *unary;
 			struct token *next;
@@ -648,7 +662,24 @@ static struct token *unary_expression(struct token *token, struct expression **t
 			*tree = unary;
 			return next;
 		}
+		/* possibly constant ones */
+		if (match_oplist(token->special, '+', '-', '~', '!', 0)) {
+		    	struct expression *unop;
+			struct expression *unary;
+			struct token *next;
 
+			next = cast_expression(token->next, &unop);
+			if (!unop) {
+				sparse_error(token->pos, "Syntax error in unary expression");
+				return next;
+			}
+			unary = alloc_expression(token->pos, EXPR_PREOP);
+			unary->op = token->special;
+			unary->unop = unop;
+			unary->flags = unop->flags & Int_const_expr;
+			*tree = unary;
+			return next;
+		}
 		/* Gcc extension: &&label gives the address of a label */
 		if (match_op(token, SPECIAL_LOGICAL_AND) &&
 		    token_type(token->next) == TOKEN_IDENT) {
@@ -682,6 +713,7 @@ static struct token *cast_expression(struct token *token, struct expression **tr
 		struct token *next = token->next;
 		if (lookup_type(next)) {
 			struct expression *cast = alloc_expression(next->pos, EXPR_CAST);
+			struct expression *v;
 			struct symbol *sym;
 
 			token = typename(next, &sym);
@@ -692,7 +724,14 @@ static struct token *cast_expression(struct token *token, struct expression **tr
 				return postfix_expression(token, tree, cast);
 			}
 			*tree = cast;
-			token = cast_expression(token, &cast->cast_expression);
+			token = cast_expression(token, &v);
+			if (!v)
+				return token;
+			cast->cast_expression = v;
+			if (v->flags & Int_const_expr)
+				cast->flags = Int_const_expr;
+			else if (v->flags & Float_literal) /* and _not_ int */
+				cast->flags = Int_const_expr | Float_literal;
 			return token;
 		}
 	}
@@ -712,9 +751,9 @@ static struct token *cast_expression(struct token *token, struct expression **tr
  * than create a data structure for it.
  */
 
-#define LR_BINOP_EXPRESSION(token, tree, type, inner, compare)		\
+#define __LR_BINOP_EXPRESSION(__token, tree, type, inner, compare, is_const)	\
 	struct expression *left = NULL;					\
-	struct token * next = inner(token, &left);			\
+	struct token * next = inner(__token, &left);			\
 									\
 	if (left) {							\
 		while (token_type(next) == TOKEN_SPECIAL) {		\
@@ -729,6 +768,9 @@ static struct token *cast_expression(struct token *token, struct expression **tr
 				sparse_error(next->pos, "No right hand side of '%s'-expression", show_special(op));	\
 				break;					\
 			}						\
+			if (is_const)					\
+				top->flags = left->flags & right->flags \
+						& Int_const_expr;	\
 			top->op = op;					\
 			top->left = left;				\
 			top->right = right;				\
@@ -739,6 +781,12 @@ out:									\
 	*tree = left;							\
 	return next;							\
 
+#define LR_BINOP_EXPRESSION(token, tree, type, inner, compare) \
+	__LR_BINOP_EXPRESSION((token), (tree), (type), (inner), (compare), 1)
+
+#define LR_BINOP_EXPRESSION_NONCONST(token, tree, type, inner, compare) \
+	__LR_BINOP_EXPRESSION((token), (tree), (type), (inner), (compare), 0)
+
 
 static struct token *multiplicative_expression(struct token *token, struct expression **tree)
 {
@@ -832,6 +880,14 @@ struct token *conditional_expression(struct token *token, struct expression **tr
 		token = parse_expression(token->next, &expr->cond_true);
 		token = expect(token, ':', "in conditional expression");
 		token = conditional_expression(token, &expr->cond_false);
+		if (expr->left && expr->cond_true) {
+			int is_const = expr->left->flags &
+					expr->cond_false->flags &
+					Int_const_expr;
+			if (expr->cond_true)
+				is_const &= expr->cond_true->flags;
+			expr->flags = is_const;
+		}
 	}
 	return token;
 }
@@ -862,7 +918,7 @@ struct token *assignment_expression(struct token *token, struct expression **tre
 
 static struct token *comma_expression(struct token *token, struct expression **tree)
 {
-	LR_BINOP_EXPRESSION(
+	LR_BINOP_EXPRESSION_NONCONST(
 		token, tree, EXPR_COMMA, assignment_expression,
 		(op == ',')
 	);
diff --git a/expression.h b/expression.h
index 19e0302..a913153 100644
--- a/expression.h
+++ b/expression.h
@@ -47,8 +47,14 @@ enum expression_type {
 	EXPR_OFFSETOF,
 };
 
+enum {
+	Int_const_expr = 1,
+	Float_literal = 2,
+}; /* for expr->flags */
+
 struct expression {
-	enum expression_type type;
+	enum expression_type type:8;
+	unsigned flags:8;
 	int op;
 	struct position pos;
 	struct symbol *ctype;
@@ -142,6 +148,7 @@ struct expression {
 
 /* Constant expression values */
 long long get_expression_value(struct expression *);
+long long const_expression_value(struct expression *);
 
 /* Expression parsing */
 struct token *parse_expression(struct token *token, struct expression **tree);
diff --git a/parse.c b/parse.c
index 4e8a18b..cb9f87a 100644
--- a/parse.c
+++ b/parse.c
@@ -802,7 +802,7 @@ static struct token *attribute_aligned(struct token *token, struct symbol *attr,
 	if (match_op(token, '(')) {
 		token = parens_expression(token, &expr, "in attribute");
 		if (expr)
-			alignment = get_expression_value(expr);
+			alignment = const_expression_value(expr);
 	}
 	ctype->alignment = alignment;
 	return token;
@@ -820,7 +820,7 @@ static struct token *attribute_address_space(struct token *token, struct symbol
 	token = expect(token, '(', "after address_space attribute");
 	token = conditional_expression(token, &expr);
 	if (expr)
-		ctype->as = get_expression_value(expr);
+		ctype->as = const_expression_value(expr);
 	token = expect(token, ')', "after address_space attribute");
 	return token;
 }
@@ -1258,7 +1258,7 @@ static struct token *handle_bitfield(struct token *token, struct symbol *decl)
 
 	bitfield = alloc_indirect_symbol(token->pos, ctype, SYM_BITFIELD);
 	token = conditional_expression(token->next, &expr);
-	width = get_expression_value(expr);
+	width = const_expression_value(expr);
 	bitfield->bit_size = width;
 
 	if (width < 0 || width > INT_MAX) {
@@ -1844,10 +1844,10 @@ static struct expression *index_expression(struct expression *from, struct expre
 	int idx_from, idx_to;
 	struct expression *expr = alloc_expression(from->pos, EXPR_INDEX);
 
-	idx_from = get_expression_value(from);
+	idx_from = const_expression_value(from);
 	idx_to = idx_from;
 	if (to) {
-		idx_to = get_expression_value(to);
+		idx_to = const_expression_value(to);
 		if (idx_to < idx_from || idx_from < 0)
 			warning(from->pos, "nonsense array initializer index range");
 	}
-- 
1.5.0-rc2.GIT

Re: [PATCH 16/16] fix handling of integer constant expressions

[Neil Booth]

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.

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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 ;-/

Re: [PATCH 16/16] fix handling of integer constant expressions

[Linus Torvalds]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Derek M Jones]

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:derek@knosof.co.uk
Applications Standards Conformance Testing    http://www.knosof.co.uk

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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.

Re: [PATCH 16/16] fix handling of integer constant expressions

[Neil Booth]

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.

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Neil Booth]

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.

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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.

Re: [PATCH 16/16] fix handling of integer constant expressions

[Neil Booth]

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.

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Neil Booth]

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.

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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 <viro@zeniv.linux.org.uk>
Date: Wed, 27 Jun 2007 09:10:54 -0400
Subject: [PATCH] fix the missed cannibalizing simplifications

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Neil Booth]

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 <viro@zeniv.linux.org.uk>
> Date: Wed, 27 Jun 2007 09:10:54 -0400
> Subject: [PATCH] fix the missed cannibalizing simplifications
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

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.

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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 <viro@zeniv.linux.org.uk>
> > Date: Wed, 27 Jun 2007 09:10:54 -0400
> > Subject: [PATCH] fix the missed cannibalizing simplifications
> > 
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> 
> 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.

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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.

Re: [PATCH 16/16] fix handling of integer constant expressions

[Josh Triplett]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Linus Torvalds]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Josh Triplett]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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.

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Linus Torvalds]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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.

Re: [PATCH 16/16] fix handling of integer constant expressions

[Segher Boessenkool]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Josh Triplett]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Segher Boessenkool]

> 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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Arnd Bergmann]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Segher Boessenkool]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Al Viro]

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

Re: [PATCH 16/16] fix handling of integer constant expressions

[Segher Boessenkool]

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