All of lore.kernel.org
 help / color / mirror / Atom feed
* Sparse crash when mixing int and enum in ternary operator
@ 2010-03-09  1:24 Pavel Roskin
  2010-03-09  5:43 ` Christopher Li
  0 siblings, 1 reply; 30+ messages in thread
From: Pavel Roskin @ 2010-03-09  1:24 UTC (permalink / raw)
  To: linux-sparse

Hello!

Sparse crashed when checking drivers/net/wireless/ath/ath9k/gpio.c in
Linux.  I could reduce the crash to the following simple program:

enum kind {
    GOOD
};
static void foo(enum kind k)
{
}
static void bar(int ok, int k)
{
    foo(ok ? GOOD : k);
}

Here's the gdb trace:

Starting program: /home/proski/bin/sparse kind.c
kind.c:9:12: warning: mixing different enum types

Program received signal SIGSEGV, Segmentation fault.
0x00000000004328bf in do_show_type (sym=0x3, name=0x7fffffffdda0) at show-parse.c:242
242             if (!sym || (sym->type != SYM_NODE && sym->type != SYM_ARRAY &&
(gdb) p sym
$1 = (struct symbol *) 0x3
(gdb) up
#1  0x0000000000432e95 in show_typename (sym=0x7ffff7f99190) at show-parse.c:379
379             do_show_type(sym, &name);
(gdb) p sym
$2 = (struct symbol *) 0x7ffff7f99190
(gdb) p *sym 
$3 = {type = SYM_ENUM, namespace = NS_NONE, used = 0 '\0', attr = 0 '\0', 
  enum_member = 0 '\0', bound = 0 '\0', pos = {type = 42, stream = 0, newline = 0, 
    whitespace = 0, pos = 0, line = 90177666, noexpand = 0}, endpos = {type = 9, stream = 0, 
    newline = 0, whitespace = 0, pos = 0, line = 2012974384, noexpand = 1}, ident = 0x0, 
  next_id = 0x7ffff7f99250, replace = 0x7ffff7fdaa20, scope = 0x0, {same_symbol = 0x0, 
    next_subobject = 0x0}, op = 0x3, {{expansion = 0x901700082, arglist = 0x668380, 
      used_in = 0x0}, {handler = 0x901700082, normal = 6718336}, {offset = 38678823042, 
      bit_size = 6718336, bit_offset = 0, arg_count = 0, variadic = 0, initialized = 0, 
      examined = 0, expanding = 0, evaluated = 0, string = 0, designated_init = 0, 
      array_size = 0x0, ctype = {modifiers = 140737353844496, alignment = 140737353984512, 
        contexts = 0x0, as = 0, base_type = 0x3}, arguments = 0x902400082, stmt = 0x668380, 
      symbol_list = 0x0, inline_stmt = 0x7ffff7fb8c50, inline_symbol_list = 0x7ffff7fdaa60, 
      initializer = 0x0, ep = 0x0, value = 3, definition = 0x905600082}}, {
    bb_target = 0x668380, aux = 0x668380, {kind = -128 '\200', visited = 1 '\1'}}, 
  pseudo = 0x0}
(gdb)

sym->ctype.base_type is 0x3, so sym becomes 0x3 after the first
iteration in do_show_type().

-- 
Regards,
Pavel Roskin

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-09  1:24 Sparse crash when mixing int and enum in ternary operator Pavel Roskin
@ 2010-03-09  5:43 ` Christopher Li
  2010-03-09 13:46   ` Kamil Dudka
  0 siblings, 1 reply; 30+ messages in thread
From: Christopher Li @ 2010-03-09  5:43 UTC (permalink / raw)
  To: Pavel Roskin; +Cc: linux-sparse, Kamil Dudka

On Mon, Mar 8, 2010 at 5:24 PM, Pavel Roskin <proski@gnu.org> wrote:
> Hello!
>
> Sparse crashed when checking drivers/net/wireless/ath/ath9k/gpio.c in
> Linux.  I could reduce the crash to the following simple program:
>
> enum kind {
>    GOOD
> };
> static void foo(enum kind k)
> {
> }
> static void bar(int ok, int k)
> {
>    foo(ok ? GOOD : k);
> }
>
Confirm. This is cause by the recent enum-warning patch. Without it the sparse
runs fine.

Let me see if this is some thing easy to fix.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-09  5:43 ` Christopher Li
@ 2010-03-09 13:46   ` Kamil Dudka
  2010-03-09 18:26     ` Pavel Roskin
  2010-03-09 18:35     ` Pavel Roskin
  0 siblings, 2 replies; 30+ messages in thread
From: Kamil Dudka @ 2010-03-09 13:46 UTC (permalink / raw)
  To: Christopher Li; +Cc: Pavel Roskin, linux-sparse

[-- Attachment #1: Type: Text/Plain, Size: 838 bytes --]

On Tue March 9 2010 06:43:09 Christopher Li wrote:
> On Mon, Mar 8, 2010 at 5:24 PM, Pavel Roskin <proski@gnu.org> wrote:
> > Hello!
> > 
> > Sparse crashed when checking drivers/net/wireless/ath/ath9k/gpio.c in
> > Linux.  I could reduce the crash to the following simple program:
> > 
> > enum kind {
> >    GOOD
> > };
> > static void foo(enum kind k)
> > {
> > }
> > static void bar(int ok, int k)
> > {
> >    foo(ok ? GOOD : k);
> > }
> 
> Confirm. This is cause by the recent enum-warning patch. Without it the
> sparse runs fine.

Thank both of you for tacking down the bug!

> Let me see if this is some thing easy to fix.

It's easy to fix as soon as I understand how EXPR_CONDITIONAL/EXPR_SELECT
work in sparse.  A fix from scratch is attached, but I'll need more time
for testing it and to write some extra test-cases.

Kamil

[-- Attachment #2: sparse-enum.patch --]
[-- Type: text/x-patch, Size: 994 bytes --]

diff --git a/evaluate.c b/evaluate.c
index d3d5e6f..214b2b7 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -327,13 +327,28 @@ warn_for_int_to_enum_conversion (struct expression *expr, struct symbol *typeb)
 }
 
 static void
-warn_for_enum_conversions(struct expression *expr, struct symbol *type)
+do_warn_for_enum_conversions(struct expression *expr, struct symbol *type)
 {
 	warn_for_different_enum_types (expr, type);
 	warn_for_enum_to_int_conversion (expr, type);
 	warn_for_int_to_enum_conversion (expr, type);
 }
 
+static void
+warn_for_enum_conversions(struct expression *expr, struct symbol *type)
+{
+	switch (expr->type) {
+		case EXPR_CONDITIONAL:
+		case EXPR_SELECT:
+			do_warn_for_enum_conversions(expr->cond_true, type);
+			do_warn_for_enum_conversions(expr->cond_false, type);
+			break;
+
+		default:
+			do_warn_for_enum_conversions(expr, type);
+	}
+}
+
 /*
  * This gets called for implicit casts in assignments and
  * integer promotion. We often want to try to move the

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-09 13:46   ` Kamil Dudka
@ 2010-03-09 18:26     ` Pavel Roskin
  2010-03-09 18:35     ` Pavel Roskin
  1 sibling, 0 replies; 30+ messages in thread
From: Pavel Roskin @ 2010-03-09 18:26 UTC (permalink / raw)
  To: Kamil Dudka; +Cc: Christopher Li, linux-sparse

On Tue, 2010-03-09 at 14:46 +0100, Kamil Dudka wrote:
> On Tue March 9 2010 06:43:09 Christopher Li wrote:
> > On Mon, Mar 8, 2010 at 5:24 PM, Pavel Roskin <proski@gnu.org> wrote:
> > > Hello!
> > > 
> > > Sparse crashed when checking drivers/net/wireless/ath/ath9k/gpio.c in
> > > Linux.  I could reduce the crash to the following simple program:
> > > 
> > > enum kind {
> > >    GOOD
> > > };
> > > static void foo(enum kind k)
> > > {
> > > }
> > > static void bar(int ok, int k)
> > > {
> > >    foo(ok ? GOOD : k);
> > > }
> > 
> > Confirm. This is cause by the recent enum-warning patch. Without it the
> > sparse runs fine.
> 
> Thank both of you for tacking down the bug!
> 
> > Let me see if this is some thing easy to fix.
> 
> It's easy to fix as soon as I understand how EXPR_CONDITIONAL/EXPR_SELECT
> work in sparse.  A fix from scratch is attached, but I'll need more time
> for testing it and to write some extra test-cases.

Now sparse crashes on crypto/tcrypt.c, where it didn't crash before:

Core was generated by `sparse -D__linux__ -Dlinux -D__STDC__ -Dunix
-D__unix__ -Wbitwise -Wno-return-v'.
Program terminated with signal 11, Segmentation fault.
#0  0x0000000000408131 in warn_for_different_enum_types (expr=0x0,
typeb=0x7f3b69632850) at evaluate.c:248
248             struct position pos = expr->pos;
(gdb) p expr
$1 = (struct expression *) 0x0
(gdb) where
#0  0x0000000000408131 in warn_for_different_enum_types (expr=0x0,
typeb=0x7f3b69632850) at evaluate.c:248
#1  0x000000000040843c in do_warn_for_enum_conversions (expr=0x0,
type=0x7f3b69632850) at evaluate.c:332
#2  0x000000000040849d in warn_for_enum_conversions
(expr=0x7f3b690378d0, type=0x7f3b69632850) at evaluate.c:343
#3  0x00000000004084ee in cast_to (old=0x7f3b690378d0,
type=0x7f3b69632850) at evaluate.c:363
#4  0x000000000040b035 in compatible_assignment_types
(expr=0x7f3b690378d0, target=0x7f3b69632850, rp=0x7f3b6903bd38,
where=0x664a00 "argument 3") at evaluate.c:1465
#5  0x000000000040ccec in evaluate_arguments (f=0x7f3b696323f0,
fn=0x7f3b696324d0, head=0x7f3b6903bd10) at evaluate.c:2221
#6  0x000000000040e82f in evaluate_call (expr=0x7f3b690377d0) at
evaluate.c:2888
#7  0x000000000040efc5 in evaluate_expression (expr=0x7f3b690377d0) at
evaluate.c:3059
#8  0x0000000000409a07 in evaluate_conditional (expr=0x7f3b690377d0,
iterator=0) at evaluate.c:937
#9  0x000000000040a3ef in evaluate_conditional_expression
(expr=0x7f3b69037950) at evaluate.c:1176
#10 0x000000000040efd6 in evaluate_expression (expr=0x7f3b69037950) at
evaluate.c:3062
#11 0x000000000040f3ff in evaluate_return_expression
(stmt=0x7f3b69044980) at evaluate.c:3181
#12 0x000000000040fd5b in evaluate_statement (stmt=0x7f3b69044980) at
evaluate.c:3404
#13 0x000000000040fe3c in evaluate_statement (stmt=0x7f3b69044930) at
evaluate.c:3426
#14 0x000000000040f343 in evaluate_symbol (sym=0x7f3b690280f0) at
evaluate.c:3158
#15 0x000000000040f3a7 in evaluate_symbol_list (list=0x7f3b69c06710) at
evaluate.c:3171
#16 0x00000000004075f2 in sparse (filename=0x7fff22e43882
"/home/proski/src/linux-2.6/crypto/tcrypt.c") at lib.c:984
#17 0x0000000000401a1f in main (argc=65, argv=0x7fff22e42348) at
sparse.c:284
(gdb) up 2
#2  0x000000000040849d in warn_for_enum_conversions
(expr=0x7f3b690378d0, type=0x7f3b69632850) at evaluate.c:343
343
do_warn_for_enum_conversions(expr->cond_true, type);
(gdb) p expr->cond_true
$1 = (struct expression *) 0x0
(gdb) p expr->cond_false
$2 = (struct expression *) 0x7f3b6a387b10

-- 
Regards,
Pavel Roskin

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-09 13:46   ` Kamil Dudka
  2010-03-09 18:26     ` Pavel Roskin
@ 2010-03-09 18:35     ` Pavel Roskin
  2010-03-09 19:06       ` Kamil Dudka
  1 sibling, 1 reply; 30+ messages in thread
From: Pavel Roskin @ 2010-03-09 18:35 UTC (permalink / raw)
  To: Kamil Dudka; +Cc: Christopher Li, linux-sparse

On Tue, 2010-03-09 at 14:46 +0100, Kamil Dudka wrote:

P.S. Sorry, I just realized I could add more useful information.  That's
the complete expr in warn_for_enum_conversions():

(gdb) p *expr
$4 = {type = EXPR_CONDITIONAL, flags = 0, op = 63, pos = {type = 7,
stream = 3, newline = 0, whitespace = 1, pos = 47, line = 897, noexpand
= 0}, ctype = 0x668bc0, {{value = 139893141633168, taint = 0, 
      enum_type = 0x7f3b6a387b10}, fvalue = 0, string = 0x7f3b69037890,
{unop = 0x7f3b69037890, op_value = 0}, {symbol = 0x7f3b69037890,
symbol_name = 0x0}, statement = 0x7f3b69037890, {left = 0x7f3b69037890, 
      right = 0x0}, {deref = 0x7f3b69037890, member = 0x0}, {base =
0x7f3b69037890, r_bitpos = 0, r_nrbits = 0}, {cast_type =
0x7f3b69037890, cast_expression = 0x0}, {conditional = 0x7f3b69037890, 
      cond_true = 0x0, cond_false = 0x7f3b6a387b10}, {fn =
0x7f3b69037890, args = 0x0}, {label_symbol = 0x7f3b69037890}, expr_list
= 0x7f3b69037890, {expr_ident = 0x7f3b69037890, field = 0x0, 
      ident_expression = 0x7f3b6a387b10}, {idx_from = 1761835152, idx_to
= 32571, idx_expression = 0x0}, {init_offset = 1761835152, init_nr =
32571, init_expr = 0x0}, {in = 0x7f3b69037890, down = 0x0, {
        ident = 0x7f3b6a387b10, index = 0x7f3b6a387b10}}}}

And that's where it happens (in the code being checked):

static int do_alg_test(const char *alg, u32 type, u32 mask)
{
        return crypto_has_alg(alg, type, mask ?: CRYPTO_ALG_TYPE_MASK) ?
               0 : -ENOENT;
}

I removed the outside conditional, and sparse would still crash on this:

crypto_has_alg(alg, type, mask ?: CRYPTO_ALG_TYPE_MASK);

but not on this:

crypto_has_alg(alg, type, mask ? mask: CRYPTO_ALG_TYPE_MASK);

Apparently, the "?:" notation is confusing sparse now.

-- 
Regards,
Pavel Roskin

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-09 18:35     ` Pavel Roskin
@ 2010-03-09 19:06       ` Kamil Dudka
  2010-03-09 19:15         ` Josh Triplett
  0 siblings, 1 reply; 30+ messages in thread
From: Kamil Dudka @ 2010-03-09 19:06 UTC (permalink / raw)
  To: Pavel Roskin; +Cc: Christopher Li, linux-sparse

On Tuesday 09 of March 2010 19:35:19 Pavel Roskin wrote:
> And that's where it happens (in the code being checked):
>
> static int do_alg_test(const char *alg, u32 type, u32 mask)
> {
>         return crypto_has_alg(alg, type, mask ?: CRYPTO_ALG_TYPE_MASK) ?
>                0 : -ENOENT;
> }

Thank you for the feedback!  Just a few words to explain what happened.  I 
didn't take the conditional operator into account at all while working on 
this.  Today's patch should not only solve the crash, but also extend the 
enum type analysis for expressions containing conditional operators, so far 
without any test cases ...  and yes, it caused yet another crash instead :-)

> I removed the outside conditional, and sparse would still crash on this:
>
> crypto_has_alg(alg, type, mask ?: CRYPTO_ALG_TYPE_MASK);
>
> but not on this:
>
> crypto_has_alg(alg, type, mask ? mask: CRYPTO_ALG_TYPE_MASK);
>
> Apparently, the "?:" notation is confusing sparse now.

To be frank, I've never seen that notation before.  From what I understand, 
the variants above should be equivalent with each other, right?

Kamil

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-09 19:06       ` Kamil Dudka
@ 2010-03-09 19:15         ` Josh Triplett
  2010-03-09 20:11           ` Pavel Roskin
  0 siblings, 1 reply; 30+ messages in thread
From: Josh Triplett @ 2010-03-09 19:15 UTC (permalink / raw)
  To: Kamil Dudka; +Cc: Pavel Roskin, Christopher Li, linux-sparse

On Tue, Mar 09, 2010 at 08:06:23PM +0100, Kamil Dudka wrote:
> On Tuesday 09 of March 2010 19:35:19 Pavel Roskin wrote:
> > I removed the outside conditional, and sparse would still crash on this:
> >
> > crypto_has_alg(alg, type, mask ?: CRYPTO_ALG_TYPE_MASK);
> >
> > but not on this:
> >
> > crypto_has_alg(alg, type, mask ? mask: CRYPTO_ALG_TYPE_MASK);
> >
> > Apparently, the "?:" notation is confusing sparse now.
> 
> To be frank, I've never seen that notation before.  From what I understand, 
> the variants above should be equivalent with each other, right?

Yes, except that the first variant would not evaluate "mask" twice, even
if it consisted of an expression with side effects.  See
http://gcc.gnu.org/onlinedocs/gcc/Conditionals.html .

(When I go to look up something in the GCC manual, more often than not I
end up in Chapter 6, "Extensions to the C Language Family". :) )

- Josh Triplett

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-09 19:15         ` Josh Triplett
@ 2010-03-09 20:11           ` Pavel Roskin
  2010-03-09 20:29             ` Kamil Dudka
  0 siblings, 1 reply; 30+ messages in thread
