[PATCH] D60742: [analyzer] RegionStore: Enable loading default bindings from variables.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC358722: [analyzer] Make default bindings to variables 
actually work. (authored by dergachev, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D60742?vs=195500&id=195842#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60742/new/

https://reviews.llvm.org/D60742

Files:
  lib/StaticAnalyzer/Core/RegionStore.cpp
  test/Analysis/string.c
  unittests/StaticAnalyzer/CMakeLists.txt
  unittests/StaticAnalyzer/StoreTest.cpp

Index: test/Analysis/string.c
===
--- test/Analysis/string.c
+++ test/Analysis/string.c
@@ -1554,3 +1554,9 @@
   // This should be true.
   clang_analyzer_eval(x == 0x101); // expected-warning{{UNKNOWN}}
 }
+
+void memset29_plain_int_zero() {
+  short x;
+  memset(&x, 0, sizeof(short));
+  clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
+}
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1927,7 +1927,10 @@
   const VarRegion *R) {
 
   // Check if the region has a binding.
-  if (const Optional &V = B.getDirectBinding(R))
+  if (Optional V = B.getDirectBinding(R))
+return *V;
+
+  if (Optional V = B.getDefaultBinding(R))
 return *V;
 
   // Lazily derive a value for the VarRegion.
Index: unittests/StaticAnalyzer/StoreTest.cpp
===
--- unittests/StaticAnalyzer/StoreTest.cpp
+++ unittests/StaticAnalyzer/StoreTest.cpp
@@ -0,0 +1,105 @@
+//===- unittests/StaticAnalyzer/StoreTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Reusables.h"
+
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+// 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();
+
+const auto *VDX0 = findDeclByName(D, "x0");
+const auto *VDY0 = findDeclByName(D, "y0");
+const auto *VDZ0 = findDeclByName(D, "z0");
+const auto *VDX1 = findDeclByName(D, "x1");
+const auto *VDY1 = findDeclByName(D, "y1");
+assert(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);
+
+// Bind(Zero)
+Store StX0 =
+StMgr.Bind(StInit, LX0, Zero).getStore();
+ASSERT_EQ(Zero, StMgr.getBinding(StX0, LX0, ACtx.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()));
+
+// BindDefaultZero()
+Store StZ0 =
+StMgr.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()));
+
+// Bind(One)
+Store StX1 =
+StMgr.Bind(StInit, LX1, One).getStore();
+ASSERT_EQ(One, StMgr.getBinding(StX1, LX1, ACtx.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()));
+  }
+
+public:
+  VariableBindConsumer(CompilerInstance &C) : ExprEngi

[PATCH] D60742: [analyzer] RegionStore: Enable loading default bindings from variables.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 195500.
NoQ added a comment.

Fxd constructors and destructors.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60742/new/

https://reviews.llvm.org/D60742

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/string.c
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/StoreTest.cpp

Index: clang/unittests/StaticAnalyzer/StoreTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/StoreTest.cpp
@@ -0,0 +1,105 @@
+//===- unittests/StaticAnalyzer/StoreTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Reusables.h"
+
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+// 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();
+
+const auto *VDX0 = findDeclByName(D, "x0");
+const auto *VDY0 = findDeclByName(D, "y0");
+const auto *VDZ0 = findDeclByName(D, "z0");
+const auto *VDX1 = findDeclByName(D, "x1");
+const auto *VDY1 = findDeclByName(D, "y1");
+assert(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);
+
+// Bind(Zero)
+Store StX0 =
+StMgr.Bind(StInit, LX0, Zero).getStore();
+ASSERT_EQ(Zero, StMgr.getBinding(StX0, LX0, ACtx.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()));
+
+// BindDefaultZero()
+Store StZ0 =
+StMgr.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()));
+
+// Bind(One)
+Store StX1 =
+StMgr.Bind(StInit, LX1, One).getStore();
+ASSERT_EQ(One, StMgr.getBinding(StX1, LX1, ACtx.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()));
+  }
+
+public:
+  VariableBindConsumer(CompilerInstance &C) : ExprEngineConsumer(C) {}
+
+  bool HandleTopLevelDecl(DeclGroupRef DG) override {
+for (const auto *D : DG)
+  performTest(D);
+return true;
+  }
+};
+
+class VariableBindAction : public ASTFrontendAction {
+public:
+  std::unique_ptr CreateASTConsumer(CompilerInstance &Compiler,
+ StringRef File) override {
+return llvm::make_unique(Compiler);
+  }
+};
+
+TEST(Store, VariableBind) {
+  EXPECT_TRUE(tooling::runToolOnCode(
+  new VariableBindAction, "void foo() { int x0, y0, z0, x1, y1; }"));
+}
+
+} // namespace
+} // namespace ento
+} // namespace clang
Index: clang/unittests/StaticAnalyzer/CMakeLists.txt
===
--- clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -4,6 +4,7 @@
 
 add_clang_unittest(StaticAnalysisTests
   AnalyzerOptionsTest.cpp
+  StoreTest.cpp
   RegisterCustomCheckersTest.cpp
   SymbolReaperTest.cpp
   )
