[analyzer] Add portability package for the checkers.

[NoQ]

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

[NoQ]

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]

include/clang/StaticAnalyzer/Checkers/Checkers.td

454 ↗

(On Diff #102176)

Does this need to be in unix?

[NoQ]

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]

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]

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]

Whoops, forgot to actually attach the patch. Here.

[zaks.anna]

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]

In D34102#783161, @zaks.anna wrote:

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.

[zaks.anna]

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]

In D34102#788138, @zaks.anna wrote:

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

In D34102#788138, @zaks.anna wrote:

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.

cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td

 //
 // The checker hierarchy under OptIn should mirror that in Alpha: checkers
 // should be organized as if they were at the top level.
 //
 // Note: OptIn is *not* intended for checkers that are too noisy to be on by
 // default. Such checkers belong in the alpha package.
 def OptIn : Package<"optin">;
 
+// In the Portability package reside checkers for finding code that relies on
+// implementation-defined behavior. Such checks are wanted for cross-platform
+// development, but unwanted for developers who target only a single platform.
+def PortabilityOptIn : Package<"portability">, InPackage<OptIn>;
+
 def Nullability : Package<"nullability">;
 
 def Cplusplus : Package<"cplusplus">;
 def CplusplusAlpha : Package<"cplusplus">, InPackage<Alpha>, Hidden;
 def CplusplusOptIn : Package<"cplusplus">, InPackage<OptIn>;
 
 def Valist : Package<"valist">;
 

 } // end "alpha.security.taint"
 
 //===----------------------------------------------------------------------===//
 // Unix API checkers.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = Unix in {
 
-def UnixAPIChecker : Checker<"API">,
+def UnixAPIMisuseChecker : Checker<"API">,
   HelpText<"Check calls to various UNIX/Posix functions">,
   DescFile<"UnixAPIChecker.cpp">;
 
 def MallocChecker: Checker<"Malloc">,
   HelpText<"Check for memory leaks, double free, and use-after-free problems. Traces memory managed by malloc()/free().">,
   DescFile<"MallocChecker.cpp">;
 
 def MallocSizeofChecker : Checker<"MallocSizeof">,

 let ParentPackage = CloneDetectionAlpha in {
 
 def CloneChecker : Checker<"CloneChecker">,
   HelpText<"Reports similar pieces of code.">,
   DescFile<"CloneChecker.cpp">;
 
 } // end "clone"
 
+//===----------------------------------------------------------------------===//
+// Portability checkers.
+//===----------------------------------------------------------------------===//
+
+let ParentPackage = PortabilityOptIn in {
+
+def UnixAPIPortabilityChecker : Checker<"UnixAPI">,
+  HelpText<"Finds implementation-defined behavior in UNIX/Posix functions">,
+  DescFile<"UnixAPIChecker.cpp">;
+
+} // end optin.portability

