[RFC] show_type() format problems

[RFC] show_type() format problems

[Al Viro]

int __attribute__((address_space(1))) **p;
int *__attribute__((address_space(1))) *q;

void foo(void)
{
        p = q;
}

quite predictably gives a warning.  The contents of that warning,
however, is somewhat unfortunate:

test.c:6:4: warning: incorrect type in assignment (different address spaces)
test.c:6:4:    expected int **[addressable] [toplevel] p<asn:1>
test.c:6:4:    got int **[addressable] [toplevel] q<asn:1>

The reason is simple: we put <asn:...> *after* the identifier.  *, of
course, goes before it.  So when we have a pointer to pointer, there's
no way to tell which of them had brought address_space.

Do we want to keep the current behaviour?  It's obviously not nice -
especially when we get warnings like one above.

We also can't tell pointer to array from array of pointers.  Does anybody
object against making it look more like C declarations?  I.e. put <asn:...>
together with modifiers and at least add parens when needed?

Believe me, I do realize that it will change build logs.  I probably have
more of those than just about anybody else (several years worth of sparse
runs on the kernel for couple dozens of targets).  And yes, it'll hurt.
I don't see a better alternative, though; we might be able to tweak the
output to deal with ambiguities and still keep the same results for (very)
simple cases, but if we are tweaking it at all we really ought to go for
something recognizable for normal C programmers...

Comments?

Re: [RFC] show_type() format problems

[Josh Triplett]

On Thu, 2007-07-12 at 10:14 +0100, Al Viro wrote:
> int __attribute__((address_space(1))) **p;
> int *__attribute__((address_space(1))) *q;
> 
> void foo(void)
> {
>         p = q;
> }
> 
> quite predictably gives a warning.  The contents of that warning,
> however, is somewhat unfortunate:
> 
> test.c:6:4: warning: incorrect type in assignment (different address spaces)
> test.c:6:4:    expected int **[addressable] [toplevel] p<asn:1>
> test.c:6:4:    got int **[addressable] [toplevel] q<asn:1>
> 
> The reason is simple: we put <asn:...> *after* the identifier.  *, of
> course, goes before it.  So when we have a pointer to pointer, there's
> no way to tell which of them had brought address_space.
> 
> Do we want to keep the current behaviour?  It's obviously not nice -
> especially when we get warnings like one above.
>
> We also can't tell pointer to array from array of pointers.  Does anybody
> object against making it look more like C declarations?  I.e. put <asn:...>
> together with modifiers and at least add parens when needed?

Please do go ahead and change the output.  I'd love for show_type to
output something as close to a parsable C type as possible.

> Believe me, I do realize that it will change build logs.  I probably have
> more of those than just about anybody else (several years worth of sparse
> runs on the kernel for couple dozens of targets).  And yes, it'll hurt.
> I don't see a better alternative, though; we might be able to tweak the
> output to deal with ambiguities and still keep the same results for (very)
> simple cases, but if we are tweaking it at all we really ought to go for
> something recognizable for normal C programmers...

I do understand the concern, but I think that consistency of build logs
matters far less than sanity for the users of *current* Sparse.  Let's
no go making purely gratuitous output changes, but here we have a good
reason to change the output.

- Josh Triplett

Re: [RFC] show_type() format problems

[Al Viro]

On Thu, Jul 12, 2007 at 09:22:10AM -0700, Josh Triplett wrote:
> Please do go ahead and change the output.  I'd love for show_type to
> output something as close to a parsable C type as possible.

How does below sound?  At least it makes output unambiguous and much
closer to normal C syntax.  We could do better (e.g. lift the things
like "extern", etc. in front of everything else), but that's a separate
problem...

diff --git a/show-parse.c b/show-parse.c
index aae8b74..9a1f796 100644
--- a/show-parse.c
+++ b/show-parse.c
@@ -226,11 +226,28 @@ const char *builtin_ctypename(struct ctype *ctype)
 	return NULL;
 }
 
