state of clang(1)'s -Wshift-count-negative and -Wshift-overflow warnings

[Alexander Best]

On Thu Nov  3 11, Adrian Chadd wrote:
> Hi,
> 
> Please submit a PR and I'll fix the AR5210 code. I'll have to find an
> AR5210 though to test it against though...

filed under kern/162366 and assigned to you. ;)

cheers.
alex

> 
> 
> Adrian
> 
> On 3 November 2011 13:08, Dimitry Andric <dim at freebsd.org> 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.
> >
> > 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.
> >