Date: Thu, 03 Nov 2011 21:08:46 +0100 From: Dimitry Andric <dim@FreeBSD.org> To: Alexander Best <arundel@freebsd.org> Cc: Adrian Chadd <adrian@FreeBSD.org>, freebsd-toolchain@freebsd.org Subject: Re: state of clang(1)'s -Wshift-count-negative and -Wshift-overflow warnings Message-ID: <4EB2F4CE.8050806@FreeBSD.org> In-Reply-To: <20111103190312.GA6709@freebsd.org> References: <20111103104523.GA30132@freebsd.org> <4EB2AF08.9010806@FreeBSD.org> <20111103190312.GA6709@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2011-11-03 20:03, Alexander Best wrote: > On Thu Nov 3 11, Dimitry Andric wrote: >> On 2011-11-03 11:45, Alexander Best wrote: >> ... >>> /usr/git-freebsd-head/sys/dev/ath/ath_hal/ar5210/ar5210_power.c:36:3: warning: signed shift result (0x200000000) requires 35 bits to represent, but 'int' only has 32 bits [-Wshift-overflow] >>> OS_REG_RMW_FIELD(ah, AR_SCR, AR_SCR_SLE, AR_SCR_SLE_ALLOW); >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> /usr/git-freebsd-head/sys/dev/ath/ath_hal/ah_internal.h:471:42: note: expanded from macro 'OS_REG_RMW_FIELD' >>> (OS_REG_READ(_a, _r) &~ (_f)) | (((_v) << _f##_S) & (_f))) >>> ^ >>> /usr/git-freebsd-head/sys/dev/ath/ah_osdep.h:127:49: note: expanded from macro 'OS_REG_WRITE' >>> (bus_space_handle_t)(_ah)->ah_sh, (_reg), (_val)) ... >> Those warnings are bogus, and due to this bug: Actually, I was too quick with this one, since it isn't bogus. The macro invocation: OS_REG_RMW_FIELD(ah, AR_SCR, AR_SCR_SLE, AR_SCR_SLE_ALLOW); ultimately expands to: bus_space_write_4((bus_space_tag_t)(ah)->ah_st, (bus_space_handle_t)(ah)->ah_sh, (0x4004), ((bus_space_read_4((bus_space_tag_t)(ah)->ah_st, (bus_space_handle_t)(ah)->ah_sh, (0x4004)) &~ (0x00030000)) | (((0x00020000) << 16) & (0x00030000)))); The problem part is ((0x00020000) << 16), which is an overflow. I'm not sure how clang concludes that the result (0x200000000) needs 35 bits to represent, as it seems to use 34 bits to me. But that it doesn't fit into a signed integer is crystal-clear. E.g. this is a real bug! Probably something in the macro needs to explicitly cast to 64 integers, or another workaround must be found. The other warning: > In file included from /usr/git-freebsd-head/sys/dev/ath/ath_hal/ah_regdomain.c:99: > /usr/git-freebsd-head/sys/dev/ath/ath_hal/ah_regdomain/ah_rd_domains.h:69:15: warning: shift count is negative [-Wshift-count-negative] > .chan11a = BM4(F1_4950_4980, > ^~~~~~~~~~~~~~~~~ > /usr/git-freebsd-head/sys/dev/ath/ath_hal/ah_regdomain/ah_rd_domains.h:41:4: note: expanded from macro 'BM4' > W1(_fa) | W1(_fb) | W1(_fc) | W1(_fd) } > ^ > /usr/git-freebsd-head/sys/dev/ath/ath_hal/ah_regdomain/ah_rd_domains.h:34:45: note: expanded from macro 'W1' > (((_a) > 63 && (_a) < 128 ? (((uint64_t) 1)<<((_a)-64)) : (uint64_t) 0)) > ^ ~~~~~~~~~ is indeed bogus, since the macro makes sure the shift count never becomes negative. (N.B.: This only happens for global initializations, *not* if you would use the same macro in a function.) >> http://llvm.org/bugs/show_bug.cgi?id=10030 >> >> Unfortunately, it is still not fixed for the 3.0 release branch, and I >> don't expect it will be fixed for the actual release. > > thanks for the info. so how about something like > > diff --git a/sys/conf/kern.mk b/sys/conf/kern.mk > index a0a595f..3cb13de 100644 > --- a/sys/conf/kern.mk > +++ b/sys/conf/kern.mk > @@ -1,12 +1,28 @@ > # $FreeBSD$ > > # > -# Warning flags for compiling the kernel and components of the kernel: > +# XXX Disable bogus llvm warnings, complaining about perfectly valid shifts. > +# See http://llvm.org/bugs/show_bug.cgi?id=10030 for more details. > +# > +.if ${CC:T:Mclang} == "clang" > +NOSHIFTWARNS= -Wno-shift-count-negative -Wno-shift-count-overflow \ > + -Wno-shift-overflow > +.endif > + > > ...and then add ${NOSHIFTWARNS} to the end of CWARNFLAGS? No, this is a way too big hammer, because it eliminates the useful warnings together with the false positives. It would be better to only apply this band-aid for the specific source files that need it, and even then, I would rather wait for the proper fix from upstream.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4EB2F4CE.8050806>