[cfe-dev] Clang Static Analyzer (different.ConversionToBool check)
Artem Dergachev via cfe-dev
cfe-dev at lists.llvm.org
Sun Dec 3 14:30:31 PST 2017
Hello,
I asked to share the details because i suspected that you might be
trying to implement an "all-paths" check, which is not a good idea.
Consider:
char *str = nullptr;
if (coin)
str = "abc";
bool b = str;
On this example there exists a path (on which the coin is, say, 1) on
which "abc" is converted to a boolean. However, the code is valid, and
the checker should not flag it, because there exists another path (coin
is 0) on which str is null, and the conversion is therefore non-trivial.
The analyzer's path-sensitive engine is a great tool for finding
singular paths on which certain events occur, however it is bad at
finding situations when a property (invariant) holds on all paths. Even
if you try to enumerate all paths traversed by the analyzer, the
analyzer simply does not always explore all paths.
There's no nice'n'easy tool in the analyzer to easily solve all-paths
problems, you'd have to write the whole data flow analysis yourself if
you want this to work (eg. DeadStores checker is made this way).
On 11/30/17 12:04 PM, Alexey Knyshev wrote:
> Good time of day!
>
> First of all I would like to show cases I've included in current test:
>
> const char *str = "abc";
> bool b = str; // no-warning
> ...
>
> return str; // warning
> ...
> return b; // no-warning
>
> bool implStrCast() {
> return "" // warn
> }
>
>
> const char *text() {
> return "some text";
> }
>
> bool implBoolCast() {
> return text(); // warning
> }
>
> bool explBoolCast() {
> bool r = text(); // no-warning
> return r; // no-warning
> }
>
>
> bool implFlootCast() {
> return 1.; // warning
> }
>
>
> bool explBoolCast2() {
> return text() == 0; // no-warning
> }
>
>
> bool boolProxy(int _, bool x, bool y);
> ...
> boolProxy(1, getInteger(), (bool)1); // warn (only for second arg)
>
>
> I'd really like to know more about the approach you've taken with
> this checker.
>
> Current implementation checks 2 types of statements: CallExpr &
> ReturnStmt.
> First one actually aimed to check unexpected implicit casts of
> arguments passed to CallExpr (last example code). ReturnStmt for other
> presented cases. I could attach diff if it's appropriate here.
>
> Nowadays we try to make each test file correspond to a single checker.
>
> I'll consider separation of new checker into new one (including
> dedicated tests of course).
>
> But i don't think that there is a need for special code in the
> checker that detects testing environment and changes the behavior.
>
> I meant detecting & ignoring just special Calls (such as mentioned
> /clang_analyzer_eval/) only if we running in test env.
>
> Thanks, Alexey
>
> 2017-11-30 19:34 GMT+03:00 Artem Dergachev <noqnoqneo at gmail.com
> <mailto:noqnoqneo at gmail.com>>:
>
> (forgot to add cfe-dev on reply, re-replying, sry)
>
> Hello,
>
> I'd really like to know more about the approach you've taken with
> this checker. What kinds of conversions does it flag? Eg.:
>
> bool foo() {
> return ""; // warn.
> // or...
> char *str1 = "";
> return str1; // warn?
> // or...
> char *null_str = 0;
> return null_str; // nope.
> }
>
> It's not clear to me why would it warn on converting something to
> an int. In C, there doesn't seem to be an easy way to discriminate
> between int and boolean types. You might eventually want to
> hardcode a list of common library functions that treat their int
> arguments as boolean flags. And even if we have
> platform/library-specific typedefs such as ObjC BOOL, they are
> usually quite distinct from plain ints (eg. by typedef sugar).
>
> Then, ideally new checkers shouldn't be enabled on existing tests.
> Nowadays we try to make each test file correspond to a single
> checker. So you're running into a problem that arises because we
> didn't take this approach before, so we have some tests that have
> the whole alpha.core enabled (which is gross).
>
> If you're running into existing test file with a lot of eg.
> clang_analyzer_eval(BOOL) calls, and many of those suddenly get
> flagged, i think these are true positives. The checker works as
> intended, and we could just add expected-warnings on these lines.
> If there are too many such lines, i'd probably suggest to disable
> the checker for this file, i.e. adding
> -analyzer-disable-checker=alpha.core.YourChecker to the run-line
> at the top of the file. Separate files for the checker should be
> enough.
>
> But i don't think that there is a need for special code in the
> checker that detects testing environment and changes the behavior.
> After all, that defeats the purpose of testing.
>
> On 11/29/17 12:20 PM, Alexey Knyshev via cfe-dev wrote:
>
> Hello everyone!
>
> I have implemented a prototype of
> "different.ConversionToBool(C, C++)".
> According to list of potential checkers
> (https://clang-analyzer.llvm.org/potential_checkers.html
> <https://clang-analyzer.llvm.org/potential_checkers.html>
> <https://clang-analyzer.llvm.org/potential_checkers.html
> <https://clang-analyzer.llvm.org/potential_checkers.html>>)
> I've implemented it as the part of existing
> alpha.core.BoolAssignment checker. There is one problem I've
> faced while testing changes via llvm-lit. There are many
> FnChecks calls like /clang_analyzer_eval/ in
> Analysis/casts.[c/m] which are called with integer arguments
> instead of bool according declaration. Obviously it leads to
> false positives in implemented checker (implicit cast to
> boolean). Actually, the question is: What is the best way to
> avoid such false-positives? I came up with the following
> possible solutions:
>
> 1. Is there way to distinguish FnCheck CallExpr from common
> CallExpr (any flag or whatever). If so, it's quite simple to
> ignore them in the checker.
> 2. Is there way to determine if checker is running in testing
> env? And afterwards filter any calls to FnChecks
> like/clang_analyzer_eval /by name. As it was implemented in
> /CStringChecker:evalCall/.
> 3. Add special FnCheck like
> /clang_analyser_eval_implicit_cast(bool)/ which is supposed to
> replace /clang_analyzer_eval /in cast tests.
>
> Any suggestions?
>
> Thanks, Alexey K
>
> --
> linkedin.com/profile <http://linkedin.com/profile>
> <https://www.linkedin.com/profile/view?id=AAMAABn6oKQBDhBteiQnWsYm-S9yxT7wQkfWhSw
> <https://www.linkedin.com/profile/view?id=AAMAABn6oKQBDhBteiQnWsYm-S9yxT7wQkfWhSw>>
>
> github.com/alexeyknyshev <http://github.com/alexeyknyshev>
> <http://github.com/alexeyknyshev
> <http://github.com/alexeyknyshev>>
> bitbucket.org/alexeyknyshev
> <http://bitbucket.org/alexeyknyshev>
> <https://bitbucket.org/alexeyknyshev/
> <https://bitbucket.org/alexeyknyshev/>>
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev>
>
>
>
>
>
> --
> linkedin.com/profile
> <https://www.linkedin.com/profile/view?id=AAMAABn6oKQBDhBteiQnWsYm-S9yxT7wQkfWhSw>
>
> github.com/alexeyknyshev <http://github.com/alexeyknyshev>
> bitbucket.org/alexeyknyshev <https://bitbucket.org/alexeyknyshev/>
More information about the cfe-dev
mailing list