From: Pavel Roskin @ 2010-03-09 20:11 UTC (permalink / raw)
  To: Josh Triplett; +Cc: Kamil Dudka, Christopher Li, linux-sparse

On Tue, 2010-03-09 at 11:15 -0800, Josh Triplett wrote:
> On Tue, Mar 09, 2010 at 08:06:23PM +0100, Kamil Dudka wrote:
> > On Tuesday 09 of March 2010 19:35:19 Pavel Roskin wrote:
> > > I removed the outside conditional, and sparse would still crash on this:
> > >
> > > crypto_has_alg(alg, type, mask ?: CRYPTO_ALG_TYPE_MASK);
> > >
> > > but not on this:
> > >
> > > crypto_has_alg(alg, type, mask ? mask: CRYPTO_ALG_TYPE_MASK);
> > >
> > > Apparently, the "?:" notation is confusing sparse now.
> > 
> > To be frank, I've never seen that notation before.  From what I understand, 
> > the variants above should be equivalent with each other, right?
> 
> Yes, except that the first variant would not evaluate "mask" twice, even
> if it consisted of an expression with side effects.  See
> http://gcc.gnu.org/onlinedocs/gcc/Conditionals.html .
> 
> (When I go to look up something in the GCC manual, more often than not I
> end up in Chapter 6, "Extensions to the C Language Family". :) )

Ironically, the fix for :? may benefit from that operator:

do_warn_for_enum_conversions(expr->cond_true ?: expr->conditional, type);
do_warn_for_enum_conversions(expr->cond_false ?: expr->conditional, type);

At least I was able to run sparse on the whole kernel (wireless-testing,
which is based on 2.6.34-rc1) without crashing or reporting anything
strange.

Actually, omitting the false conditional appears to be invalid.

-- 
Regards,
Pavel Roskin

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-09 20:11           ` Pavel Roskin
@ 2010-03-09 20:29             ` Kamil Dudka
  2010-03-09 23:30               ` Kamil Dudka
  0 siblings, 1 reply; 30+ messages in thread
From: Kamil Dudka @ 2010-03-09 20:29 UTC (permalink / raw)
  To: Pavel Roskin; +Cc: Josh Triplett, Christopher Li, linux-sparse

On Tuesday 09 of March 2010 21:11:57 Pavel Roskin wrote:
> Ironically, the fix for :? may benefit from that operator:
>
> do_warn_for_enum_conversions(expr->cond_true ?: expr->conditional, type);

Yeah, that's exactly what I've tried ;-)

> do_warn_for_enum_conversions(expr->cond_false ?: expr->conditional, type);

Does it mean the cond_false may be also omitted?  I can't image how the enum 
conversion analysis can be useful in that case.  I've ensured the optional 
analysis would no more crash on another corner case in the first place:

--- a/evaluate.c
+++ b/evaluate.c
@@ -327,13 +327,35 @@ warn_for_int_to_enum_conversion (struct expression 
*expr, struct symbol *typeb)
 }

 static void
-warn_for_enum_conversions(struct expression *expr, struct symbol *type)
+do_warn_for_enum_conversions(struct expression *expr, struct symbol *type)
 {
+       if (!expr || !type)
+               /* do not crash when there is nothing to check */
+               return;
+
        warn_for_different_enum_types (expr, type);
        warn_for_enum_to_int_conversion (expr, type);
        warn_for_int_to_enum_conversion (expr, type);
 }

> At least I was able to run sparse on the whole kernel (wireless-testing,
> which is based on 2.6.34-rc1) without crashing or reporting anything
> strange.
>
> Actually, omitting the false conditional appears to be invalid.

Unfortunately it's not that easy.  I am still getting a non-sense warning for:

static void foo(void)
{
    enum { VAL } y, x = VAL;
    y = x ?: VAL;
}

$ ./sparse enum.c
enum.c:4:9: warning: conversion of
enum.c:4:9:     int to
enum.c:4:9:     int enum <noident>

I need to somehow get over the EXPR_IMPLIED_CAST to dig the original enum_type 
from there...

Kamil

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-09 20:29             ` Kamil Dudka
@ 2010-03-09 23:30               ` Kamil Dudka
  2010-03-10  1:09                 ` Pavel Roskin
  0 siblings, 1 reply; 30+ messages in thread
From: Kamil Dudka @ 2010-03-09 23:30 UTC (permalink / raw)
  To: Pavel Roskin; +Cc: Josh Triplett, Christopher Li, linux-sparse

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

On Tuesday 09 of March 2010 21:29:15 Kamil Dudka wrote:
> Unfortunately it's not that easy.  I am still getting a non-sense warning
> for:
>
> static void foo(void)
> {
>     enum { VAL } y, x = VAL;
>     y = x ?: VAL;
> }
>
> $ ./sparse enum.c
> enum.c:4:9: warning: conversion of
> enum.c:4:9:     int to
> enum.c:4:9:     int enum <noident>
>
> I need to somehow get over the EXPR_IMPLIED_CAST to dig the original
> enum_type from there...

Hopefully I got it.  The promissed fix, which includes new test-cases for ?:, 
is attached.

Kamil

[-- Attachment #2: 0001--Wenum-mismatch-now-works-better-with-conditional-op.patch --]
[-- Type: text/x-diff, Size: 17053 bytes --]

From b6aa6a18318c83e317b1488ce9c0409e109d89fa Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Tue, 9 Mar 2010 23:45:25 +0100
Subject: [PATCH 1/2] -Wenum-mismatch now works better with conditional op.

It used to crash instead of reporting the warnings properly.  Reported by
Pavel Roskin.  Additionally noticed that -Wenum-to-int is too noisy, it should
be emitted only for assignments.

Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 evaluate.c                 |   28 +++++++++++-
 validation/enum-common.c   |   15 ++++++
 validation/enum-from-int.c |   63 +++++++++++++++-----------
 validation/enum-mismatch.c |  100 ++++++++++++++++++++++++++----------------
 validation/enum-to-int.c   |  105 ++++++++++++++++++++++++++++++++++++--------
 5 files changed, 227 insertions(+), 84 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index d3d5e6f..b4c2758 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -327,13 +327,39 @@ warn_for_int_to_enum_conversion (struct expression *expr, struct symbol *typeb)
 }
 
 static void
-warn_for_enum_conversions(struct expression *expr, struct symbol *type)
+do_warn_for_enum_conversions(struct expression *expr, struct symbol *type)
 {
+	if (expr && expr->type == EXPR_IMPLIED_CAST)
+		/* look through the enum/int implied cast */
+		expr = expr->cast_expression;
+
+	if (!expr || !type)
+		/* do not crash when there is nothing to check */
+		return;
+
 	warn_for_different_enum_types (expr, type);
 	warn_for_enum_to_int_conversion (expr, type);
 	warn_for_int_to_enum_conversion (expr, type);
 }
 
+static void
+warn_for_enum_conversions(struct expression *expr, struct symbol *type)
+{
+	switch (expr->type) {
+	case EXPR_CONDITIONAL:
+	case EXPR_SELECT:
+		do_warn_for_enum_conversions(expr->cond_true
+					     /* handle the "?:" quirk */
+					     ?: expr->conditional,
+					     type);
+		do_warn_for_enum_conversions(expr->cond_false, type);
+		break;
+
+	default:
+		do_warn_for_enum_conversions(expr, type);
+	}
+}
+
 /*
  * This gets called for implicit casts in assignments and
  * integer promotion. We often want to try to move the
diff --git a/validation/enum-common.c b/validation/enum-common.c
index f940fef..06fedac 100644
--- a/validation/enum-common.c
+++ b/validation/enum-common.c
@@ -46,6 +46,10 @@ static void always_ok(void)
 		default:
 			take_int(VALUE_C);
 	}
+
+	var_a = var_b ? var_a : VALUE_A;
+	var_b = VALUE_A ? VALUE_B : var_b;
+	take_enum_of_type_a(var_a ?: var_a);
 }
 
 static void trigger_enum_mismatch(void)
@@ -74,6 +78,13 @@ static void trigger_enum_mismatch(void)
 	var_a = VALUE_B;
 	var_b = VALUE_C;
 	anon_enum_var = VALUE_A;
+
+	// conditional operator
+	anon_enum_var = i ? VALUE_A : VALUE_B;
+	var_a = anon_enum_var ? VALUE_A : VALUE_B;
+	var_b = anon_enum_var ? VALUE_A : VALUE_B;
+	take_enum_of_type_a(var_a ?: var_b);
+	take_enum_of_type_a(var_b ?: var_a);
 }
 
 static void trigger_int_to_enum_conversion(void)
@@ -90,6 +101,10 @@ static void trigger_int_to_enum_conversion(void)
 	anon_enum_var = i;
 	var_a = (int) VALUE_A;
 	var_a = (int) VALUE_B;
+
+	// conditional operator
+	take_enum_of_type_a(var_a ?: i);
+	take_enum_of_type_a(i ?: var_a);
 }
 
 static void trigger_enum_to_int_conversion(void)
diff --git a/validation/enum-from-int.c b/validation/enum-from-int.c
index 15b1e4d..92a7e94 100644
--- a/validation/enum-from-int.c
+++ b/validation/enum-from-int.c
@@ -5,32 +5,41 @@
  * check-command: sparse -Wno-enum-mismatch $file
  *
  * check-error-start
-enum-common.c:84:45: warning: conversion of
-enum-common.c:84:45:     int to
-enum-common.c:84:45:     int enum ENUM_TYPE_A 
-enum-common.c:85:45: warning: conversion of
-enum-common.c:85:45:     int to
-enum-common.c:85:45:     int enum ENUM_TYPE_A 
-enum-common.c:82:22: warning: conversion of
-enum-common.c:82:22:     int to
-enum-common.c:82:22:     int enum ENUM_TYPE_A 
-enum-common.c:87:17: warning: conversion of
-enum-common.c:87:17:     int to
-enum-common.c:87:17:     int enum ENUM_TYPE_A 
-enum-common.c:88:17: warning: conversion of
-enum-common.c:88:17:     int to
-enum-common.c:88:17:     int enum ENUM_TYPE_B 
-enum-common.c:89:25: warning: conversion of
-enum-common.c:89:25:     int to
-enum-common.c:89:25:     int enum <noident> 
-enum-common.c:90:25: warning: conversion of
-enum-common.c:90:25:     int to
-enum-common.c:90:25:     int enum <noident> 
-enum-common.c:91:18: warning: conversion of
-enum-common.c:91:18:     int to
-enum-common.c:91:18:     int enum ENUM_TYPE_A 
-enum-common.c:92:18: warning: conversion of
-enum-common.c:92:18:     int to
-enum-common.c:92:18:     int enum ENUM_TYPE_A 
+enum-common.c:95:45: warning: conversion of
+enum-common.c:95:45:     int to
+enum-common.c:95:45:     int enum ENUM_TYPE_A 
+enum-common.c:96:45: warning: conversion of
+enum-common.c:96:45:     int to
+enum-common.c:96:45:     int enum ENUM_TYPE_A 
+enum-common.c:93:22: warning: conversion of
+enum-common.c:93:22:     int to
+enum-common.c:93:22:     int enum ENUM_TYPE_A 
+enum-common.c:98:17: warning: conversion of
+enum-common.c:98:17:     int to
+enum-common.c:98:17:     int enum ENUM_TYPE_A 
+enum-common.c:99:17: warning: conversion of
+enum-common.c:99:17:     int to
+enum-common.c:99:17:     int enum ENUM_TYPE_B 
+enum-common.c:100:25: warning: conversion of
+enum-common.c:100:25:     int to
+enum-common.c:100:25:     int enum <noident> 
+enum-common.c:101:25: warning: conversion of
+enum-common.c:101:25:     int to
+enum-common.c:101:25:     int enum <noident> 
+enum-common.c:102:18: warning: conversion of
+enum-common.c:102:18:     int to
+enum-common.c:102:18:     int enum ENUM_TYPE_A 
+enum-common.c:103:18: warning: conversion of
+enum-common.c:103:18:     int to
+enum-common.c:103:18:     int enum ENUM_TYPE_A 
+enum-common.c:106:38: warning: conversion of
+enum-common.c:106:38:     int to
+enum-common.c:106:38:     int enum ENUM_TYPE_A 
+enum-common.c:107:29: warning: conversion of
+enum-common.c:107:29:     int to
+enum-common.c:107:29:     int enum ENUM_TYPE_A 
+enum-common.c:107:29: warning: conversion of
+enum-common.c:107:29:     int to
+enum-common.c:107:29:     int enum ENUM_TYPE_A 
  * check-error-end
  */
diff --git a/validation/enum-mismatch.c b/validation/enum-mismatch.c
index 6db016f..2fc2fea 100644
--- a/validation/enum-mismatch.c
+++ b/validation/enum-mismatch.c
@@ -5,44 +5,68 @@
  * check-command: sparse -Wenum-mismatch -Wno-int-to-enum $file
  *
  * check-error-start
-enum-common.c:57:45: warning: mixing different enum types
-enum-common.c:57:45:     int enum ENUM_TYPE_B  versus
-enum-common.c:57:45:     int enum ENUM_TYPE_A 
-enum-common.c:58:45: warning: mixing different enum types
-enum-common.c:58:45:     int enum ENUM_TYPE_B  versus
-enum-common.c:58:45:     int enum ENUM_TYPE_A 
-enum-common.c:54:22: warning: mixing different enum types
-enum-common.c:54:22:     int enum ENUM_TYPE_B  versus
-enum-common.c:54:22:     int enum ENUM_TYPE_A 
-enum-common.c:55:22: warning: mixing different enum types
-enum-common.c:55:22:     int enum <noident>  versus
-enum-common.c:55:22:     int enum ENUM_TYPE_A 
-enum-common.c:64:45: warning: mixing different enum types
-enum-common.c:64:45:     int enum <noident>  versus
-enum-common.c:64:45:     int enum ENUM_TYPE_A 
-enum-common.c:65:45: warning: mixing different enum types
-enum-common.c:65:45:     int enum <noident>  versus
-enum-common.c:65:45:     int enum ENUM_TYPE_A 
-enum-common.c:62:22: warning: mixing different enum types
-enum-common.c:62:22:     int enum ENUM_TYPE_A  versus
-enum-common.c:62:22:     int enum <noident> 
-enum-common.c:69:17: warning: mixing different enum types
-enum-common.c:69:17:     int enum ENUM_TYPE_B  versus
-enum-common.c:69:17:     int enum ENUM_TYPE_A 
-enum-common.c:70:17: warning: mixing different enum types
-enum-common.c:70:17:     int enum <noident>  versus
-enum-common.c:70:17:     int enum ENUM_TYPE_B 
-enum-common.c:71:25: warning: mixing different enum types
-enum-common.c:71:25:     int enum ENUM_TYPE_A  versus
-enum-common.c:71:25:     int enum <noident> 
+enum-common.c:61:45: warning: mixing different enum types
+enum-common.c:61:45:     int enum ENUM_TYPE_B  versus
+enum-common.c:61:45:     int enum ENUM_TYPE_A 
+enum-common.c:62:45: warning: mixing different enum types
+enum-common.c:62:45:     int enum ENUM_TYPE_B  versus
+enum-common.c:62:45:     int enum ENUM_TYPE_A 
+enum-common.c:58:22: warning: mixing different enum types
+enum-common.c:58:22:     int enum ENUM_TYPE_B  versus
+enum-common.c:58:22:     int enum ENUM_TYPE_A 
+enum-common.c:59:22: warning: mixing different enum types
+enum-common.c:59:22:     int enum <noident>  versus
+enum-common.c:59:22:     int enum ENUM_TYPE_A 
+enum-common.c:68:45: warning: mixing different enum types
+enum-common.c:68:45:     int enum <noident>  versus
+enum-common.c:68:45:     int enum ENUM_TYPE_A 
+enum-common.c:69:45: warning: mixing different enum types
+enum-common.c:69:45:     int enum <noident>  versus
+enum-common.c:69:45:     int enum ENUM_TYPE_A 
+enum-common.c:66:22: warning: mixing different enum types
+enum-common.c:66:22:     int enum ENUM_TYPE_A  versus
+enum-common.c:66:22:     int enum <noident> 
+enum-common.c:73:17: warning: mixing different enum types
+enum-common.c:73:17:     int enum ENUM_TYPE_B  versus
+enum-common.c:73:17:     int enum ENUM_TYPE_A 
 enum-common.c:74:17: warning: mixing different enum types
