[analyzer] Allow bindings of the CompoundLiteralRegion

[vsavchenko]

Summary

CompoundLiteralRegions have been properly modeled before, but
'getBindingForElement` was not changed to accommodate this change
properly.

rdar://problem/46144644

[NoQ]

Yay unit tests! Let's squash those mutually cancelling bugs.

clang/unittests/StaticAnalyzer/StoreTest.cpp

22–32

Why was this necessary? All these variables seem to be already available in the surrounding scope as fields in ExprEngineConsumer.

[vsavchenko]

Fix code review remarks.

[NoQ]

Great, thanks!

clang/unittests/StaticAnalyzer/StoreTest.cpp

73–76

The reason i usually specify S to St is i want to avoid confusion with SourceManager. It still causes confusion with [Program]StateManager but those two rarely appear in the same code because ProgramStateManager provides wrappers for most of the StoreManager.

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

if (Result.second)
                                                             Result.second);
   }
 
   return Result;
 }
 
 SVal RegionStoreManager::getBindingForElement(RegionBindingsConstRef B,
                                               const ElementRegion* R) {
-  // We do not currently model bindings of the CompoundLiteralregion.
-  if (isa<CompoundLiteralRegion>(R->getBaseRegion()))
-    return UnknownVal();
-
   // Check if the region has a binding.
   if (const Optional<SVal> &V = B.getDirectBinding(R))
     return *V;
 
   const MemRegion* superR = R->getSuperRegion();
 
   // Check if the region is an element region of a string literal.
   if (const StringRegion *StrR = dyn_cast<StringRegion>(superR)) {

clang/test/Analysis/compound-literals.c

-// RUN: %clang_cc1 -triple=i386-apple-darwin10 -analyze -analyzer-checker=debug.ExprInspection -verify %s
+// RUN: %clang_cc1 -triple=i386-apple-darwin10 -verify %s -analyze \
+// RUN:   -analyzer-checker=debug.ExprInspection
+
+#define NULL 0
 void clang_analyzer_eval(int);
 
 // pr28449: Used to crash.
 void foo(void) {
   static const unsigned short array[] = (const unsigned short[]){0x0F00};
   clang_analyzer_eval(array[0] == 0x0F00); // expected-warning{{TRUE}}
 }
+
+// check that we propagate info through compound literal regions
+void bar() {
+  int *integers = (int[]){1, 2, 3};
+  clang_analyzer_eval(integers[0] == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(integers[1] == 2); // expected-warning{{TRUE}}
+  clang_analyzer_eval(integers[2] == 3); // expected-warning{{TRUE}}
+
+  int **pointers = (int *[]){&integers[0], NULL};
+  clang_analyzer_eval(pointers[0] == NULL); // expected-warning{{FALSE}}
+  clang_analyzer_eval(pointers[1] == NULL); // expected-warning{{TRUE}}
+}

clang/test/Analysis/retain-release-compound-literal.m

+// RUN: %clang_analyze_cc1 -verify -Wno-objc-root-class %s \
+// RUN:   -analyzer-checker=core,osx.cocoa.RetainCount
+
+#define NULL 0
+#define CF_RETURNS_RETAINED __attribute__((cf_returns_retained))
+#define CF_CONSUMED __attribute__((cf_consumed))
+
+void clang_analyzer_eval(int);
+
+typedef const void *CFTypeRef;
+
+extern CFTypeRef CFCreate() CF_RETURNS_RETAINED;
+extern CFTypeRef CFRetain(CFTypeRef cf);
+extern void CFRelease(CFTypeRef cf);
+
+void bar(CFTypeRef *v) {}
+
+void test1() {
+  CFTypeRef *values = (CFTypeRef[]){
+      CFCreate(),  // no-warning
+      CFCreate(),  // expected-warning{{leak}}
+      CFCreate()}; // no-warning
+  CFRelease(values[0]);
+  CFRelease(values[2]);
+}

clang/unittests/StaticAnalyzer/StoreTest.cpp

 
 #include "clang/Tooling/Tooling.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace ento {
 namespace {
 
+class StoreTestConsumer : public ExprEngineConsumer {
+public:
+  StoreTestConsumer(CompilerInstance &C) : ExprEngineConsumer(C) {}
+
+  struct TestContext {
+    StoreManager &SManager;
+    SValBuilder &Builder;
+    MemRegionManager &MRManager;
+    ASTContext &ASTContext;
+
+    /* implicit */ TestContext(ExprEngine &Engine)
+        : SManager(Engine.getStoreManager()), Builder(Engine.getSValBuilder()),
+          MRManager(SManager.getRegionManager()),
+          ASTContext(Engine.getContext()) {}
+  };
+
+  bool HandleTopLevelDecl(DeclGroupRef DG) override {
+    for (const auto *D : DG)
+      performTest(Eng, D);
+    return true;
+  }
+
+private:
+  virtual void performTest(TestContext Context, const Decl *D) = 0;
+};
+
+template <class ConsumerTy> class TestAction : public ASTFrontendAction {
+public:
+  std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &Compiler,
+                                                 StringRef File) override {
+    return std::make_unique<ConsumerTy>(Compiler);
+  }
+};
+
 // Test that we can put a value into an int-type variable and load it
 // back from that variable. Test what happens if default bindings are used.
-class VariableBindConsumer : public ExprEngineConsumer {
-  void performTest(const Decl *D) {
-    StoreManager &StMgr = Eng.getStoreManager();
-    SValBuilder &SVB = Eng.getSValBuilder();
-    MemRegionManager &MRMgr = StMgr.getRegionManager();
-    const ASTContext &ACtx = Eng.getContext();
-
+class VariableBindConsumer : public StoreTestConsumer {
+  void performTest(TestContext Ctxt, const Decl *D) {
     const auto *VDX0 = findDeclByName<VarDecl>(D, "x0");
     const auto *VDY0 = findDeclByName<VarDecl>(D, "y0");
     const auto *VDZ0 = findDeclByName<VarDecl>(D, "z0");
     const auto *VDX1 = findDeclByName<VarDecl>(D, "x1");
     const auto *VDY1 = findDeclByName<VarDecl>(D, "y1");
-    assert(VDX0 && VDY0 && VDZ0 && VDX1 && VDY1);
+
+    ASSERT_TRUE(VDX0 && VDY0 && VDZ0 && VDX1 && VDY1);
 
     const StackFrameContext *SFC =
         Eng.getAnalysisDeclContextManager().getStackFrame(D);
 
-    Loc LX0 = loc::MemRegionVal(MRMgr.getVarRegion(VDX0, SFC));
-    Loc LY0 = loc::MemRegionVal(MRMgr.getVarRegion(VDY0, SFC));
-    Loc LZ0 = loc::MemRegionVal(MRMgr.getVarRegion(VDZ0, SFC));
-    Loc LX1 = loc::MemRegionVal(MRMgr.getVarRegion(VDX1, SFC));
-    Loc LY1 = loc::MemRegionVal(MRMgr.getVarRegion(VDY1, SFC));
-
-    Store StInit = StMgr.getInitialStore(SFC).getStore();
-    SVal Zero = SVB.makeZeroVal(ACtx.IntTy);
-    SVal One = SVB.makeIntVal(1, ACtx.IntTy);
-    SVal NarrowZero = SVB.makeZeroVal(ACtx.CharTy);
+    Loc LX0 = loc::MemRegionVal(Ctxt.MRManager.getVarRegion(VDX0, SFC));
+    Loc LY0 = loc::MemRegionVal(Ctxt.MRManager.getVarRegion(VDY0, SFC));
+    Loc LZ0 = loc::MemRegionVal(Ctxt.MRManager.getVarRegion(VDZ0, SFC));
+    Loc LX1 = loc::MemRegionVal(Ctxt.MRManager.getVarRegion(VDX1, SFC));
+    Loc LY1 = loc::MemRegionVal(Ctxt.MRManager.getVarRegion(VDY1, SFC));
+
+    Store StInit = Ctxt.SManager.getInitialStore(SFC).getStore();
+    SVal Zero = Ctxt.Builder.makeZeroVal(Ctxt.ASTContext.IntTy);
+    SVal One = Ctxt.Builder.makeIntVal(1, Ctxt.ASTContext.IntTy);
+    SVal NarrowZero = Ctxt.Builder.makeZeroVal(Ctxt.ASTContext.CharTy);
 
     // Bind(Zero)
-    Store StX0 =
-        StMgr.Bind(StInit, LX0, Zero).getStore();
-    ASSERT_EQ(Zero, StMgr.getBinding(StX0, LX0, ACtx.IntTy));
+    Store StX0 = Ctxt.SManager.Bind(StInit, LX0, Zero).getStore();
+    EXPECT_EQ(Zero, Ctxt.SManager.getBinding(StX0, LX0, Ctxt.ASTContext.IntTy));
 
     // BindDefaultInitial(Zero)
     Store StY0 =
-        StMgr.BindDefaultInitial(StInit, LY0.getAsRegion(), Zero).getStore();
-    ASSERT_EQ(Zero, StMgr.getBinding(StY0, LY0, ACtx.IntTy));
-    ASSERT_EQ(Zero, *StMgr.getDefaultBinding(StY0, LY0.getAsRegion()));
+        Ctxt.SManager.BindDefaultInitial(StInit, LY0.getAsRegion(), Zero)
+            .getStore();
+    EXPECT_EQ(Zero, Ctxt.SManager.getBinding(StY0, LY0, Ctxt.ASTContext.IntTy));
+    EXPECT_EQ(Zero, *Ctxt.SManager.getDefaultBinding(StY0, LY0.getAsRegion()));
 
     // BindDefaultZero()
     Store StZ0 =
-        StMgr.BindDefaultZero(StInit, LZ0.getAsRegion()).getStore();
+        Ctxt.SManager.BindDefaultZero(StInit, LZ0.getAsRegion()).getStore();
     // BindDefaultZero wipes the region with '0 S8b', not with out Zero.
     // Direct load, however, does give us back the object of the type
     // that we specify for loading.
-    ASSERT_EQ(Zero, StMgr.getBinding(StZ0, LZ0, ACtx.IntTy));
-    ASSERT_EQ(NarrowZero, *StMgr.getDefaultBinding(StZ0, LZ0.getAsRegion()));
+    EXPECT_EQ(Zero, Ctxt.SManager.getBinding(StZ0, LZ0, Ctxt.ASTContext.IntTy));
+    EXPECT_EQ(NarrowZero,
+              *Ctxt.SManager.getDefaultBinding(StZ0, LZ0.getAsRegion()));
 
     // Bind(One)
-    Store StX1 =
-        StMgr.Bind(StInit, LX1, One).getStore();
-    ASSERT_EQ(One, StMgr.getBinding(StX1, LX1, ACtx.IntTy));
+    Store StX1 = Ctxt.SManager.Bind(StInit, LX1, One).getStore();
+    EXPECT_EQ(One, Ctxt.SManager.getBinding(StX1, LX1, Ctxt.ASTContext.IntTy));
 
     // BindDefaultInitial(One)
     Store StY1 =
-        StMgr.BindDefaultInitial(StInit, LY1.getAsRegion(), One).getStore();
-    ASSERT_EQ(One, StMgr.getBinding(StY1, LY1, ACtx.IntTy));
-    ASSERT_EQ(One, *StMgr.getDefaultBinding(StY1, LY1.getAsRegion()));
+        Ctxt.SManager.BindDefaultInitial(StInit, LY1.getAsRegion(), One)
+            .getStore();
+    EXPECT_EQ(One, Ctxt.SManager.getBinding(StY1, LY1, Ctxt.ASTContext.IntTy));
+    EXPECT_EQ(One, *Ctxt.SManager.getDefaultBinding(StY1, LY1.getAsRegion()));
   }
 
 public:
-  VariableBindConsumer(CompilerInstance &C) : ExprEngineConsumer(C) {}
+  using StoreTestConsumer::StoreTestConsumer;
+};
 
-  bool HandleTopLevelDecl(DeclGroupRef DG) override {
-    for (const auto *D : DG)
-      performTest(D);
-    return true;
-  }
-};
+TEST(Store, VariableBind) {
+  EXPECT_TRUE(tooling::runToolOnCode(
+      std::make_unique<TestAction<VariableBindConsumer>>(),
+      "void foo() { int x0, y0, z0, x1, y1; }"));
 
-class VariableBindAction : public ASTFrontendAction {
-public:
-  std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &Compiler,
-                                                 StringRef File) override {
-    return std::make_unique<VariableBindConsumer>(Compiler);
+class LiteralCompoundConsumer : public StoreTestConsumer {
+  void performTest(TestContext Ctxt, const Decl *D) {
+    using namespace ast_matchers;
+
+    const auto *CL = findNode<CompoundLiteralExpr>(D, compoundLiteralExpr());
+
+    const StackFrameContext *SFC =
+        Eng.getAnalysisDeclContextManager().getStackFrame(D);
+
+    QualType Int = Ctxt.ASTContext.IntTy;
+
+    // Get region for 'test'
+    const SubRegion *CLRegion =
+        Ctxt.MRManager.getCompoundLiteralRegion(CL, SFC);
+
+    // Get value for 'test[0]'
+    NonLoc Zero = Ctxt.Builder.makeIntVal(0, false);
+    loc::MemRegionVal ZeroElement(Ctxt.MRManager.getElementRegion(
+        Ctxt.ASTContext.IntTy, Zero, CLRegion, Ctxt.ASTContext));
+
+    Store StInit = Ctxt.SManager.getInitialStore(SFC).getStore();
+    // Let's bind constant 1 to 'test[0]'
+    SVal One = Ctxt.Builder.makeIntVal(1, Int);
+    Store StX = Ctxt.SManager.Bind(StInit, ZeroElement, One).getStore();
+
+    // And make sure that we can read this binding back as it was
+    EXPECT_EQ(One, Ctxt.SManager.getBinding(StX, ZeroElement, Int));
   }
+
+public:
+  using StoreTestConsumer::StoreTestConsumer;
 };
 
-TEST(Store, VariableBind) {
-  EXPECT_TRUE(tooling::runToolOnCode(std::make_unique<VariableBindAction>(),
-                                     "void foo() { int x0, y0, z0, x1, y1; }"));
+TEST(Store, LiteralCompound) {
+  EXPECT_TRUE(tooling::runToolOnCode(
+      std::make_unique<TestAction<LiteralCompoundConsumer>>(),
+      "void foo() { int *test = (int[]){ 1, 2, 3 }; }", "input.c"));
 }
 
 } // namespace
 } // namespace ento
 } // namespace clang