[PATCH] D48764: [Analyzer] Hotfix for iterator checkers: Mark left-hand side of `SymIntExpr` objects as live in the program state maps.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL337151: [Analyzer] Mark `SymbolData` parts of iterator
position as live in program… (authored by baloghadamsoftware, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D48764?vs=153698=155634#toc
Repository:
rL LLVM
https://reviews.llvm.org/D48764
Files:
cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -488,14 +488,18 @@
// alive
auto RegionMap = State->get();
for (const auto Reg : RegionMap) {
-const auto Pos = Reg.second;
-SR.markLive(Pos.getOffset());
+const auto Offset = Reg.second.getOffset();
+for (auto i = Offset->symbol_begin(); i != Offset->symbol_end(); ++i)
+ if (isa(*i))
+SR.markLive(*i);
}
auto SymbolMap = State->get();
for (const auto Sym : SymbolMap) {
-const auto Pos = Sym.second;
-SR.markLive(Pos.getOffset());
+const auto Offset = Sym.second.getOffset();
+for (auto i = Offset->symbol_begin(); i != Offset->symbol_end(); ++i)
+ if (isa(*i))
+SR.markLive(*i);
}
auto ContMap = State->get();
@@ -1157,21 +1161,31 @@
const IteratorPosition ,
bool Equal) {
auto = State->getStateManager().getSValBuilder();
+
+ // FIXME: This code should be reworked as follows:
+ // 1. Subtract the operands using evalBinOp().
+ // 2. Assume that the result doesn't overflow.
+ // 3. Compare the result to 0.
+ // 4. Assume the result of the comparison.
const auto comparison =
SVB.evalBinOp(State, BO_EQ, nonloc::SymbolVal(Pos1.getOffset()),
-nonloc::SymbolVal(Pos2.getOffset()),
SVB.getConditionType())
- .getAs();
+nonloc::SymbolVal(Pos2.getOffset()),
+SVB.getConditionType());
- if (comparison) {
-auto NewState = State->assume(*comparison, Equal);
-if (const auto CompSym = comparison->getAsSymbol()) {
- return assumeNoOverflow(NewState, cast(CompSym)->getLHS(),
2);
-}
+ assert(comparison.getAs() &&
+"Symbol comparison must be a `DefinedSVal`");
-return NewState;
+ auto NewState = State->assume(comparison.castAs(), Equal);
+ if (const auto CompSym = comparison.getAsSymbol()) {
+assert(isa(CompSym) &&
+ "Symbol comparison must be a `SymIntExpr`");
+assert(BinaryOperator::isComparisonOp(
+ cast(CompSym)->getOpcode()) &&
+ "Symbol comparison must be a comparison");
+return assumeNoOverflow(NewState, cast(CompSym)->getLHS(), 2);
}
- return State;
+ return NewState;
}
bool isZero(ProgramStateRef State, const NonLoc ) {
@@ -1225,14 +1239,12 @@
auto = State->getStateManager().getSValBuilder();
const auto comparison =
- SVB.evalBinOp(State, Opc, NL1, NL2, SVB.getConditionType())
- .getAs();
+SVB.evalBinOp(State, Opc, NL1, NL2, SVB.getConditionType());
- if (comparison) {
-return !State->assume(*comparison, false);
- }
+ assert(comparison.getAs() &&
+"Symbol comparison must be a `DefinedSVal`");
- return false;
+ return !State->assume(comparison.castAs(), false);
}
} // namespace
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -488,14 +488,18 @@
// alive
auto RegionMap = State->get();
for (const auto Reg : RegionMap) {
-const auto Pos = Reg.second;
-SR.markLive(Pos.getOffset());
+const auto Offset = Reg.second.getOffset();
+for (auto i = Offset->symbol_begin(); i != Offset->symbol_end(); ++i)
+ if (isa(*i))
+SR.markLive(*i);
}
auto SymbolMap = State->get();
for (const auto Sym : SymbolMap) {
-const auto Pos = Sym.second;
-SR.markLive(Pos.getOffset());
+const auto Offset = Sym.second.getOffset();
+for (auto i = Offset->symbol_begin(); i != Offset->symbol_end(); ++i)
+ if (isa(*i))
+SR.markLive(*i);
}
auto ContMap = State->get();
@@ -1157,21 +1161,31 @@
const IteratorPosition ,
bool Equal) {
auto = State->getStateManager().getSValBuilder();
+
+ // FIXME: This code should be reworked as follows:
+ // 1. Subtract the operands using evalBinOp().
+ // 2. Assume that the result doesn't overflow.
+ // 3. Compare the result to 0.
+ // 4. Assume the result of the comparison.
const auto comparison =
SVB.evalBinOp(State, BO_EQ,
[PATCH] D48764: [Analyzer] Hotfix for iterator checkers: Mark left-hand side of `SymIntExpr` objects as live in the program state maps.
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Looks good otherwise, please commit. https://reviews.llvm.org/D48764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48764: [Analyzer] Hotfix for iterator checkers: Mark left-hand side of `SymIntExpr` objects as live in the program state maps.
NoQ added a comment.
Looks good with minor comments.
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:493
+for (auto i = Offset->symbol_begin(); i != Offset->symbol_end(); ++i)
+ SR.markLive(*i);
}
Let's only mark `SymbolData`-type symbols as live; that should be enough.
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1168
+ assert(comparison.getAs() &&
+"Symbol comparison must by a `DefinedSVal`");
+
Typo: "be".
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1170-1177
+ auto NewState = State->assume(comparison.castAs(), Equal);
+ if (const auto CompSym = comparison.getAsSymbol()) {
+assert(isa(CompSym) &&
+ "Symbol comparison must be a `SymIntExpr`");
+assert(BinaryOperator::isComparisonOp(
+ cast(CompSym)->getOpcode()) &&
+ "Symbol comparison must be a comparison");
I believe this code should be reworked as follows:
1. Subtract the operands using `evalBinOp()`.
2. Assume that the result doesn't overflow.
3. Compare the result to 0.
4. Assume the result of the comparison.
This should hopefully yield the same result without ever needing to introspect
the `SymExpr`.
I suggest a FIXME.
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1237
+ assert(comparison.getAs() &&
+"Symbol comparison must by a `DefinedSVal`");
Also typo.
https://reviews.llvm.org/D48764
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48764: [Analyzer] Hotfix for iterator checkers: Mark left-hand side of `SymIntExpr` objects as live in the program state maps.
baloghadamsoftware updated this revision to Diff 153698.
baloghadamsoftware added a comment.
Updated according to the comments and assertions added to fail the tests
without the fix.
https://reviews.llvm.org/D48764
Files:
lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -488,14 +488,16 @@
// alive
auto RegionMap = State->get();
for (const auto Reg : RegionMap) {
-const auto Pos = Reg.second;
-SR.markLive(Pos.getOffset());
+const auto Offset = Reg.second.getOffset();
+for (auto i = Offset->symbol_begin(); i != Offset->symbol_end(); ++i)
+ SR.markLive(*i);
}
auto SymbolMap = State->get();
for (const auto Sym : SymbolMap) {
-const auto Pos = Sym.second;
-SR.markLive(Pos.getOffset());
+const auto Offset = Sym.second.getOffset();
+for (auto i = Offset->symbol_begin(); i != Offset->symbol_end(); ++i)
+ SR.markLive(*i);
}
auto ContMap = State->get();
@@ -1159,19 +1161,23 @@
auto = State->getStateManager().getSValBuilder();
const auto comparison =
SVB.evalBinOp(State, BO_EQ, nonloc::SymbolVal(Pos1.getOffset()),
-nonloc::SymbolVal(Pos2.getOffset()),
SVB.getConditionType())
- .getAs();
-
- if (comparison) {
-auto NewState = State->assume(*comparison, Equal);
-if (const auto CompSym = comparison->getAsSymbol()) {
- return assumeNoOverflow(NewState, cast(CompSym)->getLHS(),
2);
-}
-
-return NewState;
+nonloc::SymbolVal(Pos2.getOffset()),
+SVB.getConditionType());
+
+ assert(comparison.getAs() &&
+"Symbol comparison must by a `DefinedSVal`");
+
+ auto NewState = State->assume(comparison.castAs(), Equal);
+ if (const auto CompSym = comparison.getAsSymbol()) {
+assert(isa(CompSym) &&
+ "Symbol comparison must be a `SymIntExpr`");
+assert(BinaryOperator::isComparisonOp(
+ cast(CompSym)->getOpcode()) &&
+ "Symbol comparison must be a comparison");
+return assumeNoOverflow(NewState, cast(CompSym)->getLHS(), 2);
}
- return State;
+ return NewState;
}
bool isZero(ProgramStateRef State, const NonLoc ) {
@@ -1225,14 +1231,12 @@
auto = State->getStateManager().getSValBuilder();
const auto comparison =
- SVB.evalBinOp(State, Opc, NL1, NL2, SVB.getConditionType())
- .getAs();
+SVB.evalBinOp(State, Opc, NL1, NL2, SVB.getConditionType());
- if (comparison) {
-return !State->assume(*comparison, false);
- }
+ assert(comparison.getAs() &&
+"Symbol comparison must by a `DefinedSVal`");
- return false;
+ return !State->assume(comparison.castAs(), false);
}
} // namespace
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -488,14 +488,16 @@
// alive
auto RegionMap = State->get();
for (const auto Reg : RegionMap) {
-const auto Pos = Reg.second;
-SR.markLive(Pos.getOffset());
+const auto Offset = Reg.second.getOffset();
+for (auto i = Offset->symbol_begin(); i != Offset->symbol_end(); ++i)
+ SR.markLive(*i);
}
auto SymbolMap = State->get();
for (const auto Sym : SymbolMap) {
-const auto Pos = Sym.second;
-SR.markLive(Pos.getOffset());
+const auto Offset = Sym.second.getOffset();
+for (auto i = Offset->symbol_begin(); i != Offset->symbol_end(); ++i)
+ SR.markLive(*i);
}
auto ContMap = State->get();
@@ -1159,19 +1161,23 @@
auto = State->getStateManager().getSValBuilder();
const auto comparison =
SVB.evalBinOp(State, BO_EQ, nonloc::SymbolVal(Pos1.getOffset()),
-nonloc::SymbolVal(Pos2.getOffset()), SVB.getConditionType())
- .getAs();
-
- if (comparison) {
-auto NewState = State->assume(*comparison, Equal);
-if (const auto CompSym = comparison->getAsSymbol()) {
- return assumeNoOverflow(NewState, cast(CompSym)->getLHS(), 2);
-}
-
-return NewState;
+nonloc::SymbolVal(Pos2.getOffset()),
+SVB.getConditionType());
+
+ assert(comparison.getAs() &&
+"Symbol comparison must by a `DefinedSVal`");
+
+ auto NewState = State->assume(comparison.castAs(), Equal);
+ if (const auto CompSym = comparison.getAsSymbol()) {
+assert(isa(CompSym) &&
+ "Symbol comparison must be a `SymIntExpr`");
+assert(BinaryOperator::isComparisonOp(
+ cast(CompSym)->getOpcode()) &&
+ "Symbol comparison must be a comparison");
+return assumeNoOverflow(NewState, cast(CompSym)->getLHS(), 2);
}
- return State;
+ return NewState;
}
bool
[PATCH] D48764: [Analyzer] Hotfix for iterator checkers: Mark left-hand side of `SymIntExpr` objects as live in the program state maps.
NoQ added a comment. That's right. You only need to mark "atomic" symbols (`SymbolData`) as live, and expressions that contain them would automatically become live. So i think you should just iterate through a `symbol_iterator` and mark all `SymbolData` symbols you encounter as live. Is this a hotfix for a test failure? Otherwise it'd be great to have tests. https://reviews.llvm.org/D48764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48764: [Analyzer] Hotfix for iterator checkers: Mark left-hand side of `SymIntExpr` objects as live in the program state maps.
baloghadamsoftware created this revision.
baloghadamsoftware added reviewers: NoQ, mikhail.ramalho.
Herald added subscribers: a.sidorin, dkrupp, rnkovacs, szepet, xazax.hun,
whisperity.
Herald added a reviewer: george.karpenkov.
Marking a symbolic expression as live is not recursive. In our checkers we
either use conjured symbols or conjured symbols plus/minus concrete integers to
represent abstract positions of iterators, so we must mark the left-hand side
of these expressions as live to prevent them from getting reaped.
https://reviews.llvm.org/D48764
Files:
lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -489,12 +489,16 @@
auto RegionMap = State->get();
for (const auto Reg : RegionMap) {
const auto Pos = Reg.second;
+if (const auto *SIE = dyn_cast(Pos.getOffset()))
+ SR.markLive(SIE->getLHS());
SR.markLive(Pos.getOffset());
}
auto SymbolMap = State->get();
for (const auto Sym : SymbolMap) {
const auto Pos = Sym.second;
+if (const auto *SIE = dyn_cast(Pos.getOffset()))
+ SR.markLive(SIE->getLHS());
SR.markLive(Pos.getOffset());
}
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -489,12 +489,16 @@
auto RegionMap = State->get();
for (const auto Reg : RegionMap) {
const auto Pos = Reg.second;
+if (const auto *SIE = dyn_cast(Pos.getOffset()))
+ SR.markLive(SIE->getLHS());
SR.markLive(Pos.getOffset());
}
auto SymbolMap = State->get();
for (const auto Sym : SymbolMap) {
const auto Pos = Sym.second;
+if (const auto *SIE = dyn_cast(Pos.getOffset()))
+ SR.markLive(SIE->getLHS());
SR.markLive(Pos.getOffset());
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
