Designated initializers for fields in anonymous structs and unions

Designated initializers for fields in anonymous structs and unions

[josh]

GCC 4.6 and newer support initializing designated initializers for fields in
anonymous structs and unions.  However, sparse does not.  Test case:

union U {
    int a;
    struct {
        int b;
        int c;
    };
};

static union U u = {
    .b = 0,
    .c = 0,
};

struct S {
    int a;
    union {
        int b;
        int c;
    };
};

static struct S s = {
    .a = 0,
    .b = 0,
};



GCC handles this just fine; sparse says:

test.c:10:6: error: unknown field name in initializer
test.c:11:6: error: unknown field name in initializer
test.c:24:6: error: unknown field name in initializer

Sparse needs to handle this, and we should add the above as a test case.

(We also need an appropriate extension to the test for
__attribute__((designated_init)).)

- Josh Triplett

Re: Designated initializers for fields in anonymous structs and unions

[Linus Torvalds]

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

On Thu, Jul 31, 2014 at 11:10 AM,  <josh@joshtriplett.org> wrote:
> GCC 4.6 and newer support initializing designated initializers for fields in
> anonymous structs and unions.  However, sparse does not.

I sent patches for this back in April, based on a report from Hans
Verkuil. My patches fixed the parsing part, but had a (known) bug with
getting the offsets right, so your test-case actually retulrs in

  t.c:23:6: warning: Initializer entry defined twice
  t.c:24:6:   also defined here

because the offset calculation for "b" was wrong, and as a result
sparse thinks "a" and "b" overlap. John Keeping had a patch that tried
to fix that too.

I think they are all in

   https://github.com/johnkeeping/sparse/

but not in the "official" sparse tree, which hasn't seen any work
since January afaik. Maybe there are other trees than John's..

I'm attaching my set of sparse patches from early April in case
anybody wants to look at them. No guarantees, and as mentioned there's
t least an offset bug, but they should be an improvement, at least.

                    Linus

