]> granicus.if.org Git - clang/commitdiff
[analyzer] Trust global initializers when analyzing main().
authorArtem Dergachev <artem.dergachev@gmail.com>
Wed, 28 Aug 2019 18:44:32 +0000 (18:44 +0000)
committerArtem Dergachev <artem.dergachev@gmail.com>
Wed, 28 Aug 2019 18:44:32 +0000 (18:44 +0000)
If the global variable has an initializer, we'll ignore it because we're usually
not analyzing the program from the beginning, which means that the global
variable may have changed before we start our analysis.

However when we're analyzing main() as the top-level function, we can rely
on global initializers to still be valid. At least in C; in C++ we have global
constructors that can still break this logic.

This patch allows the Static Analyzer to load constant initializers from
global variables if the top-level function of the current analysis is main().

Differential Revision: https://reviews.llvm.org/D65361

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@370244 91177308-0d34-0410-b5e6-96231b3b80d8

lib/StaticAnalyzer/Core/RegionStore.cpp
test/Analysis/main.c [new file with mode: 0644]
test/Analysis/main.cpp [new file with mode: 0644]

index 86468e44aa03a90c99ca3cdc95dfd4f804ca5bfe..8e4fd7d19b7b2d488f234a2557e75a8c917de0d8 100644 (file)
@@ -155,28 +155,42 @@ class RegionBindingsRef : public llvm::ImmutableMapRef<const MemRegion *,
                                  ClusterBindings> {
   ClusterBindings::Factory *CBFactory;
 
+  // This flag indicates whether the current bindings are within the analysis
+  // that has started from main(). It affects how we perform loads from
+  // global variables that have initializers: if we have observed the
+  // program execution from the start and we know that these variables
+  // have not been overwritten yet, we can be sure that their initializers
+  // are still relevant. This flag never gets changed when the bindings are
+  // updated, so it could potentially be moved into RegionStoreManager
+  // (as if it's the same bindings but a different loading procedure)
+  // however that would have made the manager needlessly stateful.
+  bool IsMainAnalysis;
+
 public:
   typedef llvm::ImmutableMapRef<const MemRegion *, ClusterBindings>
           ParentTy;
 
   RegionBindingsRef(ClusterBindings::Factory &CBFactory,
                     const RegionBindings::TreeTy *T,
-                    RegionBindings::TreeTy::Factory *F)
+                    RegionBindings::TreeTy::Factory *F,
+                    bool IsMainAnalysis)
       : llvm::ImmutableMapRef<const MemRegion *, ClusterBindings>(T, F),
-        CBFactory(&CBFactory) {}
+        CBFactory(&CBFactory), IsMainAnalysis(IsMainAnalysis) {}
 
-  RegionBindingsRef(const ParentTy &P, ClusterBindings::Factory &CBFactory)
+  RegionBindingsRef(const ParentTy &P,
+                    ClusterBindings::Factory &CBFactory,
+                    bool IsMainAnalysis)
       : llvm::ImmutableMapRef<const MemRegion *, ClusterBindings>(P),
-        CBFactory(&CBFactory) {}
+        CBFactory(&CBFactory), IsMainAnalysis(IsMainAnalysis) {}
 
   RegionBindingsRef add(key_type_ref K, data_type_ref D) const {
     return RegionBindingsRef(static_cast<const ParentTy *>(this)->add(K, D),
-                             *CBFactory);
+                             *CBFactory, IsMainAnalysis);
   }
 
   RegionBindingsRef remove(key_type_ref K) const {
     return RegionBindingsRef(static_cast<const ParentTy *>(this)->remove(K),
-                             *CBFactory);
+                             *CBFactory, IsMainAnalysis);
   }
 
   RegionBindingsRef addBinding(BindingKey K, SVal V) const;
