Automerge: [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 <[email protected]>

Author: T-Gruber

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

@@ -69,14 +69,20 @@ 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.
   if (std::optional<Loc> L = V.getAs<Loc>())
     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);
-}
+}

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