From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 126895 invoked by alias); 17 Oct 2016 17:11:39 -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 126883 invoked by uid 89); 17 Oct 2016 17:11:39 -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= 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:11:28 +0000 Received: (qmail 7870 invoked from network); 17 Oct 2016 19:11:25 +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:11:25 +0200 Date: Mon, 17 Oct 2016 17:11: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: <20161017171124.GB303@x4> References: <87fuol77ri.fsf@mid.deneb.enyo.de> <20161017152304.GA303@x4> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-SW-Source: 2016-10/txt/msg01332.txt.bz2 On 2016.10.17 at 16:51 +0000, Bernd Edlinger wrote: > On 10/17/16 17:23, Markus Trippelsdorf wrote: > > On 2016.09.29 at 18:52 +0000, Bernd Edlinger wrote: > >> On 09/29/16 20:03, Jason Merrill wrote: > >>> On Wed, Sep 28, 2016 at 12:09 PM, Bernd Edlinger > >>> wrote: > >>>> On 09/28/16 16:41, Jason Merrill wrote: > >>>>> On Tue, Sep 27, 2016 at 11:10 AM, Bernd Edlinger > >>>>> wrote: > >>>>>> On 09/27/16 16:42, Jason Merrill wrote: > >>>>>>> On Tue, Sep 27, 2016 at 10:28 AM, Bernd Edlinger > >>>>>>> wrote: > >>>>>>>> On 09/27/16 16:10, Florian Weimer wrote: > >>>>>>>>> * Bernd Edlinger: > >>>>>>>>> > >>>>>>>>>>> “0 << 0” is used in a similar context, to create a zero constant for a > >>>>>>>>>>> multi-bit subfield of an integer. > >>>>>>>>>>> > >>>>>>>>>>> This example comes from GDB, in bfd/elf64-alpha.c: > >>>>>>>>>>> > >>>>>>>>>>> | insn = INSN_ADDQ | (16 << 21) | (0 << 16) | (0 << 0); > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Of course that is not a boolean context, and will not get a warning. > >>>>>>>>>> > >>>>>>>>>> Question is if "if (1 << 0)" is possibly a miss-spelled "if (1 < 0)". > >>>>>>>>>> > >>>>>>>>>> Maybe 1 and 0 come from macro expansion.... > >>>>>>>>> > >>>>>>>>> But what's the intent of treating 1 << 0 and 0 << 0 differently in the > >>>>>>>>> patch, then? > >>>>>>>> > >>>>>>>> I am not sure if it was a good idea. > >>>>>>>> > >>>>>>>> I saw, we had code of the form > >>>>>>>> bool flag = 1 << 2; > >>>>>>>> > >>>>>>>> another value LOOKUP_PROTECT is 1 << 0, and > >>>>>>>> bool flag = 1 << 0; > >>>>>>>> > >>>>>>>> would at least not overflow the allowed value range of a boolean. > >>>>>>> > >>>>>>> Assigning a bit mask to a bool variable is still probably not what was > >>>>>>> intended, even if it doesn't change the value. > >>>>>> > >>>>>> That works for me too. > >>>>>> I can simply remove that exception. > >>>>> > >>>>> Sounds good. > >>>> > >>>> Great. Is that an "OK with that change"? > >>> > >>> What do you think about dropping the TYPE_UNSIGNED exception as well? > >>> I don't see what difference that makes. > >>> > >> > >> > >> If I drop that exception, then I could also drop the check for > >> INTEGER_TYPE and the whole if, because I think other types can not > >> happen, but if they are allowed they are as well bogus here. > >> > >> I can try a bootstrap and see if there are false positives. > >> > >> But I can do that as well in a follow-up patch, this should probably > >> be done step by step, especially when it may trigger some false > >> positives. > >> > >> I think I could also add more stuff, like unary + or - ? > >> or maybe also binary +, -, * and / ? > >> > >> We already discussed making this a multi-level option, > >> and maybe enabling the higher level explicitly in the > >> boot-strap. > >> > >> As long as the warning continues to find more bugs than false > >> positives, it is probably worth extending it to more cases. > >> > >> However unsigned integer shift are not undefined if they overflow. > >> > >> It is possible that this warning will then trigger also on valid > >> code that does loop termination with unsigned int left shifting. > >> I dont have a real example, but maybe like this hypothetical C-code: > >> > >> unsigned int x=1, bits=0; > >> while (x << bits) bits++; > >> printf("bits=%d\n", bits); > >> > >> > >> Is it OK for everybody to warn for this on -Wall, or maybe only > >> when -Wextra or for instance -Wint-in-bool-context=2 is used ? > > > > 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. -- Markus