-enum-common.c:74:17:     int enum ENUM_TYPE_B  versus
-enum-common.c:74:17:     int enum ENUM_TYPE_A 
-enum-common.c:75:17: warning: mixing different enum types
-enum-common.c:75:17:     int enum <noident>  versus
-enum-common.c:75:17:     int enum ENUM_TYPE_B 
-enum-common.c:76:25: warning: mixing different enum types
-enum-common.c:76:25:     int enum ENUM_TYPE_A  versus
-enum-common.c:76:25:     int enum <noident> 
+enum-common.c:74:17:     int enum <noident>  versus
+enum-common.c:74:17:     int enum ENUM_TYPE_B 
+enum-common.c:75:25: warning: mixing different enum types
+enum-common.c:75:25:     int enum ENUM_TYPE_A  versus
+enum-common.c:75:25:     int enum <noident> 
+enum-common.c:78:17: warning: mixing different enum types
+enum-common.c:78:17:     int enum ENUM_TYPE_B  versus
+enum-common.c:78:17:     int enum ENUM_TYPE_A 
+enum-common.c:79:17: warning: mixing different enum types
+enum-common.c:79:17:     int enum <noident>  versus
+enum-common.c:79:17:     int enum ENUM_TYPE_B 
+enum-common.c:80:25: warning: mixing different enum types
+enum-common.c:80:25:     int enum ENUM_TYPE_A  versus
+enum-common.c:80:25:     int enum <noident> 
+enum-common.c:83:29: warning: mixing different enum types
+enum-common.c:83:29:     int enum ENUM_TYPE_A  versus
+enum-common.c:83:29:     int enum <noident> 
+enum-common.c:83:39: warning: mixing different enum types
+enum-common.c:83:39:     int enum ENUM_TYPE_B  versus
+enum-common.c:83:39:     int enum <noident> 
+enum-common.c:84:43: warning: mixing different enum types
+enum-common.c:84:43:     int enum ENUM_TYPE_B  versus
+enum-common.c:84:43:     int enum ENUM_TYPE_A 
+enum-common.c:85:33: warning: mixing different enum types
+enum-common.c:85:33:     int enum ENUM_TYPE_A  versus
+enum-common.c:85:33:     int enum ENUM_TYPE_B 
+enum-common.c:86:29: warning: mixing different enum types
+enum-common.c:86:29:     int enum ENUM_TYPE_A  versus
+enum-common.c:86:29:     int enum ENUM_TYPE_B 
+enum-common.c:86:38: warning: mixing different enum types
+enum-common.c:86:38:     int enum ENUM_TYPE_B  versus
+enum-common.c:86:38:     int enum ENUM_TYPE_A 
+enum-common.c:87:29: warning: mixing different enum types
+enum-common.c:87:29:     int enum ENUM_TYPE_B  versus
+enum-common.c:87:29:     int enum ENUM_TYPE_A 
+enum-common.c:87:29: warning: mixing different enum types
+enum-common.c:87:29:     int enum ENUM_TYPE_B  versus
+enum-common.c:87:29:     int enum ENUM_TYPE_A 
  * check-error-end
  */
diff --git a/validation/enum-to-int.c b/validation/enum-to-int.c
index a981ce5..33fdeeb 100644
--- a/validation/enum-to-int.c
+++ b/validation/enum-to-int.c
@@ -5,23 +5,92 @@
  * check-command: sparse -Wenum-to-int -Wno-enum-mismatch -Wno-int-to-enum $file
  *
  * check-error-start
-enum-common.c:97:13: warning: conversion of
-enum-common.c:97:13:     int enum ENUM_TYPE_A  to
-enum-common.c:97:13:     int
-enum-common.c:98:13: warning: conversion of
-enum-common.c:98:13:     int enum ENUM_TYPE_B  to
-enum-common.c:98:13:     int
-enum-common.c:103:34: warning: conversion of
-enum-common.c:103:34:     int enum ENUM_TYPE_A  to
-enum-common.c:103:34:     int
-enum-common.c:104:34: warning: conversion of
-enum-common.c:104:34:     int enum ENUM_TYPE_B  to
-enum-common.c:104:34:     int
-enum-common.c:100:22: warning: conversion of
-enum-common.c:100:22:     int enum ENUM_TYPE_A  to
-enum-common.c:100:22:     int
-enum-common.c:101:22: warning: conversion of
-enum-common.c:101:22:     int enum ENUM_TYPE_B  to
-enum-common.c:101:22:     int
+enum-common.c:50:25: warning: conversion of
+enum-common.c:50:25:     int enum ENUM_TYPE_A  to
+enum-common.c:50:25:     int
+enum-common.c:50:25: warning: conversion of
+enum-common.c:50:25:     int enum ENUM_TYPE_A  to
+enum-common.c:50:25:     int
+enum-common.c:50:33: warning: conversion of
+enum-common.c:50:33:     int enum ENUM_TYPE_A  to
+enum-common.c:50:33:     int
+enum-common.c:51:27: warning: conversion of
+enum-common.c:51:27:     int enum ENUM_TYPE_B  to
+enum-common.c:51:27:     int
+enum-common.c:51:37: warning: conversion of
+enum-common.c:51:37:     int enum ENUM_TYPE_B  to
+enum-common.c:51:37:     int
+enum-common.c:52:29: warning: conversion of
+enum-common.c:52:29:     int enum ENUM_TYPE_A  to
+enum-common.c:52:29:     int
+enum-common.c:52:38: warning: conversion of
+enum-common.c:52:38:     int enum ENUM_TYPE_A  to
+enum-common.c:52:38:     int
+enum-common.c:83:29: warning: conversion of
+enum-common.c:83:29:     int enum ENUM_TYPE_A  to
+enum-common.c:83:29:     int
+enum-common.c:83:29: warning: conversion of
+enum-common.c:83:29:     int enum ENUM_TYPE_A  to
+enum-common.c:83:29:     int
+enum-common.c:83:39: warning: conversion of
+enum-common.c:83:39:     int enum ENUM_TYPE_B  to
+enum-common.c:83:39:     int
+enum-common.c:84:33: warning: conversion of
+enum-common.c:84:33:     int enum ENUM_TYPE_A  to
+enum-common.c:84:33:     int
+enum-common.c:84:33: warning: conversion of
+enum-common.c:84:33:     int enum ENUM_TYPE_A  to
+enum-common.c:84:33:     int
+enum-common.c:84:43: warning: conversion of
+enum-common.c:84:43:     int enum ENUM_TYPE_B  to
+enum-common.c:84:43:     int
+enum-common.c:85:33: warning: conversion of
+enum-common.c:85:33:     int enum ENUM_TYPE_A  to
+enum-common.c:85:33:     int
+enum-common.c:85:33: warning: conversion of
+enum-common.c:85:33:     int enum ENUM_TYPE_A  to
+enum-common.c:85:33:     int
+enum-common.c:85:43: warning: conversion of
+enum-common.c:85:43:     int enum ENUM_TYPE_B  to
+enum-common.c:85:43:     int
+enum-common.c:86:29: warning: conversion of
+enum-common.c:86:29:     int enum ENUM_TYPE_A  to
+enum-common.c:86:29:     int
+enum-common.c:86:38: warning: conversion of
+enum-common.c:86:38:     int enum ENUM_TYPE_B  to
+enum-common.c:86:38:     int
+enum-common.c:87:29: warning: conversion of
+enum-common.c:87:29:     int enum ENUM_TYPE_B  to
+enum-common.c:87:29:     int
+enum-common.c:87:38: warning: conversion of
+enum-common.c:87:38:     int enum ENUM_TYPE_A  to
+enum-common.c:87:38:     int
+enum-common.c:106:29: warning: conversion of
+enum-common.c:106:29:     int enum ENUM_TYPE_A  to
+enum-common.c:106:29:     int
+enum-common.c:106:29: warning: conversion of
+enum-common.c:106:29:     int enum ENUM_TYPE_A  to
+enum-common.c:106:29:     int
+enum-common.c:107:34: warning: conversion of
+enum-common.c:107:34:     int enum ENUM_TYPE_A  to
+enum-common.c:107:34:     int
+enum-common.c:112:13: warning: conversion of
+enum-common.c:112:13:     int enum ENUM_TYPE_A  to
+enum-common.c:112:13:     int
+enum-common.c:113:13: warning: conversion of
+enum-common.c:113:13:     int enum ENUM_TYPE_B  to
+enum-common.c:113:13:     int
+enum-common.c:118:34: warning: conversion of
+enum-common.c:118:34:     int enum ENUM_TYPE_A  to
+enum-common.c:118:34:     int
+enum-common.c:119:34: warning: conversion of
+enum-common.c:119:34:     int enum ENUM_TYPE_B  to
+enum-common.c:119:34:     int
+enum-common.c:115:22: warning: conversion of
+enum-common.c:115:22:     int enum ENUM_TYPE_A  to
+enum-common.c:115:22:     int
+enum-common.c:116:22: warning: conversion of
+enum-common.c:116:22:     int enum ENUM_TYPE_B  to
+enum-common.c:116:22:     int
  * check-error-end
  */
-- 
1.6.1.2


[-- Attachment #3: 0002--Wenum-to-int-is-now-emitted-only-for-and-switch.patch --]
[-- Type: text/x-diff, Size: 4719 bytes --]

From 50a39380f5f97e5c379c6464eff609916175640c Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Wed, 10 Mar 2010 00:15:08 +0100
Subject: [PATCH 2/2] -Wenum-to-int is now emitted only for = and switch


Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 evaluate.c               |    3 +-
 validation/enum-to-int.c |   69 ----------------------------------------------
 2 files changed, 2 insertions(+), 70 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index b4c2758..9e5f28e 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -338,7 +338,6 @@ do_warn_for_enum_conversions(struct expression *expr, struct symbol *type)
 		return;
 
 	warn_for_different_enum_types (expr, type);
-	warn_for_enum_to_int_conversion (expr, type);
 	warn_for_int_to_enum_conversion (expr, type);
 }
 
@@ -1473,6 +1472,7 @@ Err:
 	*rp = cast_to(*rp, target);
 	return 0;
 Cast:
+	warn_for_enum_to_int_conversion (*rp, target);
 	*rp = cast_to(*rp, target);
 	return 1;
 }
