Page MenuHomePhabricator

[analyzer] Add portability package for the checkers.
ClosedPublic

Authored by NoQ on Jun 12 2017, 6:23 AM.

Details

Summary

Checkers that find implementation-defined behavior seem to better be off by default - or, at least, there should be a way to turn them off - because we're not sure if our users are developing cross-platform code or target a specific platform. If the behavior is well-defined on any particular target platform, then the user may say "this code works correctly, the behavior is documented, i personally don't care about portability, so the analyzer shouldn't warn".

I'm introducing an optin.portability package with this patch. The UNIX zero-size-malloc check is moved here, because the behavior is implementation-defined according even to the C standard, and man pages of various platforms clearly document which behavior is implemented. Of course, that behavior is different on linux vs. bsd/mac though, which is the whole point of the checker.

Suggestions/complains are very welcome. I'm thinking of enabling portability checks by default when we're cross-compiling, or maybe on per-platform basis, eg. linux/bsd developers care about portability much more often (and in this case we shouldn't probably add the optin prefix to the package).

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Jun 12 2017, 6:23 AM
NoQ added inline comments.
include/clang/StaticAnalyzer/Checkers/Checkers.td
425 ↗(On Diff #102176)

This rename is user-invisible and handy because it allows macros for registerChecker to work uniformly.

zaks.anna added inline comments.Jun 12 2017, 8:31 AM
include/clang/StaticAnalyzer/Checkers/Checkers.td
454 ↗(On Diff #102176)

Does this need to be in unix?

NoQ added inline comments.Jun 12 2017, 9:13 AM
include/clang/StaticAnalyzer/Checkers/Checkers.td
454 ↗(On Diff #102176)

Well,

  1. the description still says "UNIX/Posix" and
  2. even if we believe that malloc isn't a UNIX/Posix-specific thing, we'd also need to move our MallocChecker out of the unix package.

This is probably the right thing to do, but i didn't try to do it here. If we want this, i'd move the portability checker out of unix now and deal with MallocChecker in another patch. I can also move the portability checker's code into MallocChecker.cpp, because that's what we always wanted to do, according to a UnixAPIChecker's FIXME.

zaks.anna added inline comments.Jun 14 2017, 11:16 AM
include/clang/StaticAnalyzer/Checkers/Checkers.td
454 ↗(On Diff #102176)

If someone is interested in checking if their code is portable, they'd just enable profitability package. I do not think "unix" adds much here. I suggest to just add the check to portability package directly, change the name and make no other changes.
WDYT?

NoQ added a comment.Jun 16 2017, 8:21 AM

Here's a version of the patch without .unix. I'd still hate it to re-add the subpackages if we decide to turn some portability checkers on and off depending on language/platform (eg. checkers for portability across linux/bsd should be off on windows by default, checkers for non-portable C++ APIs should be off in plain C code, etc.).

NoQ updated this revision to Diff 102829.Jun 16 2017, 8:23 AM

Whoops, forgot to actually attach the patch. Here.

zaks.anna edited edge metadata.Jun 16 2017, 10:37 PM

eg. checkers for portability across linux/bsd should be off on windows by default, checkers for non-portable C++ APIs should be off in plain C code, etc

Is the checker you are moving to portability off and not useful on Windows?

NoQ added a comment.Jun 21 2017, 4:21 AM

eg. checkers for portability across linux/bsd should be off on windows by default, checkers for non-portable C++ APIs should be off in plain C code, etc

Is the checker you are moving to portability off and not useful on Windows?

It's the same as MallocChecker, as i explained above. A relevant code snippet from Driver.cpp:

2130     if (!IsWindowsMSVC) {
2131       CmdArgs.push_back("-analyzer-checker=unix");
2132     } else {
2133       // Enable "unix" checkers that also work on Windows.
2134       CmdArgs.push_back("-analyzer-checker=unix.API");
2135       CmdArgs.push_back("-analyzer-checker=unix.Malloc");
2136       CmdArgs.push_back("-analyzer-checker=unix.MallocSizeof");
2137       CmdArgs.push_back("-analyzer-checker=unix.MismatchedDeallocator");
2138       CmdArgs.push_back("-analyzer-checker=unix.cstring.BadSizeArg");
2139       CmdArgs.push_back("-analyzer-checker=unix.cstring.NullArg");
2140     }

My concern is not about this checker, it's about having to rename this checker if we decide that we need sub-packages for other portability checkers we may add in the future, which is totally realistic.

This just supports the statement that this particular check should not go under unix. I understand that it will be inconsistent with the name of the malloc checker, which we probably should not change as people might be relying on the package names. I think it's better to have inconsistency than having checks applicable to windows in a package named portability.unix. If there will be checks that need to be in portability and only used for unix, we could create that sub-package later on.

NoQ added a comment.Jun 27 2017, 3:20 AM

I think it's better to have inconsistency than having checks applicable to windows in a package named portability.unix.

If there will be checks that need to be in portability and only used for unix, we could create that sub-package later on.

Sounds good to me, will commit then. Still, more reasons to have aliases or tags.

This revision was automatically updated to reflect the committed changes.