Date: Fri, 4 Nov 2011 16:13:31 +0000 From: Alexander Best <arundel@freebsd.org> To: Dimitry Andric <dim@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: <20111104161331.GA45825@freebsd.org> In-Reply-To: <20111104153951.GA34311@freebsd.org> References: <20111103104523.GA30132@freebsd.org> <4EB2AF08.9010806@FreeBSD.org> <20111103190312.GA6709@freebsd.org> <4EB2F4CE.8050806@FreeBSD.org> <20111104153951.GA34311@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri Nov 4 11, Alexander Best wrote: > On Thu Nov 3 11, Dimitry Andric wrote: > > 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. > > maybe we could do the following for clang: > > .if ${CC:T:Mclang} == "clang" > WERROR?= -Werror -Wno-error=shift-count-negative ... > .else > WERROR?= -Werror > .endif wow. just discovered that this triggers a bug in clang. -Wno-error=X implies -WX, although this is not what gcc is doing. i tried clang tot and the bug has been fixed there. cheers. alex > > that way we could keep the warnings, but don't turn them into errors. the same > could be done for warnings such as -Wtautological-compare. > > cheers. > alex > > ps: could you submit the PR? i'm not really familar with how llvm expands > certain expressions. > > > > > 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?20111104161331.GA45825>