@@ -3352,6 +3352,7 @@ static void check_case_type(struct expression *switch_expr,
 	if (!switch_type || !case_type)
 		goto Bad;
 
+	warn_for_enum_to_int_conversion (case_expr, switch_type);
 	warn_for_enum_conversions(case_expr, switch_type);
 
 	sclass = classify_type(switch_type, &switch_type);
diff --git a/validation/enum-to-int.c b/validation/enum-to-int.c
index 33fdeeb..134a34d 100644
--- a/validation/enum-to-int.c
+++ b/validation/enum-to-int.c
@@ -5,75 +5,6 @@
  * check-command: sparse -Wenum-to-int -Wno-enum-mismatch -Wno-int-to-enum $file
  *
  * check-error-start
-enum-common.c:50:25: warning: conversion of
-enum-common.c:50:25:     int enum ENUM_TYPE_A  to
-enum-common.c:50:25:     int
-enum-common.c:50:25: warning: conversion of
-enum-common.c:50:25:     int enum ENUM_TYPE_A  to
-enum-common.c:50:25:     int
-enum-common.c:50:33: warning: conversion of
-enum-common.c:50:33:     int enum ENUM_TYPE_A  to
-enum-common.c:50:33:     int
-enum-common.c:51:27: warning: conversion of
-enum-common.c:51:27:     int enum ENUM_TYPE_B  to
-enum-common.c:51:27:     int
-enum-common.c:51:37: warning: conversion of
-enum-common.c:51:37:     int enum ENUM_TYPE_B  to
-enum-common.c:51:37:     int
-enum-common.c:52:29: warning: conversion of
-enum-common.c:52:29:     int enum ENUM_TYPE_A  to
-enum-common.c:52:29:     int
-enum-common.c:52:38: warning: conversion of
-enum-common.c:52:38:     int enum ENUM_TYPE_A  to
-enum-common.c:52:38:     int
-enum-common.c:83:29: warning: conversion of
-enum-common.c:83:29:     int enum ENUM_TYPE_A  to
-enum-common.c:83:29:     int
-enum-common.c:83:29: warning: conversion of
-enum-common.c:83:29:     int enum ENUM_TYPE_A  to
-enum-common.c:83:29:     int
-enum-common.c:83:39: warning: conversion of
-enum-common.c:83:39:     int enum ENUM_TYPE_B  to
-enum-common.c:83:39:     int
-enum-common.c:84:33: warning: conversion of
-enum-common.c:84:33:     int enum ENUM_TYPE_A  to
-enum-common.c:84:33:     int
-enum-common.c:84:33: warning: conversion of
-enum-common.c:84:33:     int enum ENUM_TYPE_A  to
-enum-common.c:84:33:     int
-enum-common.c:84:43: warning: conversion of
-enum-common.c:84:43:     int enum ENUM_TYPE_B  to
-enum-common.c:84:43:     int
-enum-common.c:85:33: warning: conversion of
-enum-common.c:85:33:     int enum ENUM_TYPE_A  to
-enum-common.c:85:33:     int
-enum-common.c:85:33: warning: conversion of
-enum-common.c:85:33:     int enum ENUM_TYPE_A  to
-enum-common.c:85:33:     int
-enum-common.c:85:43: warning: conversion of
-enum-common.c:85:43:     int enum ENUM_TYPE_B  to
-enum-common.c:85:43:     int
-enum-common.c:86:29: warning: conversion of
-enum-common.c:86:29:     int enum ENUM_TYPE_A  to
-enum-common.c:86:29:     int
-enum-common.c:86:38: warning: conversion of
-enum-common.c:86:38:     int enum ENUM_TYPE_B  to
-enum-common.c:86:38:     int
-enum-common.c:87:29: warning: conversion of
-enum-common.c:87:29:     int enum ENUM_TYPE_B  to
-enum-common.c:87:29:     int
-enum-common.c:87:38: warning: conversion of
-enum-common.c:87:38:     int enum ENUM_TYPE_A  to
-enum-common.c:87:38:     int
-enum-common.c:106:29: warning: conversion of
-enum-common.c:106:29:     int enum ENUM_TYPE_A  to
-enum-common.c:106:29:     int
-enum-common.c:106:29: warning: conversion of
-enum-common.c:106:29:     int enum ENUM_TYPE_A  to
-enum-common.c:106:29:     int
-enum-common.c:107:34: warning: conversion of
-enum-common.c:107:34:     int enum ENUM_TYPE_A  to
-enum-common.c:107:34:     int
 enum-common.c:112:13: warning: conversion of
 enum-common.c:112:13:     int enum ENUM_TYPE_A  to
 enum-common.c:112:13:     int
-- 
1.6.1.2


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-09 23:30               ` Kamil Dudka
@ 2010-03-10  1:09                 ` Pavel Roskin
  2010-03-10 16:05                   ` Kamil Dudka
  0 siblings, 1 reply; 30+ messages in thread
From: Pavel Roskin @ 2010-03-10  1:09 UTC (permalink / raw)
  To: Kamil Dudka; +Cc: Josh Triplett, Christopher Li, linux-sparse

On Wed, 2010-03-10 at 00:30 +0100, Kamil Dudka wrote:

> Hopefully I got it.  The promissed fix, which includes new test-cases for ?:, 
> is attached.

Looks good!  "make check" works.  Checking the whole kernel produces no
core dumps.  The warnings appear to be legitimate.

I wonder is the quirk handling could be skipped for EXPR_SELECT, but I'm
fine with it if it's harmless.

When would expr or type be NULL?  We could check type in
warn_for_enum_conversions().

-- 
Regards,
Pavel Roskin

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-10  1:09                 ` Pavel Roskin
@ 2010-03-10 16:05                   ` Kamil Dudka
  2010-03-10 20:27                     ` Pavel Roskin
  2010-03-10 21:56                     ` Christopher Li
  0 siblings, 2 replies; 30+ messages in thread
From: Kamil Dudka @ 2010-03-10 16:05 UTC (permalink / raw)
  To: Pavel Roskin; +Cc: Josh Triplett, Christopher Li, linux-sparse

[-- Attachment #1: Type: Text/Plain, Size: 659 bytes --]

On Wed March 10 2010 02:09:22 Pavel Roskin wrote:
> I wonder is the quirk handling could be skipped for EXPR_SELECT, but I'm
> fine with it if it's harmless.

That's more likely question for sparse developers.  I have no test-case which 
goes through the EXPR_SELECT path, so that I can't test it actually.

> When would expr or type be NULL?

Also no test-case here, but it's still better to skip the optional analysis 
than let it crash on invalid dereference.

> We could check type in warn_for_enum_conversions().

Sure, I've just improved it.  Thanks for the hint!  Also the order of that 
patches has been reversed, so that they are less chatty.

Kamil

[-- Attachment #2: 0001-Wenum-to-int-is-now-emitted-only-for-and-switch.patch --]
[-- Type: text/x-patch, Size: 1226 bytes --]

From 1f027ab5e1995b9e65955745cf2c979c24c16ccd Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Wed, 10 Mar 2010 16:37:25 +0100
Subject: [PATCH 1/2] -Wenum-to-int is now emitted only for = and switch


Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 evaluate.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index d3d5e6f..8726381 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -330,7 +330,6 @@ static void
 warn_for_enum_conversions(struct expression *expr, struct symbol *type)
 {
 	warn_for_different_enum_types (expr, type);
-	warn_for_enum_to_int_conversion (expr, type);
 	warn_for_int_to_enum_conversion (expr, type);
 }
 
@@ -1447,6 +1446,7 @@ Err:
 	*rp = cast_to(*rp, target);
 	return 0;
 Cast:
+	warn_for_enum_to_int_conversion (*rp, target);
 	*rp = cast_to(*rp, target);
 	return 1;
 }
@@ -3326,6 +3326,7 @@ static void check_case_type(struct expression *switch_expr,
 	if (!switch_type || !case_type)
 		goto Bad;
 
+	warn_for_enum_to_int_conversion (case_expr, switch_type);
 	warn_for_enum_conversions(case_expr, switch_type);
 
 	sclass = classify_type(switch_type, &switch_type);
-- 
1.6.6.1


[-- Attachment #3: 0002-Wenum-mismatch-now-works-better-with-conditional-op.patch --]
[-- Type: text/x-patch, Size: 13993 bytes --]

From 08b7677cf6e3a10c053dc203275a254e53f11274 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Wed, 10 Mar 2010 16:49:30 +0100
Subject: [PATCH 2/2] -Wenum-mismatch now works better with conditional op.

It used to crash instead of reporting the warnings properly.  Reported
by Pavel Roskin.

Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 evaluate.c                 |   32 ++++++++++++++-
 validation/enum-common.c   |   15 +++++++
 validation/enum-from-int.c |   63 ++++++++++++++++------------
 validation/enum-mismatch.c |  100 +++++++++++++++++++++++++++-----------------
 validation/enum-to-int.c   |   36 ++++++++--------
 5 files changed, 162 insertions(+), 84 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index 8726381..9e98f66 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -327,12 +327,42 @@ warn_for_int_to_enum_conversion (struct expression *expr, struct symbol *typeb)
 }
 
 static void
-warn_for_enum_conversions(struct expression *expr, struct symbol *type)
+do_warn_for_enum_conversions(struct expression *expr, struct symbol *type)
 {
+	if (expr && expr->type == EXPR_IMPLIED_CAST)
+		/* look through the enum/int implied cast */
+		expr = expr->cast_expression;
+
+	if (!expr)
+		/* do not crash when there is nothing to check */
+		return;
+
 	warn_for_different_enum_types (expr, type);
 	warn_for_int_to_enum_conversion (expr, type);
 }
 
+static void
+warn_for_enum_conversions(struct expression *expr, struct symbol *type)
+{
+	if (!expr || !type)
+		/* do not crash when there is nothing to check */
+		return;
+
+	switch (expr->type) {
+	case EXPR_SELECT:
+	case EXPR_CONDITIONAL:
+		do_warn_for_enum_conversions(expr->cond_true
+					     /* handle the "?:" quirk */
+					     ?: expr->conditional,
+					     type);
+		do_warn_for_enum_conversions(expr->cond_false, type);
+		break;
+
+	default:
+		do_warn_for_enum_conversions(expr, type);
+	}
+}
+
 /*
  * This gets called for implicit casts in assignments and
  * integer promotion. We often want to try to move the
diff --git a/validation/enum-common.c b/validation/enum-common.c
index f940fef..06fedac 100644
--- a/validation/enum-common.c
+++ b/validation/enum-common.c
@@ -46,6 +46,10 @@ static void always_ok(void)
 		default:
 			take_int(VALUE_C);
 	}
+
+	var_a = var_b ? var_a : VALUE_A;
+	var_b = VALUE_A ? VALUE_B : var_b;
+	take_enum_of_type_a(var_a ?: var_a);
 }
 
 static void trigger_enum_mismatch(void)
@@ -74,6 +78,13 @@ static void trigger_enum_mismatch(void)
 	var_a = VALUE_B;
 	var_b = VALUE_C;
 	anon_enum_var = VALUE_A;
+
+	// conditional operator
+	anon_enum_var = i ? VALUE_A : VALUE_B;
+	var_a = anon_enum_var ? VALUE_A : VALUE_B;
+	var_b = anon_enum_var ? VALUE_A : VALUE_B;
+	take_enum_of_type_a(var_a ?: var_b);
+	take_enum_of_type_a(var_b ?: var_a);
 }
 
 static void trigger_int_to_enum_conversion(void)
@@ -90,6 +101,10 @@ static void trigger_int_to_enum_conversion(void)
 	anon_enum_var = i;
 	var_a = (int) VALUE_A;
 	var_a = (int) VALUE_B;
+
+	// conditional operator
+	take_enum_of_type_a(var_a ?: i);
+	take_enum_of_type_a(i ?: var_a);
 }
 
 static void trigger_enum_to_int_conversion(void)
diff --git a/validation/enum-from-int.c b/validation/enum-from-int.c
index 15b1e4d..92a7e94 100644
--- a/validation/enum-from-int.c
+++ b/validation/enum-from-int.c
@@ -5,32 +5,41 @@
  * check-command: sparse -Wno-enum-mismatch $file
  *
  * check-error-start
-enum-common.c:84:45: warning: conversion of
-enum-common.c:84:45:     int to
-enum-common.c:84:45:     int enum ENUM_TYPE_A 
-enum-common.c:85:45: warning: conversion of
-enum-common.c:85:45:     int to
-enum-common.c:85:45:     int enum ENUM_TYPE_A 
-enum-common.c:82:22: warning: conversion of
-enum-common.c:82:22:     int to
-enum-common.c:82:22:     int enum ENUM_TYPE_A 
-enum-common.c:87:17: warning: conversion of
-enum-common.c:87:17:     int to
-enum-common.c:87:17:     int enum ENUM_TYPE_A 
-enum-common.c:88:17: warning: conversion of
-enum-common.c:88:17:     int to
-enum-common.c:88:17:     int enum ENUM_TYPE_B 
-enum-common.c:89:25: warning: conversion of
-enum-common.c:89:25:     int to
-enum-common.c:89:25:     int enum <noident> 
-enum-common.c:90:25: warning: conversion of
-enum-common.c:90:25:     int to
-enum-common.c:90:25:     int enum <noident> 
-enum-common.c:91:18: warning: conversion of
-enum-common.c:91:18:     int to
-enum-common.c:91:18:     int enum ENUM_TYPE_A 
-enum-common.c:92:18: warning: conversion of
-enum-common.c:92:18:     int to
-enum-common.c:92:18:     int enum ENUM_TYPE_A 
+enum-common.c:95:45: warning: conversion of
+enum-common.c:95:45:     int to
+enum-common.c:95:45:     int enum ENUM_TYPE_A 
+enum-common.c:96:45: warning: conversion of
+enum-common.c:96:45:     int to
+enum-common.c:96:45:     int enum ENUM_TYPE_A 
+enum-common.c:93:22: warning: conversion of
+enum-common.c:93:22:     int to
+enum-common.c:93:22:     int enum ENUM_TYPE_A 
+enum-common.c:98:17: warning: conversion of
+enum-common.c:98:17:     int to
+enum-common.c:98:17:     int enum ENUM_TYPE_A 
+enum-common.c:99:17: warning: conversion of
+enum-common.c:99:17:     int to
+enum-common.c:99:17:     int enum ENUM_TYPE_B 
+enum-common.c:100:25: warning: conversion of
+enum-common.c:100:25:     int to
+enum-common.c:100:25:     int enum <noident> 
+enum-common.c:101:25: warning: conversion of
+enum-common.c:101:25:     int to
+enum-common.c:101:25:     int enum <noident> 
+enum-common.c:102:18: warning: conversion of
+enum-common.c:102:18:     int to
+enum-common.c:102:18:     int enum ENUM_TYPE_A 
+enum-common.c:103:18: warning: conversion of
+enum-common.c:103:18:     int to
+enum-common.c:103:18:     int enum ENUM_TYPE_A 
+enum-common.c:106:38: warning: conversion of
+enum-common.c:106:38:     int to
+enum-common.c:106:38:     int enum ENUM_TYPE_A 
+enum-common.c:107:29: warning: conversion of
+enum-common.c:107:29:     int to
+enum-common.c:107:29:     int enum ENUM_TYPE_A 
+enum-common.c:107:29: warning: conversion of
+enum-common.c:107:29:     int to
+enum-common.c:107:29:     int enum ENUM_TYPE_A 
  * check-error-end
  */
diff --git a/validation/enum-mismatch.c b/validation/enum-mismatch.c
index 6db016f..2fc2fea 100644
--- a/validation/enum-mismatch.c
+++ b/validation/enum-mismatch.c
@@ -5,44 +5,68 @@
  * check-command: sparse -Wenum-mismatch -Wno-int-to-enum $file
  *
  * check-error-start
-enum-common.c:57:45: warning: mixing different enum types
-enum-common.c:57:45:     int enum ENUM_TYPE_B  versus
-enum-common.c:57:45:     int enum ENUM_TYPE_A 
-enum-common.c:58:45: warning: mixing different enum types
-enum-common.c:58:45:     int enum ENUM_TYPE_B  versus
-enum-common.c:58:45:     int enum ENUM_TYPE_A 
-enum-common.c:54:22: warning: mixing different enum types
-enum-common.c:54:22:     int enum ENUM_TYPE_B  versus
-enum-common.c:54:22:     int enum ENUM_TYPE_A 
-enum-common.c:55:22: warning: mixing different enum types
-enum-common.c:55:22:     int enum <noident>  versus
-enum-common.c:55:22:     int enum ENUM_TYPE_A 
-enum-common.c:64:45: warning: mixing different enum types
-enum-common.c:64:45:     int enum <noident>  versus
-enum-common.c:64:45:     int enum ENUM_TYPE_A 
-enum-common.c:65:45: warning: mixing different enum types
-enum-common.c:65:45:     int enum <noident>  versus
-enum-common.c:65:45:     int enum ENUM_TYPE_A 
-enum-common.c:62:22: warning: mixing different enum types
-enum-common.c:62:22:     int enum ENUM_TYPE_A  versus
-enum-common.c:62:22:     int enum <noident> 
-enum-common.c:69:17: warning: mixing different enum types
-enum-common.c:69:17:     int enum ENUM_TYPE_B  versus
-enum-common.c:69:17:     int enum ENUM_TYPE_A 
-enum-common.c:70:17: warning: mixing different enum types
-enum-common.c:70:17:     int enum <noident>  versus
-enum-common.c:70:17:     int enum ENUM_TYPE_B 
-enum-common.c:71:25: warning: mixing different enum types
-enum-common.c:71:25:     int enum ENUM_TYPE_A  versus
-enum-common.c:71:25:     int enum <noident> 
+enum-common.c:61:45: warning: mixing different enum types
+enum-common.c:61:45:     int enum ENUM_TYPE_B  versus
+enum-common.c:61:45:     int enum ENUM_TYPE_A 
+enum-common.c:62:45: warning: mixing different enum types
+enum-common.c:62:45:     int enum ENUM_TYPE_B  versus
+enum-common.c:62:45:     int enum ENUM_TYPE_A 
+enum-common.c:58:22: warning: mixing different enum types
+enum-common.c:58:22:     int enum ENUM_TYPE_B  versus
+enum-common.c:58:22:     int enum ENUM_TYPE_A 
+enum-common.c:59:22: warning: mixing different enum types
+enum-common.c:59:22:     int enum <noident>  versus
+enum-common.c:59:22:     int enum ENUM_TYPE_A 
+enum-common.c:68:45: warning: mixing different enum types
+enum-common.c:68:45:     int enum <noident>  versus
+enum-common.c:68:45:     int enum ENUM_TYPE_A 
+enum-common.c:69:45: warning: mixing different enum types
+enum-common.c:69:45:     int enum <noident>  versus
+enum-common.c:69:45:     int enum ENUM_TYPE_A 
+enum-common.c:66:22: warning: mixing different enum types
+enum-common.c:66:22:     int enum ENUM_TYPE_A  versus
+enum-common.c:66:22:     int enum <noident> 
+enum-common.c:73:17: warning: mixing different enum types
+enum-common.c:73:17:     int enum ENUM_TYPE_B  versus
+enum-common.c:73:17:     int enum ENUM_TYPE_A 
 enum-common.c:74:17: warning: mixing different enum types
-enum-common.c:74:17:     int enum ENUM_TYPE_B  versus
-enum-common.c:74:17:     int enum ENUM_TYPE_A 
-enum-common.c:75:17: warning: mixing different enum types
-enum-common.c:75:17:     int enum <noident>  versus
-enum-common.c:75:17:     int enum ENUM_TYPE_B 
-enum-common.c:76:25: warning: mixing different enum types
-enum-common.c:76:25:     int enum ENUM_TYPE_A  versus
-enum-common.c:76:25:     int enum <noident> 
+enum-common.c:74:17:     int enum <noident>  versus
+enum-common.c:74:17:     int enum ENUM_TYPE_B 
+enum-common.c:75:25: warning: mixing different enum types
+enum-common.c:75:25:     int enum ENUM_TYPE_A  versus
+enum-common.c:75:25:     int enum <noident> 
+enum-common.c:78:17: warning: mixing different enum types
+enum-common.c:78:17:     int enum ENUM_TYPE_B  versus
+enum-common.c:78:17:     int enum ENUM_TYPE_A 
+enum-common.c:79:17: warning: mixing different enum types
+enum-common.c:79:17:     int enum <noident>  versus
+enum-common.c:79:17:     int enum ENUM_TYPE_B 
+enum-common.c:80:25: warning: mixing different enum types
+enum-common.c:80:25:     int enum ENUM_TYPE_A  versus
+enum-common.c:80:25:     int enum <noident> 
+enum-common.c:83:29: warning: mixing different enum types
+enum-common.c:83:29:     int enum ENUM_TYPE_A  versus
+enum-common.c:83:29:     int enum <noident> 
+enum-common.c:83:39: warning: mixing different enum types
+enum-common.c:83:39:     int enum ENUM_TYPE_B  versus
+enum-common.c:83:39:     int enum <noident> 
+enum-common.c:84:43: warning: mixing different enum types
+enum-common.c:84:43:     int enum ENUM_TYPE_B  versus
+enum-common.c:84:43:     int enum ENUM_TYPE_A 
+enum-common.c:85:33: warning: mixing different enum types
+enum-common.c:85:33:     int enum ENUM_TYPE_A  versus
+enum-common.c:85:33:     int enum ENUM_TYPE_B 
+enum-common.c:86:29: warning: mixing different enum types
+enum-common.c:86:29:     int enum ENUM_TYPE_A  versus
+enum-common.c:86:29:     int enum ENUM_TYPE_B 
+enum-common.c:86:38: warning: mixing different enum types
+enum-common.c:86:38:     int enum ENUM_TYPE_B  versus
+enum-common.c:86:38:     int enum ENUM_TYPE_A 
+enum-common.c:87:29: warning: mixing different enum types
+enum-common.c:87:29:     int enum ENUM_TYPE_B  versus
+enum-common.c:87:29:     int enum ENUM_TYPE_A 
+enum-common.c:87:29: warning: mixing different enum types
+enum-common.c:87:29:     int enum ENUM_TYPE_B  versus
+enum-common.c:87:29:     int enum ENUM_TYPE_A 
  * check-error-end
  */
diff --git a/validation/enum-to-int.c b/validation/enum-to-int.c
index a981ce5..134a34d 100644
--- a/validation/enum-to-int.c
+++ b/validation/enum-to-int.c
@@ -5,23 +5,23 @@
  * check-command: sparse -Wenum-to-int -Wno-enum-mismatch -Wno-int-to-enum $file
  *
  * check-error-start
-enum-common.c:97:13: warning: conversion of
-enum-common.c:97:13:     int enum ENUM_TYPE_A  to
-enum-common.c:97:13:     int
-enum-common.c:98:13: warning: conversion of
-enum-common.c:98:13:     int enum ENUM_TYPE_B  to
-enum-common.c:98:13:     int
-enum-common.c:103:34: warning: conversion of
-enum-common.c:103:34:     int enum ENUM_TYPE_A  to
-enum-common.c:103:34:     int
-enum-common.c:104:34: warning: conversion of
-enum-common.c:104:34:     int enum ENUM_TYPE_B  to
-enum-common.c:104:34:     int
-enum-common.c:100:22: warning: conversion of
-enum-common.c:100:22:     int enum ENUM_TYPE_A  to
-enum-common.c:100:22:     int
-enum-common.c:101:22: warning: conversion of
-enum-common.c:101:22:     int enum ENUM_TYPE_B  to
-enum-common.c:101:22:     int
+enum-common.c:112:13: warning: conversion of
+enum-common.c:112:13:     int enum ENUM_TYPE_A  to
+enum-common.c:112:13:     int
+enum-common.c:113:13: warning: conversion of
+enum-common.c:113:13:     int enum ENUM_TYPE_B  to
+enum-common.c:113:13:     int
+enum-common.c:118:34: warning: conversion of
+enum-common.c:118:34:     int enum ENUM_TYPE_A  to
+enum-common.c:118:34:     int
+enum-common.c:119:34: warning: conversion of
+enum-common.c:119:34:     int enum ENUM_TYPE_B  to
+enum-common.c:119:34:     int
+enum-common.c:115:22: warning: conversion of
+enum-common.c:115:22:     int enum ENUM_TYPE_A  to
+enum-common.c:115:22:     int
+enum-common.c:116:22: warning: conversion of
+enum-common.c:116:22:     int enum ENUM_TYPE_B  to
+enum-common.c:116:22:     int
  * check-error-end
  */
-- 
1.6.6.1


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-10 16:05                   ` Kamil Dudka
@ 2010-03-10 20:27                     ` Pavel Roskin
  2010-03-10 20:44                       ` Kamil Dudka
  2010-03-10 21:03                       ` Kamil Dudka
  2010-03-10 21:56                     ` Christopher Li
  1 sibling, 2 replies; 30+ messages in thread
From: Pavel Roskin @ 2010-03-10 20:27 UTC (permalink / raw)
  To: Kamil Dudka; +Cc: Josh Triplett, Christopher Li, linux-sparse

On Wed, 2010-03-10 at 17:05 +0100, Kamil Dudka wrote:
> On Wed March 10 2010 02:09:22 Pavel Roskin wrote:
> > I wonder is the quirk handling could be skipped for EXPR_SELECT, but I'm
> > fine with it if it's harmless.
> 
> That's more likely question for sparse developers.  I have no test-case which 
> goes through the EXPR_SELECT path, so that I can't test it actually.

It looks like EXPR_CONDITIONAL is replaces with EXPR_SELECT for some
expressions, which affects code generation.  It's the same thing in
terms of syntax.  So it's fine that both are handled together.

> > When would expr or type be NULL?
> 
> Also no test-case here, but it's still better to skip the optional analysis 
> than let it crash on invalid dereference.

That makes sense for certain kinds of software, where crash is too
costly, but in case of sparse, I'd rather see it crash than ignore a
condition that may indicate an error elsewhere (perhaps both in sparse
and in the code it checks).

I thing using assert() would be a better approach.  It's already used in
sparse.

I checked the whole kernel using assert in place of the NULL checks, and
there have been no crashes.

> > We could check type in warn_for_enum_conversions().
> 
> Sure, I've just improved it.  Thanks for the hint!  Also the order of that 
> patches has been reversed, so that they are less chatty.

Maybe you could add a test case for the first patch?  What warnings does
it remove?  What is "="?  Is it assignment or initialization or both?
How about comparisons?  A better description would be nice.

-- 
Regards,
Pavel Roskin

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-10 20:27                     ` Pavel Roskin
@ 2010-03-10 20:44                       ` Kamil Dudka
  2010-03-10 21:03                       ` Kamil Dudka
  1 sibling, 0 replies; 30+ messages in thread
From: Kamil Dudka @ 2010-03-10 20:44 UTC (permalink / raw)
  To: Pavel Roskin; +Cc: Josh Triplett, Christopher Li, linux-sparse

On Wednesday 10 of March 2010 21:27:21 Pavel Roskin wrote:
> That makes sense for certain kinds of software, where crash is too
> costly, but in case of sparse, I'd rather see it crash than ignore a
> condition that may indicate an error elsewhere (perhaps both in sparse
> and in the code it checks).
>
> I thing using assert() would be a better approach.  It's already used in
> sparse.
>
> I checked the whole kernel using assert in place of the NULL checks, and
> there have been no crashes.

I completely agree with that.  Let's give some time to sparse developers to 
respond.  If nobody objects, I'll change it to asserts.

> Maybe you could add a test case for the first patch?  What warnings does
> it remove?  What is "="?  Is it assignment or initialization or both?

Both of them.  Maybe worth to add a test-case for the initialization to make 
it more obvious...

> How about comparisons?

Definitely not.  That was the main goal of the 0001-Wenum-to-int patch.  It 
was really noisy since enum values are first implicitly casted to ints and 
then compared.  So there was no way to type-safely compare two enums without 
producing 6 lines of warnings.

> A better description would be nice.

Sure thing.  I didn't realize it was ambiguous.

Thank you for the review!

Kamil

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-10 20:27                     ` Pavel Roskin
  2010-03-10 20:44                       ` Kamil Dudka
@ 2010-03-10 21:03                       ` Kamil Dudka
  1 sibling, 0 replies; 30+ messages in thread
From: Kamil Dudka @ 2010-03-10 21:03 UTC (permalink / raw)
  To: Pavel Roskin; +Cc: Josh Triplett, Christopher Li, linux-sparse

On Wednesday 10 of March 2010 21:27:21 Pavel Roskin wrote:
> Maybe you could add a test case for the first patch?  What warnings does
> it remove?

Sorry, I over looked that question.  It's already covered by the second patch.  
Try to revert it and you'll see the noise.  It had been perhaps more obvious 
before reordering of the patches :-)