cfe/trunk/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp

 };
 
 namespace {
 class UnixAPIChecker : public Checker< check::PreStmt<CallExpr> > {
   mutable std::unique_ptr<BugType> BT_open, BT_pthreadOnce, BT_mallocZero;
   mutable Optional<uint64_t> Val_O_CREAT;
 
 public:
+  DefaultBool CheckMisuse, CheckPortability;
+
   void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;
 
   void CheckOpen(CheckerContext &C, const CallExpr *CE) const;
   void CheckOpenAt(CheckerContext &C, const CallExpr *CE) const;
 
   void CheckPthreadOnce(CheckerContext &C, const CallExpr *CE) const;
   void CheckCallocZero(CheckerContext &C, const CallExpr *CE) const;
   void CheckMallocZero(CheckerContext &C, const CallExpr *CE) const;
void UnixAPIChecker::checkPreStmt(const CallExpr *CE,
   const DeclContext *NamespaceCtx = FD->getEnclosingNamespaceContext();
   if (NamespaceCtx && isa<NamespaceDecl>(NamespaceCtx))
     return;
 
   StringRef FName = C.getCalleeName(FD);
   if (FName.empty())
     return;
 
-  SubChecker SC =
+  if (CheckMisuse) {
+    if (SubChecker SC =
             llvm::StringSwitch<SubChecker>(FName)
                 .Case("open", &UnixAPIChecker::CheckOpen)
                 .Case("openat", &UnixAPIChecker::CheckOpenAt)
                 .Case("pthread_once", &UnixAPIChecker::CheckPthreadOnce)
+                .Default(nullptr)) {
+      (this->*SC)(C, CE);
+    }
+  }
+  if (CheckPortability) {
+    if (SubChecker SC =
+            llvm::StringSwitch<SubChecker>(FName)
                 .Case("calloc", &UnixAPIChecker::CheckCallocZero)
                 .Case("malloc", &UnixAPIChecker::CheckMallocZero)
                 .Case("realloc", &UnixAPIChecker::CheckReallocZero)
                 .Case("reallocf", &UnixAPIChecker::CheckReallocfZero)
-      .Cases("alloca", "__builtin_alloca", &UnixAPIChecker::CheckAllocaZero)
+                .Cases("alloca", "__builtin_alloca",
+                       &UnixAPIChecker::CheckAllocaZero)
                 .Case("__builtin_alloca_with_align",
                       &UnixAPIChecker::CheckAllocaWithAlignZero)
                 .Case("valloc", &UnixAPIChecker::CheckVallocZero)
-      .Default(nullptr);
-
-  if (SC)
+                .Default(nullptr)) {
       (this->*SC)(C, CE);
     }
+  }
+}
 
 //===----------------------------------------------------------------------===//
 // Registration.
 //===----------------------------------------------------------------------===//
 
-void ento::registerUnixAPIChecker(CheckerManager &mgr) {
-  mgr.registerChecker<UnixAPIChecker>();
+#define REGISTER_CHECKER(Name)                                                 \
+  void ento::registerUnixAPI##Name##Checker(CheckerManager &mgr) {             \
+    mgr.registerChecker<UnixAPIChecker>()->Check##Name = true;                 \
   }
+
+REGISTER_CHECKER(Misuse)
+REGISTER_CHECKER(Portability)

cfe/trunk/test/Analysis/malloc-overflow2.c

 // RUN: %clang_analyze_cc1 -triple x86_64-unknown-unknown -analyzer-checker=alpha.security.MallocOverflow,unix -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-unknown -analyzer-checker=alpha.security.MallocOverflow,unix,optin.portability -DPORTABILITY -verify %s
 
 typedef __typeof__(sizeof(int)) size_t;
 extern void *malloc(size_t);
 extern void free(void *ptr);
 
 void *malloc(unsigned long s);
 
 struct table {

 
 static int table_build_1(struct table *t) {
   t->nentry = (sizeof(struct table) * 2 + 31) / 32;
   t->table = (unsigned *)malloc(sizeof(unsigned) * t->nentry); // no-warning
   return t->nentry;
 }
 
 void *f(int n) {
-  return malloc(n * 0 * sizeof(int)); // expected-warning {{Call to 'malloc' has an allocation size of 0 bytes}}
+  return malloc(n * 0 * sizeof(int));
+#ifdef PORTABILITY
+  // expected-warning@-2{{Call to 'malloc' has an allocation size of 0 bytes}}
+#endif
 }

cfe/trunk/test/Analysis/unix-fns.c

-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,unix.API,osx.API %s -analyzer-store=region -analyzer-output=plist -analyzer-eagerly-assume -analyzer-config faux-bodies=true -analyzer-config path-diagnostics-alternate=false -fblocks -verify -o %t.plist
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,unix.API,osx.API,optin.portability %s -analyzer-store=region -analyzer-output=plist -analyzer-eagerly-assume -analyzer-config faux-bodies=true -analyzer-config path-diagnostics-alternate=false -fblocks -verify -o %t.plist
 // RUN: FileCheck --input-file=%t.plist %s
 // RUN: mkdir -p %t.dir
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.API,osx.API -analyzer-output=html -analyzer-config faux-bodies=true -fblocks -o %t.dir %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.API,osx.API,optin.portability -analyzer-output=html -analyzer-config faux-bodies=true -fblocks -o %t.dir %s
 // RUN: rm -fR %t.dir
 struct _opaque_pthread_once_t {
   long __sig;
   char __opaque[8];
 };
 typedef struct _opaque_pthread_once_t    __darwin_pthread_once_t;
 typedef __darwin_pthread_once_t pthread_once_t;
 int pthread_once(pthread_once_t *, void (*)(void));