[PATCH] D55701: [analyzer] Pass the correct loc Expr from VisitIncDecOp to evalStore
This revision was automatically updated to reflect the committed changes.
Closed by commit rC350528: [analyzer] Pass the correct loc Expr from
VisitIncDecOp to evalStore (authored by r.stahl, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D55701?vs=180487=180489#toc
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55701/new/
https://reviews.llvm.org/D55701
Files:
lib/StaticAnalyzer/Core/ExprEngineC.cpp
unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
Index: lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -1052,7 +1052,7 @@
// 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;
@@ -1120,7 +1120,7 @@
// 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);
Index: unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
===
--- unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
+++ unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
@@ -21,16 +21,7 @@
namespace ento {
namespace {
-class CustomChecker : public Checker {
-public:
- void checkASTCodeBody(const Decl *D, AnalysisManager ,
-BugReporter ) const {
-BR.EmitBasicReport(D, this, "Custom diagnostic", categories::LogicError,
- "Custom diagnostic description",
- PathDiagnosticLocation(D, Mgr.getSourceManager()), {});
- }
-};
-
+template
class TestAction : public ASTFrontendAction {
class DiagConsumer : public PathDiagnosticConsumer {
llvm::raw_ostream
@@ -59,23 +50,55 @@
Compiler.getAnalyzerOpts()->CheckersControlList = {
{"custom.CustomChecker", true}};
AnalysisConsumer->AddCheckerRegistrationFn([](CheckerRegistry ) {
- Registry.addChecker("custom.CustomChecker", "Description",
- "");
+ Registry.addChecker("custom.CustomChecker", "Description", "");
});
return std::move(AnalysisConsumer);
}
};
+template
+bool runCheckerOnCode(const std::string , std::string ) {
+ llvm::raw_string_ostream OS(Diags);
+ return tooling::runToolOnCode(new TestAction(OS), Code);
+}
+template
+bool runCheckerOnCode(const std::string ) {
+ std::string Diags;
+ return runCheckerOnCode(Code, Diags);
+}
+
+
+class CustomChecker : public Checker {
+public:
+ void checkASTCodeBody(const Decl *D, AnalysisManager ,
+BugReporter ) 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("void f() {;}", Diags));
EXPECT_EQ(Diags, "custom.CustomChecker:Custom diagnostic description");
}
+class LocIncDecChecker : public Checker {
+public:
+ void checkLocation(SVal Loc, bool IsLoad, const Stmt *S,
+ CheckerContext ) const {
+auto UnaryOp = dyn_cast(S);
+if (UnaryOp && !IsLoad)
+ EXPECT_FALSE(UnaryOp->isIncrementOp());
+ }
+};
+
+TEST(RegisterCustomCheckers, CheckLocationIncDec) {
+ EXPECT_TRUE(
+ runCheckerOnCode("void f() { int *p; (*p)++; }"));
+}
+
}
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55701: [analyzer] Pass the correct loc Expr from VisitIncDecOp to evalStore
r.stahl added a comment.
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(S))
{
if (unaryE->isIncrementDecrementOp())
{
S = unaryE->getSubExpr();
}
}
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55701/new/
https://reviews.llvm.org/D55701
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55701: [analyzer] Pass the correct loc Expr from VisitIncDecOp to evalStore
r.stahl updated this revision to Diff 180487.
r.stahl added a comment.
rebased
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55701/new/
https://reviews.llvm.org/D55701
Files:
lib/StaticAnalyzer/Core/ExprEngineC.cpp
unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
Index: unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
===
--- unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
+++ unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
@@ -21,16 +21,7 @@
namespace ento {
namespace {
-class CustomChecker : public Checker {
-public:
- void checkASTCodeBody(const Decl *D, AnalysisManager ,
-BugReporter ) const {
-BR.EmitBasicReport(D, this, "Custom diagnostic", categories::LogicError,
- "Custom diagnostic description",
- PathDiagnosticLocation(D, Mgr.getSourceManager()), {});
- }
-};
-
+template
class TestAction : public ASTFrontendAction {
class DiagConsumer : public PathDiagnosticConsumer {
llvm::raw_ostream
@@ -59,23 +50,55 @@
Compiler.getAnalyzerOpts()->CheckersControlList = {
{"custom.CustomChecker", true}};
AnalysisConsumer->AddCheckerRegistrationFn([](CheckerRegistry ) {
- Registry.addChecker("custom.CustomChecker", "Description",
- "");
+ Registry.addChecker("custom.CustomChecker", "Description", "");
});
return std::move(AnalysisConsumer);
}
};
+template
+bool runCheckerOnCode(const std::string , std::string ) {
+ llvm::raw_string_ostream OS(Diags);
+ return tooling::runToolOnCode(new TestAction(OS), Code);
+}
+template
+bool runCheckerOnCode(const std::string ) {
+ std::string Diags;
+ return runCheckerOnCode(Code, Diags);
+}
+
+
+class CustomChecker : public Checker {
+public:
+ void checkASTCodeBody(const Decl *D, AnalysisManager ,
+BugReporter ) 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("void f() {;}", Diags));
EXPECT_EQ(Diags, "custom.CustomChecker:Custom diagnostic description");
}
+class LocIncDecChecker : public Checker {
+public:
+ void checkLocation(SVal Loc, bool IsLoad, const Stmt *S,
+ CheckerContext ) const {
+auto UnaryOp = dyn_cast(S);
+if (UnaryOp && !IsLoad)
+ EXPECT_FALSE(UnaryOp->isIncrementOp());
+ }
+};
+
+TEST(RegisterCustomCheckers, CheckLocationIncDec) {
+ EXPECT_TRUE(
+ runCheckerOnCode("void f() { int *p; (*p)++; }"));
+}
+
}
}
}
Index: lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -1052,7 +1052,7 @@
// 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;
@@ -1120,7 +1120,7 @@
// 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);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55701: [analyzer] Pass the correct loc Expr from VisitIncDecOp to evalStore
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. 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. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55701/new/ https://reviews.llvm.org/D55701 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55701: [analyzer] Pass the correct loc Expr from VisitIncDecOp to evalStore
r.stahl added a comment. cfe-dev thread with short discussion: http://lists.llvm.org/pipermail/cfe-dev/2018-April/057562.html Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55701/new/ https://reviews.llvm.org/D55701 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55701: [analyzer] Pass the correct loc Expr from VisitIncDecOp to evalStore
r.stahl created this revision.
r.stahl added reviewers: NoQ, dcoughlin, george.karpenkov.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus,
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
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.
Repository:
rC Clang
https://reviews.llvm.org/D55701
Files:
lib/StaticAnalyzer/Core/ExprEngineC.cpp
unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
Index: unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
===
--- unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
+++ unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
@@ -21,16 +21,7 @@
namespace ento {
namespace {
-class CustomChecker : public Checker {
-public:
- void checkASTCodeBody(const Decl *D, AnalysisManager ,
-BugReporter ) const {
-BR.EmitBasicReport(D, this, "Custom diagnostic", categories::LogicError,
- "Custom diagnostic description",
- PathDiagnosticLocation(D, Mgr.getSourceManager()), {});
- }
-};
-
+template
class TestAction : public ASTFrontendAction {
class DiagConsumer : public PathDiagnosticConsumer {
llvm::raw_ostream
@@ -59,22 +50,55 @@
Compiler.getAnalyzerOpts()->CheckersControlList = {
{"custom.CustomChecker", true}};
AnalysisConsumer->AddCheckerRegistrationFn([](CheckerRegistry ) {
- Registry.addChecker("custom.CustomChecker", "Description");
+ Registry.addChecker("custom.CustomChecker", "Description");
});
return std::move(AnalysisConsumer);
}
};
+template
+bool runCheckerOnCode(const std::string , std::string ) {
+ llvm::raw_string_ostream OS(Diags);
+ return tooling::runToolOnCode(new TestAction(OS), Code);
+}
+template
+bool runCheckerOnCode(const std::string ) {
+ std::string Diags;
+ return runCheckerOnCode(Code, Diags);
+}
+
+
+class CustomChecker : public Checker {
+public:
+ void checkASTCodeBody(const Decl *D, AnalysisManager ,
+BugReporter ) 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("void f() {;}", Diags));
EXPECT_EQ(Diags, "custom.CustomChecker:Custom diagnostic description");
}
+class LocIncDecChecker : public Checker {
+public:
+ void checkLocation(SVal Loc, bool IsLoad, const Stmt *S,
+ CheckerContext ) const {
+auto UnaryOp = dyn_cast(S);
+if (UnaryOp && !IsLoad)
+ EXPECT_FALSE(UnaryOp->isIncrementOp());
+ }
+};
+
+TEST(RegisterCustomCheckers, CheckLocationIncDec) {
+ EXPECT_TRUE(
+ runCheckerOnCode("void f() { int *p; (*p)++; }"));
+}
+
}
}
}
Index: lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -1052,7 +1052,7 @@
// 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;
@@ -1120,7 +1120,7 @@
// 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);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