@@ -206,7 +220,13 @@ public:
 
   /// Return the internal tree as a Store.
   Store asStore() const {
-    return asImmutableMap().getRootWithoutRetain();
+    llvm::PointerIntPair<Store, 1, bool> Ptr = {
+        asImmutableMap().getRootWithoutRetain(), IsMainAnalysis};
+    return reinterpret_cast<Store>(Ptr.getOpaqueValue());
+  }
+
+  bool isMainAnalysis() const {
+    return IsMainAnalysis;
   }
 
   void printJson(raw_ostream &Out, const char *NL = "\n",
@@ -381,8 +401,15 @@ public:
   ///  casts from arrays to pointers.
   SVal ArrayToPointer(Loc Array, QualType ElementTy) override;
 
+  /// Creates the Store that correctly represents memory contents before
+  /// the beginning of the analysis of the given top-level stack frame.
   StoreRef getInitialStore(const LocationContext *InitLoc) override {
-    return StoreRef(RBFactory.getEmptyMap().getRootWithoutRetain(), *this);
+    bool IsMainAnalysis = false;
+    if (const auto *FD = dyn_cast<FunctionDecl>(InitLoc->getDecl()))
+      IsMainAnalysis = FD->isMain() && !Ctx.getLangOpts().CPlusPlus;
+    return StoreRef(RegionBindingsRef(
+        RegionBindingsRef::ParentTy(RBFactory.getEmptyMap(), RBFactory),
+        CBFactory, IsMainAnalysis).asStore(), *this);
   }
 
   //===-------------------------------------------------------------------===//
@@ -608,9 +635,13 @@ public: // Part of public interface to class.
   //===------------------------------------------------------------------===//
 
   RegionBindingsRef getRegionBindings(Store store) const {
-    return RegionBindingsRef(CBFactory,
-                             static_cast<const RegionBindings::TreeTy*>(store),
-                             RBFactory.getTreeFactory());
+    llvm::PointerIntPair<Store, 1, bool> Ptr;
+    Ptr.setFromOpaqueValue(const_cast<void *>(store));
+    return RegionBindingsRef(
+        CBFactory,
+        static_cast<const RegionBindings::TreeTy *>(Ptr.getPointer()),
+        RBFactory.getTreeFactory(),
+        Ptr.getInt());
   }
 
   void printJson(raw_ostream &Out, Store S, const char *NL = "\n",
@@ -1671,10 +1702,12 @@ SVal RegionStoreManager::getBindingForElement(RegionBindingsConstRef B,
       return svalBuilder.makeIntVal(c, T);
     }
   } else if (const VarRegion *VR = dyn_cast<VarRegion>(superR)) {
-    // Check if the containing array is const and has an initialized value.
+    // Check if the containing array has an initialized value that we can trust.
+    // We can trust a const value or a value of a global initializer in main().
     const VarDecl *VD = VR->getDecl();
-    // Either the array or the array element has to be const.
-    if (VD->getType().isConstQualified() || R->getElementType().isConstQualified()) {
+    if (VD->getType().isConstQualified() ||
+        R->getElementType().isConstQualified() ||
+        (B.isMainAnalysis() && VD->hasGlobalStorage())) {
       if (const Expr *Init = VD->getAnyInitializer()) {
         if (const auto *InitList = dyn_cast<InitListExpr>(Init)) {
           // The array index has to be known.
@@ -1763,8 +1796,11 @@ SVal RegionStoreManager::getBindingForField(RegionBindingsConstRef B,
     const VarDecl *VD = VR->getDecl();
     QualType RecordVarTy = VD->getType();
     unsigned Index = FD->getFieldIndex();
-    // Either the record variable or the field has to be const qualified.
-    if (RecordVarTy.isConstQualified() || Ty.isConstQualified())
+    // Either the record variable or the field has an initializer that we can
+    // trust. We trust initializers of constants and, additionally, respect
+    // initializers of globals when analyzing main().
+    if (RecordVarTy.isConstQualified() || Ty.isConstQualified() ||
+        (B.isMainAnalysis() && VD->hasGlobalStorage()))
       if (const Expr *Init = VD->getAnyInitializer())
         if (const auto *InitList = dyn_cast<InitListExpr>(Init)) {
           if (Index < InitList->getNumInits()) {
@@ -1982,6 +2018,12 @@ SVal RegionStoreManager::getBindingForVar(RegionBindingsConstRef B,
   if (isa<GlobalsSpaceRegion>(MS)) {
     QualType T = VD->getType();
 
+    // If we're in main(), then global initializers have not become stale yet.
+    if (B.isMainAnalysis())
+      if (const Expr *Init = VD->getAnyInitializer())
+        if (Optional<SVal> V = svalBuilder.getConstantVal(Init))
+          return *V;
+
     // Function-scoped static variables are default-initialized to 0; if they
     // have an initializer, it would have been processed by now.
     // FIXME: This is only true when we're starting analysis from main().
diff --git a/test/Analysis/main.c b/test/Analysis/main.c
new file mode 100644 (file)
index 0000000..aa4c506
--- /dev/null
@@ -0,0 +1,32 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+int x = 1;
+
+struct {
+  int a, b;
+} s = {2, 3};
+
+int arr[] = {4, 5, 6};
+
+void clang_analyzer_eval(int);
+
+int main() {
+  // In main() we know that the initial values are still valid.
+  clang_analyzer_eval(x == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(s.a == 2); // expected-warning{{TRUE}}
+  clang_analyzer_eval(s.b == 3); // expected-warning{{TRUE}}
+  clang_analyzer_eval(arr[0] == 4); // expected-warning{{TRUE}}
+  clang_analyzer_eval(arr[1] == 5); // expected-warning{{TRUE}}
+  clang_analyzer_eval(arr[2] == 6); // expected-warning{{TRUE}}
+  return 0;
+}
+
+void foo() {
+  // In other functions these values may already be overwritten.
+  clang_analyzer_eval(x == 1); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(s.a == 2); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(s.b == 3); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(arr[0] == 4); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(arr[1] == 5); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(arr[2] == 6); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+}
diff --git a/test/Analysis/main.cpp b/test/Analysis/main.cpp
new file mode 100644 (file)
index 0000000..c230c38
--- /dev/null
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+int x = 1;
+
+struct {
+  int a, b;
+} s = {2, 3};
+
+int arr[] = {4, 5, 6};
+
+void clang_analyzer_eval(int);
+
+int main() {
+  // Do not trust global initializers in C++.
+  clang_analyzer_eval(x == 1); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(s.a == 2); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(s.b == 3); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(arr[0] == 4); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(arr[1] == 5); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(arr[2] == 6); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  return 0;
+}