From owner-freebsd-toolchain@FreeBSD.ORG Thu Nov 3 20:53:50 2011 Return-Path: Delivered-To: freebsd-toolchain@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id C8ECD106566B; Thu, 3 Nov 2011 20:53:50 +0000 (UTC) (envelope-from adrian.chadd@gmail.com) Received: from mail-vw0-f54.google.com (mail-vw0-f54.google.com [209.85.212.54]) by mx1.freebsd.org (Postfix) with ESMTP id 66C7D8FC13; Thu, 3 Nov 2011 20:53:50 +0000 (UTC) Received: by vws11 with SMTP id 11so2370894vws.13 for ; Thu, 03 Nov 2011 13:53:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=HkJ0i6msqvZrYs6TYck9ZeanHFgmBWJYLHLxsU4SgKM=; b=OsciTBg8F4HSfYfySv8TOynbymfTfI8oQdd2LL3BNbf2z9t2Solzs8bWDfAc/ozFfz lZ2igtzLKB4wSs+p2YHMa7NX/0W66OjCtYNm9ZGqGz1/iBIXZRN/UgGqcxa7J9EgINyA hwZRJphr4D8iKtVa8UvCo06rxYnw1J9uBGbWA= MIME-Version: 1.0 Received: by 10.52.177.3 with SMTP id cm3mr11347964vdc.89.1320352136096; Thu, 03 Nov 2011 13:28:56 -0700 (PDT) Sender: adrian.chadd@gmail.com Received: by 10.52.29.198 with HTTP; Thu, 3 Nov 2011 13:28:56 -0700 (PDT) In-Reply-To: <4EB2F4CE.8050806@FreeBSD.org> References: <20111103104523.GA30132@freebsd.org> <4EB2AF08.9010806@FreeBSD.org> <20111103190312.GA6709@freebsd.org> <4EB2F4CE.8050806@FreeBSD.org> Date: Thu, 3 Nov 2011 13:28:56 -0700 X-Google-Sender-Auth: V7ZzCXtTCxZJVn2s3Ykn8FQ6RtQ Message-ID: From: Adrian Chadd To: Dimitry Andric Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: Alexander Best , freebsd-toolchain@freebsd.org Subject: Re: state of clang(1)'s -Wshift-count-negative and -Wshift-overflow warnings X-BeenThere: freebsd-toolchain@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Maintenance of FreeBSD's integrated toolchain List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 03 Nov 2011 20:53:50 -0000 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... Adrian On 3 November 2011 13:08, Dimitry Andric wrote: > On 2011-11-03 20:03, Alexander Best wrote: >> On Thu Nov =A03 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, b= ut 'int' only has 32 bits [-Wshift-overflow] >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 OS_REG_RMW_FIELD(ah, AR_SCR, AR_SCR_SL= E, AR_SCR_SLE_ALLOW); >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~= ~~~~~~~~~~~~~~~~~~~~ >>>> /usr/git-freebsd-head/sys/dev/ath/ath_hal/ah_internal.h:471:42: note: = expanded from macro 'OS_REG_RMW_FIELD' >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (OS_REG_READ(_a, _r) &~ (_f)) | (((_v)= << _f##_S) & (_f))) >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0^ >>>> /usr/git-freebsd-head/sys/dev/ath/ah_osdep.h:127:49: note: expanded fr= om macro 'OS_REG_WRITE' >>>> =A0 =A0 =A0 =A0 =A0 =A0 (bus_space_handle_t)(_ah)->ah_sh, (_reg), (_va= l)) > ... >>> Those warnings are bogus, and due to this bug: > > Actually, I was too quick with this one, since it isn't bogus. =A0The > macro invocation: > > =A0OS_REG_RMW_FIELD(ah, AR_SCR, AR_SCR_SLE, AR_SCR_SLE_ALLOW); > > ultimately expands to: > > =A0bus_space_write_4((bus_space_tag_t)(ah)->ah_st, (bus_space_handle_t)(a= h)->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. =A0I'm no= t > sure how clang concludes that the result (0x200000000) needs 35 bits to > represent, as it seems to use 34 bits to me. =A0But that it doesn't fit > into a signed integer is crystal-clear. > > E.g. this is a real bug! =A0Probably 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_regdo= main.c:99: >> /usr/git-freebsd-head/sys/dev/ath/ath_hal/ah_regdomain/ah_rd_domains.h:6= 9:15: warning: shift count is negative [-Wshift-count-negative] >> =A0 =A0 =A0 =A0 =A0.chan11a =A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D BM4(F1_4950_= 4980, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ^~~~= ~~~~~~~~~~~~~ >> /usr/git-freebsd-head/sys/dev/ath/ath_hal/ah_regdomain/ah_rd_domains.h:4= 1:4: note: expanded from macro 'BM4' >> =A0 =A0 =A0 =A0 =A0 W1(_fa) | W1(_fb) | W1(_fc) | W1(_fd) } >> =A0 =A0 =A0 =A0 =A0 ^ >> /usr/git-freebsd-head/sys/dev/ath/ath_hal/ah_regdomain/ah_rd_domains.h:3= 4:45: note: expanded from macro 'W1' >> =A0 =A0 =A0 =A0 (((_a) > 63 && (_a) < 128 ? (((uint64_t) 1)<<((_a)-64)) = : (uint64_t) 0)) >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0^ ~~~~~~~~~ > > is indeed bogus, since the macro makes sure the shift count never > becomes negative. =A0(N.B.: This only happens for global initializations, > *not* if you would use the same macro in a function.) > > >>> =A0 http://llvm.org/bugs/show_bug.cgi?id=3D10030 >>> >>> 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 @@ >> =A0# $FreeBSD$ >> >> =A0# >> -# Warning flags for compiling the kernel and components of the kernel: >> +# XXX Disable bogus llvm warnings, complaining about perfectly valid sh= ifts. >> +# See http://llvm.org/bugs/show_bug.cgi?id=3D10030 for more details. >> +# >> +.if ${CC:T:Mclang} =3D=3D "clang" >> +NOSHIFTWARNS=3D =A0-Wno-shift-count-negative -Wno-shift-count-overflow = \ >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 -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. >