Index: clang/test/Analysis/string.c
===
--- clang/test/Analysis/string.c
+++ clang/test/Analysis/string

[PATCH] D60742: [analyzer] RegionStore: Enable loading default bindings from variables.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ marked an inline comment as done.
NoQ added inline comments.



Comment at: clang/unittests/StaticAnalyzer/StoreTest.cpp:49
+// Bind(Zero)
+Store StX0 =
+StMgr.Bind(StInit, LX0, Zero).getStore();

a_sidorin wrote:
> This can fit one line.
Just wanted to make line breaks similar in all tests, for the sake of 
OCD-friendliness.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60742/new/

https://reviews.llvm.org/D60742



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60742: [analyzer] RegionStore: Enable loading default bindings from variables.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.

I like the test even more than the change itself!




Comment at: clang/unittests/StaticAnalyzer/StoreTest.cpp:49
+// Bind(Zero)
+Store StX0 =
+StMgr.Bind(StInit, LX0, Zero).getStore();

This can fit one line.



Comment at: clang/unittests/StaticAnalyzer/StoreTest.cpp:82
+  VariableBindConsumer(CompilerInstance &C) : ExprEngineConsumer(C) {}
+  ~VariableBindConsumer() override {}
+

Do we need this dtor declaration?



Comment at: clang/unittests/StaticAnalyzer/StoreTest.cpp:93
+public:
+  VariableBindAction() {}
+  std::unique_ptr CreateASTConsumer(CompilerInstance &Compiler,

`= default`? (Or it seems like we can remove it at all)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60742/new/

https://reviews.llvm.org/D60742



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60742: [analyzer] RegionStore: Enable loading default bindings from variables.

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.

LGTM.




Comment at: clang/unittests/StaticAnalyzer/StoreTest.cpp:20
+// back from that variable. Test what happens if default bindings are used.
+class VariableBindConsumer : public ExprEngineConsumer {
+  void performTest(const Decl *D) {

It is awesome to see unit tests for this!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60742/new/

https://reviews.llvm.org/D60742



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60742: [analyzer] RegionStore: Enable loading default bindings from variables.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 195280.
NoQ added a comment.

Whoops, copy-paste error.

So, like, the reason why i added a unittest was that i wanted to test 
separation of concerns: that it's specifically our Store that works correctly, 
not only the system as a whole.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60742/new/

https://reviews.llvm.org/D60742

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/string.c
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/StoreTest.cpp

Index: clang/unittests/StaticAnalyzer/StoreTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/StoreTest.cpp
@@ -0,0 +1,107 @@
+//===- unittests/StaticAnalyzer/StoreTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Reusables.h"
+
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+// 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();
+
+const auto *VDX0 = findDeclByName(D, "x0");
+const auto *VDY0 = findDeclByName(D, "y0");
+const auto *VDZ0 = findDeclByName(D, "z0");
+const auto *VDX1 = findDeclByName(D, "x1");
+const auto *VDY1 = findDeclByName(D, "y1");
+assert(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);
+
+// Bind(Zero)
+Store StX0 =
+StMgr.Bind(StInit, LX0, Zero).getStore();
+ASSERT_EQ(Zero, StMgr.getBinding(StX0, LX0, ACtx.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()));
+
+// BindDefaultZero()
+Store StZ0 =
+StMgr.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()));
+
+// Bind(One)
+Store StX1 =
+StMgr.Bind(StInit, LX1, One).getStore();
+ASSERT_EQ(One, StMgr.getBinding(StX1, LX1, ACtx.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()));
+  }
+
+public:
+  VariableBindConsumer(CompilerInstance &C) : ExprEngineConsumer(C) {}
+  ~VariableBindConsumer() override {}
+
+  bool HandleTopLevelDecl(DeclGroupRef DG) override {
+for (const auto *D : DG)
+  performTest(D);
+return true;
+  }
+};
+
+class VariableBindAction : public ASTFrontendAction {
+public:
+  VariableBindAction() {}
+  std::unique_ptr CreateASTConsumer(CompilerInstance &Compiler,
+ StringRef File) override {
+return llvm::make_unique(Compiler);
+  }
+};
+
+TEST(Store, VariableBind) {
+  EXPECT_TRUE(tooling::runToolOnCode(
+  new VariableBindAction, "void foo() { int x0, y0, z0, x1, y1; }"));
+}
+
+} // namespace
+} // namespace ento
+} // namespace clang
Index: clang/unittests/StaticAnalyzer/CMakeLists.txt
===
--- clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -4,6 +4,7 @@
 
 add_clang_unittest(StaticAnalysisTests
   AnalyzerOptionsTest.cpp
+  St

[PATCH] D60742: [analyzer] RegionStore: Enable loading default bindings from variables.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 195279.
NoQ added a comment.

Fix test file name in the top comment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60742/new/

https://reviews.llvm.org/D60742

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/string.c
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/StoreTest.cpp

Index: clang/unittests/StaticAnalyzer/StoreTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/StoreTest.cpp
@@ -0,0 +1,108 @@
+//===- unittests/StaticAnalyzer/StoreTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Reusables.h"
+
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+// 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();
+
+const auto *VDX0 = findDeclByName(D, "x0");
+const auto *VDY0 = findDeclByName(D, "y0");
+const auto *VDZ0 = findDeclByName(D, "z0");
+const auto *VDX1 = findDeclByName(D, "x1");
+const auto *VDY1 = findDeclByName(D, "y1");
+assert(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);
+
+// Bind(Zero)
+Store StX0 =
+StMgr.Bind(StInit, LX0, Zero).getStore();
+ASSERT_EQ(Zero, StMgr.getBinding(StX0, LX0, ACtx.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()));
+
+// BindDefaultZero()
+Store StZ0 =
+StMgr.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()));
+
+// Bind(One)
+Store StX1 =
+StMgr.Bind(StInit, LX1, One).getStore();
+ASSERT_EQ(One, StMgr.getBinding(StX1, LX1, ACtx.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()));
+  }
+
+public:
+  VariableBindConsumer(CompilerInstance &C) : ExprEngineConsumer(C) {}
+  ~VariableBindConsumer() override {}
+
+  bool HandleTopLevelDecl(DeclGroupRef DG) override {
+for (const auto *D : DG)
+  performTest(D);
+return true;
+  }
+};
+
+class VariableBindAction : public ASTFrontendAction {
+public:
+  VariableBindAction() {}
+  std::unique_ptr CreateASTConsumer(CompilerInstance &Compiler,
+ StringRef File) override {
+return llvm::make_unique(Compiler);
+  }
+};
+
+// Test that marking s.x as live would also make s live.
+TEST(Store, VariableBind) {
+  EXPECT_TRUE(tooling::runToolOnCode(
+  new VariableBindAction, "void foo() { int x0, y0, z0, x1, y1; }"));
+}
+
+} // namespace
+} // namespace ento
+} // namespace clang
Index: clang/unittests/StaticAnalyzer/CMakeLists.txt
===
--- clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -4,6 +4,7 @@
 
 add_clang_unittest(StaticAnalysisTests
   AnalyzerOptionsTest.cpp
+  StoreTest.cpp
   RegisterCustomCheckersTest.cpp
   SymbolReaperTest.cpp
   )
