| Message ID | AM4PR0701MB2162458A7F27D68C8AF290D8E4D00@AM4PR0701MB2162.eurprd07.prod.outlook.com |
|---|---|
| State | Superseded |
| Headers | show |
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
2016-10-17 Bernd Edlinger <bernd.edlinger@hotmail.de> * c-common.c (c_common_truthvalue_conversion): Warn only for signed integer shift ops in boolean context. Index: gcc/c-family/c-common.c =================================================================== --- gcc/c-family/c-common.c (revision 241270) +++ gcc/c-family/c-common.c (working copy) @@ -3328,8 +3328,10 @@ TREE_OPERAND (expr, 0)); case LSHIFT_EXPR: - warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context, - "<< in boolean context, did you mean '<' ?"); + if (TREE_CODE (TREE_TYPE (expr)) == INTEGER_TYPE + && !TYPE_UNSIGNED (TREE_TYPE (expr))) + warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context, + "<< in boolean context, did you mean '<' ?"); break; case COND_EXPR: