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 > >>> <bernd.edlin...@hotmail.de> wrote: > >>>> On 09/28/16 16:41, Jason Merrill wrote: > >>>>> On Tue, Sep 27, 2016 at 11:10 AM, Bernd Edlinger > >>>>> <bernd.edlin...@hotmail.de> wrote: > >>>>>> On 09/27/16 16:42, Jason Merrill wrote: > >>>>>>> On Tue, Sep 27, 2016 at 10:28 AM, Bernd Edlinger > >>>>>>> <bernd.edlin...@hotmail.de> 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