[analyzer] Make ConversionChecker load StdCLibraryFunctionsChecker

[donat.nagy]

Summary

ConversionChecker produces false positives when it encounters the
idiomatic usage of certain well-known functions (e.g. getc() and the
character classification functions like isalpha()). To eliminate these
false positives, the analyzer needs some information about semantics of
these functions. This functionality have been implemented already in
StdCLibraryFunctionsChecker, so we simply load that automatically when
ConversionChecker is loaded.

[Szelethus]

Cool!

test/Analysis/conversion.c

141

I think this comment is no longer relevant ^-^

[george.karpenkov]

The concept makes sense. @NoQ any comments? I don't recall seeing that pattern before.

test/Analysis/conversion.c

144

Also the function name should be changed as well

[Szelethus]

test/Analysis/conversion.c

158

And this one

[NoQ]

Maybe just move StdCLibraryFunctionsChecker to core? (.apiModeling?) We officially don't support disabling core, so i guess it kinda solves the issue. Also all of our languages are C-based, these functions are present on all platforms (if any of those aren't, we could split them out and keep in unix).

[donat.nagy]

Yes, moving StdCLibraryFunctionsChecker to an always-loaded package is probably a better solution than adding this one particular dependency link. (Evaluating these functions may be useful for other checkers as well, although it does not seem to change the results of other regression tests.) As an alternative to moving this checker to either the core or the apiModeling package, we could add unix.StdCLibraryFunctions to the dozen or so loaded-by-default checkers listed in lib/Driver/ToolChains/Clang.cpp without moving it to a different package. Which of these options is the best?

[NoQ]

As far as i understand, these driver-controlled thingies are for platform owners to be able to say "hey we clearly don't want this checker to be turned on on our platform". As long as there's no indication of that sort of issue, we should instead keep it all in one place, which is Checkers.td. It's already hard enough to figure out which checkers are on by default by looking at the list of checkers.

[donat.nagy]

I submitted a new patch, which moves stdCLibraryFunctions to apiModeling (https://reviews.llvm.org/D52722).

lib/StaticAnalyzer/Checkers/ConversionChecker.cpp

 //   value of target
 //
 // Many compilers and tools have similar checks that are based on semantic
 // analysis. Those checks are sound but have poor precision. ConversionChecker
 // is an alternative to those checks.
 //
 //===----------------------------------------------------------------------===//
 #include "ClangSACheckers.h"
+#include "InterCheckerAPI.h"
 #include "clang/AST/ParentMap.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 
 using namespace clang;
 using namespace ento;
bool ConversionChecker::isLossOfSign(const ImplicitCastExpr *Cast,
 
   if (!CastType->isUnsignedIntegerType() || !SubType->isSignedIntegerType())
     return false;
 
   return C.isNegative(Cast->getSubExpr());
 }
 
 void ento::registerConversionChecker(CheckerManager &mgr) {
+  registerStdCLibraryFunctionsChecker(mgr);
   mgr.registerChecker<ConversionChecker>();
 }

lib/StaticAnalyzer/Checkers/InterCheckerAPI.h

 namespace ento {
 
 /// Register the checker which evaluates CString API calls.
 void registerCStringCheckerBasic(CheckerManager &Mgr);
 
 /// Register the part of MallocChecker connected to InnerPointerChecker.
 void registerInnerPointerCheckerAux(CheckerManager &Mgr);
 
+/// Register evaluation of some basic C standard library functions.
+void registerStdCLibraryFunctionsChecker(CheckerManager &mgr);
+
 }}
 #endif /* INTERCHECKERAPI_H_ */

test/Analysis/conversion.c

 // seen some fp. For instance:  c2 = (c2 >= 'A' && c2 <= 'Z') ? c2 - 'A' + 'a' : c2;
 // there is a todo in the checker to handle calculations
 void dontwarn5() {
   signed S = -32;
   U8 = S + 10;
 }
 
 
-// false positives..
+// old false positives..
 
 int isascii(int c);
 void falsePositive1() {
   char kb2[5];
   int X = 1000;
   if (isascii(X)) {
-    // FIXME: should not warn here:
-    kb2[0] = X; // expected-warning {{Loss of precision}}
+    kb2[0] = X; // no-warning
   }
 }
 
 
 typedef struct FILE {} FILE; int getc(FILE *stream);
 # define EOF (-1)
 char reply_string[8192];
 FILE *cin;
 extern int dostuff (void);
 int falsePositive2() {
   int c, n;
   int dig;
   char *cp = reply_string;
   int pflag = 0;
   int code;
 
   for (;;) {
     dig = n = code = 0;
     while ((c = getc(cin)) != '\n') {
       if (dig < 4 && dostuff())
         code = code * 10 + (c - '0');
       if (!pflag && code == 227)
         pflag = 1;
       if (n == 0)
         n = c;
       if (c == EOF)
         return(4);
       if (cp < &reply_string[sizeof(reply_string) - 1])
-        // FIXME: should not warn here:
-        *cp++ = c; // expected-warning {{Loss of precision}}
+        *cp++ = c; // no-warning
     }
   }
 }