[clang] 9c542bc - [analyzer] performTrivialCopy triggers checkLocation before binding (#129016)

[via cfe-commits]

Author: T-Gruber
Date: 2025-03-04T17:00:55+01:00
New Revision: 9c542bcf0a1b243dd39c2ecffdd7331c15ae0fb1

URL: https://github.com/llvm/llvm-project/commit/9c542bcf0a1b243dd39c2ecffdd7331c15ae0fb1
DIFF: https://github.com/llvm/llvm-project/commit/9c542bcf0a1b243dd39c2ecffdd7331c15ae0fb1.diff

LOG: [analyzer] performTrivialCopy triggers checkLocation before binding (#129016)

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

---------

Authored-by: tobias.gruber <tobias.gruber at concentrio.io>

Added: 
    

Modified: 
    clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
    clang/unittests/StaticAnalyzer/ExprEngineVisitTest.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index f7020da2e6da2..ff4fa58857fc6 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,12 @@ 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,
+               /*isLoad=*/true);
+  for (ExplodedNode *N : Tmp)
+    evalBind(Dst, CallExpr, N, ThisVal, V, true);
 
   PostStmt PS(CallExpr, LCtx);
   for (ExplodedNode *N : Dst) {
@@ -1199,4 +1205,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..b6eeb9ce37386 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;
@@ -44,6 +46,33 @@ 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 = " + dumpToString(Loc) +
+                        ", Stmt = " + S->getStmtClassName());
+  }
+
+  void checkBind(SVal Loc, SVal Val, const Stmt *S, CheckerContext &C) const {
+    emitErrorReport(C, Bug,
+                    "checkBind: Loc = " + dumpToString(Loc) +
+                        ", Val = " + dumpToString(Val) +
+                        ", Stmt = " + S->getStmtClassName());
+  }
+
+private:
+  const BugType Bug{this, "MemAccess"};
+
+  std::string dumpToString(SVal V) const {
+    std::string StrBuf;
+    llvm::raw_string_ostream StrStream{StrBuf};
+    V.dumpToStream(StrStream);
+    return StrBuf;
+  }
+};
+
 void addExprEngineVisitPreChecker(AnalysisASTConsumer &AnalysisConsumer,
                                   AnalyzerOptions &AnOpts) {
   AnOpts.CheckersAndPackages = {{"ExprEngineVisitPreChecker", true}};
@@ -62,6 +91,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 +122,32 @@ 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 LocMsg = "checkLocation: Loc = lazyCompoundVal{0x0,MyClassRead}, "
+                       "Stmt = ImplicitCastExpr";
+  std::string BindMsg =
+      "checkBind: Loc = &MyClassWrite, Val = lazyCompoundVal{0x0,MyClassRead}, "
+      "Stmt = CXXOperatorCallExpr";
+  std::size_t LocPos = Diags.find(LocMsg);
+  std::size_t BindPos = Diags.find(BindMsg);
+  EXPECT_NE(LocPos, std::string::npos);
+  EXPECT_NE(BindPos, std::string::npos);
+  // Check order: first checkLocation is called, then checkBind.
+  // In the diagnosis, however, the messages appear in reverse order.
+  EXPECT_TRUE(LocPos > BindPos);
+}
+
 } // namespace