[-- Attachment #2: 0001-Add-warning-about-duplicate-initializers.patch --]
[-- Type: text/x-patch, Size: 1208 bytes --]

From d245709b074c8d07dba61b4a475148d4549e3697 Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sun, 30 Mar 2014 09:49:13 -0700
Subject: [PATCH 1/4] Add warning about duplicate initializers

Noticed this while looking at an independent bug reported by Hans
Verkuil.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 evaluate.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/evaluate.c b/evaluate.c
index 66556150ddac..8a53b3e884e0 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -3043,10 +3043,18 @@ static void check_duplicates(struct symbol *sym)
 {
 	int declared = 0;
 	struct symbol *next = sym;
+	int initialized = sym->initializer != NULL;
 
 	while ((next = next->same_symbol) != NULL) {
 		const char *typediff;
 		evaluate_symbol(next);
+		if (initialized && next->initializer) {
+			sparse_error(sym->pos, "symbol '%s' has multiple initializers (originally initialized at %s:%d)",
+				show_ident(sym->ident),
+				stream_name(next->pos.stream), next->pos.line);
+			/* Only warn once */
+			initialized = 0;
+		}
 		declared++;
 		typediff = type_difference(&sym->ctype, &next->ctype, 0, 0);
 		if (typediff) {
-- 
2.0.1.427.gc47372d


[-- Attachment #3: 0002-Use-any-previous-initializer-to-size-a-symbol.patch --]
[-- Type: text/x-patch, Size: 2142 bytes --]

From 17139e5d7a35c4b1793221f073a4f543f00b1cfa Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sun, 30 Mar 2014 10:13:54 -0700
Subject: [PATCH 2/4] Use any previous initializer to size a symbol

When we size a symbol, we only have one initializer per symbol, but we
may have multiple symbol declarations for the same symbol.  So make sure
to walk the "same_symbol" chain to find the initializer, rather than
assuming it is attached to the current declaration.

Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 symbol.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/symbol.c b/symbol.c
index eb6e1215ee87..4b91abd8021e 100644
--- a/symbol.c
+++ b/symbol.c
@@ -354,6 +354,15 @@ static int count_array_initializer(struct symbol *t, struct expression *expr)
 	return nr;
 }
 
+static struct expression *get_symbol_initializer(struct symbol *sym)
+{
+	do {
+		if (sym->initializer)
+			return sym->initializer;
+	} while ((sym = sym->same_symbol) != NULL);
+	return NULL;
+}
+
 static struct symbol * examine_node_type(struct symbol *sym)
 {
 	struct symbol *base_type = examine_base_type(sym);
@@ -376,12 +385,15 @@ static struct symbol * examine_node_type(struct symbol *sym)
 		sym->ctype.alignment = alignment;
 
 	/* Unsized array? The size might come from the initializer.. */
-	if (bit_size < 0 && base_type->type == SYM_ARRAY && sym->initializer) {
-		struct symbol *node_type = base_type->ctype.base_type;
-		int count = count_array_initializer(node_type, sym->initializer);
-
-		if (node_type && node_type->bit_size >= 0)
-			bit_size = node_type->bit_size * count;
+	if (bit_size < 0 && base_type->type == SYM_ARRAY) {
+		struct expression *initializer = get_symbol_initializer(sym);
+		if (initializer) {
+			struct symbol *node_type = base_type->ctype.base_type;
+			int count = count_array_initializer(node_type, initializer);
+
+			if (node_type && node_type->bit_size >= 0)
+				bit_size = node_type->bit_size * count;
+		}
 	}
 	
 	sym->bit_size = bit_size;
-- 
2.0.1.427.gc47372d


[-- Attachment #4: 0003-Allow-named-initializers-for-anonymous-union-members.patch --]
[-- Type: text/x-patch, Size: 1638 bytes --]

From 5e870f7fa535c03ac2c1ae78479327d112c2564e Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Tue, 1 Apr 2014 10:18:59 -0700
Subject: [PATCH 3/4] Allow named initializers for anonymous union members

The initializer type evaluation didn't use the function that knew about
recursively looking for member names in anonymous unions and structs.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 evaluate.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index 8a53b3e884e0..5adfc1e3ff26 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2171,17 +2171,6 @@ static int evaluate_arguments(struct symbol *f, struct symbol *fn, struct expres
 	return 1;
 }
 
-static struct symbol *find_struct_ident(struct symbol *ctype, struct ident *ident)
-{
-	struct symbol *sym;
-
-	FOR_EACH_PTR(ctype->symbol_list, sym) {
-		if (sym->ident == ident)
-			return sym;
-	} END_FOR_EACH_PTR(sym);
-	return NULL;
-}
-
 static void convert_index(struct expression *e)
 {
 	struct expression *child = e->idx_expression;
@@ -2290,11 +2279,12 @@ static struct expression *check_designators(struct expression *e,
 			}
 			e = e->idx_expression;
 		} else if (e->type == EXPR_IDENTIFIER) {
+			int offset = 0;
 			if (ctype->type != SYM_STRUCT && ctype->type != SYM_UNION) {
 				err = "field name not in struct or union";
 				break;
 			}
-			ctype = find_struct_ident(ctype, e->expr_ident);
+			ctype = find_identifier(e->expr_ident, ctype->symbol_list, &offset);
 			if (!ctype) {
 				err = "unknown field name in";
 				break;
-- 
2.0.1.427.gc47372d


[-- Attachment #5: 0004-Fix-scoping-of-extern-symbols-in-block-scope.patch --]
[-- Type: text/x-patch, Size: 1360 bytes --]

From c011597ff67765da5dacb472ddbd69dc4b51416d Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Tue, 1 Apr 2014 10:20:34 -0700
Subject: [PATCH 4/4] Fix scoping of extern symbols in block scope

We'd only match them with other symbols marked 'extern', but really, we
should match them with any top-level non-static symbol.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 symbol.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/symbol.c b/symbol.c
index 4b91abd8021e..c5e4784734a8 100644
--- a/symbol.c
+++ b/symbol.c
@@ -576,11 +576,15 @@ void check_declaration(struct symbol *sym)
 			sym->same_symbol = next;
 			return;
 		}
-		if (sym->ctype.modifiers & next->ctype.modifiers & MOD_EXTERN) {
-			if ((sym->ctype.modifiers ^ next->ctype.modifiers) & MOD_INLINE)
-				continue;
-			sym->same_symbol = next;
-			return;
+		/* Extern in block level matches a TOPLEVEL non-static symbol */
+		if (sym->ctype.modifiers & MOD_EXTERN) {
+			if ((next->ctype.modifiers & (MOD_TOPLEVEL|MOD_STATIC)) == MOD_TOPLEVEL) {
+				/* FIXME? Differs in 'inline' only? Why does that matter? */
+				if ((sym->ctype.modifiers ^ next->ctype.modifiers) & MOD_INLINE)
+					continue;
+				sym->same_symbol = next;
+				return;
+			}
 		}
 
 		if (!Wshadow || warned)
-- 
2.0.1.427.gc47372d

Re: Designated initializers for fields in anonymous structs and unions

[Linus Torvalds]

On Thu, Jul 31, 2014 at 11:39 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> but not in the "official" sparse tree, which hasn't seen any work
> since January afaik.

Ugh, I looked at the old "sparse/sparse.git" tree (and badly at that,
it stopped updating in April, not January). It's
"sparse/chrisl/sparse.git" these days. No wonder I didn't see any
recent work.

The sparse.wiki.kernel.org main page is the first google hit for
"sparse git tree" and mentions that old git repo, not Chris' new one.

That leaves just my last patch ("Fix scoping of extern") as being not
in the sparse tree, and that's irrelevant for the union initializer
thing.

Chris, any particular reason why the url changed?

            Linus

Re: Designated initializers for fields in anonymous structs and unions

[josh]

On Thu, Jul 31, 2014 at 11:50:30AM -0700, Linus Torvalds wrote:
> On Thu, Jul 31, 2014 at 11:39 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > but not in the "official" sparse tree, which hasn't seen any work
> > since January afaik.
> 
> Ugh, I looked at the old "sparse/sparse.git" tree (and badly at that,
> it stopped updating in April, not January). It's
> "sparse/chrisl/sparse.git" these days. No wonder I didn't see any
> recent work.
> 
> The sparse.wiki.kernel.org main page is the first google hit for
> "sparse git tree" and mentions that old git repo, not Chris' new one.
> 
> That leaves just my last patch ("Fix scoping of extern") as being not
> in the sparse tree, and that's irrelevant for the union initializer
> thing.
> 
> Chris, any particular reason why the url changed?

I think Chris has been trying to use chrisl as a staging tree to let
things cook a while, and then moving them over to the sparse tree.
However, I think it'd make more sense to do that as a branch rather than
a separate repo.

- Josh Triplett

Re: Designated initializers for fields in anonymous structs and unions

[Linus Torvalds]

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

On Thu, Jul 31, 2014 at 11:10 AM,  <josh@joshtriplett.org> wrote:
> Test case:
>
> struct S {
>     int a;
>     union {
>         int b;
>         int c;
>     };
> };
>
> static struct S s = {
>     .a = 0,
>     .b = 0,
> };

So sparse fails on this test-case (differently from what Josh
reported, because apparently Josh is running 0.5.0) with:

  t.c:10:6: warning: Initializer entry defined twice
  t.c:11:6:   also defined here

because the offsets are miscomputed. The conversion from
EXPR_IDENTIFIER (an initializer entry with a named identifier) to
EXPR_POS (an initializer entry with a byte offset) got this wrong.

We need to use "find_identifier()" that does the proper recursive
lookup and updates the offset, but the required structure type wasn't
passed down to convert_designators() and convert_ident(), so the patch
is a bit bigger than just changing a couple of lines.

I'm quite sure we still screw complex intiializers up . But this makes
at least Josh's test-case pass, and looks superficially sane.

Hmm?

                  Linus

[-- Attachment #2: 0001-Make-convert_ident-use-find_identifier-to-get-the-pr.patch --]
[-- Type: text/x-patch, Size: 5511 bytes --]

From 7670d933e82917ea28a7e5a5df09bcc9744dba34 Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 31 Jul 2014 19:01:36 -0700
Subject: [PATCH] Make 'convert_ident()' use 'find_identifier()' to get the
 proper member offset

The convert_ident() function used to do

	struct symbol *sym = e->field;
	e->init_offset = sym->offset;

which is wrong for embedded unnamed unions.  It really wants to do
"find_identifier()", which looks up the field name in the structure or
union type, and properly adds the offsets when it looks things up in an
anonymous structure or union.

However, we didn't pass in the right symbol type, so convert_ident()
couldn't be just trivially converted with a couple of lines.

This changes the code to pass in the struct/union type, and also
re-organizes find_identifier() to have a nicer calling convention.  We
now pass in the type, not the name-list, which makes it possible for
find_identifier() to do the peeling of the SYM_NODE if required,
simplifying the callers (including the recursive call for the anonymous
case).

Reported-by: Just Triplett <josh@joshtriplett.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 evaluate.c | 47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index 03992d03ae1d..5195decd1b2e 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -1872,13 +1872,19 @@ static struct symbol *evaluate_preop(struct expression *expr)
 	return ctype;
 }
 
-static struct symbol *find_identifier(struct ident *ident, struct symbol_list *_list, int *offset)
+static struct symbol *find_identifier(struct ident *ident, struct symbol *ctype, int *offset)
 {
-	struct ptr_list *head = (struct ptr_list *)_list;
-	struct ptr_list *list = head;
+	struct ptr_list *head, *list;
 
+	if (ctype->type == SYM_NODE)
+		ctype = ctype->ctype.base_type;
+	if (ctype->type != SYM_UNION && ctype->type != SYM_STRUCT)
+		return NULL;
+
+	head = (struct ptr_list *)ctype->symbol_list;
 	if (!head)
 		return NULL;
+	list = head;
 	do {
 		int i;
 		for (i = 0; i < list->nr; i++) {
@@ -1889,13 +1895,8 @@ static struct symbol *find_identifier(struct ident *ident, struct symbol_list *_
 				*offset = sym->offset;
 				return sym;
 			} else {
-				struct symbol *ctype = sym->ctype.base_type;
 				struct symbol *sub;
-				if (!ctype)
-					continue;
-				if (ctype->type != SYM_UNION && ctype->type != SYM_STRUCT)
-					continue;
-				sub = find_identifier(ident, ctype->symbol_list, offset);
+				sub = find_identifier(ident, sym, offset);
 				if (!sub)
 					continue;
 				*offset += sym->offset;
@@ -1965,7 +1966,7 @@ static struct symbol *evaluate_member_dereference(struct expression *expr)
 		return NULL;
 	}
 	offset = 0;
-	member = find_identifier(ident, ctype->symbol_list, &offset);
+	member = find_identifier(ident, ctype, &offset);
 	if (!member) {
 		const char *type = ctype->type == SYM_STRUCT ? "struct" : "union";
 		const char *name = "<unnamed>";
@@ -2220,23 +2221,27 @@ static void convert_index(struct expression *e)
 	e->init_expr = child;
 }
 
-static void convert_ident(struct expression *e)
+static void convert_ident(struct expression *e, struct symbol *ctype)
 {
+	int offset;
 	struct expression *child = e->ident_expression;
-	struct symbol *sym = e->field;
+
+	ctype = find_identifier(e->expr_ident, ctype, &offset);
+	if (!ctype)
+		return;
 	e->type = EXPR_POS;
-	e->init_offset = sym->offset;
+	e->init_offset = offset;
 	e->init_nr = 1;
 	e->init_expr = child;
 }
 
-static void convert_designators(struct expression *e)
+static void convert_designators(struct expression *e, struct symbol *ctype)
 {
 	while (e) {
 		if (e->type == EXPR_INDEX)
 			convert_index(e);
 		else if (e->type == EXPR_IDENTIFIER)
-			convert_ident(e);
+			convert_ident(e, ctype);
 		else
 			break;
 		e = e->init_expr;
@@ -2322,7 +2327,7 @@ static struct expression *check_designators(struct expression *e,
 				err = "field name not in struct or union";
 				break;
 			}
-			ctype = find_identifier(e->expr_ident, ctype->symbol_list, &offset);
+			ctype = find_identifier(e->expr_ident, ctype, &offset);
 			if (!ctype) {
 				err = "unknown field name in";
 				break;
@@ -2392,7 +2397,7 @@ static struct expression *next_designators(struct expression *old,
 		if (!copy) {
 			field = old->field->next_subobject;
 			if (!field) {
-				convert_ident(old);
+				convert_ident(old, ctype);
 				return NULL;
 			}
 			copy = e;
@@ -2406,7 +2411,7 @@ static struct expression *next_designators(struct expression *old,
 		new->expr_ident = field->ident;
 		new->ident_expression = copy;
 		new->ctype = field;
-		convert_ident(old);
+		convert_ident(old, ctype);
 	}
 	return new;
 }
@@ -2465,7 +2470,7 @@ static void handle_list_initializer(struct expression *expr,
 			top = next;
 			/* deeper than one designator? */
 			jumped = top != e;
-			convert_designators(last);
+			convert_designators(last, ctype);
 			last = e;
 		}
 
@@ -2497,7 +2502,7 @@ found:
 
 	} END_FOR_EACH_PTR(e);
 
-	convert_designators(last);
+	convert_designators(last, ctype);
 	expr->ctype = ctype;
 }
 
@@ -2901,7 +2906,7 @@ static struct symbol *evaluate_offsetof(struct expression *expr)
 			return NULL;
 		}
 
-		field = find_identifier(expr->ident, ctype->symbol_list, &offset);
+		field = find_identifier(expr->ident, ctype, &offset);
 		if (!field) {
 			expression_error(expr, "unknown member");
 			return NULL;
-- 
2.1.0.rc0.52.gaa544bf

Re: Designated initializers for fields in anonymous structs and unions

[Linus Torvalds]

On Thu, Jul 31, 2014 at 7:19 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I'm quite sure we still screw complex intiializers up . But this makes
> at least Josh's test-case pass, and looks superficially sane.

Hmm. It exposes other problems.

For example, the fact that we look up the identifier now exposes the
fact that "first_subobject()" creates this dummy "EXPR_IDENTIFIER"
expression that has no identifier. That will then cause
"find_identifier()" to fail, which in turn causes convert_ident() to
fail. Resulting in the parse tree having remaining EXPR_IDENTIFIER
nodes, which will later result in "unknown expression (25,0)" errors.

That's easy enough to fix with just adding the proper initializer, ie

    new->expr_ident = field->ident;

to first_subobject().

But the change apparently also exposes some problems with nested
EXPR_POS cases, so now we mis-parse initializers that have multiple
levels of naming, of the type

   struct S a = { .a.b = 1 };

I haven't had the energy to figure out what went wrong with that one,
and I'm getting rather fed up with this for today, so I'm hoping
somebody else finds this at all interesting.

              Linus

Re: Designated initializers for fields in anonymous structs and unions

[Josh Triplett]

On Thu, Jul 31, 2014 at 07:19:20PM -0700, Linus Torvalds wrote:
> On Thu, Jul 31, 2014 at 11:10 AM,  <josh@joshtriplett.org> wrote:
> > Test case:
> >
> > struct S {
> >     int a;
> >     union {
> >         int b;
> >         int c;
> >     };
> > };
> >
> > static struct S s = {
> >     .a = 0,
> >     .b = 0,
> > };
> 
> So sparse fails on this test-case (differently from what Josh
> reported, because apparently Josh is running 0.5.0) with:
> 
>   t.c:10:6: warning: Initializer entry defined twice
>   t.c:11:6:   also defined here

I had the latest bits from the main sparse repo; switching to the
chrisli repo gives me these same results.

> because the offsets are miscomputed. The conversion from
> EXPR_IDENTIFIER (an initializer entry with a named identifier) to
> EXPR_POS (an initializer entry with a byte offset) got this wrong.
> 
> We need to use "find_identifier()" that does the proper recursive
> lookup and updates the offset, but the required structure type wasn't
> passed down to convert_designators() and convert_ident(), so the patch
> is a bit bigger than just changing a couple of lines.
> 
> I'm quite sure we still screw complex intiializers up . But this makes
> at least Josh's test-case pass, and looks superficially sane.
> 
> Hmm?

Comments below.

> From 7670d933e82917ea28a7e5a5df09bcc9744dba34 Mon Sep 17 00:00:00 2001
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Thu, 31 Jul 2014 19:01:36 -0700
> Subject: [PATCH] Make 'convert_ident()' use 'find_identifier()' to get the
>  proper member offset
> 
> The convert_ident() function used to do
> 
> 	struct symbol *sym = e->field;
> 	e->init_offset = sym->offset;
> 
> which is wrong for embedded unnamed unions.  It really wants to do
> "find_identifier()", which looks up the field name in the structure or
> union type, and properly adds the offsets when it looks things up in an
> anonymous structure or union.
> 
> However, we didn't pass in the right symbol type, so convert_ident()
> couldn't be just trivially converted with a couple of lines.
> 
> This changes the code to pass in the struct/union type, and also
> re-organizes find_identifier() to have a nicer calling convention.  We
> now pass in the type, not the name-list, which makes it possible for
> find_identifier() to do the peeling of the SYM_NODE if required,
> simplifying the callers (including the recursive call for the anonymous
> case).
> 
> Reported-by: Just Triplett <josh@joshtriplett.org>

s/Just/Josh/

> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

With the above typo fixed:
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
Tested-by: Josh Triplett <josh@joshtriplett.org>

It might make sense to combine this with the handling of array
designated initializers, to allow things like .x[0].y.z[3] = ..., but
this definitely fixes the issue I ran into.

(And now I'm wondering why nobody seems to have ever created an
"unnamed array" extension.)

> ---
>  evaluate.c | 47 ++++++++++++++++++++++++++---------------------
>  1 file changed, 26 insertions(+), 21 deletions(-)
> 
> diff --git a/evaluate.c b/evaluate.c
> index 03992d03ae1d..5195decd1b2e 100644
> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -1872,13 +1872,19 @@ static struct symbol *evaluate_preop(struct expression *expr)
>  	return ctype;
>  }
>  
> -static struct symbol *find_identifier(struct ident *ident, struct symbol_list *_list, int *offset)
> +static struct symbol *find_identifier(struct ident *ident, struct symbol *ctype, int *offset)
>  {
> -	struct ptr_list *head = (struct ptr_list *)_list;
> -	struct ptr_list *list = head;
> +	struct ptr_list *head, *list;
>  
> +	if (ctype->type == SYM_NODE)
> +		ctype = ctype->ctype.base_type;
> +	if (ctype->type != SYM_UNION && ctype->type != SYM_STRUCT)
> +		return NULL;
> +
> +	head = (struct ptr_list *)ctype->symbol_list;
>  	if (!head)
>  		return NULL;
> +	list = head;
>  	do {
>  		int i;
>  		for (i = 0; i < list->nr; i++) {
> @@ -1889,13 +1895,8 @@ static struct symbol *find_identifier(struct ident *ident, struct symbol_list *_
>  				*offset = sym->offset;
>  				return sym;
>  			} else {
> -				struct symbol *ctype = sym->ctype.base_type;
>  				struct symbol *sub;
> -				if (!ctype)
> -					continue;
> -				if (ctype->type != SYM_UNION && ctype->type != SYM_STRUCT)
> -					continue;
> -				sub = find_identifier(ident, ctype->symbol_list, offset);
> +				sub = find_identifier(ident, sym, offset);
>  				if (!sub)
>  					continue;
>  				*offset += sym->offset;
> @@ -1965,7 +1966,7 @@ static struct symbol *evaluate_member_dereference(struct expression *expr)
>  		return NULL;
>  	}
>  	offset = 0;
> -	member = find_identifier(ident, ctype->symbol_list, &offset);
> +	member = find_identifier(ident, ctype, &offset);
>  	if (!member) {
>  		const char *type = ctype->type == SYM_STRUCT ? "struct" : "union";
>  		const char *name = "<unnamed>";
> @@ -2220,23 +2221,27 @@ static void convert_index(struct expression *e)
>  	e->init_expr = child;
>  }
>  
> -static void convert_ident(struct expression *e)
> +static void convert_ident(struct expression *e, struct symbol *ctype)
>  {
> +	int offset;
>  	struct expression *child = e->ident_expression;
> -	struct symbol *sym = e->field;
> +
> +	ctype = find_identifier(e->expr_ident, ctype, &offset);
> +	if (!ctype)
> +		return;
>  	e->type = EXPR_POS;
> -	e->init_offset = sym->offset;
> +	e->init_offset = offset;
>  	e->init_nr = 1;
>  	e->init_expr = child;
>  }
>  
> -static void convert_designators(struct expression *e)
> +static void convert_designators(struct expression *e, struct symbol *ctype)
>  {
>  	while (e) {
>  		if (e->type == EXPR_INDEX)
>  			convert_index(e);
>  		else if (e->type == EXPR_IDENTIFIER)
> -			convert_ident(e);
> +			convert_ident(e, ctype);
>  		else
>  			break;
>  		e = e->init_expr;
> @@ -2322,7 +2327,7 @@ static struct expression *check_designators(struct expression *e,
>  				err = "field name not in struct or union";
>  				break;
>  			}
> -			ctype = find_identifier(e->expr_ident, ctype->symbol_list, &offset);
> +			ctype = find_identifier(e->expr_ident, ctype, &offset);
>  			if (!ctype) {
>  				err = "unknown field name in";
>  				break;
> @@ -2392,7 +2397,7 @@ static struct expression *next_designators(struct expression *old,
>  		if (!copy) {
>  			field = old->field->next_subobject;
>  			if (!field) {
> -				convert_ident(old);
> +				convert_ident(old, ctype);
>  				return NULL;
>  			}
>  			copy = e;
> @@ -2406,7 +2411,7 @@ static struct expression *next_designators(struct expression *old,
>  		new->expr_ident = field->ident;
>  		new->ident_expression = copy;
>  		new->ctype = field;
> -		convert_ident(old);
> +		convert_ident(old, ctype);
>  	}
>  	return new;
>  }
> @@ -2465,7 +2470,7 @@ static void handle_list_initializer(struct expression *expr,
>  			top = next;
>  			/* deeper than one designator? */
>  			jumped = top != e;
> -			convert_designators(last);
> +			convert_designators(last, ctype);
>  			last = e;
>  		}
>  
> @@ -2497,7 +2502,7 @@ found:
>  
>  	} END_FOR_EACH_PTR(e);
>  
> -	convert_designators(last);
> +	convert_designators(last, ctype);
>  	expr->ctype = ctype;
>  }
>  
> @@ -2901,7 +2906,7 @@ static struct symbol *evaluate_offsetof(struct expression *expr)
>  			return NULL;
>  		}
>  
> -		field = find_identifier(expr->ident, ctype->symbol_list, &offset);
> +		field = find_identifier(expr->ident, ctype, &offset);
>  		if (!field) {
>  			expression_error(expr, "unknown member");
>  			return NULL;
> -- 
> 2.1.0.rc0.52.gaa544bf
> 

Re: Designated initializers for fields in anonymous structs and unions

[Linus Torvalds]

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

On Thu, Jul 31, 2014 at 7:41 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Hmm. It exposes other problems.
>
> For example, the fact that we look up the identifier now exposes the
> fact that "first_subobject()" creates this dummy "EXPR_IDENTIFIER"
> expression that has no identifier. That will then cause
> "find_identifier()" to fail, which in turn causes convert_ident() to
> fail. Resulting in the parse tree having remaining EXPR_IDENTIFIER
> nodes, which will later result in "unknown expression (25,0)" errors.
>
> That's easy enough to fix with just adding the proper initializer, ie
>
>     new->expr_ident = field->ident;
>
> to first_subobject().

Actually, sadly, while that worked for my test-cases (and for Josh's
case), it turns out to really *not* work in general: the dummy
EXPR_IDENTIFIER that we create may end up referring to a unnamed
union, which doesn't *have* an ident associated with it.

This only really triggers when you mix named initializers with then
relying on the whole "initialize the next one without a name", but
that does happen. And it fundamentally breaks that whole "use
find_identifier()" approach.

So convert_ident() really cannot use find_indent(), and really does
have to use "sym->offset". But that doesn't work for anonymous union
members, because that offset was the offset within the anonymous
union, not the parent structure.

However, the fix is "simple". We can just say that anonymous
union/structure member offsets have to be the offsets within the
parent union instead, and fix things up. So how about a patch like
this *instead* of the previous patch..

                 Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 1551 bytes --]

 evaluate.c |  2 +-
 symbol.c   | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/evaluate.c b/evaluate.c
index 03992d03ae1d..78150f83a19f 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -1898,7 +1898,7 @@ static struct symbol *find_identifier(struct ident *ident, struct symbol_list *_
 				sub = find_identifier(ident, ctype->symbol_list, offset);
 				if (!sub)
 					continue;
-				*offset += sym->offset;
+				*offset = sym->offset;
 				return sub;
 			}	
 		}
diff --git a/symbol.c b/symbol.c
index a2731348a011..32af64050a64 100644
--- a/symbol.c
+++ b/symbol.c
@@ -116,6 +116,18 @@ static int bitfield_base_size(struct symbol *sym)
 	return sym->bit_size;
 }
 
+static void update_symbol_offsets(struct symbol *sym, unsigned int offset)
+{
+	struct symbol *field;
+
+	if (sym->type != SYM_STRUCT && sym->type != SYM_UNION)
+		return;
+	FOR_EACH_PTR(sym->symbol_list, field) {
+		field->offset += offset;
+		update_symbol_offsets(field, offset);
+	} END_FOR_EACH_PTR(field);
+}
+
 /*
  * Structures are a bit more interesting to lay out
  */
@@ -174,6 +186,10 @@ static void lay_out_struct(struct symbol *sym, struct struct_union_info *info)
 	bit_size = (bit_size + align_bit_mask) & ~align_bit_mask;
 	sym->offset = bits_to_bytes(bit_size);
 
+	/* If it's an unnamed struct or union, update the offsets of the sub-members */
+	if (sym->offset && !sym->ident)
+		update_symbol_offsets(sym, sym->offset);
+
 	info->bit_size = bit_size + base_size;
 	// warning (sym->pos, "regular: offset=%d", sym->offset);
 }

Re: Designated initializers for fields in anonymous structs and unions

[Christopher Li]

On Fri, Aug 1, 2014 at 6:16 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Actually, sadly, while that worked for my test-cases (and for Josh's
> case), it turns out to really *not* work in general: the dummy
> EXPR_IDENTIFIER that we create may end up referring to a unnamed
> union, which doesn't *have* an ident associated with it.
>
> This only really triggers when you mix named initializers with then
> relying on the whole "initialize the next one without a name", but
> that does happen. And it fundamentally breaks that whole "use
> find_identifier()" approach.
>
> So convert_ident() really cannot use find_indent(), and really does
> have to use "sym->offset". But that doesn't work for anonymous union
> members, because that offset was the offset within the anonymous
> union, not the parent structure.
>
> However, the fix is "simple". We can just say that anonymous
> union/structure member offsets have to be the offsets within the
> parent union instead, and fix things up. So how about a patch like
> this *instead* of the previous patch..

Sorry I am late for the party. This approach assume the anonymous
structure is only referenced inside the structure. There is not other
external reference. However, that assumption is not true if "-fplan9-extensions"
or "-fms-extensions" was enabled. Sparse current does not support these
two extensions. But it will be a problem if we support them down the road.

According to this gcc document:

https://gcc.gnu.org/onlinedocs/gcc/Unnamed-Fields.html

One of the example is this:
     struct s1 { int a; };
     struct s2 { struct s1; };

You see, "struct s1" does not have a name, however "struct s1" can be
referenced from other symbol. So update the symbol offset in "struct s1"
will be wrong for the other reference of "struct s1".

Actually, the "find_identifier" call inside function "check_designators" already
calculate the correct offset. Only if we can pass that offset to
"convert_ident()".

Another idea is that, instead of using ident in EXPR_IDENTIFIER, we can
do one time find_identifier(). Then convert the EXPR_IDENTIFIER to an index
to the struct. It might take more than one index level for the anonymous
struct or union. BTW, that is very similar to the LLVM  performing the
"Get Element Pointer" instruction. Using the index, we can still reference
the anonymous struct.  This is not a trivial change. However it might be
the right thing to do if we want to support "-fplan9-extensions"

Chris

Re: Designated initializers for fields in anonymous structs and unions

[Christopher Li]

On Thu, Jul 31, 2014 at 11:50 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> That leaves just my last patch ("Fix scoping of extern") as being not
> in the sparse tree, and that's irrelevant for the union initializer
> thing.

I just apply that without the "FIXME" part. That two lines are from an incorrect
fix for the external inline bug. I just remove it for now. Currently
sparse will fail
at the external inline test. I will fix that in a separate patch.

Chris

Re: Designated initializers for fields in anonymous structs and unions

[Christopher Li]

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

On Sat, Aug 2, 2014 at 1:27 AM, Christopher Li <sparse@chrisli.org> wrote:
> I just apply that without the "FIXME" part. That two lines are from an incorrect
> fix for the external inline bug. I just remove it for now. Currently
> sparse will fail
> at the external inline test. I will fix that in a separate patch.

This is a follow up change to fix the extern-inline.c validation bug.
Feed back are welcome.

Consider the following case, extern inline declare after
extern declare of the same function.

extern int g(int);
extern __inline__ int g(int x)
{
        return x;
}

Sparse will give the first function global scope and
the second one file scope. Also the first one will get the
function body from the second one. That cause the failure
of the validation/extern-inlien.c

This change rebind the scope of the same_symbol chain to
the new scope. It will pass the extern-inline.c test case.

Chris

[-- Attachment #2: 0001-Make-same_symbol-list-share-the-same-scope.patch --]
[-- Type: text/x-patch, Size: 2142 bytes --]

From 7abd8a76b4e07ad60b5262e4c7858e638467fab8 Mon Sep 17 00:00:00 2001
From: Christopher Li <sparse@chrisli.org>
Date: Sat, 2 Aug 2014 10:56:59 -0700
Subject: [PATCH] Make same_symbol list share the same scope

Consider the following case, extern inline declare after
extern declare of the same function.

extern int g(int);
extern __inline__ int g(int x)
{
        return x;
}

Sparse will give the first function global scope and
the second one file scope. Also the first one will get the
function body from the second one. That cause the failure
of the validation/extern-inlien.c

This change rebind the scope of the same_symbol chain to
the new scope. It will pass the extern-inline.c test case.

Signed-off-by: Christopher Li <sparse@chrisli.org>
---
 parse.c |  1 +
 scope.c | 14 ++++++++++++++
 scope.h |  1 +
 3 files changed, 16 insertions(+)

diff --git a/parse.c b/parse.c
index 55a57a7..eaa1883 100644
--- a/parse.c
+++ b/parse.c
@@ -2611,6 +2611,7 @@ static struct token *parse_function_body(struct token *token, struct symbol *dec
 		info(prev->definition->pos, " the previous one is here");
 	} else {
 		while (prev) {
+			rebind_scope(prev, decl->scope);
 			prev->definition = decl;
 			prev = prev->same_symbol;
 		}
diff --git a/scope.c b/scope.c
index b415ace..cbf2fcf 100644
--- a/scope.c
+++ b/scope.c
@@ -46,6 +46,20 @@ void bind_scope(struct symbol *sym, struct scope *scope)
 	add_symbol(&scope->symbols, sym);
 }
 
+
+void rebind_scope(struct symbol *sym, struct scope *new)
+{
+	struct scope *old = sym->scope;
+
+	if (old == new)
+		return;
+
+	if (old)
+		delete_ptr_list_entry((struct ptr_list**) &old->symbols, sym, 1);
+
+	bind_scope(sym, new);
+}
+
 static void start_scope(struct scope **s)
 {
 	struct scope *scope = __alloc_scope(0);
diff --git a/scope.h b/scope.h
index 045c356..9bbb675 100644
--- a/scope.h
+++ b/scope.h
@@ -55,6 +55,7 @@ extern void start_function_scope(void);
 extern void end_function_scope(void);
 
 extern void bind_scope(struct symbol *, struct scope *);
+extern void rebind_scope(struct symbol *, struct scope *);
 
 extern int is_outer_scope(struct scope *);
 #endif
-- 
1.9.3

Re: Designated initializers for fields in anonymous structs and unions

[Linus Torvalds]

On Fri, Aug 1, 2014 at 10:16 PM, Christopher Li <sparse@chrisli.org> wrote:
>
> Sorry I am late for the party. This approach assume the anonymous
> structure is only referenced inside the structure.

It does indeed. I thought that was ok.

> There is not other
> external reference. However, that assumption is not true if "-fplan9-extensions"
> or "-fms-extensions" was enabled. Sparse current does not support these
> two extensions. But it will be a problem if we support them down the road.

.. and in fact I think even without those things, you can just make
the unnamed union have a type name, ie

   struct S {
      union T {
          int a;
      }
   };

the union T has a typename, but is a unnamed member of struct S. We
could use "union T" later.

> Actually, the "find_identifier" call inside function "check_designators" already
> calculate the correct offset. Only if we can pass that offset to
> "convert_ident()".

I actually tried that initially, and wanted to get rid of the whole
"check_designators()" vs "convert_designators()", but couldn't really
find a sane way to make that work. I'll look at it again, maybe I
missed something.

             LInus

Re: Designated initializers for fields in anonymous structs and unions

[Derek M Jones]

Linus,

> .. and in fact I think even without those things, you can just make
> the unnamed union have a type name, ie
>
>     struct S {
>        union T {
>            int a;
>        }
>     };
>
> the union T has a typename, but is a unnamed member of struct S. We
> could use "union T" later.

No such luck, not allowed by the C Standard.

6.7.2.1p13
"An unnamed member whose type specifier is a structure specifier with
no tag is called an anonymous structure; an unnamed member whose type
specifier is a union specifier with no tag is called an anonymous union"

But you can do this sort of thing when the type specifier is not anonymous.

-- 
Derek M. Jones                  tel: +44 (0) 1252 520 667
Knowledge Software Ltd          blog:shape-of-code.coding-guidelines.com
Software analysis               http://www.knosof.co.uk

Re: Designated initializers for fields in anonymous structs and unions

[Christopher Li]

On Sat, Aug 2, 2014 at 11:31 AM, Derek M Jones <derek@knosof.co.uk> wrote:
>> .. and in fact I think even without those things, you can just make
>> the unnamed union have a type name, ie
>>
>>     struct S {
>>        union T {
>>            int a;
>>        }
>>     };
>>
>> the union T has a typename, but is a unnamed member of struct S. We
>> could use "union T" later.
>
>
> No such luck, not allowed by the C Standard.
>
> 6.7.2.1p13
> "An unnamed member whose type specifier is a structure specifier with
> no tag is called an anonymous structure; an unnamed member whose type
> specifier is a union specifier with no tag is called an anonymous union"
>
> But you can do this sort of thing when the type specifier is not anonymous.

That is what I recall as well, not part of stander C. If you enable
the "-fplan9-extensions",
Linus' example can pass.

Chris

Re: Designated initializers for fields in anonymous structs and unions

[Linus Torvalds]

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

On Sat, Aug 2, 2014 at 11:10 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
>> Actually, the "find_identifier" call inside function "check_designators" already
>> calculate the correct offset. Only if we can pass that offset to
>> "convert_ident()".
>
> I actually tried that initially, and wanted to get rid of the whole
> "check_designators()" vs "convert_designators()", but couldn't really
> find a sane way to make that work. I'll look at it again, maybe I
> missed something.

No, I didn't miss anything.

The "trivial fix" is to just save off the offset in check_designators
(add a new field to the "EXPR_IDENTIFIER" part of "struct expression"
and then pick up that offset in "convert_ident()"

However, that has exactly the same issue with the whole fake
EXPR_IDENTIFIER created by "next_designators()". Now we need to
initialize the offset field there too. And, for the same reason as
before, the field that "next_designator()" picks may not *have* an
identifier, because the next designator may in fact be an anonymous
union.

Anyway, with all that said, maybe this really confusing and hacky
patch would work. It passes my tests. The magic offset calculation in
next_designators() has a big comment about what the heck it is doing,
and otherwise it's a fairly straightforward case of "save offset from
check_designators() time to be used by convert_ident()".

My stupid test-case is this incredibly hacky thing:

    struct A {
        int aa;
        int bb;
    };

    struct S {
        int a;
        union {
            int b;
            int c;
        } u[10];
        int dummy;
        union {
            int d;
            int e;
        };
    };

    int fn(void)
    {
        struct S s = {
            .a = 1,
            .u[2].b = 2,
            .dummy = 1,
             { 3 }
        };
        return s.dummy*1000 + s.d*100 + s.u[2].b*10 + s.a; // 1321
    }

where I use "./test-linearize" to verify that the initializer layout
matches the code generation layout (so that 'fn' function *should*
linearize to just a simple "ret.32 $1321", and with this patch it
does).

But I bet this misses some case. However, the current state wrt
initializer offsets really is very broken, so maybe it's ok.

Can anybody see anything wrong with it? It's reasonably simple. Every
place that initializes "expr->field" for an EXPR_IDENTIFIER now also
initializes "expr->offset".

              Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 2627 bytes --]

 evaluate.c   | 20 ++++++++++++++++++--
 expression.h |  1 +
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index 03992d03ae1d..31a6a49329b8 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2223,9 +2223,10 @@ static void convert_index(struct expression *e)
 static void convert_ident(struct expression *e)
 {
 	struct expression *child = e->ident_expression;
-	struct symbol *sym = e->field;
+	int offset = e->offset;
+
 	e->type = EXPR_POS;
-	e->init_offset = sym->offset;
+	e->init_offset = offset;
 	e->init_nr = 1;
 	e->init_expr = child;
 }
@@ -2277,6 +2278,7 @@ static struct expression *first_subobject(struct symbol *ctype, int class,
 		new = alloc_expression(e->pos, EXPR_IDENTIFIER);
 		new->ident_expression = e;
 		new->field = new->ctype = field;
+		new->offset = field->offset;
 	}
 	*v = new;
 	return new;
@@ -2327,6 +2329,7 @@ static struct expression *check_designators(struct expression *e,
 				err = "unknown field name in";
 				break;
 			}
+			e->offset = offset;
 			e->field = e->ctype = ctype;
 			last = e;
 			if (!e->ident_expression) {
@@ -2386,6 +2389,7 @@ static struct expression *next_designators(struct expression *old,
 	} else if (old->type == EXPR_IDENTIFIER) {
 		struct expression *copy;
 		struct symbol *field;
+		int offset = 0;
 
 		copy = next_designators(old->ident_expression,
 					old->ctype, e, v);
@@ -2397,6 +2401,17 @@ static struct expression *next_designators(struct expression *old,
 			}
 			copy = e;
 			*v = new = alloc_expression(e->pos, EXPR_IDENTIFIER);
+			/*
+			 * We can't necessarily trust "field->offset",
+			 * because the field might be in an anonymous
+			 * union, and the field offset is then the offset
+			 * within that union.
+			 *
+			 * The "old->offset - old->field->offset"
+			 * would be the offset of such an anonymous
+			 * union.
+			 */
+			offset = old->offset - old->field->offset;
 		} else {
 			field = old->field;
 			new = alloc_expression(e->pos, EXPR_IDENTIFIER);
@@ -2406,6 +2421,7 @@ static struct expression *next_designators(struct expression *old,
 		new->expr_ident = field->ident;
 		new->ident_expression = copy;
 		new->ctype = field;
+		new->offset = field->offset + offset;
 		convert_ident(old);
 	}
 	return new;
diff --git a/expression.h b/expression.h
index e31d14051e01..80b3be5f526f 100644
--- a/expression.h
+++ b/expression.h
@@ -149,6 +149,7 @@ struct expression {
 		struct expression_list *expr_list;
 		// EXPR_IDENTIFIER
 		struct /* ident_expr */ {
+			int offset;
 			struct ident *expr_ident;
 			struct symbol *field;
 			struct expression *ident_expression;

Re: Designated initializers for fields in anonymous structs and unions

[Linus Torvalds]

On Sat, Aug 2, 2014 at 12:53 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> My stupid test-case is this incredibly hacky thing:

.. actually, that test-case wasn't the one that showed the original
problem, it just ends up testing many of the other cases, notably the
"next_designator() creates that unnamed union case".

The test-case that shows the problem this patch fixes is one that
actually names the unnamed union entry (so instead of the "{ 3 }"
initializer, it uses ".d=3" to initialize the entry in the unnamed
union.

Just to clarify. See the difference in code generation by
"test-linearize" for those two cases (ie "{3}" vs ".d=3" in the
initializer).

          Linus

Re: Designated initializers for fields in anonymous structs and unions

[Christopher Li]

On Sat, Aug 2, 2014 at 12:53 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sat, Aug 2, 2014 at 11:10 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>>> Actually, the "find_identifier" call inside function "check_designators" already
>>> calculate the correct offset. Only if we can pass that offset to
>>> "convert_ident()".
>>
>> I actu
> My stupid test-case is this incredibly hacky thing:
>
>     struct A {
>         int aa;
>         int bb;
>     };
>
>     struct S {
>         int a;
>         union {
>             int b;
>             int c;
>         } u[10];
>         int dummy;
>         union {
>             int d;
>             int e;
>         };
>     };
>
>     int fn(void)
>     {
>         struct S s = {
>             .a = 1,
>             .u[2].b = 2,
>             .dummy = 1,
>              { 3 }
>         };
>         return s.dummy*1000 + s.d*100 + s.u[2].b*10 + s.a; // 1321
>     }
>
> where I use "./test-linearize" to verify that the initializer layout
> matches the code generation layout (so that 'fn' function *should*
> linearize to just a simple "ret.32 $1321", and with this patch it
> does).
>
> But I bet this misses some case. However, the current state wrt
> initializer offsets really is very broken, so maybe it's ok.
>
> Can anybody see anything wrong with it? It's reasonably simple. Every
> place that initializes "expr->field" for an EXPR_IDENTIFIER now also
> initializes "expr->offset".

I did not see any thing wrong with it. The only thing is it bump up the
expression struct size. Definitely better than what we have right now.

Chris