[cfe-commits] Support <x>-to-bool warnings for more contexts

David Blaikie dblaikie at gmail.com
Sat Apr 28 12:04:13 PDT 2012 

On Sat, Apr 28, 2012 at 11:14 AM, Ahmed Charles <ahmedcharles at gmail.com> wrote:
> One possible avenue for reducing false positives is to only have it
> fire in cases where there isn't a contextual conversion to bool, such
> as if/while statements. That would fix Nico's cases, I think.

Yeah, that's a thought. It's not like "while (42)" is actually such a
real problem - a bit silly, but I doubt any such cases are bugs, as
such.

But I think it would cause us to lose some good diagnostics, too: "x
== Y || Z" - the RHS is a contextual conversion, no? I'll have to see
whether I can detect that in the AST & prototype it, perhaps it comes
out OK or we can special case the good ones somehow.

I was thinking we could also limit it to literals or at least singular
constants.

I'm actually not sure why Nico got that null pointer one - it seems
like there was a conditional operator with a pointer on one side and
NULL on the other, with a constant condition - but I don't see how
that should've produced a conversion from NULL to bool without the
conditional operator in the way to suppress the warning. How we handle
all these is kind of crappy anyway (in part because of the
explicit/implicit conversion debacle (where explicit conversions are
all basically no-ops to force implicit conversions to happen - so
there's a lot more implicit conversions than you'd expect)) & it might
be a function of some of that crappyness.

