[analyzer] Pass the correct loc Expr from VisitIncDecOp to evalStore

[r.stahl]

Summary

The LocationE parameter of evalStore is documented as "The location expression that is stored to". When storing from an increment / decrement operator this was not satisfied. In user code this causes an inconsistency between the SVal and Stmt parameters of checkLocation.

[r.stahl]

cfe-dev thread with short discussion: http://lists.llvm.org/pipermail/cfe-dev/2018-April/057562.html

[NoQ]

Thanks! Neat test.

Can we now assert(LocationE->isGLValue()) in evalStore()? What about evalLoad()?

Also, were no other checkers affected? Like, will null pointer dereference checker now warn upon something like (*0)++ or ++(*0)?

You can commit the patch, but i would also love to see these questions investigated.

[r.stahl]

rebased

[r.stahl]

I tried adding isGLValue to evalStore and the test suite didn't complain. For evalLoad (on BoundEx) it failed in pretty much every test. Should the evalStore assert also go into trunk?

Since the analyzer behavior itself remains unchanged, I don't believe any other checker should be affected.

For myself it was pretty obvious that there is something weird going on when I didn't get the expected Stmt in my checker callback. So I added the following workaround. With this patch, the workaround is safely skipped.

The only change that this patch might cause "in the wild" is when someone used the Stmt as location, but didn't test with IncDecOps. However, as far as I can tell this should only have positive outcome.

Do I have any other means to check if other checkers were affected than to run the test suite?

// Workaround for an inconsistency with IncDecOps: The statement is not the location expr.
if (auto unaryE = dyn_cast<UnaryOperator>(S))
{
  if (unaryE->isIncrementDecrementOp())
  {
    S = unaryE->getSubExpr();
  }
}

lib/StaticAnalyzer/Core/ExprEngineC.cpp

for (ExplodedNodeSet::iterator I=Tmp.begin(), E=Tmp.end();I!=E;++I) {
 
     // Propagate unknown and undefined values.
     if (V2_untested.isUnknownOrUndef()) {
       state = state->BindExpr(U, LCtx, V2_untested);
 
       // Perform the store, so that the uninitialized value detection happens.
       Bldr.takeNodes(*I);
       ExplodedNodeSet Dst3;
-      evalStore(Dst3, U, U, *I, state, loc, V2_untested);
+      evalStore(Dst3, U, Ex, *I, state, loc, V2_untested);
       Bldr.addNodes(Dst3);
 
       continue;
     }
     DefinedSVal V2 = V2_untested.castAs<DefinedSVal>();
 
     // Handle all other values.
     BinaryOperator::Opcode Op = U->isIncrementOp() ? BO_Add : BO_Sub;
for (ExplodedNodeSet::iterator I=Tmp.begin(), E=Tmp.end();I!=E;++I) {
     if (U->isGLValue())
       state = state->BindExpr(U, LCtx, loc);
     else
       state = state->BindExpr(U, LCtx, U->isPostfix() ? V2 : Result);
 
     // Perform the store.
     Bldr.takeNodes(*I);
     ExplodedNodeSet Dst3;
-    evalStore(Dst3, U, U, *I, state, loc, Result);
+    evalStore(Dst3, U, Ex, *I, state, loc, Result);
     Bldr.addNodes(Dst3);
   }
   Dst.insert(Dst2);
 }

unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

 #include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
 #include "clang/Tooling/Tooling.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace ento {
 namespace {
 
-class CustomChecker : public Checker<check::ASTCodeBody> {
-public:
-  void checkASTCodeBody(const Decl *D, AnalysisManager &Mgr,
-                        BugReporter &BR) const {
-    BR.EmitBasicReport(D, this, "Custom diagnostic", categories::LogicError,
-                       "Custom diagnostic description",
-                       PathDiagnosticLocation(D, Mgr.getSourceManager()), {});
-  }
-};
-
+template <typename CheckerT>
 class TestAction : public ASTFrontendAction {
   class DiagConsumer : public PathDiagnosticConsumer {
     llvm::raw_ostream &Output;
 
   public:
     DiagConsumer(llvm::raw_ostream &Output) : Output(Output) {}
     void FlushDiagnosticsImpl(std::vector<const PathDiagnostic *> &Diags,
                               FilesMade *filesMade) override {
public:
   std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &Compiler,
                                                  StringRef File) override {
     std::unique_ptr<AnalysisASTConsumer> AnalysisConsumer =
         CreateAnalysisConsumer(Compiler);
     AnalysisConsumer->AddDiagnosticConsumer(new DiagConsumer(DiagsOutput));
     Compiler.getAnalyzerOpts()->CheckersControlList = {
         {"custom.CustomChecker", true}};
     AnalysisConsumer->AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
-      Registry.addChecker<CustomChecker>("custom.CustomChecker", "Description",
-                                         "");
+      Registry.addChecker<CheckerT>("custom.CustomChecker", "Description", "");
     });
     return std::move(AnalysisConsumer);
   }
 };
 
+template <typename CheckerT>
+bool runCheckerOnCode(const std::string &Code, std::string &Diags) {
+  llvm::raw_string_ostream OS(Diags);
+  return tooling::runToolOnCode(new TestAction<CheckerT>(OS), Code);
+}
+template <typename CheckerT>
+bool runCheckerOnCode(const std::string &Code) {
+  std::string Diags;
+  return runCheckerOnCode<CheckerT>(Code, Diags);
+}
+
+
+class CustomChecker : public Checker<check::ASTCodeBody> {
+public:
+  void checkASTCodeBody(const Decl *D, AnalysisManager &Mgr,
+                        BugReporter &BR) const {
+    BR.EmitBasicReport(D, this, "Custom diagnostic", categories::LogicError,
+                       "Custom diagnostic description",
+                       PathDiagnosticLocation(D, Mgr.getSourceManager()), {});
+  }
+};
 
 TEST(RegisterCustomCheckers, RegisterChecker) {
   std::string Diags;
-  {
-    llvm::raw_string_ostream OS(Diags);
-    EXPECT_TRUE(tooling::runToolOnCode(new TestAction(OS), "void f() {;}"));
-  }
+  EXPECT_TRUE(runCheckerOnCode<CustomChecker>("void f() {;}", Diags));
   EXPECT_EQ(Diags, "custom.CustomChecker:Custom diagnostic description");
 }
 
+class LocIncDecChecker : public Checker<check::Location> {
+public:
+  void checkLocation(SVal Loc, bool IsLoad, const Stmt *S,
+                     CheckerContext &C) const {
+    auto UnaryOp = dyn_cast<UnaryOperator>(S);
+    if (UnaryOp && !IsLoad)
+      EXPECT_FALSE(UnaryOp->isIncrementOp());
+  }
+};
+
+TEST(RegisterCustomCheckers, CheckLocationIncDec) {
+  EXPECT_TRUE(
+      runCheckerOnCode<LocIncDecChecker>("void f() { int *p; (*p)++; }"));
+}
+
 }
 }
 }