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

[Roman Divacky]

On Wed, Jan 19, 2011 at 04:54:34AM +1100, Bruce Evans wrote:
> On Tue, 18 Jan 2011, John Baldwin wrote:
> 
> >On Tuesday, January 18, 2011 12:00:44 pm Bruce Evans wrote:
> >>On Tue, 18 Jan 2011, John Baldwin wrote:
> >>
> >>>Log:
> >>> Remove some always-true comparisons.
> >>>
> >>> Submitted by:	clang via rdivacky
> >>>
> >>>Modified: head/sys/dev/buslogic/bt.c
> >>>
> >==============================================================================
> >>>--- head/sys/dev/buslogic/bt.c	Tue Jan 18 14:58:44 2011 (r217537)
> >>>+++ head/sys/dev/buslogic/bt.c	Tue Jan 18 15:23:16 2011 (r217538)
> >>>@@ -975,7 +975,7 @@ bt_find_probe_range(int ioport, int *por
> >>>int
> >>>bt_iop_from_bio(isa_compat_io_t bio_index)
> >>>{
> >>>-	if (bio_index >= 0 && bio_index < BT_NUM_ISAPORTS)
> >>>+	if (bio_index < BT_NUM_ISAPORTS)
> >>>		return (bt_board_ports[bio_index]);
> >>>	return (-1);
> >>>}
> >>
> >>So, what guarantees that isa_compat_io_t is unsigned and will remain so?
> >>Indexes should be ints, unless you want a sign morass.
> >
> >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).

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;

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.