[analyzer] performTrivialCopy triggers checkLocation before binding

[T-Gruber]

The triggered callbacks for the default copy constructed instance and the instance used for initialization now behave in the same way. The LHS already calls checkBind. To keep this consistent, checkLocation is now triggered accordingly for the RHS.
Further details on the previous discussion: https://discourse.llvm.org/t/checklocation-for-implicitcastexpr-of-kind-ck-noop/84729

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: None (T-Gruber)

Changes

The triggered callbacks for the default copy constructed instance and the instance used for initialization now behave in the same way. The LHS already calls checkBind. To keep this consistent, checkLocation is now triggered accordingly for the RHS.
Further details on the previous discussion: https://discourse.llvm.org/t/checklocation-for-implicitcastexpr-of-kind-ck-noop/84729

---

Full diff: https://github.com/llvm/llvm-project/pull/129016.diff

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp (+7-2)
• (modified) clang/unittests/StaticAnalyzer/ExprEngineVisitTest.cpp (+54)

diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index f7020da2e6da2..47888cf9689c7 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -69,6 +69,7 @@ void ExprEngine::performTrivialCopy(NodeBuilder &Bldr, ExplodedNode *Pred,
 
   assert(ThisRD);
   SVal V = Call.getArgSVal(0);
+  const Expr *VExpr = Call.getArgExpr(0);
 
   // If the value being copied is not unknown, load from its location to get
   // an aggregate rvalue.
@@ -76,7 +77,11 @@ void ExprEngine::performTrivialCopy(NodeBuilder &Bldr, ExplodedNode *Pred,
     V = Pred->getState()->getSVal(*L);
   else
     assert(V.isUnknownOrUndef());
-  evalBind(Dst, CallExpr, Pred, ThisVal, V, true);
+  
+  ExplodedNodeSet Tmp;
+  evalLocation(Tmp, CallExpr, VExpr, Pred, Pred->getState(), V, true);
+  for (ExplodedNode *N : Tmp)
+    evalBind(Dst, CallExpr, N, ThisVal, V, true);
 
   PostStmt PS(CallExpr, LCtx);
   for (ExplodedNode *N : Dst) {
@@ -1199,4 +1204,4 @@ void ExprEngine::VisitLambdaExpr(const LambdaExpr *LE, ExplodedNode *Pred,
 
   // FIXME: Move all post/pre visits to ::Visit().
   getCheckerManager().runCheckersForPostStmt(Dst, Tmp, LE, *this);
-}
+}
\ No newline at end of file
diff --git a/clang/unittests/StaticAnalyzer/ExprEngineVisitTest.cpp b/clang/unittests/StaticAnalyzer/ExprEngineVisitTest.cpp
index a8579f9d0f90c..19de9919701a3 100644
--- a/clang/unittests/StaticAnalyzer/ExprEngineVisitTest.cpp
+++ b/clang/unittests/StaticAnalyzer/ExprEngineVisitTest.cpp
@@ -12,6 +12,8 @@
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "llvm/Support/Casting.h"
 #include "gtest/gtest.h"
 
 using namespace clang;
@@ -27,6 +29,14 @@ void emitErrorReport(CheckerContext &C, const BugType &Bug,
   }
 }
 
+inline std::string getMemRegionName(const SVal &Val) {
+  if (auto MemVal = llvm::dyn_cast<loc::MemRegionVal>(Val))
+    return MemVal->getRegion()->getDescriptiveName(false);
+  if (auto ComVal = llvm::dyn_cast<nonloc::LazyCompoundVal>(Val))
+    return ComVal->getRegion()->getDescriptiveName(false);
+  return "";
+}
+
 #define CREATE_EXPR_ENGINE_CHECKER(CHECKER_NAME, CALLBACK, STMT_TYPE,          \
                                    BUG_NAME)                                   \
   class CHECKER_NAME : public Checker<check::CALLBACK<STMT_TYPE>> {            \
@@ -44,6 +54,21 @@ CREATE_EXPR_ENGINE_CHECKER(ExprEngineVisitPreChecker, PreStmt, GCCAsmStmt,
 CREATE_EXPR_ENGINE_CHECKER(ExprEngineVisitPostChecker, PostStmt, GCCAsmStmt,
                            "GCCAsmStmtBug")
 
+class MemAccessChecker : public Checker<check::Location, check::Bind> {
+public:
+  void checkLocation(const SVal &Loc, bool IsLoad, const Stmt *S,
+                     CheckerContext &C) const {
+    emitErrorReport(C, Bug, "checkLocation: Loc = " + getMemRegionName(Loc));
+  }
+
+  void checkBind(SVal Loc, SVal Val, const Stmt *S, CheckerContext &C) const {
+    emitErrorReport(C, Bug, "checkBind: Loc = " + getMemRegionName(Loc));
+  }
+
+private:
+  const BugType Bug{this, "MemAccess"};
+};
+
 void addExprEngineVisitPreChecker(AnalysisASTConsumer &AnalysisConsumer,
                                   AnalyzerOptions &AnOpts) {
   AnOpts.CheckersAndPackages = {{"ExprEngineVisitPreChecker", true}};
@@ -62,6 +87,15 @@ void addExprEngineVisitPostChecker(AnalysisASTConsumer &AnalysisConsumer,
   });
 }
 
+void addMemAccessChecker(AnalysisASTConsumer &AnalysisConsumer,
+                         AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages = {{"MemAccessChecker", true}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+    Registry.addChecker<MemAccessChecker>("MemAccessChecker", "Desc",
+                                          "DocsURI");
+  });
+}
+
 TEST(ExprEngineVisitTest, checkPreStmtGCCAsmStmt) {
   std::string Diags;
   EXPECT_TRUE(runCheckerOnCode<addExprEngineVisitPreChecker>(R"(
@@ -84,4 +118,24 @@ TEST(ExprEngineVisitTest, checkPostStmtGCCAsmStmt) {
   EXPECT_EQ(Diags, "ExprEngineVisitPostChecker: checkPostStmt<GCCAsmStmt>\n");
 }
 
+TEST(ExprEngineVisitTest, checkLocationAndBind) {
+  std::string Diags;
+  EXPECT_TRUE(runCheckerOnCode<addMemAccessChecker>(R"(
+    class MyClass{
+    public:
+      int Value;
+    };
+    extern MyClass MyClassWrite, MyClassRead; 
+    void top() {
+      MyClassWrite = MyClassRead;
+    }
+  )",
+                                                    Diags));
+
+  std::string RHSMsg = "checkLocation: Loc = MyClassRead";
+  std::string LHSMsg = "checkBind: Loc = MyClassWrite";
+  EXPECT_NE(Diags.find(RHSMsg), std::string::npos);
+  EXPECT_NE(Diags.find(LHSMsg), std::string::npos);
+}
+
 } // namespace

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[steakhal]

Thanks for the PR, it looks pretty good by the looks of it.
I had a couple nits inline though, check them out!