Nevertheless I'll write a separate test-case for e.g. the comparison of enum 
values.  Let me know if you have idea of yet another test-cases to improve 
the coverage.

Kamil

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-10 16:05                   ` Kamil Dudka
  2010-03-10 20:27                     ` Pavel Roskin
@ 2010-03-10 21:56                     ` Christopher Li
  2010-03-13 17:22                       ` Kamil Dudka
  1 sibling, 1 reply; 30+ messages in thread
From: Christopher Li @ 2010-03-10 21:56 UTC (permalink / raw)
  To: Kamil Dudka; +Cc: Pavel Roskin, Josh Triplett, linux-sparse

On Wed, Mar 10, 2010 at 8:05 AM, Kamil Dudka <kdudka@redhat.com> wrote:
> On Wed March 10 2010 02:09:22 Pavel Roskin wrote:
>> I wonder is the quirk handling could be skipped for EXPR_SELECT, but I'm
>> fine with it if it's harmless.
>
> That's more likely question for sparse developers.  I have no test-case which
> goes through the EXPR_SELECT path, so that I can't test it actually.

Hi, Sorry for the late reply. I did play with your patch yesterday.
I believe this is the code we are talking about:

+warn_for_enum_conversions(struct expression *expr, struct symbol *type)
+{
+	switch (expr->type) {
+	case EXPR_CONDITIONAL:
+	case EXPR_SELECT:
+		do_warn_for_enum_conversions(expr->cond_true
+					     /* handle the "?:" quirk */
+					     ?: expr->conditional,
+					     type);
+		do_warn_for_enum_conversions(expr->cond_false, type);
+		break;
+
+	default:
+		do_warn_for_enum_conversions(expr, type);
+	}
+}
+

I feel that we shouldn't do special handling for EXPR_CONDITIONAL here.
We should just make the warn_for_enum_conversions more robust.
Special handing EXPR_CONDITIONAL here is just an one off thing.
What if there is a nested EXPR_CONDITIONAL inside the expr->cond_true?
See, your do_warn_for_enum_conversions() still need to handle
EXPR_CONDITIONAL any way. So I argue that we don't need that
special case for EXPR_CONDITIONAL here.

