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
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.
_______________________________________________
freebsd-toolchain@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-toolchain
To unsubscribe, send any mail to "freebsd-toolchain-unsubscr...@freebsd.org"
