[PATCH] D38764: [Analyzer] Assume const string-like globals are non-null

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added a comment.

Looks like a great start!

There are a bunch of minor nits inline.

The one big thing is that I think your handling of 'const char *' in `typeIsConstString()` isn't quite right. 'const char *' means that the pointed-to characters can't be modified but does allow modification of the variable holding the pointer. I don't think we want to treat such variables as holding non-null pointers, since anyone could assign them to NULL. On the other hand, we do want to treat variables of type 'char * const' as holding non-null pointers since the variable can't be reassigned and (presumably) it was initialized to a non-null value. In this second case the characters could potentially be modified, but that doesn't change whether the value of the pointer itself.



================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:1
+
+//
----------------
We need to have the license preamble here.


================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:3
+//
+// This checker ensures that const string globals are assumed to be non-null.
+//
----------------
It would be good to include a motivation for why this is the right thing to do and even an example of the declarations this will trigger on.


================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:17
+
+class NonnilStringConstantsChecker : public Checker<check::Location> {
+  mutable IdentifierInfo *NSStringII = nullptr;
----------------
Since this applies to more than just Objective-C, I think it would be better to use 'null' instead of 'nil' in the name of the checker. Or even remove the 'Nonnil' prefix. What about 'GlobalStringConstantsChecker'?


================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:22
+public:
+  NonnilStringConstantsChecker(AnalyzerOptions &AO) {}
+
----------------
Does the constructor need to take an AnalyzerOptions?


================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:53
+  SVal V = UnknownVal();
+  if (location.isValid()) {
+    V = State->getSVal(location.castAs<Loc>());
----------------
Should we just early return if `location` is not valid?


================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:70
+
+bool NonnilStringConstantsChecker::isGlobalConstString(SVal val) const {
+  Optional<loc::MemRegionVal> regionVal = val.getAs<loc::MemRegionVal>();
----------------
Can you add doxygen-style comments to the implementations of these methods? I'd like to break with tradition and have comments for all new code.


================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:71
+bool NonnilStringConstantsChecker::isGlobalConstString(SVal val) const {
+  Optional<loc::MemRegionVal> regionVal = val.getAs<loc::MemRegionVal>();
+  if (!regionVal)
----------------
Note that we use capital letters for variables and parameters in Clang/LLVM.


================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:74
+    return false;
+  const VarRegion *region = dyn_cast<VarRegion>(regionVal->getAsRegion());
+  if (!region)
----------------
The convention Clang uses with dyn_cast and friends to to use 'auto *' in the variable declaration in order to not repeat the type name:
```
auto *Region = dyn_cast<VarRegion>(RegionVal->getAsRegion());


================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:93
+    // to be classified const.
+    hasOuterConst |= type.isConstQualified();
+    if (typeIsConstString(type, hasOuterConst))
----------------
Do you really want a bit-wise or here?


================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:113
+    II = T->getDecl()->getIdentifier();
+  }
+
----------------
This will leave `II` as uninitialized if your `if/else if` is not exhaustive. Are you sure that it is?


================
Comment at: test/Analysis/nonnil-string-constants.mm:41
+// For char* we do not require a pointer itself to be immutable.
+extern const char *CharStarConst;
+void charStarCheckGlobal() {
----------------
What is the rationale for treating `const char *v` as non null?

In this scenario `v` can be reassigned, right?


================
Comment at: test/Analysis/nonnil-string-constants.mm:45
+}
+
+// But the pointed data should be.
----------------
I'd also like to see a test for treating `char * const v;` as non null.


================
Comment at: test/Analysis/nonnil-string-constants.mm:55
+extern str globalStr;
+void charStarCheckTypedef() {
+  clang_analyzer_eval(globalStr); // expected-warning{{TRUE}}
----------------
```
typedef const char *str;
extern str globalStr;
```
allows the `globalStr` variable to be written to. 

Did you mean:
```
typedef char * const str;
extern str globalStr;
```




https://reviews.llvm.org/D38764