BTW, EXPR_SELECT only appear after expand stage. So you should not
see EXPR_SELECT in the evaluate stage.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-10 21:56                     ` Christopher Li
@ 2010-03-13 17:22                       ` Kamil Dudka
  2010-03-21 15:27                         ` Kamil Dudka
  0 siblings, 1 reply; 30+ messages in thread
From: Kamil Dudka @ 2010-03-13 17:22 UTC (permalink / raw)
  To: Christopher Li; +Cc: Pavel Roskin, Josh Triplett, linux-sparse

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

On Wednesday 10 of March 2010 22:56:15 Christopher Li wrote:
> I feel that we shouldn't do special handling for EXPR_CONDITIONAL here.
> We should just make the warn_for_enum_conversions more robust.
> Special handing EXPR_CONDITIONAL here is just an one off thing.
> What if there is a nested EXPR_CONDITIONAL inside the expr->cond_true?
> See, your do_warn_for_enum_conversions() still need to handle
> EXPR_CONDITIONAL any way. So I argue that we don't need that
> special case for EXPR_CONDITIONAL here.

Indeed, you're right.  This flaw should be fixed in the attached patch.

I was unsuscessfully looking for an implementation of stack within sparse.
So that I used the implicit one instead.  I know it's pretty bad idea, but
I am not aware of any easy way to handle it better.

> BTW, EXPR_SELECT only appear after expand stage. So you should not
> see EXPR_SELECT in the evaluate stage.

It makes sense, so that I've inserted assert(0) there.

Thank you for the review!  A pair of the improved patches is attached.

Kamil

[-- Attachment #2: 0001-Wenum-to-int-is-now-emitted-only-when-it-makes-sense.patch --]
[-- Type: text/x-diff, Size: 10336 bytes --]

From 73ff23e581cfb98b9460def7921b3c521b1a2410 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Sat, 13 Mar 2010 17:59:22 +0100
Subject: [PATCH 1/2] -Wenum-to-int is now emitted only when it makes sense

... namely in case of initialization, assignment and switch

Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 evaluate.c                 |    3 +-
 validation/enum-common.c   |    2 +
 validation/enum-from-int.c |   32 ++++++++++++------------
 validation/enum-mismatch.c |   58 ++++++++++++++++++++++----------------------
 validation/enum-to-int.c   |   39 ++++++++++++++++-------------
 5 files changed, 70 insertions(+), 64 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index d3d5e6f..eb1704a 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -330,7 +330,6 @@ static void
 warn_for_enum_conversions(struct expression *expr, struct symbol *type)
 {
 	warn_for_different_enum_types (expr, type);
-	warn_for_enum_to_int_conversion (expr, type);
 	warn_for_int_to_enum_conversion (expr, type);
 }
 
@@ -1447,6 +1446,7 @@ Err:
 	*rp = cast_to(*rp, target);
 	return 0;
 Cast:
+	warn_for_enum_to_int_conversion(*rp, target);
 	*rp = cast_to(*rp, target);
 	return 1;
 }
@@ -3326,6 +3326,7 @@ static void check_case_type(struct expression *switch_expr,
 	if (!switch_type || !case_type)
 		goto Bad;
 
+	warn_for_enum_to_int_conversion(case_expr, switch_type);
 	warn_for_enum_conversions(case_expr, switch_type);
 
 	sclass = classify_type(switch_type, &switch_type);
diff --git a/validation/enum-common.c b/validation/enum-common.c
index f940fef..4685cdf 100644
--- a/validation/enum-common.c
+++ b/validation/enum-common.c
@@ -27,6 +27,7 @@ static void always_ok(void)
 	var_a = (enum ENUM_TYPE_A) 0;
 	anon_enum_var = (__typeof__(anon_enum_var)) 0;
 	anon_enum_var = (__typeof__(anon_enum_var)) VALUE_A;
+	i = (var_a == VALUE_A);
 
 	switch (var_a) {
 		case VALUE_A:
@@ -94,6 +95,7 @@ static void trigger_int_to_enum_conversion(void)
 
 static void trigger_enum_to_int_conversion(void)
 {
+	int other = var_a;
 	i = var_a;
 	i = VALUE_B;
 	switch (i) {
diff --git a/validation/enum-from-int.c b/validation/enum-from-int.c
index 15b1e4d..4dc63a8 100644
--- a/validation/enum-from-int.c
+++ b/validation/enum-from-int.c
@@ -5,32 +5,32 @@
  * check-command: sparse -Wno-enum-mismatch $file
  *
  * check-error-start
-enum-common.c:84:45: warning: conversion of
-enum-common.c:84:45:     int to
-enum-common.c:84:45:     int enum ENUM_TYPE_A 
 enum-common.c:85:45: warning: conversion of
 enum-common.c:85:45:     int to
 enum-common.c:85:45:     int enum ENUM_TYPE_A 
-enum-common.c:82:22: warning: conversion of
-enum-common.c:82:22:     int to
-enum-common.c:82:22:     int enum ENUM_TYPE_A 
-enum-common.c:87:17: warning: conversion of
-enum-common.c:87:17:     int to
-enum-common.c:87:17:     int enum ENUM_TYPE_A 
+enum-common.c:86:45: warning: conversion of
+enum-common.c:86:45:     int to
+enum-common.c:86:45:     int enum ENUM_TYPE_A 
+enum-common.c:83:22: warning: conversion of
+enum-common.c:83:22:     int to
+enum-common.c:83:22:     int enum ENUM_TYPE_A 
 enum-common.c:88:17: warning: conversion of
 enum-common.c:88:17:     int to
-enum-common.c:88:17:     int enum ENUM_TYPE_B 
-enum-common.c:89:25: warning: conversion of
-enum-common.c:89:25:     int to
-enum-common.c:89:25:     int enum <noident> 
+enum-common.c:88:17:     int enum ENUM_TYPE_A 
+enum-common.c:89:17: warning: conversion of
+enum-common.c:89:17:     int to
+enum-common.c:89:17:     int enum ENUM_TYPE_B 
 enum-common.c:90:25: warning: conversion of
 enum-common.c:90:25:     int to
 enum-common.c:90:25:     int enum <noident> 
-enum-common.c:91:18: warning: conversion of
-enum-common.c:91:18:     int to
-enum-common.c:91:18:     int enum ENUM_TYPE_A 
+enum-common.c:91:25: warning: conversion of
+enum-common.c:91:25:     int to
+enum-common.c:91:25:     int enum <noident> 
 enum-common.c:92:18: warning: conversion of
 enum-common.c:92:18:     int to
 enum-common.c:92:18:     int enum ENUM_TYPE_A 
+enum-common.c:93:18: warning: conversion of
+enum-common.c:93:18:     int to
+enum-common.c:93:18:     int enum ENUM_TYPE_A 
  * check-error-end
  */
diff --git a/validation/enum-mismatch.c b/validation/enum-mismatch.c
index 6db016f..b1a4369 100644
--- a/validation/enum-mismatch.c
+++ b/validation/enum-mismatch.c
@@ -5,44 +5,44 @@
  * check-command: sparse -Wenum-mismatch -Wno-int-to-enum $file
  *
  * check-error-start
-enum-common.c:57:45: warning: mixing different enum types
-enum-common.c:57:45:     int enum ENUM_TYPE_B  versus
-enum-common.c:57:45:     int enum ENUM_TYPE_A 
 enum-common.c:58:45: warning: mixing different enum types
 enum-common.c:58:45:     int enum ENUM_TYPE_B  versus
 enum-common.c:58:45:     int enum ENUM_TYPE_A 
-enum-common.c:54:22: warning: mixing different enum types
-enum-common.c:54:22:     int enum ENUM_TYPE_B  versus
-enum-common.c:54:22:     int enum ENUM_TYPE_A 
+enum-common.c:59:45: warning: mixing different enum types
+enum-common.c:59:45:     int enum ENUM_TYPE_B  versus
+enum-common.c:59:45:     int enum ENUM_TYPE_A 
 enum-common.c:55:22: warning: mixing different enum types
-enum-common.c:55:22:     int enum <noident>  versus
+enum-common.c:55:22:     int enum ENUM_TYPE_B  versus
 enum-common.c:55:22:     int enum ENUM_TYPE_A 
-enum-common.c:64:45: warning: mixing different enum types
-enum-common.c:64:45:     int enum <noident>  versus
-enum-common.c:64:45:     int enum ENUM_TYPE_A 
+enum-common.c:56:22: warning: mixing different enum types
+enum-common.c:56:22:     int enum <noident>  versus
+enum-common.c:56:22:     int enum ENUM_TYPE_A 
 enum-common.c:65:45: warning: mixing different enum types
 enum-common.c:65:45:     int enum <noident>  versus
 enum-common.c:65:45:     int enum ENUM_TYPE_A 
-enum-common.c:62:22: warning: mixing different enum types
-enum-common.c:62:22:     int enum ENUM_TYPE_A  versus
-enum-common.c:62:22:     int enum <noident> 
-enum-common.c:69:17: warning: mixing different enum types
-enum-common.c:69:17:     int enum ENUM_TYPE_B  versus
-enum-common.c:69:17:     int enum ENUM_TYPE_A 
+enum-common.c:66:45: warning: mixing different enum types
+enum-common.c:66:45:     int enum <noident>  versus
+enum-common.c:66:45:     int enum ENUM_TYPE_A 
+enum-common.c:63:22: warning: mixing different enum types
+enum-common.c:63:22:     int enum ENUM_TYPE_A  versus
+enum-common.c:63:22:     int enum <noident> 
 enum-common.c:70:17: warning: mixing different enum types
-enum-common.c:70:17:     int enum <noident>  versus
-enum-common.c:70:17:     int enum ENUM_TYPE_B 
-enum-common.c:71:25: warning: mixing different enum types
-enum-common.c:71:25:     int enum ENUM_TYPE_A  versus
-enum-common.c:71:25:     int enum <noident> 
-enum-common.c:74:17: warning: mixing different enum types
-enum-common.c:74:17:     int enum ENUM_TYPE_B  versus
-enum-common.c:74:17:     int enum ENUM_TYPE_A 
+enum-common.c:70:17:     int enum ENUM_TYPE_B  versus
+enum-common.c:70:17:     int enum ENUM_TYPE_A 
+enum-common.c:71:17: warning: mixing different enum types
+enum-common.c:71:17:     int enum <noident>  versus
+enum-common.c:71:17:     int enum ENUM_TYPE_B 
+enum-common.c:72:25: warning: mixing different enum types
+enum-common.c:72:25:     int enum ENUM_TYPE_A  versus
+enum-common.c:72:25:     int enum <noident> 
 enum-common.c:75:17: warning: mixing different enum types
-enum-common.c:75:17:     int enum <noident>  versus
-enum-common.c:75:17:     int enum ENUM_TYPE_B 
-enum-common.c:76:25: warning: mixing different enum types
-enum-common.c:76:25:     int enum ENUM_TYPE_A  versus
-enum-common.c:76:25:     int enum <noident> 
+enum-common.c:75:17:     int enum ENUM_TYPE_B  versus
+enum-common.c:75:17:     int enum ENUM_TYPE_A 
+enum-common.c:76:17: warning: mixing different enum types
+enum-common.c:76:17:     int enum <noident>  versus
+enum-common.c:76:17:     int enum ENUM_TYPE_B 
+enum-common.c:77:25: warning: mixing different enum types
+enum-common.c:77:25:     int enum ENUM_TYPE_A  versus
+enum-common.c:77:25:     int enum <noident> 
  * check-error-end
  */
diff --git a/validation/enum-to-int.c b/validation/enum-to-int.c
index a981ce5..78a31e9 100644
--- a/validation/enum-to-int.c
+++ b/validation/enum-to-int.c
@@ -5,23 +5,26 @@
  * check-command: sparse -Wenum-to-int -Wno-enum-mismatch -Wno-int-to-enum $file
  *
  * check-error-start
-enum-common.c:97:13: warning: conversion of
-enum-common.c:97:13:     int enum ENUM_TYPE_A  to
-enum-common.c:97:13:     int
-enum-common.c:98:13: warning: conversion of
-enum-common.c:98:13:     int enum ENUM_TYPE_B  to
-enum-common.c:98:13:     int
-enum-common.c:103:34: warning: conversion of
-enum-common.c:103:34:     int enum ENUM_TYPE_A  to
-enum-common.c:103:34:     int
-enum-common.c:104:34: warning: conversion of
-enum-common.c:104:34:     int enum ENUM_TYPE_B  to
-enum-common.c:104:34:     int
-enum-common.c:100:22: warning: conversion of
-enum-common.c:100:22:     int enum ENUM_TYPE_A  to
-enum-common.c:100:22:     int
-enum-common.c:101:22: warning: conversion of
-enum-common.c:101:22:     int enum ENUM_TYPE_B  to
-enum-common.c:101:22:     int
+enum-common.c:98:21: warning: conversion of
+enum-common.c:98:21:     int enum ENUM_TYPE_A  to
+enum-common.c:98:21:     int
+enum-common.c:99:13: warning: conversion of
+enum-common.c:99:13:     int enum ENUM_TYPE_A  to
+enum-common.c:99:13:     int
+enum-common.c:100:13: warning: conversion of
+enum-common.c:100:13:     int enum ENUM_TYPE_B  to
+enum-common.c:100:13:     int
+enum-common.c:105:34: warning: conversion of
+enum-common.c:105:34:     int enum ENUM_TYPE_A  to
+enum-common.c:105:34:     int
+enum-common.c:106:34: warning: conversion of
+enum-common.c:106:34:     int enum ENUM_TYPE_B  to
+enum-common.c:106:34:     int
+enum-common.c:102:22: warning: conversion of
+enum-common.c:102:22:     int enum ENUM_TYPE_A  to
+enum-common.c:102:22:     int
+enum-common.c:103:22: warning: conversion of
+enum-common.c:103:22:     int enum ENUM_TYPE_B  to
+enum-common.c:103:22:     int
  * check-error-end
  */
-- 
1.7.0.2


[-- Attachment #3: 0002-Wenum-mismatch-now-works-better-with-conditional-op.patch --]
[-- Type: text/x-diff, Size: 15348 bytes --]

From 9dd4013046ef64d4ac7d09c336e69a3edf504ded Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Sat, 13 Mar 2010 18:07:51 +0100
Subject: [PATCH 2/2] -Wenum-mismatch now works better with conditional op.

It used to crash instead of reporting the warnings properly.  Reported
by Pavel Roskin.

Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 evaluate.c                 |   35 +++++++++++++---
 validation/enum-common.c   |   15 +++++++
 validation/enum-from-int.c |   63 ++++++++++++++++------------
 validation/enum-mismatch.c |  100 +++++++++++++++++++++++++++-----------------
 validation/enum-to-int.c   |   42 +++++++++---------
 5 files changed, 163 insertions(+), 92 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index eb1704a..ec96a71 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -8,6 +8,7 @@
  *
  * Evaluate constant expressions.
  */
+#include <assert.h>
 #include <stdlib.h>
 #include <stdarg.h>
 #include <stddef.h>
@@ -235,7 +236,7 @@ static int is_same_type(struct expression *expr, struct symbol *new)
 }
 
 static void
-resolve_sym_node (struct symbol **psym)
+resolve_sym_node(struct symbol **psym)
 {
 	struct symbol *sym = *psym;
 	if (sym->type == SYM_NODE)
@@ -243,7 +244,7 @@ resolve_sym_node (struct symbol **psym)
 }
 
 static void
-warn_for_different_enum_types (struct expression *expr, struct symbol *typeb)
+warn_for_different_enum_types(struct expression *expr, struct symbol *typeb)
 {
 	struct position pos = expr->pos;
 	struct symbol *typea = expr->ctype;
@@ -284,7 +285,7 @@ issue_conversion_warning(struct position pos,
 }
 
 static void
-warn_for_enum_to_int_conversion (struct expression *expr, struct symbol *typeb)
+warn_for_enum_to_int_conversion(struct expression *expr, struct symbol *typeb)
 {
 	struct position pos = expr->pos;
 	struct symbol *typea = expr->ctype;
@@ -307,7 +308,7 @@ warn_for_enum_to_int_conversion (struct expression *expr, struct symbol *typeb)
 }
 
 static void
-warn_for_int_to_enum_conversion (struct expression *expr, struct symbol *typeb)
+warn_for_int_to_enum_conversion(struct expression *expr, struct symbol *typeb)
 {
 	struct position pos = expr->pos;
 	struct symbol *typea = expr->ctype;
@@ -329,8 +330,30 @@ warn_for_int_to_enum_conversion (struct expression *expr, struct symbol *typeb)
 static void
 warn_for_enum_conversions(struct expression *expr, struct symbol *type)
 {
-	warn_for_different_enum_types (expr, type);
-	warn_for_int_to_enum_conversion (expr, type);
+	assert(expr);
+	assert(type);
+
+	/* look through implied cast(s) */
+	while (expr->type == EXPR_IMPLIED_CAST)
+		expr = expr->cast_expression;
+
+	switch (expr->type) {
+	case EXPR_SELECT:
+		/* EXPR_SELECT should not appear in the evaluate stage */
+		assert(0);
+
+	case EXPR_CONDITIONAL:
+		warn_for_enum_conversions(expr->cond_true
+					  /* handle the "?:" quirk */
+					  ?: expr->conditional,
+					  type);
+		warn_for_enum_conversions(expr->cond_false, type);
+		break;
+
+	default:
+		warn_for_different_enum_types(expr, type);
+		warn_for_int_to_enum_conversion(expr, type);
+	}
 }
 
 /*
diff --git a/validation/enum-common.c b/validation/enum-common.c
index 4685cdf..24fef59 100644
--- a/validation/enum-common.c
+++ b/validation/enum-common.c
@@ -47,6 +47,10 @@ static void always_ok(void)
 		default:
 			take_int(VALUE_C);
 	}
+
+	var_a = var_b ? var_a : VALUE_A;
+	var_b = VALUE_A ? VALUE_B : var_b;
+	take_enum_of_type_a(var_a ?: var_a);
 }
 
 static void trigger_enum_mismatch(void)
@@ -75,6 +79,13 @@ static void trigger_enum_mismatch(void)
 	var_a = VALUE_B;
 	var_b = VALUE_C;
 	anon_enum_var = VALUE_A;
+
+	// conditional operator
+	anon_enum_var = i ? VALUE_A : VALUE_B;
+	var_a = anon_enum_var ? VALUE_A : VALUE_B;
+	var_b = anon_enum_var ? VALUE_A : VALUE_B;
+	take_enum_of_type_a(var_a ?: var_b);
+	take_enum_of_type_a(var_b ?: var_a);
 }
 
 static void trigger_int_to_enum_conversion(void)
@@ -91,6 +102,10 @@ static void trigger_int_to_enum_conversion(void)
 	anon_enum_var = i;
 	var_a = (int) VALUE_A;
 	var_a = (int) VALUE_B;
+
+	// conditional operator
+	take_enum_of_type_a(var_a ?: i);
+	take_enum_of_type_a(i ?: var_a);
 }
 
 static void trigger_enum_to_int_conversion(void)
diff --git a/validation/enum-from-int.c b/validation/enum-from-int.c
index 4dc63a8..6465235 100644
--- a/validation/enum-from-int.c
+++ b/validation/enum-from-int.c
@@ -5,32 +5,41 @@
  * check-command: sparse -Wno-enum-mismatch $file
  *
  * check-error-start
-enum-common.c:85:45: warning: conversion of
-enum-common.c:85:45:     int to
-enum-common.c:85:45:     int enum ENUM_TYPE_A 
-enum-common.c:86:45: warning: conversion of
-enum-common.c:86:45:     int to
-enum-common.c:86:45:     int enum ENUM_TYPE_A 
-enum-common.c:83:22: warning: conversion of
-enum-common.c:83:22:     int to
-enum-common.c:83:22:     int enum ENUM_TYPE_A 
-enum-common.c:88:17: warning: conversion of
-enum-common.c:88:17:     int to
-enum-common.c:88:17:     int enum ENUM_TYPE_A 
-enum-common.c:89:17: warning: conversion of
-enum-common.c:89:17:     int to
-enum-common.c:89:17:     int enum ENUM_TYPE_B 
-enum-common.c:90:25: warning: conversion of
-enum-common.c:90:25:     int to
-enum-common.c:90:25:     int enum <noident> 
-enum-common.c:91:25: warning: conversion of
-enum-common.c:91:25:     int to
-enum-common.c:91:25:     int enum <noident> 
-enum-common.c:92:18: warning: conversion of
-enum-common.c:92:18:     int to
-enum-common.c:92:18:     int enum ENUM_TYPE_A 
-enum-common.c:93:18: warning: conversion of
-enum-common.c:93:18:     int to
-enum-common.c:93:18:     int enum ENUM_TYPE_A 
+enum-common.c:96:45: warning: conversion of
+enum-common.c:96:45:     int to
+enum-common.c:96:45:     int enum ENUM_TYPE_A 
+enum-common.c:97:45: warning: conversion of
+enum-common.c:97:45:     int to
+enum-common.c:97:45:     int enum ENUM_TYPE_A 
+enum-common.c:94:22: warning: conversion of
+enum-common.c:94:22:     int to
+enum-common.c:94:22:     int enum ENUM_TYPE_A 
+enum-common.c:99:17: warning: conversion of
+enum-common.c:99:17:     int to
+enum-common.c:99:17:     int enum ENUM_TYPE_A 
+enum-common.c:100:17: warning: conversion of
+enum-common.c:100:17:     int to
+enum-common.c:100:17:     int enum ENUM_TYPE_B 
+enum-common.c:101:25: warning: conversion of
+enum-common.c:101:25:     int to
+enum-common.c:101:25:     int enum <noident> 
+enum-common.c:102:25: warning: conversion of
+enum-common.c:102:25:     int to
+enum-common.c:102:25:     int enum <noident> 
+enum-common.c:103:18: warning: conversion of
+enum-common.c:103:18:     int to
+enum-common.c:103:18:     int enum ENUM_TYPE_A 
+enum-common.c:104:18: warning: conversion of
+enum-common.c:104:18:     int to
+enum-common.c:104:18:     int enum ENUM_TYPE_A 
+enum-common.c:107:38: warning: conversion of
+enum-common.c:107:38:     int to
+enum-common.c:107:38:     int enum ENUM_TYPE_A 
+enum-common.c:108:29: warning: conversion of
+enum-common.c:108:29:     int to
+enum-common.c:108:29:     int enum ENUM_TYPE_A 
+enum-common.c:108:29: warning: conversion of
+enum-common.c:108:29:     int to
+enum-common.c:108:29:     int enum ENUM_TYPE_A 
  * check-error-end
  */
diff --git a/validation/enum-mismatch.c b/validation/enum-mismatch.c
index b1a4369..d8d34dc 100644
--- a/validation/enum-mismatch.c
+++ b/validation/enum-mismatch.c
@@ -5,44 +5,68 @@
  * check-command: sparse -Wenum-mismatch -Wno-int-to-enum $file
  *
  * check-error-start
-enum-common.c:58:45: warning: mixing different enum types
-enum-common.c:58:45:     int enum ENUM_TYPE_B  versus
-enum-common.c:58:45:     int enum ENUM_TYPE_A 
-enum-common.c:59:45: warning: mixing different enum types
-enum-common.c:59:45:     int enum ENUM_TYPE_B  versus
-enum-common.c:59:45:     int enum ENUM_TYPE_A 
-enum-common.c:55:22: warning: mixing different enum types
-enum-common.c:55:22:     int enum ENUM_TYPE_B  versus
-enum-common.c:55:22:     int enum ENUM_TYPE_A 
-enum-common.c:56:22: warning: mixing different enum types
-enum-common.c:56:22:     int enum <noident>  versus
-enum-common.c:56:22:     int enum ENUM_TYPE_A 
-enum-common.c:65:45: warning: mixing different enum types
-enum-common.c:65:45:     int enum <noident>  versus
-enum-common.c:65:45:     int enum ENUM_TYPE_A 
-enum-common.c:66:45: warning: mixing different enum types
-enum-common.c:66:45:     int enum <noident>  versus
-enum-common.c:66:45:     int enum ENUM_TYPE_A 
-enum-common.c:63:22: warning: mixing different enum types
-enum-common.c:63:22:     int enum ENUM_TYPE_A  versus
-enum-common.c:63:22:     int enum <noident> 
-enum-common.c:70:17: warning: mixing different enum types
-enum-common.c:70:17:     int enum ENUM_TYPE_B  versus
-enum-common.c:70:17:     int enum ENUM_TYPE_A 
-enum-common.c:71:17: warning: mixing different enum types
-enum-common.c:71:17:     int enum <noident>  versus
-enum-common.c:71:17:     int enum ENUM_TYPE_B 
-enum-common.c:72:25: warning: mixing different enum types
-enum-common.c:72:25:     int enum ENUM_TYPE_A  versus
-enum-common.c:72:25:     int enum <noident> 
+enum-common.c:62:45: warning: mixing different enum types
+enum-common.c:62:45:     int enum ENUM_TYPE_B  versus
+enum-common.c:62:45:     int enum ENUM_TYPE_A 
+enum-common.c:63:45: warning: mixing different enum types
+enum-common.c:63:45:     int enum ENUM_TYPE_B  versus
+enum-common.c:63:45:     int enum ENUM_TYPE_A 
+enum-common.c:59:22: warning: mixing different enum types
+enum-common.c:59:22:     int enum ENUM_TYPE_B  versus
+enum-common.c:59:22:     int enum ENUM_TYPE_A 
+enum-common.c:60:22: warning: mixing different enum types
+enum-common.c:60:22:     int enum <noident>  versus
+enum-common.c:60:22:     int enum ENUM_TYPE_A 
+enum-common.c:69:45: warning: mixing different enum types
+enum-common.c:69:45:     int enum <noident>  versus
+enum-common.c:69:45:     int enum ENUM_TYPE_A 
+enum-common.c:70:45: warning: mixing different enum types
+enum-common.c:70:45:     int enum <noident>  versus
+enum-common.c:70:45:     int enum ENUM_TYPE_A 
+enum-common.c:67:22: warning: mixing different enum types
+enum-common.c:67:22:     int enum ENUM_TYPE_A  versus
+enum-common.c:67:22:     int enum <noident> 
+enum-common.c:74:17: warning: mixing different enum types
+enum-common.c:74:17:     int enum ENUM_TYPE_B  versus
+enum-common.c:74:17:     int enum ENUM_TYPE_A 
 enum-common.c:75:17: warning: mixing different enum types
-enum-common.c:75:17:     int enum ENUM_TYPE_B  versus
-enum-common.c:75:17:     int enum ENUM_TYPE_A 
-enum-common.c:76:17: warning: mixing different enum types
-enum-common.c:76:17:     int enum <noident>  versus
-enum-common.c:76:17:     int enum ENUM_TYPE_B 
-enum-common.c:77:25: warning: mixing different enum types
-enum-common.c:77:25:     int enum ENUM_TYPE_A  versus
-enum-common.c:77:25:     int enum <noident> 
+enum-common.c:75:17:     int enum <noident>  versus
+enum-common.c:75:17:     int enum ENUM_TYPE_B 
+enum-common.c:76:25: warning: mixing different enum types
+enum-common.c:76:25:     int enum ENUM_TYPE_A  versus
+enum-common.c:76:25:     int enum <noident> 
+enum-common.c:79:17: warning: mixing different enum types
+enum-common.c:79:17:     int enum ENUM_TYPE_B  versus
+enum-common.c:79:17:     int enum ENUM_TYPE_A 
+enum-common.c:80:17: warning: mixing different enum types
+enum-common.c:80:17:     int enum <noident>  versus
+enum-common.c:80:17:     int enum ENUM_TYPE_B 
+enum-common.c:81:25: warning: mixing different enum types
+enum-common.c:81:25:     int enum ENUM_TYPE_A  versus
+enum-common.c:81:25:     int enum <noident> 
+enum-common.c:84:29: warning: mixing different enum types
+enum-common.c:84:29:     int enum ENUM_TYPE_A  versus
+enum-common.c:84:29:     int enum <noident> 
+enum-common.c:84:39: warning: mixing different enum types
+enum-common.c:84:39:     int enum ENUM_TYPE_B  versus
+enum-common.c:84:39:     int enum <noident> 
+enum-common.c:85:43: warning: mixing different enum types
+enum-common.c:85:43:     int enum ENUM_TYPE_B  versus
+enum-common.c:85:43:     int enum ENUM_TYPE_A 
+enum-common.c:86:33: warning: mixing different enum types
+enum-common.c:86:33:     int enum ENUM_TYPE_A  versus
+enum-common.c:86:33:     int enum ENUM_TYPE_B 
+enum-common.c:87:29: warning: mixing different enum types
+enum-common.c:87:29:     int enum ENUM_TYPE_A  versus
+enum-common.c:87:29:     int enum ENUM_TYPE_B 
+enum-common.c:87:38: warning: mixing different enum types
+enum-common.c:87:38:     int enum ENUM_TYPE_B  versus
+enum-common.c:87:38:     int enum ENUM_TYPE_A 
+enum-common.c:88:29: warning: mixing different enum types
+enum-common.c:88:29:     int enum ENUM_TYPE_B  versus
+enum-common.c:88:29:     int enum ENUM_TYPE_A 
+enum-common.c:88:29: warning: mixing different enum types
+enum-common.c:88:29:     int enum ENUM_TYPE_B  versus
+enum-common.c:88:29:     int enum ENUM_TYPE_A 
  * check-error-end
  */
diff --git a/validation/enum-to-int.c b/validation/enum-to-int.c
index 78a31e9..68c43fd 100644
--- a/validation/enum-to-int.c
+++ b/validation/enum-to-int.c
@@ -5,26 +5,26 @@
  * check-command: sparse -Wenum-to-int -Wno-enum-mismatch -Wno-int-to-enum $file
  *
  * check-error-start
-enum-common.c:98:21: warning: conversion of
-enum-common.c:98:21:     int enum ENUM_TYPE_A  to
-enum-common.c:98:21:     int
-enum-common.c:99:13: warning: conversion of
-enum-common.c:99:13:     int enum ENUM_TYPE_A  to
-enum-common.c:99:13:     int
-enum-common.c:100:13: warning: conversion of
-enum-common.c:100:13:     int enum ENUM_TYPE_B  to
-enum-common.c:100:13:     int
-enum-common.c:105:34: warning: conversion of
-enum-common.c:105:34:     int enum ENUM_TYPE_A  to
-enum-common.c:105:34:     int
-enum-common.c:106:34: warning: conversion of
-enum-common.c:106:34:     int enum ENUM_TYPE_B  to
-enum-common.c:106:34:     int
-enum-common.c:102:22: warning: conversion of
-enum-common.c:102:22:     int enum ENUM_TYPE_A  to
-enum-common.c:102:22:     int
-enum-common.c:103:22: warning: conversion of
-enum-common.c:103:22:     int enum ENUM_TYPE_B  to
-enum-common.c:103:22:     int
+enum-common.c:113:21: warning: conversion of
+enum-common.c:113:21:     int enum ENUM_TYPE_A  to
+enum-common.c:113:21:     int
+enum-common.c:114:13: warning: conversion of
+enum-common.c:114:13:     int enum ENUM_TYPE_A  to
+enum-common.c:114:13:     int
+enum-common.c:115:13: warning: conversion of
+enum-common.c:115:13:     int enum ENUM_TYPE_B  to
+enum-common.c:115:13:     int
+enum-common.c:120:34: warning: conversion of
+enum-common.c:120:34:     int enum ENUM_TYPE_A  to
+enum-common.c:120:34:     int
+enum-common.c:121:34: warning: conversion of
+enum-common.c:121:34:     int enum ENUM_TYPE_B  to
+enum-common.c:121:34:     int
+enum-common.c:117:22: warning: conversion of
+enum-common.c:117:22:     int enum ENUM_TYPE_A  to
+enum-common.c:117:22:     int
+enum-common.c:118:22: warning: conversion of
+enum-common.c:118:22:     int enum ENUM_TYPE_B  to
+enum-common.c:118:22:     int
  * check-error-end
  */
-- 
1.7.0.2


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-13 17:22                       ` Kamil Dudka
@ 2010-03-21 15:27                         ` Kamil Dudka
  2010-03-24 10:07                           ` Christopher Li
  0 siblings, 1 reply; 30+ messages in thread
