[cfe-commits] Support <x>-to-bool warnings for more contexts
Ahmed Charles
ahmedcharles at gmail.com
Sat Apr 28 11:14:26 PDT 2012
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.
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