From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15910 invoked by alias); 17 Oct 2016 15:23:22 -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 15798 invoked by uid 89); 17 Oct 2016 15:23:21 -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=positives, multi-level, multilevel 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 15:23:11 +0000 Received: (qmail 29811 invoked from network); 17 Oct 2016 17:23:04 +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 17:23:04 +0200 Date: Mon, 17 Oct 2016 15:23: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: <20161017152304.GA303@x4> References: <87shslv769.fsf@mid.deneb.enyo.de> <87fuol77ri.fsf@mid.deneb.enyo.de> 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/msg01314.txt.bz2 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: return ((a.high & 0x7fff) == 0x7fff) && (a.low<<1); if ( (uint32_t) ( aSig<<( shiftCount & 31 ) ) ) { && (uint64_t) (extractFloatx80Frac(a) << 1)) if ((plen < KEYLENGTH) && (key << plen)) -- Markus