From owner-freebsd-toolchain@FreeBSD.ORG Thu Nov 3 10:45:23 2011 Return-Path: Delivered-To: freebsd-toolchain@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 1233) id 9A6F51065670; Thu, 3 Nov 2011 10:45:23 +0000 (UTC) Date: Thu, 3 Nov 2011 10:45:23 +0000 From: Alexander Best To: freebsd-toolchain@freebsd.org Message-ID: <20111103104523.GA30132@freebsd.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Subject: 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 10:45:23 -0000 hi there, i think the following warnings were discussed once before: 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)) and /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)) iirc, back then, it was labeled as a clang bug. however testing with clang tot, i still get those warnings. so i just wanted to ask again, whether the warnings are really bogus, or if these warnings actually indicate issues during shifting? cheers. alex From owner-freebsd-toolchain@FreeBSD.ORG Thu Nov 3 11:01:52 2011 Return-Path: Delivered-To: freebsd-toolchain@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 1233) id EFA1E106566B; Thu, 3 Nov 2011 11:01:52 +0000 (UTC) Date: Thu, 3 Nov 2011 11:01:52 +0000 From: Alexander Best To: Gerald Pfeifer , Matthias Andree , Warner Losh , Roman Divacky Message-ID: <20111103110152.GA31693@freebsd.org> References: <20111017152548.GA66978@freebsd.org> <4E9CAC1A.5040709@FreeBSD.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Cc: freebsd-toolchain@freebsd.org Subject: Re: [toolchain] disable -Wtautological-compare for clang 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 11:01:53 -0000 On Mon Oct 17 11, Gerald Pfeifer wrote: > On Tue, 18 Oct 2011, Matthias Andree wrote: > >> any chance we could disable -Wtautological-compare for clang? i don't > >> think comparing an unsigned int against < 0 is worth a warning. > >> actually it's always nice to have such a seatbelt, in case somebody > >> changes the type to int and forgets to introduce such a check. > > If your code must be unclean in such a way that it uses deliberately > > dead code "just in case someone breaks the semantics", can you not use > > -Wno-tautological-compare in that situation? > > It's not as straightforward, sadly, which is why I mentioned I am > on the fence somehow. > > if (TYPE_MIN <= var && var <= TYPE_MAX) > > or > > if (var < TYPE_MIN || var > TYPE_MAX) > > are not that uncommon, in well written application, and if TYPE_MIN > then evaluates to 0, we'll get a warning. > > > Unless someone goes to paranoia mode and sprays unneeded checks like you > > suggest all over the code like an ugly graffity, all such warnings are > > worth investigating. In code I've hand my eyes and/or hands on, the > > better part of these warnings were pointing to true bugs. > > In my experience some were, while others were of the class above. however keeping these checks means that for gcc we can never turn on -Wextra, since it enables these checks. while, running clang they can be turned off via "-Wno-tautological-compare", there's no way of turning them off under gcc, except for removing the -Wextra flag again. cheers. alex > > Gerald From owner-freebsd-toolchain@FreeBSD.ORG Thu Nov 3 11:01:55 2011 Return-Path: Delivered-To: freebsd-toolchain@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 1233) id 0C9A51065675; Thu, 3 Nov 2011 11:01:55 +0000 (UTC) Date: Thu, 3 Nov 2011 11:01:55 +0000 From: Alexander Best To: Gerald Pfeifer , Matthias Andree , Warner Losh , Roman Divacky Message-ID: <20111103110155.GA32672@freebsd.org> References: <20111017152548.GA66978@freebsd.org> <12466AAB-625F-4D20-80E6-2D4235ED81EC@bsdimp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <12466AAB-625F-4D20-80E6-2D4235ED81EC@bsdimp.com> Cc: freebsd-toolchain@freebsd.org Subject: Re: [toolchain] disable -Wtautological-compare for clang 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 11:01:55 -0000 On Mon Oct 17 11, Warner Losh wrote: > I'm all for leaving it on because things like char are signed on some architectures and unsigned on others. This leads to bugs that only appear on one architecture. This warning will, at least, flag those usages. -funsigned-char / -fsigned-char could be used in that case. cheers. alex > > On Oct 17, 2011, at 10:56 AM, Gerald Pfeifer wrote: > > > On Mon, 17 Oct 2011, Alexander Best wrote: > >> any chance we could disable -Wtautological-compare for clang? i don't > >> think comparing an unsigned int against < 0 is worth a warning. actually > >> it's always nice to have such a seatbelt, in case somebody changes the > >> type to int and forgets to introduce such a check. > > > > I am on the fence on this one, for when I used the equivalent warning > > in GCC this did found a number of real issues that I then addressed for > > Wine (as an example). > > > > Gerald > > _______________________________________________ > > freebsd-toolchain@freebsd.org mailing list > > http://lists.freebsd.org/mailman/listinfo/freebsd-toolchain > > To unsubscribe, send any mail to "freebsd-toolchain-unsubscribe@freebsd.org" > > > > > From owner-freebsd-toolchain@FreeBSD.ORG Thu Nov 3 15:11:06 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 BD7E0106566C; Thu, 3 Nov 2011 15:11:05 +0000 (UTC) (envelope-from dim@FreeBSD.org) Received: from tensor.andric.com (cl-327.ede-01.nl.sixxs.net [IPv6:2001:7b8:2ff:146::2]) by mx1.freebsd.org (Postfix) with ESMTP id 80C918FC0C; Thu, 3 Nov 2011 15:11:05 +0000 (UTC) Received: from [IPv6:2001:7b8:3a7:0:e9f1:c2dd:1955:9aa2] (unknown [IPv6:2001:7b8:3a7:0:e9f1:c2dd:1955:9aa2]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by tensor.andric.com (Postfix) with ESMTPSA id 4C8965C37; Thu, 3 Nov 2011 16:11:04 +0100 (CET) Message-ID: <4EB2AF08.9010806@FreeBSD.org> Date: Thu, 03 Nov 2011 16:11:04 +0100 From: Dimitry Andric Organization: The FreeBSD Project User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0) Gecko/20111031 Thunderbird/8.0 MIME-Version: 1.0 To: Alexander Best References: <20111103104523.GA30132@freebsd.org> In-Reply-To: <20111103104523.GA30132@freebsd.org> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: 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 15:11:06 -0000 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)) > > iirc, back then, it was labeled as a clang bug. however testing with clang tot, > i still get those warnings. so i just wanted to ask again, whether the warnings > are really bogus, or if these warnings actually indicate issues during > shifting? Those warnings are bogus, and due to this bug: 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. From owner-freebsd-toolchain@FreeBSD.ORG Thu Nov 3 19:03:12 2011 Return-Path: Delivered-To: freebsd-toolchain@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 1233) id 31CEB1065674; Thu, 3 Nov 2011 19:03:12 +0000 (UTC) Date: Thu, 3 Nov 2011 19:03:12 +0000 From: Alexander Best To: Dimitry Andric Message-ID: <20111103190312.GA6709@freebsd.org> References: <20111103104523.GA30132@freebsd.org> <4EB2AF08.9010806@FreeBSD.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4EB2AF08.9010806@FreeBSD.org> Cc: 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 19:03:12 -0000 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)) > > > > iirc, back then, it was labeled as a clang bug. however testing with clang tot, > > i still get those warnings. so i just wanted to ask again, whether the warnings > > are really bogus, or if these warnings actually indicate issues during > > shifting? > > Those warnings are bogus, and due to this bug: > > 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? cheers. alex From owner-freebsd-toolchain@FreeBSD.ORG Thu Nov 3 20:08:47 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 0B7EA1065677; Thu, 3 Nov 2011 20:08:47 +0000 (UTC) (envelope-from dim@FreeBSD.org) Received: from tensor.andric.com (cl-327.ede-01.nl.sixxs.net [IPv6:2001:7b8:2ff:146::2]) by mx1.freebsd.org (Postfix) with ESMTP id AB3C68FC08; Thu, 3 Nov 2011 20:08:46 +0000 (UTC) Received: from [IPv6:2001:7b8:3a7:0:e9f1:c2dd:1955:9aa2] (unknown [IPv6:2001:7b8:3a7:0:e9f1:c2dd:1955:9aa2]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by tensor.andric.com (Postfix) with ESMTPSA id C03395C37; Thu, 3 Nov 2011 21:08:45 +0100 (CET) Message-ID: <4EB2F4CE.8050806@FreeBSD.org> Date: Thu, 03 Nov 2011 21:08:46 +0100 From: Dimitry Andric Organization: The FreeBSD Project User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0) Gecko/20111031 Thunderbird/8.0 MIME-Version: 1.0 To: Alexander Best References: <20111103104523.GA30132@freebsd.org> <4EB2AF08.9010806@FreeBSD.org> <20111103190312.GA6709@freebsd.org> In-Reply-To: <20111103190312.GA6709@freebsd.org> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Adrian Chadd , 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:08:47 -0000 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. 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. > From owner-freebsd-toolchain@FreeBSD.ORG Fri Nov 4 15:39:51 2011 Return-Path: Delivered-To: freebsd-toolchain@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 1233) id 49EC01065673; Fri, 4 Nov 2011 15:39:51 +0000 (UTC) Date: Fri, 4 Nov 2011 15:39:51 +0000 From: Alexander Best To: Dimitry Andric Message-ID: <20111104153951.GA34311@freebsd.org> References: <20111103104523.GA30132@freebsd.org> <4EB2AF08.9010806@FreeBSD.org> <20111103190312.GA6709@freebsd.org> <4EB2F4CE.8050806@FreeBSD.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4EB2F4CE.8050806@FreeBSD.org> Cc: Adrian Chadd , 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: Fri, 04 Nov 2011 15:39:51 -0000 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. From owner-freebsd-toolchain@FreeBSD.ORG Fri Nov 4 15:51:53 2011 Return-Path: Delivered-To: freebsd-toolchain@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 1233) id 794971065672; Fri, 4 Nov 2011 15:51:53 +0000 (UTC) Date: Fri, 4 Nov 2011 15:51:53 +0000 From: Alexander Best To: Dimitry Andric Message-ID: <20111104155153.GA41408@freebsd.org> References: <20111103104523.GA30132@freebsd.org> <4EB2AF08.9010806@FreeBSD.org> <20111103190312.GA6709@freebsd.org> <4EB2F4CE.8050806@FreeBSD.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4EB2F4CE.8050806@FreeBSD.org> Cc: Adrian Chadd , 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: Fri, 04 Nov 2011 15:51:53 -0000 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.) and what about this one? /usr/git-freebsd-head/sys/dev/ath/ath_hal/ah_regdomain/ah_rd_domains.h:629:15: error: shift count >= width of type [-Werror,-Wshift-count-overflow] .chan11a = BM4(W2_5260_5320, ^~~~~~~~~~~~~~~~~ /usr/git-freebsd-head/sys/dev/ath/ath_hal/ah_regdomain/ah_rd_domains.h:40:34: note: expanded from macro 'BM4' { W0(_fa) | W0(_fb) | W0(_fc) | W0(_fd), \ ^ /usr/git-freebsd-head/sys/dev/ath/ath_hal/ah_regdomain/ah_rd_domains.h:32:44: note: expanded from macro 'W0' (((_a) >= 0 && (_a) < 64 ? (((uint64_t) 1)<<(_a)) : (uint64_t) 0)) ^ ~~~~ this is being reported by -Wshift-count-overflow and not -Wshift-count-negative, like the other ones. cheers. alex > > > >> 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. From owner-freebsd-toolchain@FreeBSD.ORG Fri Nov 4 16:13:31 2011 Return-Path: Delivered-To: freebsd-toolchain@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 1233) id ECCBB106566C; Fri, 4 Nov 2011 16:13:31 +0000 (UTC) Date: Fri, 4 Nov 2011 16:13:31 +0000 From: Alexander Best To: Dimitry Andric Message-ID: <20111104161331.GA45825@freebsd.org> References: <20111103104523.GA30132@freebsd.org> <4EB2AF08.9010806@FreeBSD.org> <20111103190312.GA6709@freebsd.org> <4EB2F4CE.8050806@FreeBSD.org> <20111104153951.GA34311@freebsd.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20111104153951.GA34311@freebsd.org> Cc: Adrian Chadd , 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: Fri, 04 Nov 2011 16:13:32 -0000 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. From owner-freebsd-toolchain@FreeBSD.ORG Sat Nov 5 10:21:02 2011 Return-Path: Delivered-To: freebsd-toolchain@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 1233) id 1E7FA1065670; Sat, 5 Nov 2011 10:21:02 +0000 (UTC) Date: Sat, 5 Nov 2011 10:21:02 +0000 From: Alexander Best To: freebsd-toolchain@freebsd.org Message-ID: <20111105102102.GA54596@freebsd.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Subject: [poc] buildkernel + clang + -Werror 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: Sat, 05 Nov 2011 10:21:02 -0000 i'm sending this mail to the mailinglist simply to prevent my work being list. i've experimented with the -Werror and -Wno-error= options and got to the point where i was able to compile GENERIC on amd64 with clang: # # XXX The following options might indicate real problems and need to be # investigated: # array-bounds, conversion, format, format-security, shift-count-overflow, # and shift-overflow. # .if ${CC:T:Mclang} == "clang" WERROR?= -Werror -Wno-error=array-bounds -Wno-error=conversion \ -Wno-error=empty-body -Wno-error=format -Wno-error=format-extra-args \ -Wno-error=format-invalid-specifier -Wno-error=format-security \ -Wno-error=shift-count-negative -Wno-error=shift-count-overflow \ -Wno-error=shift-overflow -Wno-error=tautological-compare .else WERROR?= -Werror .endif a few drawbacks... 1) this will only work with clang tot, since the clang version that ships with HEAD atm doesn't understand 'shift-count-negative'; it is being implied by -Werror and cannot be turned off seperately. 2) there is a bug in the clang version that ships with HEAD, where -Wno-error=X implies -WX. this is not correct (see gcc(1) man page) and was fixed in clang tot. 3) the 'format-*' options are only necessary for clang tot, since it doesn't understand '-fformat-extensions'. if 1) and 2) can be fixed and the clang release that ships with HEAD can be used for compilation, the 'format-*' options can probably be removed. 4) as noted in the comment, a few flags might indicate *real* issues in the code. i've merely enabled these to prove that clang can in fact compile the src with -Werror being set. however these options probably shouldn't be set, but rather investigated. cheers. alex ps: maybe the vendor commits that fixed 1) and 2) could be imported into the the clang version in HEAD. From owner-freebsd-toolchain@FreeBSD.ORG Sat Nov 5 19:14:37 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 500861065670; Sat, 5 Nov 2011 19:14:37 +0000 (UTC) (envelope-from dim@FreeBSD.org) Received: from tensor.andric.com (cl-327.ede-01.nl.sixxs.net [IPv6:2001:7b8:2ff:146::2]) by mx1.freebsd.org (Postfix) with ESMTP id 146AD8FC18; Sat, 5 Nov 2011 19:14:37 +0000 (UTC) Received: from [IPv6:2001:7b8:3a7:0:5502:9023:fa7c:ca0f] (unknown [IPv6:2001:7b8:3a7:0:5502:9023:fa7c:ca0f]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by tensor.andric.com (Postfix) with ESMTPSA id 6159A5C59; Sat, 5 Nov 2011 20:14:36 +0100 (CET) Message-ID: <4EB58B1D.30705@FreeBSD.org> Date: Sat, 05 Nov 2011 20:14:37 +0100 From: Dimitry Andric Organization: The FreeBSD Project User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0) Gecko/20111031 Thunderbird/8.0 MIME-Version: 1.0 To: Alexander Best References: <20111105102102.GA54596@freebsd.org> In-Reply-To: <20111105102102.GA54596@freebsd.org> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: freebsd-toolchain@freebsd.org Subject: Re: [poc] buildkernel + clang + -Werror 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: Sat, 05 Nov 2011 19:14:37 -0000 On 2011-11-05 11:21, Alexander Best wrote: > i'm sending this mail to the mailinglist simply to prevent my work being list. > i've experimented with the -Werror and -Wno-error= options and got to the point > where i was able to compile GENERIC on amd64 with clang: ... Why don't you just use NO_WERROR? If you are aiming to simply suppress warnings, that is a way simpler solution. The code that causes the warnings should simply be fixed, unless it is the contrib area, and would make merging harder, or if the warnings are false positives. Only in the latter case, and only if clang cannot be fixed right now (such as with the ?: operator problem), there should be a workaround. And even then as local as possible, so share/mk/*.mk is not the right place to add a fix. > 1) this will only work with clang tot, since the clang version that ships with > HEAD atm doesn't understand 'shift-count-negative'; it is being implied by > -Werror and cannot be turned off seperately. > 2) there is a bug in the clang version that ships with HEAD, where -Wno-error=X > implies -WX. this is not correct (see gcc(1) man page) and was fixed in > clang tot. I'll have a look if it's possible to import these. Since head now has clang from the 3.0 release branch, and the idea is to ship FreeBSD 9.0 with the 3.0 release, or something as close as possible to it, I don't plan on importing clang trunk anytime soon. From owner-freebsd-toolchain@FreeBSD.ORG Sat Nov 5 20:38:46 2011 Return-Path: Delivered-To: freebsd-toolchain@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 1233) id 82FF9106566C; Sat, 5 Nov 2011 20:38:46 +0000 (UTC) Date: Sat, 5 Nov 2011 20:38:46 +0000 From: Alexander Best To: Dimitry Andric Message-ID: <20111105203846.GA37114@freebsd.org> References: <20111105102102.GA54596@freebsd.org> <4EB58B1D.30705@FreeBSD.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4EB58B1D.30705@FreeBSD.org> Cc: freebsd-toolchain@freebsd.org Subject: Re: [poc] buildkernel + clang + -Werror 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: Sat, 05 Nov 2011 20:38:46 -0000 On Sat Nov 5 11, Dimitry Andric wrote: > On 2011-11-05 11:21, Alexander Best wrote: > > i'm sending this mail to the mailinglist simply to prevent my work being list. > > i've experimented with the -Werror and -Wno-error= options and got to the point > > where i was able to compile GENERIC on amd64 with clang: > ... > > Why don't you just use NO_WERROR? If you are aiming to simply suppress > warnings, that is a way simpler solution. because setting NO_WERROR to certain -Wno-error= options will complain about broken code, which gets committed. that way clang might be used for tinderbox, whereas with NO_WERROR simply defined to nothing, broken or unclean commits will not make tinderbox fail. if we could set NO_WERROR to bogus -Wno-error= options, clang will then only error out with *real* bugs. > > The code that causes the warnings should simply be fixed, unless it is > the contrib area, and would make merging harder, or if the warnings are > false positives. but lets take the follwing case: uint x; if (x < 0 | x > 256) this is correct code, yet clang will complain. so i think it's safe to set -Wno-error=tautological-compare on the other hand /usr/git-freebsd-head/sys/dev/ath/ath_hal/ar5212/ar5212_misc.c:577:24: warning: implicit conversion from enumeration type 'HAL_STATUS' to different enumeration type 'HAL_BOOL' [-Wconversion] return HAL_EINVAL; ~~~~~~ ^~~~~~~~~~ 1 warning generated. might be a problem, so -Wno-error=conversion should not be set and the case should be fixed inside the code. > > Only in the latter case, and only if clang cannot be fixed right now > (such as with the ?: operator problem), there should be a workaround. > And even then as local as possible, so share/mk/*.mk is not the right > place to add a fix. > > > > 1) this will only work with clang tot, since the clang version that ships with > > HEAD atm doesn't understand 'shift-count-negative'; it is being implied by > > -Werror and cannot be turned off seperately. > > 2) there is a bug in the clang version that ships with HEAD, where -Wno-error=X > > implies -WX. this is not correct (see gcc(1) man page) and was fixed in > > clang tot. > > I'll have a look if it's possible to import these. Since head now has > clang from the 3.0 release branch, and the idea is to ship FreeBSD 9.0 > with the 3.0 release, or something as close as possible to it, I don't > plan on importing clang trunk anytime soon. unfortunately latest clang tot is broken again. :( i got /usr/git-freebsd-head/sys/dev/sound/pcm/sound.c:85:2: error: invalid conversion specifier 'b' [-Werror,-Wformat-invalid-specifier] SNDSTAT_PREPARE_PCM_END(); ^~~~~~~~~~~~~~~~~~~~~~~~ although -Wno-error=format-invalid-specifier was specified. cheers. alex