[clang][dataflow] Fix result object location for builtin <=>.
#88726
Conversation
The newly added test causes an assertion failure in `PropagateResultObject()` without the fix added here.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis Author: None (martinboehme) ChangesThe newly added test causes an assertion failure in Full diff: https://github.com/llvm/llvm-project/pull/88726.diff 2 Files Affected:
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index bea15ce9bd24d1..ee2581143e1141 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -508,6 +508,11 @@ class ResultObjectVisitor : public RecursiveASTVisitor<ResultObjectVisitor> {
isa<CXXStdInitializerListExpr>(E)) {
return;
}
+ if (auto *Op = dyn_cast<BinaryOperator>(E);
+ Op && Op->getOpcode() == BO_Cmp) {
+ // Builtin `<=>` returns a `std::strong_ordering` object.
+ return;
+ }
if (auto *InitList = dyn_cast<InitListExpr>(E)) {
if (!InitList->isSemanticForm())
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 00dafb2988c690..d8bcc3da4b8b1c 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3098,6 +3098,58 @@ TEST(TransferTest, ResultObjectLocationForCXXOperatorCallExpr) {
});
}
+// Check that the `std::strong_ordering` object returned by builtin `<=>` has a
+// correctly modeled result object location.
+TEST(TransferTest, ResultObjectLocationForBuiltinSpaceshipOperator) {
+ std::string Code = R"(
+ namespace std {
+ // This is the minimal definition required to get
+ // `Sema::CheckComparisonCategoryType()` to accept this fake.
+ struct strong_ordering {
+ enum class ordering { less, equal, greater };
+ ordering o;
+ static const strong_ordering less;
+ static const strong_ordering equivalent;
+ static const strong_ordering equal;
+ static const strong_ordering greater;
+ };
+
+ inline constexpr strong_ordering strong_ordering::less =
+ { strong_ordering::ordering::less };
+ inline constexpr strong_ordering strong_ordering::equal =
+ { strong_ordering::ordering::equal };
+ inline constexpr strong_ordering strong_ordering::equivalent =
+ { strong_ordering::ordering::equal };
+ inline constexpr strong_ordering strong_ordering::greater =
+ { strong_ordering::ordering::greater };
+ }
+ void target(int i, int j) {
+ auto ordering = i <=> j;
+ // [[p]]
+ }
+ )";
+ using ast_matchers::binaryOperator;
+ using ast_matchers::hasOperatorName;
+ using ast_matchers::match;
+ using ast_matchers::selectFirst;
+ using ast_matchers::traverse;
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+ auto *Spaceship = selectFirst<BinaryOperator>(
+ "op",
+ match(binaryOperator(hasOperatorName("<=>")).bind("op"), ASTCtx));
+
+ EXPECT_EQ(
+ &Env.getResultObjectLocation(*Spaceship),
+ &getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "ordering"));
+ },
+ LangStandard::lang_cxx20);
+}
+
TEST(TransferTest, ResultObjectLocationForStdInitializerListExpr) {
std::string Code = R"(
namespace std {
|
|
@llvm/pr-subscribers-clang Author: None (martinboehme) ChangesThe newly added test causes an assertion failure in Full diff: https://github.com/llvm/llvm-project/pull/88726.diff 2 Files Affected:
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index bea15ce9bd24d1..ee2581143e1141 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -508,6 +508,11 @@ class ResultObjectVisitor : public RecursiveASTVisitor<ResultObjectVisitor> {
isa<CXXStdInitializerListExpr>(E)) {
return;
}
+ if (auto *Op = dyn_cast<BinaryOperator>(E);
+ Op && Op->getOpcode() == BO_Cmp) {
+ // Builtin `<=>` returns a `std::strong_ordering` object.
+ return;
+ }
if (auto *InitList = dyn_cast<InitListExpr>(E)) {
if (!InitList->isSemanticForm())
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 00dafb2988c690..d8bcc3da4b8b1c 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3098,6 +3098,58 @@ TEST(TransferTest, ResultObjectLocationForCXXOperatorCallExpr) {
});
}
+// Check that the `std::strong_ordering` object returned by builtin `<=>` has a
+// correctly modeled result object location.
+TEST(TransferTest, ResultObjectLocationForBuiltinSpaceshipOperator) {
+ std::string Code = R"(
+ namespace std {
+ // This is the minimal definition required to get
+ // `Sema::CheckComparisonCategoryType()` to accept this fake.
+ struct strong_ordering {
+ enum class ordering { less, equal, greater };
+ ordering o;
+ static const strong_ordering less;
+ static const strong_ordering equivalent;
+ static const strong_ordering equal;
+ static const strong_ordering greater;
+ };
+
+ inline constexpr strong_ordering strong_ordering::less =
+ { strong_ordering::ordering::less };
+ inline constexpr strong_ordering strong_ordering::equal =
+ { strong_ordering::ordering::equal };
+ inline constexpr strong_ordering strong_ordering::equivalent =
+ { strong_ordering::ordering::equal };
+ inline constexpr strong_ordering strong_ordering::greater =
+ { strong_ordering::ordering::greater };
+ }
+ void target(int i, int j) {
+ auto ordering = i <=> j;
+ // [[p]]
+ }
+ )";
+ using ast_matchers::binaryOperator;
+ using ast_matchers::hasOperatorName;
+ using ast_matchers::match;
+ using ast_matchers::selectFirst;
+ using ast_matchers::traverse;
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+ auto *Spaceship = selectFirst<BinaryOperator>(
+ "op",
+ match(binaryOperator(hasOperatorName("<=>")).bind("op"), ASTCtx));
+
+ EXPECT_EQ(
+ &Env.getResultObjectLocation(*Spaceship),
+ &getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "ordering"));
+ },
+ LangStandard::lang_cxx20);
+}
+
TEST(TransferTest, ResultObjectLocationForStdInitializerListExpr) {
std::string Code = R"(
namespace std {
|
| @@ -508,6 +508,11 @@ class ResultObjectVisitor : public RecursiveASTVisitor<ResultObjectVisitor> { | |||
| isa<CXXStdInitializerListExpr>(E)) { | |||
| return; | |||
| } | |||
| if (auto *Op = dyn_cast<BinaryOperator>(E); | |||
There was a problem hiding this comment.
aside: Might lines 506 through 553 be better expressed as a switch on E->getStmtClass()?
There was a problem hiding this comment.
I'm not sure.
-
The cases involving a
dyn_castneed a cast anyway, as they access properties of the cast class. So if we introduced a switch, we would need acaseand a cast. -
This leaves the
isa<>cases above. IIUC,isa<>essentially compiles down to a test onE->getStmtClass(). So in terms of performance, replacing theisa<>calls above with a switch statement shouldn't make much of a difference. I think theisa<>version is more readable though.
So in all, I don't see a clear advantage, and I would prefer to keep this as-is.
There was a problem hiding this comment.
I guess the isa calls were what we really jumped out since they amount to N calls and tests on getStmtClass rather than one and a jump. But, readability wins over performance here.
But, I do wonder about readability win from converting to a switch because it will directly express which cases are covered in this function, rather than leaving it implicit in a series of ifs. FWIW.
There was a problem hiding this comment.
I guess the
isacalls were what we really jumped out since they amount to N calls and tests ongetStmtClassrather than one and a jump. But, readability wins over performance here.
I see what you mean -- yes, the switch statement would likely perform better here, but I agree that readability is more important than this micro-optimization.
But, I do wonder about readability win from converting to a switch because it will directly express which cases are covered in this function, rather than leaving it implicit in a series of
ifs. FWIW.
I still have my doubts whether this will really be more readable, but I'll explore this. I'd prefer to do this as a separate PR though, as I'd like to keep this PR focused on the fix I'm making here. (If I convert the function to a switch statement in this PR, those code changes would drown out the few lines of actual behavioral change I'm making.)
There was a problem hiding this comment.
See #88865 for the PR that refactors this into a switch statement -- let's continue discussion there.
|
CI failure looks unrelated. |
…atement. See also discussion in llvm#88726.
…atement. See also discussion in llvm#88726.
…atement. See also discussion in llvm#88726.
The newly added test causes an assertion failure in
PropagateResultObject()without the fix added here.