svn commit: r217538 - in head/sys/dev: buslogic cs

[Bruce Evans]

On Tue, 18 Jan 2011, Roman Divacky wrote:

> On Wed, Jan 19, 2011 at 04:54:34AM +1100, Bruce Evans wrote:
>> On Tue, 18 Jan 2011, John Baldwin wrote:
>>> ...
>>> Gah, I trusted the clang warning too much.  enum's are ints in C yes?  So
>>> clang has a bug if it thinks an enum value cannot be negative.  In practice
>>> all the callers of this routine do not pass in negative values, but the
>>> compiler shouldn't warn about enum's bogusly.
>>
>> I didn't know it was a problem already when I asked.  I thought
>> isa_compat_io_t was global, but it is private to bt.
>>
>>> Is clang assuming that only defined values for an enum are ever passed in?
>>> If
>>> so we probably don't want to turn that checking on for the kernel as we
>>> violate that in lots of places.
>>
>> enum values are int, but an enum type may be implemented as unsigned if
>> that makes no difference for conforming code.  I think conforming code can
>> only used declared enum values.  Thus if all declared values are >= 0, as
>> is the case here, then the enum type may be implemented as unsigned.
>> Apparently, this happens for clang here, and it checks what it would do
>> itself.  Other compilers might do it differently.
>
> No, it's a real bug - C99 says that:
>
> If an int can represent all values of the original type, the value is converted
> to an int; otherwise, it is converted to an unsigned int. " (C99 6.3.1.1p2).

Er, 6.3.1.1p2 just describes the binary promotions in a expression,
and this clause describes the unary promotion part of that.  It has
little to do with enums.  It says for example than u_char promotes to
int provided UCHAR_MAX <= INT_MAX.  enum types start as signed, so
they stay signed in promotions

Here are more details for enums:
- 6.4.4.3p2 paraphrased: enum constants have type int.  gcc with certain
   warnings warns about ones which are outside the range of int.
- 6.7.2.1p4 paraphrased: an enum type is any integer type that can represent
   all the enum constants for the enum.  The choice is otherwise implementation-
   defined.

> Thus in this case the enum must be (signed) int.
>
> The problem is that this bug is in gcc too, compare:
>
> witten ~# cat enum1.c
> #include <stdio.h>
>
> static enum { foo, bar = 1U } z;

No, this only has to hold values 0 and 1, so it can be any standard type
and perhaps even an implementation-defined 1-bit unsigned type (not 1
bit signed signed since that can only hold 0 and -1).  The U on 1U has
no effect.

>
> int main (void)
> {
>    int r = 0;
>
>    if (z - 1 < 0)
>       r++;
>
>    printf("r = %i\n", r);
> }
>
> witten ~# gcc enum1.c && ./a.out
> r = 0
> witten ~# g++ enum1.c && ./a.out
> r = 1
>
> So clang is just emulating gcc bug which results in this unfortunate bogus warning.

This tells us that gcc chose to use an unsigned type for the enum.  z - 1
is an integer expression (after promoting z), so it is valid.  The result
also tells us that gcc wasted space for the enum type, since if it were
1-bit unsigned, uint8_t or uint16_t then it would promote to int and z - 1
would be (int)-1.

The bug is that since the choice is implementation-defined, you can't base
portable error checking on the choice made.

Bruce