> From: Nico Weber
> Sent: 4/23/2012 4:37 PM
> To: David Blaikie
> Cc: llvm cfe
> Subject: Re: [cfe-commits] Support <x>-to-bool warnings for more
> contexts
> On Sat, Apr 21, 2012 at 9:54 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> On Sat, Apr 21, 2012 at 9:16 PM, Nico Weber <thakis at chromium.org> wrote:
>>> I gave this a try in chrome. Here's two cases where this warns on that
>>> make me doubtful of this patch.
>>
>> I agree in its current state it'll need some tweaking to improve the
>> accuracy of the cases it opens up. Or are you saying you think it's
>> non-viable on principle/beyond correction?
>
> I was just commenting on the patch as-is.
>
>>
>>> 1.) It warns on code like
>>>
>>>        while(YY_CURRENT_BUFFER){ ... }
>>>
>>> where YY_CURRENT_BUFFER is something that's defined flex:
>>>
>>> ./compiler/glslang_lex.cpp:2878:8: warning: implicit conversion of
>>> NULL constant to 'bool' [-Wnull-conversion]
>>>        while(YY_CURRENT_BUFFER){
>>>              ^~~~~~~~~~~~~~~~~
>>> ./compiler/glslang_lex.cpp:307:29: note: expanded from macro 'YY_CURRENT_BUFFER'
>>>                          : NULL)
>>>
>>> If you use flex, you have to turn off Wnull-conversion because of this
>>> issue. Before the patch, Wnull-conversion was a useful warning.
>>
>> Hmm - wonder what the right fix for this is...
>>
>> I wouldn't mind seeing the full definition of YY_CURRENT_BUFFER if you
>> have a chance to send it to me. It /sounds/ like the conditional
>> operator being used there isn't doing what the author thinks it's
>> doing (it's probably got a bool argument on the LHS & so the NULL on
>> the rhs is always being converted to 'false' & should just be written
>> that way).
>>
>>> 2.) It warns on this:
>>>
>>> ../../third_party/skia/src/core/SkScalerContext.cpp:380:9: warning:
>>> implicit conversion from 'int' to 'bool' changes value from 160 to
>>> true [-Wconstant-conversion]
>>>    if (SK_FREETYPE_LCD_LERP) {
>>>        ^~~~~~~~~~~~~~~~~~~~
>>> ../../third_party/skia/src/core/SkScalerContext.cpp:372:33: note:
>>> expanded from macro 'SK_FREETYPE_LCD_LERP'
>>> #define SK_FREETYPE_LCD_LERP    160
>>>                                ^~~
>>>
>>> This is fairly common in code.
>>
>> Yep - my thinking was that we could reduce the -Wconstant-conversion
>> cases that convert to bool could be limited to literals rather than
>> arbitrary expressions (though we'd have to skip the macro/constant
>> cases too - but that might miss a lot of really good cases... )
>>
>>> (The warning did find a few cases where we're saying 'return NULL' but
>>> should be saying 'return false', but nothing interesting.
>>
>> Curious - given all the fun things I found I'm surprised it didn't hit
>> other fun things in chromium. Thanks for giving it a go, though.
>>
>>> I didn't do
>>> a full build of chrome because the build died fairly quickly due to
>>> visibility issues caused by one of espindola's recent patches, so I
>>> tracked that down instead.)
>>
>> Fair enough,
>> - David
>>
>>>
>>> Nico
>>>
>>> On Wed, Apr 18, 2012 at 3:42 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>> On Wed, Apr 18, 2012 at 3:04 PM, Nico Weber <thakis at chromium.org> wrote:
>>>>> On Wed, Apr 18, 2012 at 11:36 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>>> On Fri, Apr 6, 2012 at 4:42 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>>>> On Mon, Apr 2, 2012 at 9:44 PM, Nico Weber <thakis at chromium.org> wrote:
>>>>>>>> Do you have any numbers on bug / false positive ratios before and
>>>>>>>> after this change?
>>>>>>>
>>>>>>> I'm surprised this didn't catch more - but I found only 2 cases where
>>>>>>> this diagnostic fired (on the same function call, no less) & they seem
>>>>>>> like perfectly reasonable true positives. Something like:
>>>>>>>
>>>>>>> void func(bool, bool);
>>>>>>> func(0.7, 0.3);
>>>>>>>
>>>>>>> I'm not really sure what the author intended, but I'm fairly certain
>>>>>>> they didn't get it (unless their intent was to confuse future
>>>>>>> readers).
>>>>>>
>>>>>> So this was a little more positive than it looks - these were the new
>>>>>> warnings for -Wliteral-conversion that were found by this patch. The
>>>>>> new warnings for -Wconstant-conversion (these were the vast majority
>>>>>> of the new warnings for my change  - though we don't use
>>>>>> -Wnull-conversion at the moment, so I haven't measured the increase in
>>>>>> that warning, for example) are a bit more difficult.
>>>>>>
>>>>>> While a lot of cases were legitimate, there are a few major false
>>>>>> positive cases:
>>>>>
>>>>> This sounds to me like "more trouble than it's worth". Did you find
>>>>> any interesting bugs with this?
>>>>
>>>> Quite a few, yes. Here's a smattering of examples:
>>>>
>>>> enum X { A, B, COUNT };
>>>> std::vector<bool> b(true, COUNT);
>>>>
>>>> x &= !flag; // in xerces, actually
>>>>
>>>> void log_if(int severity, bool condition);
>>>> log_if(condition, 3);
>>>>
>>>> bool func() { ... return ERROR_CODE_FOO; } // various kinds of error
>>>> codes, often enums
>>>>
>>>> bool b;
>>>> int i;
>>>> ...
>>>> b = 10; // user seems to have jumbled up the variables, or their types
>>>> i = true;
>>>> // similar mistakes to this, except with function calls
>>>> ("set_new_uid(5)" when the flag was really about whether a new uid is
>>>> created, not specifying the uid value itself)
>>>> // a lot of these, admittedly, come up in test code where more
>>>> constants are used - though I'm not sure how much better that makes me
>>>> feel about them
>>>>
>>>> void func(int);
>>>> func(FLAG1 || FLAG2); // should be FLAG1 | FLAG2
>>>>
>>>> if (FLAG1 || FLAG2) // should be "(x == FLAG1 || x == FLAG2)"
>>>>
>>>> bool maxThings = INT_MAX; // fairly clear mistake in the declaration
>>>> of this type
>>>> void func(int);
>>>> func(maxThings);
>>>>
>>>> (x & !(sizeof(void*) - 1)) // probably meant '~' not '!', I believe
>>>>
>>>> if (0 == x && FLAG) // similar to previous examples
>>>>
>>>> bool status;
>>>> ...
>>>> status = -4; // yay, random constants!
>>>>
>>>> while (1729) // I've no idea what this person had in mind... but OK,
>>>> probably working as they intended
>>>>
>>>> if (some_const % other_const) // false positive
>>>>
>>>> bool func() {
>>>>  ...
>>>>    return 0;
>>>>  ...
>>>>    return 1;
>>>>  ...
>>>>    return 2; // aha! :/
>>>> }
>>>>
>>>> Well, that's a rough sample - the enum flag kind of cases seem pretty
>>>> common, or just passing literals of the wrong type to functions or
>>>> constructors (sometimes not as literals, but as constants defined
>>>> elsewhere).
>>>>
>>>>>
>>>>> Nico
>>>>>
>>>>>>
>>>>>> * in the /existing/ warning, we have a 'false'-ish positive involving
>>>>>> code like this: int i = std::string::npos; ... if (i ==
>>>>>> std::string::npos) - npos is actually, say, LONG_MAX, so when stored
>>>>>> in an int it truncates to -1, but it compares == to -1 just fine.
>>>>>> Perhaps we could subcategorize -Wconstant-conversion to allow these
>>>>>> particular cases that happen to map back/forth non-destructively?
>>>>>>
>>>>>> * The major case of false positives with my improved warning amounts
>>>>>> to a use case like this: #define MY_ALLOC(Type, Count)
>>>>>> malloc(sizeof(Type) * ((Count) ? Count : 1)) // the actual code is a
>>>>>> bit more involved, but it's in Python's PyMem_NEW macro
>>>>>>  The problem is that when you pass a compile-time constant count, now
>>>>>> we appear to be truncating an integer (stuffing that big count into
>>>>>> zero or one of a boolean). It would be nice if we could somehow detect
>>>>>> the case where a macro parameter is used inside a constant expression
>>>>>> & flag that constant expression as "not so constant". This logic will
>>>>>> be necessary for improvements to Clang's unreachable code diagnostic
>>>>>> anyway (we need to know when constant expressions might still vary
>>>>>> depending on the build settings (or 'call' sites in the case of
>>>>>> macros))
>>>>>>  * equally, improvements to allow for sizeof expressions to trigger
>>>>>> similar "not quite constant" flags would be good. While "if
>>>>>> (sizeof(X))" is silly & we can happily warn on that, "if (sizeof(X) -
>>>>>> 3)" might be less clear cut (or sizeof in some other part of a
>>>>>> constant expression) - though I haven't seen (m)any false positives
>>>>>> like this.
>>>>>>
>>>>>> * Template parameters - this leads to code a lot like macros:
>>>>>> template<int N> void func() { ... if (N) { ... } }; I've currently
>>>>>> worked around this by having "IgnoreParenImpCasts" not ignore
>>>>>> SubstNonTypeTemplateParmExprs - this is a bit of a dirty hack (both
>>>>>> because this code was presumably written this way for a reason -
>>>>>> though removing it doesn't regress any test cases - and because I
>>>>>> don't think it falls down as soon as N is a subexpression such as "if
>>>>>> (N - 3)")
>>>>>>
>>>>>> Any thoughts on whether or not these are reasonable goals and how best
>>>>>> to achieve them would be most welcome,
>>>>>>
>>>>>> - David
>>>>>>
>>>>>>>
>>>>>>> - David
>>>>>>>
>>>>>>>> On Mon, Apr 2, 2012 at 6:03 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>>>>>> SemaChecking.cpp:3989 currently returns early from checking implicit
>>>>>>>>> conversions after it tests some specific X-to-boolean cases (including
>>>>>>>>> string and funciton literals) but before checking various other cases
>>>>>>>>> later on like NULL-to-X and wide integer literal to narrow integer.
>>>>>>>>>
>>>>>>>>> This change removes the early return, fixes the diagnostic (to
>>>>>>>>> correctly emit the fact that non-zero literals produce a "true"
>>>>>>>>> boolean value rather than simply truncating the larger integer
>>>>>>>>> literal), and updates the tests. In some cases the test cases were
>>>>>>>>> fixed or updated (//expected-warning), in others I simply suppressed
>>>>>>>>> the diagnostic because there adding the expected-warnings would've
>>>>>>>>> added a lot of noise to the test cases*.
>>>>>>>>>
>>>>>>>>> * This last case is a little bit questionable: in one specific case we
>>>>>>>>> produce a really good diagnostic about constant integer literals used
>>>>>>>>> in boolean contexts:
>>>>>>>>>
>>>>>>>>> int f1();
>>>>>>>>> bool f2() {
>>>>>>>>>  return f1() && 42;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> we produce:
>>>>>>>>>
>>>>>>>>> conv.cpp:3:15: warning: use of logical '&&' with constant operand
>>>>>>>>> [-Wconstant-logical-operand]
>>>>>>>>>  return f1() && 42;
>>>>>>>>>              ^  ~~
>>>>>>>>> conv.cpp:3:15: note: use '&' for a bitwise operation
>>>>>>>>>  return f1() && 42;
>>>>>>>>>              ^~
>>>>>>>>>              &
>>>>>>>>> conv.cpp:3:15: note: remove constant to silence this warning
>>>>>>>>>  return f1() && 42;
>>>>>>>>>             ~^~~~~
>>>>>>>>>
>>>>>>>>> But then with my patch we get an extra diagnostic after the above warning/notes:
>>>>>>>>>
>>>>>>>>> conv.cpp:3:18: warning: implicit conversion from 'int' to 'bool'
>>>>>>>>> changes value from 42 to true [-Wconstant-conversion]
>>>>>>>>>  return f1() && 42;
>>>>>>>>>              ~~ ^~
>>>>>>>>>
>>>>>>>>> which isn't great - since we already gave a much more specific
>>>>>>>>> diagnosis of the problem in the first warning. If there's some nice
>>>>>>>>> way that we could suppress the second one whenever the first one is
>>>>>>>>> provided (unless the first one is only a warning and the second is
>>>>>>>>> -Werror'd?) I'd be happy to implement that.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Another thing I noticed as I was exploring this. We have a warning for
>>>>>>>>> float-literal-to-int such as:
>>>>>>>>>
>>>>>>>>> conv.cpp:2:9: warning: implicit conversion turns literal
>>>>>>>>> floating-point number into integer: 'double' to 'int'
>>>>>>>>> [-Wliteral-conversion]
>>>>>>>>> int i = 3.1415;
>>>>>>>>>    ~   ^~~~~~
>>>>>>>>>
>>>>>>>>> But this warning is off-by-default. Why is that? It's already
>>>>>>>>> relatively conservative (allowing things like : "int i = 3.0" because
>>>>>>>>> 3.0 converts to an int without loss of precision) - though it's not a
>>>>>>>>> DiagnoseRuntimeBehavior, which it could be changed to (to be
>>>>>>>>> consistent with similar things for integers like "unsigned char c =
>>>>>>>>> 256").
>>>>>>>>>
>>>>>>>>> Or is it really that common to deliberately use floating point
>>>>>>>>> literals to initialize integer values?
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> cfe-commits mailing list
>>>>>>>>> cfe-commits at cs.uiuc.edu
>>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>>>>>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

More information about the cfe-commits mailing list