[PATCH] D151201: [clang][dataflow] Fix a crash in `getLogicOperatorSubExprValue()`.
This revision was automatically updated to reflect the committed changes.
mboehme marked an inline comment as done.
Closed by commit rG7f6c3a9033b6: [clang][dataflow] Fix a crash in
`getLogicOperatorSubExprValue()`. (authored by mboehme).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151201/new/
https://reviews.llvm.org/D151201
Files:
clang/lib/Analysis/FlowSensitive/Transfer.cpp
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -5140,6 +5140,26 @@
});
}
+// Repro for a crash that used to occur with chained short-circuiting logical
+// operators.
+TEST(TransferTest, ChainedLogicalOps) {
+ std::string Code = R"(
+bool target() {
+ bool b = true || false || false || false;
+ // [[p]]
+ return b;
+}
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+auto &B = getValueForDecl(ASTCtx, Env, "b");
+EXPECT_TRUE(Env.flowConditionImplies(B));
+ });
+}
+
// Repro for a crash that used to occur when we call a `noreturn` function
// within one of the operands of a `&&` or `||` operator.
TEST(TransferTest, NoReturnFunctionInsideShortCircuitedBooleanOp) {
Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -194,24 +194,13 @@
auto &Loc = Env.createStorageLocation(*S);
Env.setStorageLocation(*S, Loc);
- BoolValue *LHSVal = getLogicOperatorSubExprValue(*LHS);
- // If the LHS was not reachable, this BinaryOperator would also not be
- // reachable, and we would never get here.
- assert(LHSVal != nullptr);
- BoolValue *RHSVal = getLogicOperatorSubExprValue(*RHS);
- if (RHSVal == nullptr) {
-// If the RHS isn't reachable and we evaluate this BinaryOperator,
-// then the value of the LHS must have triggered the short-circuit
-// logic. This implies that the value of the entire expression must be
-// equal to the value of the LHS.
-Env.setValue(Loc, *LHSVal);
-break;
- }
+ BoolValue &LHSVal = getLogicOperatorSubExprValue(*LHS);
+ BoolValue &RHSVal = getLogicOperatorSubExprValue(*RHS);
if (S->getOpcode() == BO_LAnd)
-Env.setValue(Loc, Env.makeAnd(*LHSVal, *RHSVal));
+Env.setValue(Loc, Env.makeAnd(LHSVal, RHSVal));
else
-Env.setValue(Loc, Env.makeOr(*LHSVal, *RHSVal));
+Env.setValue(Loc, Env.makeOr(LHSVal, RHSVal));
break;
}
case BO_NE:
@@ -805,35 +794,29 @@
}
private:
- /// If `SubExpr` is reachable, returns a non-null pointer to the value for
- /// `SubExpr`. If `SubExpr` is not reachable, returns nullptr.
- BoolValue *getLogicOperatorSubExprValue(const Expr &SubExpr) {
+ /// Returns the value for the sub-expression `SubExpr` of a logic operator.
+ BoolValue &getLogicOperatorSubExprValue(const Expr &SubExpr) {
// `SubExpr` and its parent logic operator might be part of different basic
// blocks. We try to access the value that is assigned to `SubExpr` in the
// corresponding environment.
-const Environment *SubExprEnv = StmtToEnv.getEnvironment(SubExpr);
-if (!SubExprEnv)
- return nullptr;
-
-if (auto *Val =
-dyn_cast_or_null(SubExprEnv->getValueStrict(SubExpr)))
- return Val;
-
-if (Env.getValueStrict(SubExpr) == nullptr) {
- // Sub-expressions that are logic operators are not added in basic blocks
- // (e.g. see CFG for `bool d = a && (b || c);`). If `SubExpr` is a logic
- // operator, it may not have been evaluated and assigned a value yet. In
- // that case, we need to first visit `SubExpr` and then try to get the
- // value that gets assigned to it.
+if (const Environment *SubExprEnv = StmtToEnv.getEnvironment(SubExpr))
+ if (auto *Val =
+ dyn_cast_or_null(SubExprEnv->getValueStrict(SubExpr)))
+return *Val;
+
+// The sub-expression may lie within a basic block that isn't reachable,
+// even if we need it to evaluate the current (reachable) expression
+// (see https://discourse.llvm.org/t/70775). In this case, visit `SubExpr`
+// within the current environment and then try to get the value that gets
+// assigned to it.
+if (Env.getValueStrict(SubExpr) == nullptr)
Visit(&SubExpr);
-}
-
if (auto *Val = dyn_cast_or_null(Env.getValueStrict(SubExpr)))
- return Val;
+ return *Val;
// If the value of `SubExpr` is still
[PATCH] D151201: [clang][dataflow] Fix a crash in `getLogicOperatorSubExprValue()`.
mboehme marked an inline comment as done.
mboehme added inline comments.
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:5064
+bool target() {
+ return true || false || false || false;
+}
xazax.hun wrote:
> Should we also test that the value of the expression is `true` in the
> analysis state?
Good point -- done.
I've also verified that this test continues to trigger the assert-fail if the
fix is not present.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151201/new/
https://reviews.llvm.org/D151201
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D151201: [clang][dataflow] Fix a crash in `getLogicOperatorSubExprValue()`.
mboehme updated this revision to Diff 525501.
mboehme added a comment.
Add check that the value of the expression is true.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151201/new/
https://reviews.llvm.org/D151201
Files:
clang/lib/Analysis/FlowSensitive/Transfer.cpp
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -5056,6 +5056,26 @@
});
}
+// Repro for a crash that used to occur with chained short-circuiting logical
+// operators.
+TEST(TransferTest, ChainedLogicalOps) {
+ std::string Code = R"(
+bool target() {
+ bool b = true || false || false || false;
+ // [[p]]
+ return b;
+}
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+auto &B = getValueForDecl(ASTCtx, Env, "b");
+EXPECT_TRUE(Env.flowConditionImplies(B));
+ });
+}
+
// Repro for a crash that used to occur when we call a `noreturn` function
// within one of the operands of a `&&` or `||` operator.
TEST(TransferTest, NoReturnFunctionInsideShortCircuitedBooleanOp) {
Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -194,24 +194,13 @@
auto &Loc = Env.createStorageLocation(*S);
Env.setStorageLocation(*S, Loc);
- BoolValue *LHSVal = getLogicOperatorSubExprValue(*LHS);
- // If the LHS was not reachable, this BinaryOperator would also not be
- // reachable, and we would never get here.
- assert(LHSVal != nullptr);
- BoolValue *RHSVal = getLogicOperatorSubExprValue(*RHS);
- if (RHSVal == nullptr) {
-// If the RHS isn't reachable and we evaluate this BinaryOperator,
-// then the value of the LHS must have triggered the short-circuit
-// logic. This implies that the value of the entire expression must be
-// equal to the value of the LHS.
-Env.setValue(Loc, *LHSVal);
-break;
- }
+ BoolValue &LHSVal = getLogicOperatorSubExprValue(*LHS);
+ BoolValue &RHSVal = getLogicOperatorSubExprValue(*RHS);
if (S->getOpcode() == BO_LAnd)
-Env.setValue(Loc, Env.makeAnd(*LHSVal, *RHSVal));
+Env.setValue(Loc, Env.makeAnd(LHSVal, RHSVal));
else
-Env.setValue(Loc, Env.makeOr(*LHSVal, *RHSVal));
+Env.setValue(Loc, Env.makeOr(LHSVal, RHSVal));
break;
}
case BO_NE:
@@ -806,35 +795,29 @@
}
private:
- /// If `SubExpr` is reachable, returns a non-null pointer to the value for
- /// `SubExpr`. If `SubExpr` is not reachable, returns nullptr.
- BoolValue *getLogicOperatorSubExprValue(const Expr &SubExpr) {
+ /// Returns the value for the sub-expression `SubExpr` of a logic operator.
+ BoolValue &getLogicOperatorSubExprValue(const Expr &SubExpr) {
// `SubExpr` and its parent logic operator might be part of different basic
// blocks. We try to access the value that is assigned to `SubExpr` in the
// corresponding environment.
-const Environment *SubExprEnv = StmtToEnv.getEnvironment(SubExpr);
-if (!SubExprEnv)
- return nullptr;
-
-if (auto *Val =
-dyn_cast_or_null(SubExprEnv->getValueStrict(SubExpr)))
- return Val;
-
-if (Env.getValueStrict(SubExpr) == nullptr) {
- // Sub-expressions that are logic operators are not added in basic blocks
- // (e.g. see CFG for `bool d = a && (b || c);`). If `SubExpr` is a logic
- // operator, it may not have been evaluated and assigned a value yet. In
- // that case, we need to first visit `SubExpr` and then try to get the
- // value that gets assigned to it.
+if (const Environment *SubExprEnv = StmtToEnv.getEnvironment(SubExpr))
+ if (auto *Val =
+ dyn_cast_or_null(SubExprEnv->getValueStrict(SubExpr)))
+return *Val;
+
+// The sub-expression may lie within a basic block that isn't reachable,
+// even if we need it to evaluate the current (reachable) expression
+// (see https://discourse.llvm.org/t/70775). In this case, visit `SubExpr`
+// within the current environment and then try to get the value that gets
+// assigned to it.
+if (Env.getValueStrict(SubExpr) == nullptr)
Visit(&SubExpr);
-}
-
if (auto *Val = dyn_cast_or_null(Env.getValueStrict(SubExpr)))
- return Val;
+ return *Val;
// If the value of `SubExpr` is still unknown, we create a fresh symbolic
// boolean value for it.
-return &Env.makeAtomicBoolValue();
+retu
[PATCH] D151201: [clang][dataflow] Fix a crash in `getLogicOperatorSubExprValue()`.
xazax.hun added inline comments.
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:5064
+bool target() {
+ return true || false || false || false;
+}
Should we also test that the value of the expression is `true` in the analysis
state?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151201/new/
https://reviews.llvm.org/D151201
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D151201: [clang][dataflow] Fix a crash in `getLogicOperatorSubExprValue()`.
mboehme created this revision.
Herald added subscribers: martong, xazax.hun.
Herald added a reviewer: NoQ.
Herald added a project: All.
mboehme requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
This patch adds a test that crashes without the fix.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D151201
Files:
clang/lib/Analysis/FlowSensitive/Transfer.cpp
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -5056,6 +5056,20 @@
});
}
+// Repro for a crash that used to occur with chained short-circuiting logical
+// operators.
+TEST(TransferTest, ChainedLogicalOps) {
+ std::string Code = R"(
+bool target() {
+ return true || false || false || false;
+}
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {});
+}
+
// Repro for a crash that used to occur when we call a `noreturn` function
// within one of the operands of a `&&` or `||` operator.
TEST(TransferTest, NoReturnFunctionInsideShortCircuitedBooleanOp) {
Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -194,24 +194,13 @@
auto &Loc = Env.createStorageLocation(*S);
Env.setStorageLocation(*S, Loc);
- BoolValue *LHSVal = getLogicOperatorSubExprValue(*LHS);
- // If the LHS was not reachable, this BinaryOperator would also not be
- // reachable, and we would never get here.
- assert(LHSVal != nullptr);
- BoolValue *RHSVal = getLogicOperatorSubExprValue(*RHS);
- if (RHSVal == nullptr) {
-// If the RHS isn't reachable and we evaluate this BinaryOperator,
-// then the value of the LHS must have triggered the short-circuit
-// logic. This implies that the value of the entire expression must be
-// equal to the value of the LHS.
-Env.setValue(Loc, *LHSVal);
-break;
- }
+ BoolValue &LHSVal = getLogicOperatorSubExprValue(*LHS);
+ BoolValue &RHSVal = getLogicOperatorSubExprValue(*RHS);
if (S->getOpcode() == BO_LAnd)
-Env.setValue(Loc, Env.makeAnd(*LHSVal, *RHSVal));
+Env.setValue(Loc, Env.makeAnd(LHSVal, RHSVal));
else
-Env.setValue(Loc, Env.makeOr(*LHSVal, *RHSVal));
+Env.setValue(Loc, Env.makeOr(LHSVal, RHSVal));
break;
}
case BO_NE:
@@ -811,35 +800,29 @@
}
private:
- /// If `SubExpr` is reachable, returns a non-null pointer to the value for
- /// `SubExpr`. If `SubExpr` is not reachable, returns nullptr.
- BoolValue *getLogicOperatorSubExprValue(const Expr &SubExpr) {
+ /// Returns the value for the sub-expression `SubExpr` of a logic operator.
+ BoolValue &getLogicOperatorSubExprValue(const Expr &SubExpr) {
// `SubExpr` and its parent logic operator might be part of different basic
// blocks. We try to access the value that is assigned to `SubExpr` in the
// corresponding environment.
-const Environment *SubExprEnv = StmtToEnv.getEnvironment(SubExpr);
-if (!SubExprEnv)
- return nullptr;
-
-if (auto *Val =
-dyn_cast_or_null(SubExprEnv->getValueStrict(SubExpr)))
- return Val;
-
-if (Env.getValueStrict(SubExpr) == nullptr) {
- // Sub-expressions that are logic operators are not added in basic blocks
- // (e.g. see CFG for `bool d = a && (b || c);`). If `SubExpr` is a logic
- // operator, it may not have been evaluated and assigned a value yet. In
- // that case, we need to first visit `SubExpr` and then try to get the
- // value that gets assigned to it.
+if (const Environment *SubExprEnv = StmtToEnv.getEnvironment(SubExpr))
+ if (auto *Val =
+ dyn_cast_or_null(SubExprEnv->getValueStrict(SubExpr)))
+return *Val;
+
+// The sub-expression may lie within a basic block that isn't reachable,
+// even if we need it to evaluate the current (reachable) expression
+// (see https://discourse.llvm.org/t/70775). In this case, visit `SubExpr`
+// within the current environment and then try to get the value that gets
+// assigned to it.
+if (Env.getValueStrict(SubExpr) == nullptr)
Visit(&SubExpr);
-}
-
if (auto *Val = dyn_cast_or_null(Env.getValueStrict(SubExpr)))
- return Val;
+ return *Val;
// If the value of `SubExpr` is still unknown, we create a fresh symbolic
// boolean value for it.
-return &Env.makeAtomicBoolValue();
+return Env.makeAtomicBoolValue();
}
// If context sensitivity is enabled, try to analyze the body of the ca
