On Tue, Apr 17, 2018 at 03:30:02AM +0200, Luc Van Oostenryck wrote:
> On Mon, Apr 16, 2018 at 08:27:43AM -0700, Linus Torvalds wrote:
> > On Mon, Apr 16, 2018 at 1:48 AM, Luc Van Oostenryck wrote:
> > >
> > > I'm back on this topic but I'm seeing some oddities related to enums
> > > and I'm wondering if these are desired behaviour or just bugs or
> > > implementation flaws. Any clarification is very welcome.
> > >
> > > 1) type_difference() doesn't make a distinction between an enum and an
> > > int
> >
> > That's just being lazy - in standard C (ie the absence of __bitwise),
> > enums work like int for all type checking.
> >
> > So that whole "just peel off the enum type, look at the base type" is
> > just because of the normal C semantics. Maybe
> >
> > Note that it is wrong *even for standard C*.
> >
> > You can pass a "pointer to enum", and the lazy cherck in
> > type_difference() means that it thinks it's the same as "pointer to
> > int".
> >
> > So the type_difference handling is actively wrong, and much too
> > permissive, but nobody cared enough to ever get it right.
>
> OK, thanks for the info.
> I wanted to be sure that it wasn't so on purpose because the code
> in type_difference() looks as if it could have been so.
I hadn't read the standard till now and it appears that the situation
is more annoying that I thought. From what I understand of it:
1) The enumeration *constants* are allways of type 'int' (C11 6.7.2.2p3).
So, following the standard, it's impossible to have an enumeration
constant that is not representable as an 'int' although it's OK to
have such value for the initializer. Uh.
GCC doesn't seems to respect that and for each enumeration uses a
type that is large enough for all values (but I find nothing in
GCC's doc about it).
sparse does essentially the same as GCC.
2) Each enumeration is a distinct type (C11 6.2.5p16). But every
enumeration type is compatible with 'char', a signed integer or an
unsigned type (c11 6.7.2.2p4).
So, yes having something like:
enum e { ... };
int foo(void);
enum e foo(void);
*may* be perfectly legit and GCC will accept this without any warning
*if* 'int' is the choosen compatible type.
So, yes, I think the current code in sparse was purposely written so
to match the standard (I think it's Al that wrote this part).
OTOH, since we don't know what will be the compatible type, we shouldn't
mix up an enumeration type and some integer type and thus we would like
to have sparse issuing at least a warning when it happen.
The patch I posted a few days ago, essentially remove the part
"is compatible with one of the integer type" and shows about 200 places
in the kernel when there is some mixup between an enum type and 'int'
or 'unsigned int'.
The question now is "what do we want for sparse?", "do we want a flag
for it?" and "what do we want as default for this flag?".
> > > 2) if you define an enum like:
> > > enum e2 {
> > > NIL = 0,
> > > ONE = 1U,
> > > DUO = 2LL,
> > > };
> > >
> > > then the type of NIL, ONE & DUO is int, unsigned int and long long.
> >
> > Yeah, I think that's wrong too, they should all be 'enum e2' as you say.
Mmmm, 'enum e2' would be the type I think they should have but this will
be against the standard and against what GCC do (see above and C11 or C99
6.7.2.2p3 or C89 6.5.2.2).
> > And I think it comes from the same thing above: we never were very
> > careful with enums. Sparse tried to be careful with types, but it was
> > always mainly about other things (ie notably the whole "__user"
> > annotation), and enums were more of a "make it not complain".
Looking closer at the code, there some code to force all the constant
to have the same type (cast_enum_list()) but it uses cast_value() that
don't change the type, only the value. So it's just a one-liner to fix it.
-- Luc
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
