[RFC PATCH] Fix -Wtypesign

[RFC PATCH] Fix -Wtypesign

[Pavel Roskin]

Hello!

The current sparse has problems with the -Wtypesign flag.  If causes
bogus warnings.  Take for example following test.c:

static unsigned char foobar(void);
static unsigned char foobar(void)
{
	return 0;
}

$ sparse -Wtypesign test.c
test.c:2:22: error: symbol 'foobar' redeclared with different type
(originally declared at test.c:1) - different signedness

Debugging shows that the function definition doesn't have the
signed/unsigned modifiers, unlike the function declaration:

2412                    typediff = type_difference(sym, next, 0, 0);
(gdb) p/ sym->ctype.modifiers
$1 = 0x800004
(gdb) p/ next->ctype.modifiers
$2 = 0x800084

0x80 is MOD_UNSIGNED.

The signedness modifiers are set in evaluate_symbol(), which hasn't been
called for sym yet.  I have devised following patch, that appears to fix
the problem:

---
Fix -Wtypesign

Run evaluate_symbol() before check_duplicates() so that the signedness
of the token is known by the time the token is compared with other
tokens.

Signed-off-by: Pavel Roskin <proski@gnu.org>
---

 evaluate.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index 42005eb..2561f38 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2473,8 +2473,8 @@ void evaluate_symbol_list(struct symbol_
 	struct symbol *sym;
 
 	FOR_EACH_PTR(list, sym) {
-		check_duplicates(sym);
 		evaluate_symbol(sym);
+		check_duplicates(sym);
 	} END_FOR_EACH_PTR(sym);
 }

I tested -Wtypesign on some Linux drivers.  It found some real sign
mismatches and no bogus warnings.

Still, I haven't seen sparse code until today, so the patch should be
evaluated carefully.

-- 
Regards,
Pavel Roskin

Re: [RFC PATCH] Fix -Wtypesign

[Linus Torvalds]

On Thu, 13 Jul 2006, Pavel Roskin wrote:
>
> Fix -Wtypesign
> 
> Run evaluate_symbol() before check_duplicates() so that the signedness
> of the token is known by the time the token is compared with other
> tokens.
> 
> Signed-off-by: Pavel Roskin <proski@gnu.org>
> ---
> 
>  evaluate.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/evaluate.c b/evaluate.c
> index 42005eb..2561f38 100644
> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -2473,8 +2473,8 @@ void evaluate_symbol_list(struct symbol_
>  	struct symbol *sym;
>  
>  	FOR_EACH_PTR(list, sym) {
> -		check_duplicates(sym);
>  		evaluate_symbol(sym);
> +		check_duplicates(sym);


That, on the face of it, looks "Obviously Correct(tm)".

I'm wondering if I had some reason for doing them in what is obviously the 
wrong order, which worries me a bit, but quite frankly, the most likely 
reasons is just that it was a thinko.

(Long long ago, I did "check_duplicates()" from _within_ evaluation, 
which had serious recursion issues, so there's been confusion here 
before).

		Linus