sparse handles int64_t type wrong

sparse handles int64_t type wrong

[Yura Pakhuchiy]

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

Hi,

sparse 0.2 produces following errors on attached code:
a.c:15:7: warning: incorrect type in argument 1 (different type sizes)
a.c:15:7:    expected int [long] [usertype] *baz
a.c:15:7:    got int *<noident>
a.c:13:10: warning: shift too big (32) for type int 

However I believe it should not. Please fix!

-- 
Thanks,
        Yura

[-- Attachment #2: a.c --]
[-- Type: text/x-csrc, Size: 161 bytes --]

#include <sys/types.h>

typedef int64_t s64;

void foo(s64 *baz)
{
}

int main(int argc, char *argv[])
{
	s64 bar;

	if (bar >> 32)
		;
	foo(&bar);
	return 0;
}

Re: sparse handles int64_t type wrong

[Linus Torvalds]

On Tue, 12 Dec 2006, Yura Pakhuchiy wrote:
>
> sparse 0.2 produces following errors on attached code:
> a.c:15:7: warning: incorrect type in argument 1 (different type sizes)
> a.c:15:7:    expected int [long] [usertype] *baz
> a.c:15:7:    got int *<noident>
> a.c:13:10: warning: shift too big (32) for type int 
> 
> However I believe it should not. Please fix!

Did you use "-m64" if you are doing this on an architecture with 64-bit 
/usr/include?

		Linus

Re: sparse handles int64_t type wrong

[Yura Pakhuchiy]

On Tue, 12 Dec 2006 at 11:44 -0800, Linus Torvalds wrote:
> 
> On Tue, 12 Dec 2006, Yura Pakhuchiy wrote:
> >
> > sparse 0.2 produces following errors on attached code:
> > a.c:15:7: warning: incorrect type in argument 1 (different type sizes)
> > a.c:15:7:    expected int [long] [usertype] *baz
> > a.c:15:7:    got int *<noident>
> > a.c:13:10: warning: shift too big (32) for type int 
> > 
> > However I believe it should not. Please fix!
> 
> Did you use "-m64" if you are doing this on an architecture with 64-bit 
> /usr/include?

I use 32bit machine. But I tried to append -m64 anyway and this does not
helps.

-- 
Best regards,
        Yura

Re: sparse handles int64_t type wrong

[Linus Torvalds]

On Tue, 12 Dec 2006, Linus Torvalds wrote:
> 
> Did you use "-m64" if you are doing this on an architecture with 64-bit 
> /usr/include?

Never mind, that's not it.

The problem is that the standard headers use _this_ for "int64_t":

	typedef int int64_t __attribute__ ((__mode__ (__DI__)));

and sparse just says "ok, int64_t is an 'int'"

The "__attribute__((__mode__()))" thing has always been a quick hack - 
sparse actually tries to parse it, but not very well.

If you use 

	typedef long long s64;

sparse gets it right.

In fact, sparse even gets it right if you do

	typedef int __attribute__((__mode__(__DI__))) int64_t;

because then the attribute gets attached to the underlying type, before it 
gets bound to the typedef.

I'll take a look if I can fix typedef to accept the crapola __attibute__ 
syntax.

Putting the attributes at the end really _does_ suck. It's as if you were 
to write

	typedef void * const_ptr_t const;

and that obviously isn't supposed to work. Why gcc thinks attributes can 
go at the end will never be clear to me.

Gcc attributes suck. Some gcc extensions really were thought out really 
really badly.

		Linus

Re: sparse handles int64_t type wrong

[Yura Pakhuchiy]

On Tue, 12 Dec 2006 at 12:02 -0800, Linus Torvalds wrote:
> On Tue, 12 Dec 2006, Linus Torvalds wrote:
> > 
> > Did you use "-m64" if you are doing this on an architecture with 64-bit 
> > /usr/include?
> 
> Never mind, that's not it.
> 
> The problem is that the standard headers use _this_ for "int64_t":
> 
> 	typedef int int64_t __attribute__ ((__mode__ (__DI__)));
> 
> and sparse just says "ok, int64_t is an 'int'"
> 
> The "__attribute__((__mode__()))" thing has always been a quick hack - 
> sparse actually tries to parse it, but not very well.
> 
> If you use 
> 
> 	typedef long long s64;
> 
> sparse gets it right.
> 
> In fact, sparse even gets it right if you do
> 
> 	typedef int __attribute__((__mode__(__DI__))) int64_t;
> 
> because then the attribute gets attached to the underlying type, before it 
> gets bound to the typedef.

Great! This works for me. So I will probably use
	#ifdef __CHECKER__
	typedef int __attribute__((__mode__(__DI__))) int64_t;
	#endif
until it will be fixed in sparse.

> I'll take a look if I can fix typedef to accept the crapola __attibute__ 
> syntax.
> 
> Putting the attributes at the end really _does_ suck. It's as if you were 
> to write
> 
> 	typedef void * const_ptr_t const;
> 
> and that obviously isn't supposed to work. Why gcc thinks attributes can 
> go at the end will never be clear to me.
> 
> Gcc attributes suck. Some gcc extensions really were thought out really 
> really badly.

Thanks for quick reply and explanations!

-- 
Best regards,
        Yura

[PATCH][RFC] Re: sparse handles int64_t type wrong

[Christopher Li]

I took a stab at it.  It seems that direct_declarator() does handle
attributes, but it does not change the ctype to the declarator.
It is kind of awkward that the type is already build when it hit
the attribute in the end. It need to go back to overwrite
the already parsed type.

I have this patch seems make the test case happy. I am not sure
this is correct or not.
 
Chris
 

Index: sparse/parse.c
===================================================================
--- sparse.orig/parse.c	2006-12-12 21:12:29.000000000 -0800
+++ sparse/parse.c	2006-12-12 21:12:47.000000000 -0800
@@ -837,6 +837,10 @@ static struct token *handle_attributes(s
 			struct ctype thistype = { 0, };
 			token = attribute_specifier(token->next, &thistype);
 			apply_ctype(token->pos, &thistype, ctype);
+			if (is_int_type(ctype->base_type) && ctype->modifiers & MOD_SPECIFIER) {
+				ctype->base_type = ctype_integer(ctype->modifiers);
+				ctype->modifiers &= ~MOD_SPECIFIER;
+			}
 			continue;
 		}
 		if (match_idents(token, &asm_ident, &__asm_ident, &__asm___ident)) {
Index: sparse/show-parse.c
===================================================================
--- sparse.orig/show-parse.c	2006-12-12 21:12:29.000000000 -0800
+++ sparse/show-parse.c	2006-12-12 21:12:47.000000000 -0800
@@ -98,7 +98,7 @@ const char *modifier_string(unsigned lon
 	const char *res,**ptr, *names[] = {
 		"auto", "register", "static", "extern",
 		"const", "volatile", "[signed]", "[unsigned]",
-		"[char]", "[short]", "[long]", "[long]",
+		"[char]", "[short]", "[long]", "[long long]",
 		"[typdef]", "[structof]", "[unionof]", "[enum]",
 		"[typeof]", "[attribute]", "inline", "[addressable]",
 		"[nocast]", "[noderef]", "[accessed]", "[toplevel]",
Index: sparse/test-parsing.c
===================================================================

Re: [PATCH][RFC] Re: sparse handles int64_t type wrong

[Linus Torvalds]

On Tue, 12 Dec 2006, Christopher Li wrote:
>
> I took a stab at it.  It seems that direct_declarator() does handle
> attributes, but it does not change the ctype to the declarator.
> It is kind of awkward that the type is already build when it hit
> the attribute in the end. It need to go back to overwrite
> the already parsed type.

Yes. I actually tried to fix this by moving the type "finalization" part 
later, but it got really really nasty.

> I have this patch seems make the test case happy. I am not sure
> this is correct or not.

It looks correct to me, although not wonderfully pretty. But it's simpler 
than the "don't do the int/fp type finalization until later" that I 
couldn't even get to work (not that I spent a lot of time on it, but 
still)

		Linus

Re: [PATCH][RFC] Re: sparse handles int64_t type wrong

[Christopher Li]

On Wed, Dec 13, 2006 at 07:36:20AM -0800, Linus Torvalds wrote:
> 
> It looks correct to me, although not wonderfully pretty. But it's simpler 
> than the "don't do the int/fp type finalization until later" that I 
> couldn't even get to work (not that I spent a lot of time on it, but 
> still)

Do you mean some thing like this?  I might leave some abstract ctype
in the tree, it is hard to find all all the user of ctype before
it get finalized.  It seems work with the test case in my hand.

Chris

Index: sparse/parse.c
===================================================================
--- sparse.orig/parse.c	2006-12-13 16:10:32.000000000 -0800
+++ sparse/parse.c	2006-12-13 16:15:42.000000000 -0800
@@ -70,12 +70,43 @@ struct statement *alloc_statement(struct
 
 static struct token *struct_declaration_list(struct token *token, struct symbol_list **list);
 
+static int apply_modifiers(struct position pos, struct ctype *ctype)
+{
+	/* Turn the "virtual types" into real types with real sizes etc */
+
+	if (ctype->base_type == &int_type) {
+		ctype->base_type = ctype_integer(ctype->modifiers);
+		ctype->modifiers &= ~MOD_SPECIFIER;
+	} else if (ctype->base_type == &fp_type) {
+		ctype->base_type = ctype_fp(ctype->modifiers);
+		ctype->modifiers &= ~MOD_SPECIFIER;
+	}
+
+	if (ctype->modifiers & MOD_BITWISE) {
+		struct symbol *type;
+		ctype->modifiers &= ~(MOD_BITWISE | MOD_SPECIFIER);
+		if (!is_int_type(ctype->base_type)) {
+			sparse_error(pos, "invalid modifier");
+			return 1;
+		}
+		type = alloc_symbol(pos, SYM_BASETYPE);
+		*type = *ctype->base_type;
+		type->ctype.base_type = ctype->base_type;
+		type->type = SYM_RESTRICT;
+		type->ctype.modifiers &= ~MOD_SPECIFIER;
+		ctype->base_type = type;
+		create_fouled(type);
+	}
+	return 0;
+}
+
 static struct symbol * indirect(struct position pos, struct ctype *ctype, int type)
 {
 	struct symbol *sym = alloc_symbol(pos, type);
 
 	sym->ctype.base_type = ctype->base_type;
 	sym->ctype.modifiers = ctype->modifiers & ~MOD_STORAGE;
+	apply_modifiers(pos, &sym->ctype);
 
 	ctype->base_type = sym;
 	ctype->modifiers &= MOD_STORAGE;
@@ -724,7 +755,6 @@ static void check_modifiers(struct posit
 		     modifier_string (wrong));
 }
 
-
 static struct token *declaration_specifiers(struct token *next, struct ctype *ctype, int qual)
 {
 	struct token *token;
@@ -778,7 +808,6 @@ static struct token *declaration_specifi
 		apply_ctype(token->pos, &thistype, ctype);
 	}
 
-	/* Turn the "virtual types" into real types with real sizes etc */
 	if (!ctype->base_type) {
 		struct symbol *base = &incomplete_ctype;
 
@@ -791,28 +820,6 @@ static struct token *declaration_specifi
 			base = &int_type;
 		ctype->base_type = base;
 	}
-	if (ctype->base_type == &int_type) {
-		ctype->base_type = ctype_integer(ctype->modifiers);
-		ctype->modifiers &= ~MOD_SPECIFIER;
-	} else if (ctype->base_type == &fp_type) {
-		ctype->base_type = ctype_fp(ctype->modifiers);
-		ctype->modifiers &= ~MOD_SPECIFIER;
-	}
-	if (ctype->modifiers & MOD_BITWISE) {
-		struct symbol *type;
-		ctype->modifiers &= ~(MOD_BITWISE | MOD_SPECIFIER);
-		if (!is_int_type(ctype->base_type)) {
-			sparse_error(token->pos, "invalid modifier");
-			return token;
-		}
-		type = alloc_symbol(token->pos, SYM_BASETYPE);
-		*type = *ctype->base_type;
-		type->ctype.base_type = ctype->base_type;
-		type->type = SYM_RESTRICT;
-		type->ctype.modifiers &= ~MOD_SPECIFIER;
-		ctype->base_type = type;
-		create_fouled(type);
-	}
 	return token;
 }
 
@@ -1001,6 +1008,7 @@ static struct token *declaration_list(st
 			token = handle_bitfield(token, decl);
 			token = handle_attributes(token, &decl->ctype);
 		}
+		apply_modifiers(token->pos, &decl->ctype);
 		add_symbol(list, decl);
 		if (!match_op(token, ','))
 			break;
@@ -1034,6 +1042,7 @@ static struct token *parameter_declarati
 	*tree = sym;
 	token = declarator(token, sym, &ident);
 	sym->ident = ident;
+	apply_modifiers(token->pos, &sym->ctype);
 	return token;
 }
 
@@ -1042,7 +1051,9 @@ struct token *typename(struct token *tok
 	struct symbol *sym = alloc_symbol(token->pos, SYM_NODE);
 	*p = sym;
 	token = declaration_specifiers(token, &sym->ctype, 0);
-	return declarator(token, sym, NULL);
+	token = declarator(token, sym, NULL);
+	apply_modifiers(token->pos, &sym->ctype);
+	return token;
 }
 
 static struct token *expression_statement(struct token *token, struct expression **tree)
@@ -1770,6 +1781,8 @@ struct token *external_declaration(struc
 	decl = alloc_symbol(token->pos, SYM_NODE);
 	decl->ctype = ctype;
 	token = declarator(token, decl, &ident);
+	apply_modifiers(token->pos, &decl->ctype);
+	apply_modifiers(token->pos, &ctype);
 
 	/* Just a type declaration? */
 	if (!ident)
@@ -1830,6 +1843,7 @@ struct token *external_declaration(struc
 		decl->ctype = ctype;
 		token = declaration_specifiers(token, &decl->ctype, 1);
 		token = declarator(token, decl, &ident);
+		apply_modifiers(token->pos, &decl->ctype);
 		if (!ident) {
 			sparse_error(token->pos, "expected identifier name in type definition");
 			return token;
Index: sparse/show-parse.c
===================================================================
--- sparse.orig/show-parse.c	2006-12-13 16:10:32.000000000 -0800
+++ sparse/show-parse.c	2006-12-13 16:10:36.000000000 -0800
@@ -98,7 +98,7 @@ const char *modifier_string(unsigned lon
 	const char *res,**ptr, *names[] = {
 		"auto", "register", "static", "extern",
 		"const", "volatile", "[signed]", "[unsigned]",
-		"[char]", "[short]", "[long]", "[long]",
+		"[char]", "[short]", "[long]", "[long long]",
 		"[typdef]", "[structof]", "[unionof]", "[enum]",
 		"[typeof]", "[attribute]", "inline", "[addressable]",
 		"[nocast]", "[noderef]", "[accessed]", "[toplevel]",
Index: sparse/test-parsing.c
===================================================================

Re: [PATCH][RFC] Re: sparse handles int64_t type wrong

[Linus Torvalds]

On Wed, 13 Dec 2006, Christopher Li wrote:
> 
> Do you mean some thing like this?  I might leave some abstract ctype
> in the tree, it is hard to find all all the user of ctype before
> it get finalized.  It seems work with the test case in my hand.

Yes, this is I think the proper way to do it. 

		Linus

Re: [PATCH][RFC] Re: sparse handles int64_t type wrong

[Christopher Li]

Never mind my previous patch. It is too simple and wrong.
It can't really tell the C type is abstract or not because
int_ctype has base_type to int_type as well. It ends up applying
the modifiers to int_ctype.

You are right about it is very nasty to delay the finalization
of the C type. Back to drawing board.

Chris

On Wed, Dec 13, 2006 at 04:57:59PM -0800, Linus Torvalds wrote:
> 
> 
> On Wed, 13 Dec 2006, Christopher Li wrote:
> > 
> > Do you mean some thing like this?  I might leave some abstract ctype
> > in the tree, it is hard to find all all the user of ctype before
> > it get finalized.  It seems work with the test case in my hand.
> 
> Yes, this is I think the proper way to do it. 
> 
> 		Linus