From: Kamil Dudka @ 2010-03-21 15:27 UTC (permalink / raw)
  To: Christopher Li; +Cc: Pavel Roskin, Josh Triplett, linux-sparse

On Saturday 13 of March 2010 18:22:48 Kamil Dudka wrote:
> I was unsuscessfully looking for an implementation of stack within sparse.
> So that I used the implicit one instead.  I know it's pretty bad idea, but
> I am not aware of any easy way to handle it better.

I could probably use a list instead.  Nevertheless the evaluation of EXPR_COND 
also works recursively on the runtime stack - see evaluate_expression() and 
evaluate_conditional_expression().  So that I don't think it's an actual 
problem here.

Let me know if the patch needs some additional improvements.

Kamil

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-21 15:27                         ` Kamil Dudka
@ 2010-03-24 10:07                           ` Christopher Li
  2010-03-24 10:51                             ` Kamil Dudka
  2010-03-27  9:16                             ` Kamil Dudka
  0 siblings, 2 replies; 30+ messages in thread
From: Christopher Li @ 2010-03-24 10:07 UTC (permalink / raw)
  To: Kamil Dudka; +Cc: Pavel Roskin, Josh Triplett, linux-sparse

On Sun, Mar 21, 2010 at 8:27 AM, Kamil Dudka <kdudka@redhat.com> wrote:
> On Saturday 13 of March 2010 18:22:48 Kamil Dudka wrote:
>> I was unsuscessfully looking for an implementation of stack within sparse.
>> So that I used the implicit one instead.  I know it's pretty bad idea, but
>> I am not aware of any easy way to handle it better.
>
> I could probably use a list instead.  Nevertheless the evaluation of EXPR_COND
> also works recursively on the runtime stack - see evaluate_expression() and
> evaluate_conditional_expression().  So that I don't think it's an actual
> problem here.
>
> Let me know if the patch needs some additional improvements.


Hi Kamil,

I take a look at the new patch. BTW, you don't need to do assert(0),
you can use die() for that.

Currently if I run sparse over the parse.c, it will generate a lot of
new warnings.

$ ./sparse -D__x86_64__=1 parse.c
parse.c:1564:67: warning: conversion of
parse.c:1564:67:     int to
parse.c:1564:67:     int enum namespace
expression.h:202:76: warning: conversion of
expression.h:202:76:     int to
expression.h:202:76:     int enum namespace
expression.h:202:76: warning: conversion of
expression.h:202:76:     int to
expression.h:202:76:     int enum namespace
parse.c:182:30: warning: conversion of
parse.c:182:30:     int to
parse.c:182:30:     int enum keyword
parse.c:208:30: warning: conversion of
parse.c:208:30:     int to
parse.c:208:30:     int enum keyword
parse.c:215:30: warning: conversion of
parse.c:215:30:     int to
parse.c:215:30:     int enum keyword
parse.c:235:30: warning: conversion of
parse.c:235:30:     int to
parse.c:235:30:     int enum keyword
parse.c:482:12: warning: symbol 'ignored_attributes' was not declared.
Should it be static?
parse.c:619:22: warning: conversion of
parse.c:619:22:     int to
parse.c:619:22:     int enum statement_type
parse.c:1426:61: warning: conversion of
parse.c:1426:61:     int to
parse.c:1426:61:     int enum namespace
expression.h:202:76: warning: conversion of
expression.h:202:76:     int to
expression.h:202:76:     int enum namespace
parse.c:1533:67: warning: conversion of
parse.c:1533:67:     int to
parse.c:1533:67:     int enum namespace
expression.h:202:76: warning: conversion of
expression.h:202:76:     int to
expression.h:202:76:     int enum namespace
expression.h:202:76: warning: conversion of
expression.h:202:76:     int to
expression.h:202:76:     int enum namespace
expression.h:202:76: warning: conversion of
expression.h:202:76:     int to
expression.h:202:76:     int enum namespace


That is just too much. Most of the warning is coming from enum or operation.
e.g.
.type = KW_SPECIFIER | KW_SHORT,
lookup_keyword(token->ident, NS_KEYWORD | NS_TYPEDEF);

I think those source are fine. Force enum cast there make the source code
lworse. We can add special case for enum OR enum, more special cases.
But where is the end?

It comes down to we try to treat enum as a different type than int. It seems
works for the simple case. But in real world, people do use enum as int type
and expect enum can mix with int.

At this point I am leaning towards moving this enum warning to a topic branch.
Let the people who want to use it play with it first. It currently
give too many warning.
If people do feel that warning is useful, even bette, it catch some real bugs. I
will consider merge it again.

You obvious spend a lot of time on this. I feel bad about pushing it back too.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-24 10:07                           ` Christopher Li
@ 2010-03-24 10:51                             ` Kamil Dudka
  2010-03-27  9:16                             ` Kamil Dudka
  1 sibling, 0 replies; 30+ messages in thread
From: Kamil Dudka @ 2010-03-24 10:51 UTC (permalink / raw)
  To: Christopher Li; +Cc: Pavel Roskin, Josh Triplett, linux-sparse

Hi Chris,

On Wed March 24 2010 11:07:04 Christopher Li wrote:
> That is just too much. Most of the warning is coming from enum or
> operation. e.g.
> .type = KW_SPECIFIER | KW_SHORT,
> lookup_keyword(token->ident, NS_KEYWORD | NS_TYPEDEF);
> 
> I think those source are fine. Force enum cast there make the source code

I completely agree on that.

> lworse. We can add special case for enum OR enum, more special cases.
> But where is the end?

Let's handle the bitwise operators and we'll see.

> It comes down to we try to treat enum as a different type than int. It

From my point of view it _is_ different type than int.  At least gcc 
distinguishes among enum and int on the level I grab the type-info.

> seems works for the simple case. But in real world, people do use enum as
> int type and expect enum can mix with int.

What about just changing the default?  We can keep the current -Wenum-mismatch 
(in its working variant) and those two new warnings provide only as option.

> At this point I am leaning towards moving this enum warning to a topic
> branch. Let the people who want to use it play with it first. It currently
> give too many warning.

Sure.

> If people do feel that warning is useful, even bette, it catch some real
> bugs. I will consider merge it again.

Yes, it makes sense to me.

> You obvious spend a lot of time on this. I feel bad about pushing it back
> too.

Spent time should be definitely not considered as a criterion
for inclusion ;-)

Kamil

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-24 10:07                           ` Christopher Li
  2010-03-24 10:51                             ` Kamil Dudka
@ 2010-03-27  9:16                             ` Kamil Dudka
  2010-03-27  9:29                               ` Josh Triplett
  1 sibling, 1 reply; 30+ messages in thread
From: Kamil Dudka @ 2010-03-27  9:16 UTC (permalink / raw)
  To: Christopher Li; +Cc: Pavel Roskin, Josh Triplett, linux-sparse

On Wednesday 24 of March 2010 11:07:04 Christopher Li wrote:
> That is just too much. Most of the warning is coming from enum or
> operation. e.g.
> .type = KW_SPECIFIER | KW_SHORT,
> lookup_keyword(token->ident, NS_KEYWORD | NS_TYPEDEF);

Looking once again, I don't think that's the case.  The warnings do not come 
from the "enum or" operation, but they come from passing the integral result 
to enum variable (or arg).  Instead of weaking my patch, we may improve the 
code of sparse and add explicit casts back to enum:

    .type = (enum keyword) (KW_SPECIFIER | KW_SHORT),
    lookup_keyword(token->ident, (enum namespace) (NS_KEYWORD | NS_TYPEDEF));


The whole problem can be narrowed down to a simple test-case:

    int main()
    {
        enum {
            A = 0x1,
            B = 0x2
        } val = A | B;

        return val;
    }


Here is what sparse gives:
$ ./sparse enum.c
enum.c:1:10: warning: non-ANSI function declaration of function 'main'
enum.c:6:15: warning: conversion of
enum.c:6:15:     int to
enum.c:6:15:     int enum <noident>


Here is what g++ gives:
$ g++ enum.c
enum.c: In function ‘int main()’:
enum.c:6: error: invalid conversion from ‘int’ to ‘main()::<anonymous enum>’

Kamil
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-27  9:16                             ` Kamil Dudka
@ 2010-03-27  9:29                               ` Josh Triplett
  2010-03-27  9:53                                 ` [PATCH] eliminate insane conversions from int to enum Kamil Dudka
  2010-03-29 18:05                                 ` Sparse crash when mixing int and enum in ternary operator Christopher Li
  0 siblings, 2 replies; 30+ messages in thread
From: Josh Triplett @ 2010-03-27  9:29 UTC (permalink / raw)
  To: Kamil Dudka; +Cc: Christopher Li, Pavel Roskin, linux-sparse

On Sat, Mar 27, 2010 at 10:16:38AM +0100, Kamil Dudka wrote:
> On Wednesday 24 of March 2010 11:07:04 Christopher Li wrote:
> > That is just too much. Most of the warning is coming from enum or
> > operation. e.g.
> > .type = KW_SPECIFIER | KW_SHORT,
> > lookup_keyword(token->ident, NS_KEYWORD | NS_TYPEDEF);
> 
> Looking once again, I don't think that's the case.  The warnings do not come 
> from the "enum or" operation, but they come from passing the integral result 
> to enum variable (or arg).  Instead of weaking my patch, we may improve the 
> code of sparse and add explicit casts back to enum:
> 
>     .type = (enum keyword) (KW_SPECIFIER | KW_SHORT),
>     lookup_keyword(token->ident, (enum namespace) (NS_KEYWORD | NS_TYPEDEF));

That looks wrong.  .type doesn't contain a value of type "enum keyword",
it contains the bitwise or of such values, which won't represent a valid
enum value.  Thus, .type should have an integral type, not an enum type.
The same goes for the second parameter of lookup_keyword.

> The whole problem can be narrowed down to a simple test-case:
> 
>     int main()
>     {
>         enum {
>             A = 0x1,
>             B = 0x2
>         } val = A | B;

This code seems semantically wrong, as described above.  val should only
have values 0x1 or 0x2, and you've assigned it 0x3, which doesn't
represent a valid value of its enum type.

> Here is what sparse gives:
> $ ./sparse enum.c
> enum.c:1:10: warning: non-ANSI function declaration of function 'main'
> enum.c:6:15: warning: conversion of
> enum.c:6:15:     int to
> enum.c:6:15:     int enum <noident>
> 
> 
> Here is what g++ gives:
> $ g++ enum.c
> enum.c: In function ‘int main()’:
> enum.c:6: error: invalid conversion from ‘int’ to ‘main()::<anonymous enum>’

Yup, both of these warnings seem correct.  Don't fix them by casting,
fix them by declaring "val" with an appropriate integral type.

- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH] eliminate insane conversions from int to enum
  2010-03-27  9:29                               ` Josh Triplett
@ 2010-03-27  9:53                                 ` Kamil Dudka
  2010-03-29 18:11                                   ` Christopher Li
  2010-03-29 18:05                                 ` Sparse crash when mixing int and enum in ternary operator Christopher Li
  1 sibling, 1 reply; 30+ messages in thread
From: Kamil Dudka @ 2010-03-27  9:53 UTC (permalink / raw)
  To: Josh Triplett; +Cc: Christopher Li, Pavel Roskin, linux-sparse

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

On Saturday 27 of March 2010 10:29:50 Josh Triplett wrote:
> > Here is what sparse gives:
> > $ ./sparse enum.c
> > enum.c:1:10: warning: non-ANSI function declaration of function 'main'
> > enum.c:6:15: warning: conversion of
> > enum.c:6:15:     int to
> > enum.c:6:15:     int enum <noident>
> >
> >
> > Here is what g++ gives:
> > $ g++ enum.c
> > enum.c: In function ‘int main()’:
> > enum.c:6: error: invalid conversion from ‘int’ to ‘main()::<anonymous
> > enum>’
>
> Yup, both of these warnings seem correct.  Don't fix them by casting,
> fix them by declaring "val" with an appropriate integral type.

patch attached

Kamil

[-- Attachment #2: 0001-eliminate-insane-conversions-from-int-to-enum.patch --]
[-- Type: text/x-diff, Size: 3461 bytes --]

From 44d964b9479affff31ad5690bedfe580ee3b50fa Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Sat, 27 Mar 2010 10:48:34 +0100
Subject: [PATCH] eliminate insane conversions from int to enum


Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 expression.h |    1 -
 parse.c      |    2 +-
 parse.h      |    2 ++
 symbol.c     |    4 ++--
 symbol.h     |    8 ++++----
 5 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/expression.h b/expression.h
index 81f70ad..fe08a5f 100644
--- a/expression.h
+++ b/expression.h
@@ -206,7 +206,6 @@ static inline int lookup_type(struct token *token)
 }
 
 /* Statement parsing */
-struct statement *alloc_statement(struct position pos, int type);
 struct token *initializer(struct expression **tree, struct token *token);
 struct token *compound_statement(struct token *, struct statement *);
 
diff --git a/parse.c b/parse.c
index 7c6dce7..adaa132 100644
--- a/parse.c
+++ b/parse.c
@@ -613,7 +613,7 @@ static int SENTINEL_ATTR match_idents(struct token *token, ...)
 }
 
 
-struct statement *alloc_statement(struct position pos, int type)
+struct statement *alloc_statement(struct position pos, enum statement_type type)
 {
 	struct statement *stmt = __alloc_statement(0);
 	stmt->type = type;
diff --git a/parse.h b/parse.h
index 02b8585..6d51726 100644
--- a/parse.h
+++ b/parse.h
@@ -115,6 +115,8 @@ struct statement {
 	};
 };
 
+struct statement *alloc_statement(struct position pos, enum statement_type type);
+
 extern struct symbol_list *function_computed_target_list;
 extern struct statement_list *function_computed_goto_list;
 
diff --git a/symbol.c b/symbol.c
index 96dfbfa..8c46e20 100644
--- a/symbol.c
+++ b/symbol.c
@@ -39,12 +39,12 @@ void access_symbol(struct symbol *sym)
 	}
 }
 
-struct symbol *lookup_symbol(struct ident *ident, enum namespace ns)
+struct symbol *lookup_symbol(struct ident *ident, int ns_mask)
 {
 	struct symbol *sym;
 
 	for (sym = ident->symbols; sym; sym = sym->next_id) {
-		if (sym->namespace & ns) {
+		if (sym->namespace & ns_mask) {
 			sym->used = 1;
 			return sym;
 		}
diff --git a/symbol.h b/symbol.h
index e567305..7921aa1 100644
--- a/symbol.h
+++ b/symbol.h
@@ -97,7 +97,7 @@ struct decl_state {
 };
 
 struct symbol_op {
-	enum keyword type;
+	int type;
 	int (*evaluate)(struct expression *);
 	int (*expand)(struct expression *, int);
 	int (*args)(struct expression *);
@@ -267,7 +267,7 @@ extern void access_symbol(struct symbol *);
 extern const char * type_difference(struct ctype *c1, struct ctype *c2,
 	unsigned long mod1, unsigned long mod2);
 
-extern struct symbol *lookup_symbol(struct ident *, enum namespace);
+extern struct symbol *lookup_symbol(struct ident *, int ns_mask);
 extern struct symbol *create_symbol(int stream, const char *name, int type, int namespace);
 extern void init_symbols(void);
 extern void init_ctype(void);
@@ -367,11 +367,11 @@ static inline int get_sym_type(struct symbol *type)
 	return type->type;
 }
 
-static inline struct symbol *lookup_keyword(struct ident *ident, enum namespace ns)
+static inline struct symbol *lookup_keyword(struct ident *ident, int ns_mask)
 {
 	if (!ident->keyword)
 		return NULL;
-	return lookup_symbol(ident, ns);
+	return lookup_symbol(ident, ns_mask);
 }
 
 #define is_restricted_type(type) (get_sym_type(type) == SYM_RESTRICT)
-- 
1.7.0.2


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-27  9:29                               ` Josh Triplett
  2010-03-27  9:53                                 ` [PATCH] eliminate insane conversions from int to enum Kamil Dudka
@ 2010-03-29 18:05                                 ` Christopher Li
  2010-03-29 18:17                                   ` Kamil Dudka
  2010-03-29 21:26                                   ` Josh Triplett
  1 sibling, 2 replies; 30+ messages in thread