Index: clang/test/Analysis/string.c
==

[PATCH] D60742: [analyzer] RegionStore: Enable loading default bindings from variables.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, 
mikhail.ramalho, Szelethus, baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, a.sidorin, szepet, 
mgorny.
Herald added a project: clang.

D44934  added modeling for `memset()` that, 
for the case of setting memory to 0, boils down to default-binding a concrete 0 
to the memory region.

Surprisingly, in some cases RegionStore fails to load default bindings from a 
region. That is, it assumes that the region either has a direct binding (i.e., 
initialized via assignment), or it is uninitialized. In particular, this 
applies to local variables regions of integer/pointer/float types.

It seems that a lot of code (eg., `invalidateRegions()`) carefully works around 
this problem by adding either a direct binding or a default binding, depending 
on what `RegionStore` expects. This essentially duplicates `RegionStore`'s 
already-convoluted logic on this subject on the caller side.

Our options are:

1. Duplicate this logic again in `CStringChecker`.
2. Let `RegionStore` automatically do the direct binding for such plain 
integers.
3. Add support for loading default bindings from variables.

Option 1 is a failure to improve checker API in an aspect in which it's already 
very bad, so i didn't go for it. In options 2 and 3 the difference is entirely 
within `RegionStore`. I believe that option 2 is slightly superior because it 
produces a more "normalized" data structure and, additionally, because stores 
are more rare than loads (unless they are "dead stores"^^), so it should be 
slightly more performant. But it is also harder to implement, as it requires 
gathering the logic of when exactly do we need a direct binding in one place, 
so i went for option 3 in order to address the regression and simplify that 
logic. I did not yet try to see how other callers of `bindDefault***()` can be 
simplified.


Repository:
  rC Clang

https://reviews.llvm.org/D60742

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/string.c
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/StoreTest.cpp

Index: clang/unittests/StaticAnalyzer/StoreTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/StoreTest.cpp
@@ -0,0 +1,108 @@
+//===- unittests/StaticAnalyzer/RegionStoreTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Reusables.h"
+
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+// 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();
+
+const auto *VDX0 = findDeclByName(D, "x0");
+const auto *VDY0 = findDeclByName(D, "y0");
+const auto *VDZ0 = findDeclByName(D, "z0");
+const auto *VDX1 = findDeclByName(D, "x1");
+const auto *VDY1 = findDeclByName(D, "y1");
+assert(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);
+
+// Bind(Zero)
+Store StX0 =
+StMgr.Bind(StInit, LX0, Zero).getStore();
+ASSERT_EQ(Zero, StMgr.getBinding(StX0, LX0, ACtx.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()));
+
+// BindDefaultZero()
+Store StZ0 =
+StMgr.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 t