Clang Static Analyzer (different.ConversionToBool check)

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) 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

(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.

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

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).

Let’s suppose that we have 2 functions:

1st one:

bool implCastToBoolWithBranching(int coin) {

char *str = 0;
if (coin)
str = “abc”;
return str; // warn

}

2nd:

bool explCastToBoolWithBranching(int coin) {

char *str = 0;
if (coin)
str = “abc”;
bool b = str; // no-warn
return b;

}

Current implementation isn’t path-sensitive but it actually catches only implicit casts to boolean.

BTW, I’m going to file diff-review today, so we can actually discuss implementation details. Sorry for delay.

Thanks