From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 52928 invoked by alias); 17 Oct 2016 17:44:48 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 52876 invoked by uid 89); 17 Oct 2016 17:44:44 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Hx-languages-length:2539 X-HELO: mail.ud10.udmedia.de Received: from ud10.udmedia.de (HELO mail.ud10.udmedia.de) (194.117.254.50) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 17 Oct 2016 17:44:34 +0000 Received: (qmail 19367 invoked from network); 17 Oct 2016 19:44:30 +0200 Received: from ip5b405f78.dynamic.kabel-deutschland.de (HELO x4) (ud10?360p3@91.64.95.120) by mail.ud10.udmedia.de with ESMTPSA (ECDHE-RSA-AES256-SHA encrypted, authenticated); 17 Oct 2016 19:44:30 +0200 Date: Mon, 17 Oct 2016 17:44:00 -0000 From: Markus Trippelsdorf To: Bernd Edlinger Cc: Jason Merrill , Florian Weimer , "gcc-patches@gcc.gnu.org" , Jeff Law Subject: Re: [PATCH] Make -Wint-in-bool-context warn on suspicious shift ops Message-ID: <20161017174429.GC303@x4> References: <20161017152304.GA303@x4> <20161017171124.GB303@x4> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-SW-Source: 2016-10/txt/msg01336.txt.bz2 On 2016.10.17 at 17:30 +0000, Bernd Edlinger wrote: > On 10/17/16 19:11, Markus Trippelsdorf wrote: > >>> I'm seeing this warning a lot in valid low level C code for unsigned > >>> integers. And I must say it look bogus in this context. Some examples: > > > > (All these examples are from qemu trunk.) > > > >>> return ((a.high & 0x7fff) == 0x7fff) && (a.low<<1); > >>> > > > > typedef struct { > > uint64_t low; > > uint16_t high; > > } floatx80; > > > > static inline int floatx80_is_any_nan(floatx80 a) > > { > > return ((a.high & 0x7fff) == 0x7fff) && (a.low<<1); > > } > > > >> With the shift op, the result depends on integer promotion rules, > >> and if the value is signed, it can invoke undefined behavior. > >> > >> But if a.low is a unsigned short for instance, a warning would be > >> more than justified here. > > > >>> if ( (uint32_t) ( aSig<<( shiftCount & 31 ) ) ) { > >>> > >> > >> Yes interesting, aSig is signed int, right? > > > > No, it is uint32_t. > > > >> So if the << will overflow, the code is invoking undefined behavior. > >> > >> > >>> && (uint64_t) (extractFloatx80Frac(a) << 1)) > >>> > >> > >> What is the result type of extractFloatx80Frac() ? > > > > static inline uint64_t extractFloatx80Frac( floatx80 a ) > > > >> > >>> if ((plen < KEYLENGTH) && (key << plen)) > >>> > >> > >> This is from linux, yes, I have not seen that with the first > >> version where the warning is only for signed shift ops. > >> > >> At first sight it looks really, like could it be that "key < plen" > >> was meant? But yes, actually it works correctly as long > >> as int is 32 bit, if int is 64 bits, that code would break > >> immediately. > > > > u8 plen; > > u32 key; > > > >> I think in the majority of code, where the author was aware of > >> possible overflow issues and integer promotion rules, he will > >> have used unsigned integer types, of sufficient precision. > > > > As I wrote above, all these warning were for unsigned integer types. > > And all examples are perfectly valid code as far as I can see. > > > > I would be fine with disabling the warning in cases where the shift > is done in unsigned int. Note however, that the linux code is > dependent on sizeof(int)<=sizeof(u32), but the warning would certainly > be more helpful if it comes back at the day when int starts to be 64 > bits wide. > > > How about this untested patch, does it reduce the false positive rate > for you? Yes, now everything is fine. Thank you. -- Markus