Date: Tue, 18 Jan 2011 19:26:55 +0100 From: Roman Divacky <rdivacky@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, John Baldwin <jhb@freebsd.org> Subject: Re: svn commit: r217538 - in head/sys/dev: buslogic cs Message-ID: <20110118182655.GA77102@freebsd.org> In-Reply-To: <20110119044805.Y2631@besplex.bde.org> References: <201101181523.p0IFNGeB042079@svn.freebsd.org> <20110119035030.W2099@besplex.bde.org> <201101181215.46266.jhb@freebsd.org> <20110119044805.Y2631@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110118182655.GA77102>