| Message ID | AM4PR0701MB2162CF4DE9EAAFFE605F4BB0E4CA0@AM4PR0701MB2162.eurprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
On Sun, Sep 25, 2016 at 3:46 AM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > This patch makes -Wint-in-bool-context warn on suspicious integer left > shifts, when the integer is signed, which is most likely some kind of > programming error, for instance using "<<" instead of "<". > > The warning is motivated by the fact, that an overflow on integer shift > left is undefined behavior, even if gcc won't optimize the shift based > on the undefined behavior. > > So in absence of undefined behavior the boolean result does not depend > on the shift value, thus the whole shifting is pointless. It's pointless for unsigned integers, too; why not warn for them as well? And why not warn for 0 << 0 and 1 << 0, which are just as pointless? Jason
* Jason Merrill: > On Sun, Sep 25, 2016 at 3:46 AM, Bernd Edlinger > <bernd.edlinger@hotmail.de> wrote: >> This patch makes -Wint-in-bool-context warn on suspicious integer left >> shifts, when the integer is signed, which is most likely some kind of >> programming error, for instance using "<<" instead of "<". >> >> The warning is motivated by the fact, that an overflow on integer shift >> left is undefined behavior, even if gcc won't optimize the shift based >> on the undefined behavior. >> >> So in absence of undefined behavior the boolean result does not depend >> on the shift value, thus the whole shifting is pointless. > > It's pointless for unsigned integers, too; why not warn for them as > well? And why not warn for 0 << 0 and 1 << 0, which are just as > pointless? “1 << 0“ is often used in a sequence of flag mask definitions. This example is from <bits/termios.h>: | /* Terminal control structure. */ | struct termios | { | /* Input modes. */ | tcflag_t c_iflag; | #define IGNBRK (1 << 0) /* Ignore break condition. */ | #define BRKINT (1 << 1) /* Signal interrupt on break. */ | #define IGNPAR (1 << 2) /* Ignore characters with parity errors. */ | #define PARMRK (1 << 3) /* Mark parity and framing errors. */ “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);
Hi, On Tue, 27 Sep 2016, Jason Merrill wrote: > On Sun, Sep 25, 2016 at 3:46 AM, Bernd Edlinger > <bernd.edlinger@hotmail.de> wrote: > > This patch makes -Wint-in-bool-context warn on suspicious integer left > > shifts, when the integer is signed, which is most likely some kind of > > programming error, for instance using "<<" instead of "<". > > > > The warning is motivated by the fact, that an overflow on integer shift > > left is undefined behavior, even if gcc won't optimize the shift based > > on the undefined behavior. > > > > So in absence of undefined behavior the boolean result does not depend > > on the shift value, thus the whole shifting is pointless. > > It's pointless for unsigned integers, too; why not warn for them as > well? Um, because left shift on unsigned integers is never undefined, so !!(1u << a) is meaningful and effectively tests if a < CHAR_BITS*sizeof(unsigned) ? Ciao, Michael.
On 09/27/16 14:49, Florian Weimer wrote: > * Jason Merrill: > >> On Sun, Sep 25, 2016 at 3:46 AM, Bernd Edlinger >> <bernd.edlinger@hotmail.de> wrote: >>> This patch makes -Wint-in-bool-context warn on suspicious integer left >>> shifts, when the integer is signed, which is most likely some kind of >>> programming error, for instance using "<<" instead of "<". >>> >>> The warning is motivated by the fact, that an overflow on integer shift >>> left is undefined behavior, even if gcc won't optimize the shift based >>> on the undefined behavior. >>> >>> So in absence of undefined behavior the boolean result does not depend >>> on the shift value, thus the whole shifting is pointless. >> >> It's pointless for unsigned integers, too; why not warn for them as >> well? And why not warn for 0 << 0 and 1 << 0, which are just as >> pointless? > > “1 << 0“ is often used in a sequence of flag mask definitions. This > example is from <bits/termios.h>: > > | /* Terminal control structure. */ > | struct termios > | { > | /* Input modes. */ > | tcflag_t c_iflag; > | #define IGNBRK (1 << 0) /* Ignore break condition. */ > | #define BRKINT (1 << 1) /* Signal interrupt on break. */ > | #define IGNPAR (1 << 2) /* Ignore characters with parity errors. */ > | #define PARMRK (1 << 3) /* Mark parity and framing errors. */ > > “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.... Bernd.
* 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?
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. Bernd.
On Tue, Sep 27, 2016 at 10:28 AM, Bernd Edlinger <bernd.edlinger@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. Jason
On 09/27/16 16:42, Jason Merrill wrote: > On Tue, Sep 27, 2016 at 10:28 AM, Bernd Edlinger > <bernd.edlinger@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. Bernd.
On Tue, Sep 27, 2016 at 11:10 AM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > On 09/27/16 16:42, Jason Merrill wrote: >> On Tue, Sep 27, 2016 at 10:28 AM, Bernd Edlinger >> <bernd.edlinger@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. Jason
On 09/28/16 16:41, Jason Merrill wrote: > On Tue, Sep 27, 2016 at 11:10 AM, Bernd Edlinger > <bernd.edlinger@hotmail.de> wrote: >> On 09/27/16 16:42, Jason Merrill wrote: >>> On Tue, Sep 27, 2016 at 10:28 AM, Bernd Edlinger >>> <bernd.edlinger@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"? Bernd.
On Wed, Sep 28, 2016 at 12:09 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > On 09/28/16 16:41, Jason Merrill wrote: >> On Tue, Sep 27, 2016 at 11:10 AM, Bernd Edlinger >> <bernd.edlinger@hotmail.de> wrote: >>> On 09/27/16 16:42, Jason Merrill wrote: >>>> On Tue, Sep 27, 2016 at 10:28 AM, Bernd Edlinger >>>> <bernd.edlinger@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. Jason
On 09/29/16 20:03, Jason Merrill wrote: > On Wed, Sep 28, 2016 at 12:09 PM, Bernd Edlinger > <bernd.edlinger@hotmail.de> wrote: >> On 09/28/16 16:41, Jason Merrill wrote: >>> On Tue, Sep 27, 2016 at 11:10 AM, Bernd Edlinger >>> <bernd.edlinger@hotmail.de> wrote: >>>> On 09/27/16 16:42, Jason Merrill wrote: >>>>> On Tue, Sep 27, 2016 at 10:28 AM, Bernd Edlinger >>>>> <bernd.edlinger@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 ? Bernd.
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.edlinger@hotmail.de> wrote: > >> On 09/28/16 16:41, Jason Merrill wrote: > >>> On Tue, Sep 27, 2016 at 11:10 AM, Bernd Edlinger > >>> <bernd.edlinger@hotmail.de> wrote: > >>>> On 09/27/16 16:42, Jason Merrill wrote: > >>>>> On Tue, Sep 27, 2016 at 10:28 AM, Bernd Edlinger > >>>>> <bernd.edlinger@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: return ((a.high & 0x7fff) == 0x7fff) && (a.low<<1); if ( (uint32_t) ( aSig<<( shiftCount & 31 ) ) ) { && (uint64_t) (extractFloatx80Frac(a) << 1)) if ((plen < KEYLENGTH) && (key << plen))
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.edlinger@hotmail.de> wrote: >>>> On 09/28/16 16:41, Jason Merrill wrote: >>>>> On Tue, Sep 27, 2016 at 11:10 AM, Bernd Edlinger >>>>> <bernd.edlinger@hotmail.de> wrote: >>>>>> On 09/27/16 16:42, Jason Merrill wrote: >>>>>>> On Tue, Sep 27, 2016 at 10:28 AM, Bernd Edlinger >>>>>>> <bernd.edlinger@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: > > 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? So if the << will overflow, the code is invoking undefined behavior. > && (uint64_t) (extractFloatx80Frac(a) << 1)) > What is the result type of extractFloatx80Frac() ? > 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. 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. I think all of the places where I have seen this warning was issued for wrong code it was with signed integer shifts. Bernd.
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.edlinger@hotmail.de> wrote: > >>>> On 09/28/16 16:41, Jason Merrill wrote: > >>>>> On Tue, Sep 27, 2016 at 11:10 AM, Bernd Edlinger > >>>>> <bernd.edlinger@hotmail.de> wrote: > >>>>>> On 09/27/16 16:42, Jason Merrill wrote: > >>>>>>> On Tue, Sep 27, 2016 at 10:28 AM, Bernd Edlinger > >>>>>>> <bernd.edlinger@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
Index: gcc/c-family/c-common.c =================================================================== --- gcc/c-family/c-common.c (revision 240437) +++ gcc/c-family/c-common.c (working copy) @@ -4651,6 +4651,19 @@ c_common_truthvalue_conversion (location_t locatio return c_common_truthvalue_conversion (location, TREE_OPERAND (expr, 0)); + case LSHIFT_EXPR: + /* Warn on signed integer left shift, except 0 << 0, 1 << 0. */ + if (TREE_CODE (TREE_TYPE (expr)) == INTEGER_TYPE + && !TYPE_UNSIGNED (TREE_TYPE (expr)) + && !(TREE_CODE (TREE_OPERAND (expr, 0)) == INTEGER_CST + && TREE_CODE (TREE_OPERAND (expr, 1)) == INTEGER_CST + && (integer_zerop (TREE_OPERAND (expr, 0)) + || integer_onep (TREE_OPERAND (expr, 0))) + && integer_zerop (TREE_OPERAND (expr, 1)))) + warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context, + "<< on signed integer in boolean context"); + break; + case COND_EXPR: if (warn_int_in_bool_context && !from_macro_definition_at (EXPR_LOCATION (expr))) Index: gcc/cp/parser.c =================================================================== --- gcc/cp/parser.c (revision 240437) +++ gcc/cp/parser.c (working copy) @@ -11172,7 +11172,7 @@ cp_parser_condition (cp_parser* parser) { tree pushed_scope; bool non_constant_p; - bool flags = LOOKUP_ONLYCONVERTING; + int flags = LOOKUP_ONLYCONVERTING; /* Create the declaration. */ decl = start_decl (declarator, &type_specifiers, Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 240437) +++ gcc/doc/invoke.texi (working copy) @@ -5927,7 +5927,8 @@ of the C++ standard. @opindex Wno-int-in-bool-context Warn for suspicious use of integer values where boolean values are expected, such as conditional expressions (?:) using non-boolean integer constants in -boolean context, like @code{if (a <= b ? 2 : 3)}. +boolean context, like @code{if (a <= b ? 2 : 3)}. Or left shifting of a +signed integer in boolean context, like @code{for (a = 0; 1 << a; a++);}. This warning is enabled by @option{-Wall}. @item -Wno-int-to-pointer-cast Index: gcc/testsuite/c-c++-common/Wint-in-bool-context.c =================================================================== --- gcc/testsuite/c-c++-common/Wint-in-bool-context.c (revision 240437) +++ gcc/testsuite/c-c++-common/Wint-in-bool-context.c (working copy) @@ -25,5 +25,7 @@ int foo (int a, int b) if (b ? 1+1 : 1) /* { dg-warning "boolean context" } */ return 7; + for (a = 0; 1 << a; a++); /* { dg-warning "boolean context" } */ + return 0; }