[cfe-commits] Support <x>-to-bool warnings for more contexts
Sean Silva
chisophugis at gmail.com
Wed Feb 4 13:09:42 PST 2015
Thanks. I don't really have time to work on it, but thanks for the
reference.
-- Sean Silva
On Wed, Feb 4, 2015 at 12:51 PM, David Blaikie <dblaikie at gmail.com> wrote:
> Sean - here's the thread on that old patch (that might've caught Kostya's
> "bool NumFoo = 10" case), in case you're interested.
>
> On Tue, May 15, 2012 at 9:59 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>> Committed some of the easier parts of this separately in r156826 -
>> providing (floating) literal-to-bool and NULL-to-bool, but not
>> exposing the problems with constant-to-bool until I can iron out the
>> false positives.
>>
>> On Mon, Apr 23, 2012 at 4:35 PM, Nico Weber <thakis at chromium.org> wrote:
>> > 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
>> >>>>>>>>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150204/eeda7714/attachment.html>
More information about the cfe-commits
mailing list