From: Christopher Li @ 2010-03-29 18:05 UTC (permalink / raw)
  To: Josh Triplett; +Cc: Kamil Dudka, Pavel Roskin, linux-sparse

On Sat, Mar 27, 2010 at 2:29 AM, Josh Triplett <josh@joshtriplett.org> wrote:
>>     .type = (enum keyword) (KW_SPECIFIER | KW_SHORT),
>>     lookup_keyword(token->ident, (enum namespace) (NS_KEYWORD | NS_TYPEDEF));
>
> That looks wrong.  .type doesn't contain a value of type "enum keyword",
> it contains the bitwise or of such values, which won't represent a valid
> enum value.  Thus, .type should have an integral type, not an enum type.
> The same goes for the second parameter of lookup_keyword.

According to the new "strict enum" rules which don't allow enum type to accept
value other than the enum list above, that is right.

But I argue that this strict enum rules does not make sense, after this change,
the code don't not gain any thing. In fact, it looks worse.

Sparse use a loosely define enum namespace for member "namespace". It means
it has all this basic enum value, from NS_MACRO to NS_KEYWORD. In my mind,
all combination of this value consider enum namespace as well. It is
just that I can't
list all of them in the enum list.

Using enum namespace for member "namespace" has benefit here. It is clear that
which set of value it belongs to. E.g. if you assign SYM_NODE into "namespace"
member it *looks* is obvious wrong.

It also helps people understand which set of value belongs to
"namespace" member.
Make "namespace" a plain int, that message is lost. It become very
confusing for new comer
what value was allowed in this int type.

So back to my point. It seems making the enum more strict is just make up rules
and gain nothing in real life. It makes code looks worse just to make
strict enum type
happy.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] eliminate insane conversions from int to enum
  2010-03-27  9:53                                 ` [PATCH] eliminate insane conversions from int to enum Kamil Dudka
@ 2010-03-29 18:11                                   ` Christopher Li
  0 siblings, 0 replies; 30+ messages in thread
From: Christopher Li @ 2010-03-29 18:11 UTC (permalink / raw)
  To: Kamil Dudka; +Cc: Josh Triplett, Pavel Roskin, linux-sparse

On Sat, Mar 27, 2010 at 2:53 AM, Kamil Dudka <kdudka@redhat.com> wrote:
> On Saturday 27 of March 2010 10:29:50 Josh Triplett wrote:
>> > Here is what sparse gives:
>> > $ ./sparse enum.c
>> > enum.c:1:10: warning: non-ANSI function declaration of function 'main'
>> > enum.c:6:15: warning: conversion of
>> > enum.c:6:15:     int to
>> > enum.c:6:15:     int enum <noident>
>> >
>> >
>> > Here is what g++ gives:
>> > $ g++ enum.c
>> > enum.c: In function ‘int main()’:
>> > enum.c:6: error: invalid conversion from ‘int’ to ‘main()::<anonymous
>> > enum>’
>>
>> Yup, both of these warnings seem correct.  Don't fix them by casting,
>> fix them by declaring "val" with an appropriate integral type.
>
> patch attached

I mention that in my other email. I don't agree this is a better change.
It is just for the sake of making the enum warning happy and make code
harder to understand.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-29 18:05                                 ` Sparse crash when mixing int and enum in ternary operator Christopher Li
@ 2010-03-29 18:17                                   ` Kamil Dudka
  2010-03-29 18:48                                     ` Christopher Li
  2010-03-29 21:26                                   ` Josh Triplett
  1 sibling, 1 reply; 30+ messages in thread
From: Kamil Dudka @ 2010-03-29 18:17 UTC (permalink / raw)
  To: Christopher Li; +Cc: Josh Triplett, Pavel Roskin, linux-sparse

On Mon March 29 2010 20:05:08 Christopher Li wrote:
> Using enum namespace for member "namespace" has benefit here. It is clear
> that which set of value it belongs to. E.g. if you assign SYM_NODE into
> "namespace" member it *looks* is obvious wrong.

We are able to catch assignment of SYM_NODE to 'enum namespace'.  But we are 
not able to catch (SYM_NODE | SYM_ENUM) to 'enum namespace', so that the patch 
makes no difference.  Or am I missing anything?

> It also helps people understand which set of value belongs to
> "namespace" member.
> Make "namespace" a plain int, that message is lost. It become very
> confusing for new comer
> what value was allowed in this int type.

The identifier ns_mask sounds clear to me, maybe namespace_mask would be 
better.  But yes ... I am not a new comer here :-)

> So back to my point. It seems making the enum more strict is just make up
> rules and gain nothing in real life. It makes code looks worse just to
> make strict enum type
> happy.

I don't think the code looks worse, nevertheless respect your attitude.

Kamil

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-29 18:17                                   ` Kamil Dudka
@ 2010-03-29 18:48                                     ` Christopher Li
  2010-03-29 19:23                                       ` Kamil Dudka
  0 siblings, 1 reply; 30+ messages in thread
From: Christopher Li @ 2010-03-29 18:48 UTC (permalink / raw)
  To: Kamil Dudka; +Cc: Josh Triplett, Pavel Roskin, linux-sparse

On Mon, Mar 29, 2010 at 11:17 AM, Kamil Dudka <kdudka@redhat.com> wrote:
> On Mon March 29 2010 20:05:08 Christopher Li wrote:
>> Using enum namespace for member "namespace" has benefit here. It is clear
>> that which set of value it belongs to. E.g. if you assign SYM_NODE into
>> "namespace" member it *looks* is obvious wrong.
>
> We are able to catch assignment of SYM_NODE to 'enum namespace'.  But we are
> not able to catch (SYM_NODE | SYM_ENUM) to 'enum namespace', so that the patch
> makes no difference.  Or am I missing anything?

I am refering to your patch in other email:

struct symbol_op {
-          enum keyword type;
+         int type;

That is worse. Because you make type as plain int, I can assign NS_KEYWORD there
and you wouldn't able to catch any thing right? So you make the enum
so strict that
we can't use it any more.

Currently the compiler might not able to catch it. But it look obvious
wrong to human
eye if you assign other class of enum there.

>
> I don't think the code looks worse, nevertheless respect your attitude.

See my previous comment.

I just want to make sense of this thing. Currently I see a lot of down sides,
forcing people to do more explicit cast or avoid using the enum type completely.
What is the up side again?

Remember, too much warning can be a bad thing as well. It makes people
don't want
to use the tool or just turn off the warning completely. When I run sparse over
its own source code, I consider those warning useless because I don't
have a better
way to fix it. I think the source code is fine as it is.

BTW, I have move the enum warning to a new branch:

http://git.kernel.org/?p=devel/sparse/sparse.enum-warning.git;a=summary

Some people might find it useful in special occasions. I want to heard about it.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-29 18:48                                     ` Christopher Li
@ 2010-03-29 19:23                                       ` Kamil Dudka
  2010-03-30  5:29                                         ` Pavel Roskin
  0 siblings, 1 reply; 30+ messages in thread
From: Kamil Dudka @ 2010-03-29 19:23 UTC (permalink / raw)
  To: Christopher Li; +Cc: Josh Triplett, Pavel Roskin, linux-sparse

On Mon March 29 2010 20:48:38 Christopher Li wrote:
> On Mon, Mar 29, 2010 at 11:17 AM, Kamil Dudka <kdudka@redhat.com> wrote:
> > On Mon March 29 2010 20:05:08 Christopher Li wrote:
> >> Using enum namespace for member "namespace" has benefit here. It is
> >> clear that which set of value it belongs to. E.g. if you assign
> >> SYM_NODE into "namespace" member it *looks* is obvious wrong.
> > 
> > We are able to catch assignment of SYM_NODE to 'enum namespace'.  But we
> > are not able to catch (SYM_NODE | SYM_ENUM) to 'enum namespace', so that
> > the patch makes no difference.  Or am I missing anything?
> 
> I am refering to your patch in other email:
> 
> struct symbol_op {
> -          enum keyword type;
> +         int type;
> 
> That is worse. Because you make type as plain int, I can assign NS_KEYWORD

Have you tried it with sparse without my patch applied?  You indeed can assign 
NS_KEYWORD to 'enum keyword' and sparse is silent ;-)

> there and you wouldn't able to catch any thing right? So you make the enum
> so strict that
> we can't use it any more.
> 
> Currently the compiler might not able to catch it. But it look obvious
> wrong to human
> eye if you assign other class of enum there.

Type-info makes sense for compilers, analyzers, etc.  If you treat it as 
plain-text information for humans with no underlying support of that tools, 
it's equal to Hungarian notation IMO.

> > I don't think the code looks worse, nevertheless respect your attitude.
> 
> See my previous comment.
> 
> I just want to make sense of this thing. Currently I see a lot of down
> sides, forcing people to do more explicit cast or avoid using the enum
> type completely. What is the up side again?
> 
> Remember, too much warning can be a bad thing as well. It makes people
> don't want
> to use the tool or just turn off the warning completely. When I run sparse
> over its own source code, I consider those warning useless because I don't
> have a better
> way to fix it. I think the source code is fine as it is.

Sure, I don't object it anyhow.  As a C++ programmer I've certainly different 
attitude to type safety, thus biased.  I believe kernel developers would more 
likely agree with your point of view.

> BTW, I have move the enum warning to a new branch:
> 
> http://git.kernel.org/?p=devel/sparse/sparse.enum-warning.git;a=summary
> 
> Some people might find it useful in special occasions. I want to heard
> about it.

Looks great, though it really wasn't my intention to fork anything :-)

Kamil

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-29 18:05                                 ` Sparse crash when mixing int and enum in ternary operator Christopher Li
  2010-03-29 18:17                                   ` Kamil Dudka
@ 2010-03-29 21:26                                   ` Josh Triplett
  1 sibling, 0 replies; 30+ messages in thread
From: Josh Triplett @ 2010-03-29 21:26 UTC (permalink / raw)
  To: Christopher Li; +Cc: Kamil Dudka, Pavel Roskin, linux-sparse

On Mon, Mar 29, 2010 at 11:05:08AM -0700, Christopher Li wrote:
> On Sat, Mar 27, 2010 at 2:29 AM, Josh Triplett <josh@joshtriplett.org> wrote:
> >>     .type = (enum keyword) (KW_SPECIFIER | KW_SHORT),
> >>     lookup_keyword(token->ident, (enum namespace) (NS_KEYWORD | NS_TYPEDEF));
> >
> > That looks wrong.  .type doesn't contain a value of type "enum keyword",
> > it contains the bitwise or of such values, which won't represent a valid
> > enum value.  Thus, .type should have an integral type, not an enum type.
> > The same goes for the second parameter of lookup_keyword.
> 
> According to the new "strict enum" rules which don't allow enum type to accept
> value other than the enum list above, that is right.
> 
> But I argue that this strict enum rules does not make sense, after this change,
> the code don't not gain any thing. In fact, it looks worse.
> 
> Sparse use a loosely define enum namespace for member "namespace". It means
> it has all this basic enum value, from NS_MACRO to NS_KEYWORD. In my mind,
> all combination of this value consider enum namespace as well. It is
> just that I can't
> list all of them in the enum list.
> 
> Using enum namespace for member "namespace" has benefit here. It is clear that
> which set of value it belongs to. E.g. if you assign SYM_NODE into "namespace"
> member it *looks* is obvious wrong.

No language that I know of with an enumerated type has such a behavior,
including C.

However, we could easily produce a type that *does* have the behavior
you want, with __attribute__((bitwise)).  That would produce a type that
doesn't mix with int, but allows bitwise operations like | and &.  That
seems like a much better fit than enums.

> It also helps people understand which set of value belongs to
> "namespace" member.
> Make "namespace" a plain int, that message is lost. It become very
> confusing for new comer
> what value was allowed in this int type.

I agree that we should have *some* better documentation than just "int",
but C alone doesn't do that.  Because C has historically just treated
"enum" as "int", people abuse the enum types in many different ways
because compilers didn't complain.  In particular, the use of enum to
define a set of related constants rather than #define or const works in
some cases (when the constants get used as standalone values) and not in
others (bitwise flags).

If you *really* want to allow "bitwise enums", what about allowing
__attribute__((bitwise)) on the enum definition?  That would help
clarify what the enum really means.  It seems like a mistake to me to
allow 0x3 in an enum whose values only allow 0x1 and 0x2, but at least
tagging it with bitwise would document that usage and distinguish it
from enums that really do list all the permissible values.

> So back to my point. It seems making the enum more strict is just make up rules
> and gain nothing in real life. It makes code looks worse just to make
> strict enum type
> happy.

On the contrary, making enum more strict helps improve static typing,
preventing many abuses of enums.  This only seems like a problem in code
that abused enums, like Sparse did. :)

- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: Sparse crash when mixing int and enum in ternary operator
  2010-03-29 19:23                                       ` Kamil Dudka
@ 2010-03-30  5:29                                         ` Pavel Roskin
  0 siblings, 0 replies; 30+ messages in thread
From: Pavel Roskin @ 2010-03-30  5:29 UTC (permalink / raw)
  To: Kamil Dudka; +Cc: Christopher Li, Josh Triplett, linux-sparse

On Mon, 2010-03-29 at 21:23 +0200, Kamil Dudka wrote:

> Sure, I don't object it anyhow.  As a C++ programmer I've certainly different 
> attitude to type safety, thus biased.  I believe kernel developers would more 
> likely agree with your point of view.

If you run patched sparse on the kernel and fix the first ten warnings
in a nice, unobtrusive way, and there is no warning that you cannot fix,
chances are that the kernel developers will like your changes, even if
it makes the code look more like C++ or even Java :-)

If the fixes involve casts or conversion of enum types to integers or a
serious code reorganization, then perhaps the kernel developers won't
like your changes.

-- 
Regards,
Pavel Roskin

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2010-03-30  5:30 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-03-09  1:24 Sparse crash when mixing int and enum in ternary operator Pavel Roskin
2010-03-09  5:43 ` Christopher Li
2010-03-09 13:46   ` Kamil Dudka
2010-03-09 18:26     ` Pavel Roskin
2010-03-09 18:35     ` Pavel Roskin
2010-03-09 19:06       ` Kamil Dudka
2010-03-09 19:15         ` Josh Triplett
2010-03-09 20:11           ` Pavel Roskin
2010-03-09 20:29             ` Kamil Dudka
2010-03-09 23:30               ` Kamil Dudka
2010-03-10  1:09                 ` Pavel Roskin
2010-03-10 16:05                   ` Kamil Dudka
2010-03-10 20:27                     ` Pavel Roskin
2010-03-10 20:44                       ` Kamil Dudka
2010-03-10 21:03                       ` Kamil Dudka
2010-03-10 21:56                     ` Christopher Li
2010-03-13 17:22                       ` Kamil Dudka
2010-03-21 15:27                         ` Kamil Dudka
2010-03-24 10:07                           ` Christopher Li
2010-03-24 10:51                             ` Kamil Dudka
2010-03-27  9:16                             ` Kamil Dudka
2010-03-27  9:29                               ` Josh Triplett
2010-03-27  9:53                                 ` [PATCH] eliminate insane conversions from int to enum Kamil Dudka
2010-03-29 18:11                                   ` Christopher Li
2010-03-29 18:05                                 ` Sparse crash when mixing int and enum in ternary operator Christopher Li
2010-03-29 18:17                                   ` Kamil Dudka
2010-03-29 18:48                                     ` Christopher Li
2010-03-29 19:23                                       ` Kamil Dudka
2010-03-30  5:29                                         ` Pavel Roskin
2010-03-29 21:26                                   ` Josh Triplett

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.