-static void do_show_type(struct symbol *sym, struct type_name *name)
+static void do_show_type(struct symbol *sym, struct type_name *name,
+			 unsigned long mod, int as, int was_ptr)
 {
-	int modlen;
-	const char *mod;
 	const char *typename;
+	int is_ptr = was_ptr;
+
+	if (!sym || (sym->type != SYM_NODE && sym->type != SYM_ARRAY &&
+		     sym->type != SYM_BITFIELD)) {
+		const char *s;
+		size_t len;
+
+		if (as)
+			prepend(name, "<asn:%d>", as);
+
+		s = modifier_string(mod);
+		len = strlen(s);
+		name->start -= len;    
+		memcpy(name->start, s, len);  
+		mod = 0;
+		as = 0;
+	}
+
 	if (!sym)
 		return;
 
@@ -246,9 +263,14 @@ static void do_show_type(struct symbol *sym, struct type_name *name)
 	switch (sym->type) {
 	case SYM_PTR:
 		prepend(name, "*");
+		mod = sym->ctype.modifiers;
+		as = sym->ctype.as;
+		is_ptr = 1;
 		break;
 	case SYM_FN:
-		prepend(name, "( ");
+		if (was_ptr)
+			prepend(name, "( ");
+		is_ptr = 0;
 		break;
 	case SYM_STRUCT:
 		prepend(name, "struct %s ", show_ident(sym->ident));
@@ -264,9 +286,13 @@ static void do_show_type(struct symbol *sym, struct type_name *name)
 
 	case SYM_NODE:
 		append(name, "%s", show_ident(sym->ident));
+		mod |= sym->ctype.modifiers;
+		as |= sym->ctype.as;
 		break;
 
 	case SYM_BITFIELD:
+		mod |= sym->ctype.modifiers;
+		as |= sym->ctype.as;
 		append(name, ":%d", sym->bit_size);
 		break;
 
@@ -275,6 +301,11 @@ static void do_show_type(struct symbol *sym, struct type_name *name)
 		break;
 
 	case SYM_ARRAY:
+		mod |= sym->ctype.modifiers;
+		as |= sym->ctype.as;
+		if (was_ptr)
+			prepend(name, "( ");
+		is_ptr = 0;
 		break;
 
 	case SYM_RESTRICT:
@@ -288,26 +319,21 @@ static void do_show_type(struct symbol *sym, struct type_name *name)
 		return;
 	}
 
-	mod = modifier_string(sym->ctype.modifiers);
-	modlen = strlen(mod);
-	name->start -= modlen;    
-	memcpy(name->start, mod, modlen);  
-
-	do_show_type(sym->ctype.base_type, name);
-
-	/* Postpend */
-	if (sym->ctype.as)
-		append(name, "<asn:%d>", sym->ctype.as);
+	do_show_type(sym->ctype.base_type, name, mod, as, is_ptr);
 
 	switch (sym->type) {
 	case SYM_PTR:
-		return; 
+		return;
 
 	case SYM_FN:
-		append(name, " )( ... )");
+		if (was_ptr)
+			append(name, " )");
+		append(name, "( ... )");
 		return;
 
 	case SYM_ARRAY:
+		if (was_ptr)
+			append(name, " )");
 		append(name, "[%lld]", get_expression_value(sym->array_size));
 		return;
 
@@ -330,7 +356,7 @@ void show_type(struct symbol *sym)
 	struct type_name name;
 
 	name.start = name.end = array+100;
-	do_show_type(sym, &name);
+	do_show_type(sym, &name, 0, 0, 0);
 	*name.end = 0;
 	printf("%s", name.start);
 }
@@ -341,7 +367,7 @@ const char *show_typename(struct symbol *sym)
 	struct type_name name;
 
 	name.start = name.end = array+100;
-	do_show_type(sym, &name);
+	do_show_type(sym, &name, 0, 0, 0);
 	*name.end = 0;
 	return name.start;
 }