[clang] [clang][dataflow] Remove buggy assertion. (PR #67311)
https://github.com/kinu approved this pull request. Ah- thanks! I just didn't want to loosen the check too much, but looks like this wasn't the only condition. https://github.com/llvm/llvm-project/pull/67311 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Remove RecordValue.getLog() usage in HTMLLogger (PR #65645)
https://github.com/kinu created https://github.com/llvm/llvm-project/pull/65645:
We can dump the same information from RecordStorageLocation.
Tested the behavior before and after patch, that generates the field values in
the HTML
in both cases (and also made sure that removing the relevant code makes the
field values
in the HTML go away)
>From 74c4f7999db0cda33aade39a916a122cea2f11b6 Mon Sep 17 00:00:00 2001
From: Kinuko Yasuda
Date: Thu, 7 Sep 2023 17:22:17 +
Subject: [PATCH] [clang][dataflow] Remove RecordValue.getLog() usage in
HTMLLogger
We can dump the same information from RecordStorageLocation.
Tested the behavior before and after patch, that generates the
field values in the HTML in both cases
(And also made sure if we remove the code it goes away)
---
.../lib/Analysis/FlowSensitive/HTMLLogger.cpp | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
index b1bfe10db202435..a5f64021eb6ba4b 100644
--- a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
+++ b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
@@ -95,6 +95,7 @@ class ModelDumper {
switch (V.getKind()) {
case Value::Kind::Integer:
+case Value::Kind::Record:
case Value::Kind::TopBool:
case Value::Kind::AtomicBool:
case Value::Kind::FormulaBool:
@@ -103,14 +104,6 @@ class ModelDumper {
JOS.attributeObject(
"pointee", [&] { dump(cast(V).getPointeeLoc()); });
break;
-case Value::Kind::Record:
- for (const auto &Child : cast(V).getLoc().children())
-JOS.attributeObject("f:" + Child.first->getNameAsString(), [&] {
- if (Child.second)
-if (Value *Val = Env.getValue(*Child.second))
- dump(*Val);
-});
- break;
}
for (const auto& Prop : V.properties())
@@ -136,6 +129,15 @@ class ModelDumper {
JOS.attribute("type", L.getType().getAsString());
if (auto *V = Env.getValue(L))
dump(*V);
+
+if (auto *RLoc = dyn_cast(&L)) {
+ for (const auto &Child : RLoc->children())
+JOS.attributeObject("f:" + Child.first->getNameAsString(), [&] {
+ if (Child.second)
+if (Value *Val = Env.getValue(*Child.second))
+ dump(*Val);
+});
+}
}
llvm::DenseSet Visited;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Remove RecordValue.getLog() usage in HTMLLogger (PR #65645)
https://github.com/kinu review_requested https://github.com/llvm/llvm-project/pull/65645 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Remove RecordValue.getLog() usage in HTMLLogger (PR #65645)
kinu wrote: @martinboehme @Xazax-hun @ymand if anyone can review this small patch that removes one more dependency on RecordValue.getLoc()... thanks! https://github.com/llvm/llvm-project/pull/65645 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Don't crash when BlockToState is called from unreachable path (PR #65732)
https://github.com/kinu created https://github.com/llvm/llvm-project/pull/65732:
When we call `getEnvironment`, `BlockToState[BlockId]` for the block can return
null even if CFCtx.isBlockReachable(B) returns true if it is called from a
particular block that is marked unreachable to the block.
>From 1f805ef6a1c5697299eab149a9c0552e9195c259 Mon Sep 17 00:00:00 2001
From: Kinuko Yasuda
Date: Fri, 8 Sep 2023 09:03:35 +
Subject: [PATCH] [clang][dataflow] Don't crash when BlockToState doesn't have
unreached block
When we call `getEnvironment`, `BlockToState[BlockId]` for the block can return
null even if CFCtx.isBlockReachable(B) returns true if it is called from a
particular block that is marked unreachable to the block.
(An example is in `EvaluateBlockWithUnreachablePreds` in TransferTest.cpp)
---
clang/lib/Analysis/FlowSensitive/Transfer.cpp | 15 +-
.../Analysis/FlowSensitive/TransferTest.cpp | 20 +++
2 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 67d8be392ae6053..b46c947c691b9b9 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -43,7 +43,20 @@ const Environment *StmtToEnvMap::getEnvironment(const Stmt
&S) const {
if (!CFCtx.isBlockReachable(*BlockIt->getSecond()))
return nullptr;
const auto &State = BlockToState[BlockIt->getSecond()->getBlockID()];
- assert(State);
+ if (!(State)) {
+LLVM_DEBUG({
+ // State can be null when this block is unreachable from the block that
+ // called this method.
+ bool hasUnreachableEdgeFromPred = false;
+ for (auto B : BlockIt->getSecond()->preds())
+if (!B) {
+ hasUnreachableEdgeFromPred = true;
+ break;
+}
+ assert(hasUnreachableEdgeFromPred);
+});
+return nullptr;
+ }
return &State->Env;
}
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 177970ac52a67eb..1fa800044c288d4 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -5853,4 +5853,24 @@ TEST(TransferTest, AnonymousStructWithReferenceField) {
});
}
+TEST(TransferTest, EvaluateBlockWithUnreachablePreds) {
+ // This is a crash repro.
+ // `false` block may not have been processed when we try to evalute the `||`
+ // after visiting `true`, because it is not necessary (and therefore the edge
+ // is marked unreachable). Trying to get the analysis state via
+ // `getEnvironment` for the subexpression should still not crash.
+ std::string Code = R"(
+int cast(int i) {
+ if ((i < 0 && true) || false) {
+return 0;
+ }
+ return 0;
+}
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {});
+}
+
} // namespace
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Don't crash when BlockToState is called from unreachable path (PR #65732)
https://github.com/kinu review_requested https://github.com/llvm/llvm-project/pull/65732 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Don't crash when BlockToState is called from unreachable path (PR #65732)
https://github.com/kinu review_requested https://github.com/llvm/llvm-project/pull/65732 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Don't crash when BlockToState is called from unreachable path (PR #65732)
@@ -43,7 +43,20 @@ const Environment *StmtToEnvMap::getEnvironment(const Stmt
&S) const {
if (!CFCtx.isBlockReachable(*BlockIt->getSecond()))
return nullptr;
const auto &State = BlockToState[BlockIt->getSecond()->getBlockID()];
- assert(State);
+ if (!(State)) {
+LLVM_DEBUG({
+ // State can be null when this block is unreachable from the block that
+ // called this method.
+ bool hasUnreachableEdgeFromPred = false;
+ for (auto B : BlockIt->getSecond()->preds())
+if (!B) {
+ hasUnreachableEdgeFromPred = true;
+ break;
+}
+ assert(hasUnreachableEdgeFromPred);
kinu wrote:
This check can be done more thoroughly (maybe with some pre-processing) if we
want, but can be a bit costly. We won't hit this very often though
https://github.com/llvm/llvm-project/pull/65732
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Don't crash when BlockToState is called from unreachable path (PR #65732)
kinu wrote: CC @martinboehme @Xazax-hun @ymand https://github.com/llvm/llvm-project/pull/65732 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Merge `RecordValue`s with different locations correctly. (PR #65319)
@@ -121,18 +121,18 @@ static Value *mergeDistinctValues(QualType Type, Value
&Val1,
Value *MergedVal = nullptr;
if (auto *RecordVal1 = dyn_cast(&Val1)) {
-[[maybe_unused]] auto *RecordVal2 = cast(&Val2);
-
-// Values to be merged are always associated with the same location in
-// `LocToVal`. The location stored in `RecordVal` should therefore also
-// be the same.
-assert(&RecordVal1->getLoc() == &RecordVal2->getLoc());
-
-// `RecordVal1` and `RecordVal2` may have different properties associated
-// with them. Create a new `RecordValue` without any properties so that we
-// soundly approximate both values. If a particular analysis needs to merge
-// properties, it should do so in `DataflowAnalysis::merge()`.
-MergedVal = &MergedEnv.create(RecordVal1->getLoc());
+auto *RecordVal2 = cast(&Val2);
+
+if (&RecordVal1->getLoc() == &RecordVal2->getLoc())
+ // `RecordVal1` and `RecordVal2` may have different properties associated
+ // with them. Create a new `RecordValue` without any properties so that
we
+ // soundly approximate both values. If a particular analysis needs to
+ // merge properties, it should do so in `DataflowAnalysis::merge()`.
+ MergedVal = &MergedEnv.create(RecordVal1->getLoc());
+else
+ // If the locations for the two records are different, need to create a
+ // completely new value.
kinu wrote:
I think you're right, I wanted to see if some comment may help but it's a bit
irrelevant here. Also given that it's going away I'm good with it, thanks!
https://github.com/llvm/llvm-project/pull/65319
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Merge `RecordValue`s with different locations correctly. (PR #65319)
@@ -121,18 +121,18 @@ static Value *mergeDistinctValues(QualType Type, Value
&Val1,
Value *MergedVal = nullptr;
if (auto *RecordVal1 = dyn_cast(&Val1)) {
-[[maybe_unused]] auto *RecordVal2 = cast(&Val2);
-
-// Values to be merged are always associated with the same location in
-// `LocToVal`. The location stored in `RecordVal` should therefore also
-// be the same.
-assert(&RecordVal1->getLoc() == &RecordVal2->getLoc());
-
-// `RecordVal1` and `RecordVal2` may have different properties associated
-// with them. Create a new `RecordValue` without any properties so that we
-// soundly approximate both values. If a particular analysis needs to merge
-// properties, it should do so in `DataflowAnalysis::merge()`.
-MergedVal = &MergedEnv.create(RecordVal1->getLoc());
+auto *RecordVal2 = cast(&Val2);
+
+if (&RecordVal1->getLoc() == &RecordVal2->getLoc())
+ // `RecordVal1` and `RecordVal2` may have different properties associated
+ // with them. Create a new `RecordValue` without any properties so that
we
+ // soundly approximate both values. If a particular analysis needs to
+ // merge properties, it should do so in `DataflowAnalysis::merge()`.
kinu wrote:
SGTM. Yeah adding 'with the same location, ...' may clarify the situation bit,
either works for me.
https://github.com/llvm/llvm-project/pull/65319
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Don't crash when BlockToState is called from unreachable path (PR #65732)
https://github.com/kinu updated https://github.com/llvm/llvm-project/pull/65732:
>From bc119f4bb478431bf85cda47dbc2a25faa59e85f Mon Sep 17 00:00:00 2001
From: Kinuko Yasuda
Date: Fri, 8 Sep 2023 09:03:35 +
Subject: [PATCH 1/2] [clang][dataflow] Don't crash when BlockToState doesn't
have unreached block
When we call `getEnvironment`, `BlockToState[BlockId]` for the block can return
null even if CFCtx.isBlockReachable(B) returns true if it is called from a
particular block that is marked unreachable to the block.
(An example is in `EvaluateBlockWithUnreachablePreds` in TransferTest.cpp)
---
clang/lib/Analysis/FlowSensitive/Transfer.cpp | 15 +-
.../Analysis/FlowSensitive/TransferTest.cpp | 20 +++
2 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 67d8be392ae6053..b46c947c691b9b9 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -43,7 +43,20 @@ const Environment *StmtToEnvMap::getEnvironment(const Stmt
&S) const {
if (!CFCtx.isBlockReachable(*BlockIt->getSecond()))
return nullptr;
const auto &State = BlockToState[BlockIt->getSecond()->getBlockID()];
- assert(State);
+ if (!(State)) {
+LLVM_DEBUG({
+ // State can be null when this block is unreachable from the block that
+ // called this method.
+ bool hasUnreachableEdgeFromPred = false;
+ for (auto B : BlockIt->getSecond()->preds())
+if (!B) {
+ hasUnreachableEdgeFromPred = true;
+ break;
+}
+ assert(hasUnreachableEdgeFromPred);
+});
+return nullptr;
+ }
return &State->Env;
}
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 177970ac52a67eb..1fa800044c288d4 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -5853,4 +5853,24 @@ TEST(TransferTest, AnonymousStructWithReferenceField) {
});
}
+TEST(TransferTest, EvaluateBlockWithUnreachablePreds) {
+ // This is a crash repro.
+ // `false` block may not have been processed when we try to evalute the `||`
+ // after visiting `true`, because it is not necessary (and therefore the edge
+ // is marked unreachable). Trying to get the analysis state via
+ // `getEnvironment` for the subexpression should still not crash.
+ std::string Code = R"(
+int cast(int i) {
+ if ((i < 0 && true) || false) {
+return 0;
+ }
+ return 0;
+}
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {});
+}
+
} // namespace
>From 144cdf860b19f867e8c61c9478713adbe719967b Mon Sep 17 00:00:00 2001
From: Kinuko Yasuda
Date: Fri, 8 Sep 2023 13:11:49 +
Subject: [PATCH 2/2] comment change to see if it triggers build
---
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 1fa800044c288d4..ec07555d7f33b3b 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -5858,7 +5858,7 @@ TEST(TransferTest, EvaluateBlockWithUnreachablePreds) {
// `false` block may not have been processed when we try to evalute the `||`
// after visiting `true`, because it is not necessary (and therefore the edge
// is marked unreachable). Trying to get the analysis state via
- // `getEnvironment` for the subexpression should still not crash.
+ // `getEnvironment` for the subexpression still should not crash.
std::string Code = R"(
int cast(int i) {
if ((i < 0 && true) || false) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Remove unused function: transferBlock() (PR #65932)
https://github.com/kinu created https://github.com/llvm/llvm-project/pull/65932:
No one seems to be using transferBlock() in TypeErasedDataflowAnalysis, it is
likely a remnant of old code
>From dc808e4edccdd8d405660729713dd169c87daf71 Mon Sep 17 00:00:00 2001
From: Kinuko Yasuda
Date: Fri, 8 Sep 2023 21:47:25 +
Subject: [PATCH] [clang][dataflow] Remove unused function: transferBlock()
No one seems to be using transferBlock() in TypeErasedDataflowAnalysis, it is
likely a remnant of old code
---
.../TypeErasedDataflowAnalysis.h | 19 ---
.../TypeErasedDataflowAnalysis.cpp| 12
2 files changed, 31 deletions(-)
diff --git
a/clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
b/clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
index 88a33d19f7d8f61..67c323dbf45e1b2 100644
--- a/clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
+++ b/clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
@@ -132,25 +132,6 @@ struct TypeErasedDataflowAnalysisState {
}
};
-/// Transfers the state of a basic block by evaluating each of its elements in
-/// the context of `Analysis` and the states of its predecessors that are
-/// available in `BlockStates`. `PostVisitCFG` (if provided) will be applied to
-/// each element in the block, after it is evaluated.
-///
-/// Requirements:
-///
-/// All predecessors of `Block` except those with loop back edges must have
-/// already been transferred. States in `BlockStates` that are set to
-/// `std::nullopt` represent basic blocks that are not evaluated yet.
-TypeErasedDataflowAnalysisState transferBlock(
-const ControlFlowContext &CFCtx,
-llvm::ArrayRef> BlockStates,
-const CFGBlock &Block, const Environment &InitEnv,
-TypeErasedDataflowAnalysis &Analysis,
-std::function
-PostVisitCFG = nullptr);
-
/// Performs dataflow analysis and returns a mapping from basic block IDs to
/// dataflow analysis states that model the respective basic blocks. Indices of
/// the returned vector correspond to basic block IDs. Returns an error if the
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 626b57b43755ec7..900aa97e9f6de64 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -495,18 +495,6 @@ transferCFGBlock(const CFGBlock &Block, AnalysisContext
&AC,
return State;
}
-TypeErasedDataflowAnalysisState transferBlock(
-const ControlFlowContext &CFCtx,
-llvm::ArrayRef> BlockStates,
-const CFGBlock &Block, const Environment &InitEnv,
-TypeErasedDataflowAnalysis &Analysis,
-std::function
-PostVisitCFG) {
- AnalysisContext AC(CFCtx, Analysis, InitEnv, BlockStates);
- return transferCFGBlock(Block, AC, PostVisitCFG);
-}
-
llvm::Expected>>
runTypeErasedDataflowAnalysis(
const ControlFlowContext &CFCtx, TypeErasedDataflowAnalysis &Analysis,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Remove unused function: transferBlock() (PR #65932)
https://github.com/kinu review_requested https://github.com/llvm/llvm-project/pull/65932 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Remove RecordValue.getLog() usage in HTMLLogger (PR #65645)
kinu wrote: Here's the screenshot for a simple code snippet  'Without the patch' image is almost identical You can see some more screenshots here: https://docs.google.com/document/d/1gJqU5g7HCUifDUJBb5NzSBC0K-9W4nyZ3zxtpbTUDdk/view https://github.com/llvm/llvm-project/pull/65645 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Remove unused function: transferBlock() (PR #65932)
https://github.com/kinu closed https://github.com/llvm/llvm-project/pull/65932 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Remove unused function: transferBlock() (PR #65932)
kinu wrote: Thanks, closing https://github.com/llvm/llvm-project/pull/65932 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Ignore assignment where base class's operator is used (PR #66364)
https://github.com/kinu created https://github.com/llvm/llvm-project/pull/66364:
In C++ it seems it is legit to use base class's operator (e.g. `using
Base::operator=`) to perform copy if the base class is the common ancestor of
the source and destination object. In such a case we shouldn't try to access
fields beyond that of the base class, however such a case seems to be very rare
(typical code would implement a copy constructor instead), and could add
complexities, so in this patch we simply bail if the method operator's parent
class is different from the type of the destination object that this framework
recognizes.
>From 0dd55ed61a14b01ea0c62c2a4ca2c50317587cb3 Mon Sep 17 00:00:00 2001
From: Kinuko Yasuda
Date: Thu, 14 Sep 2023 12:20:27 +
Subject: [PATCH] [clang][dataflow] Ignore weird assignment case where base
class's operator is used
In C++ it seems it is legit to use base class's operator (e.g. `using
Base::operator=`) to perform copy if the base class is the common
ancestor of the source and destination object. In such a case we
shouldn't try to access fields beyond that of the base class, however
such a case seems to be very rare (typical code would implement a copy
constructor instead), and could add complexities, so in this patch we
simply bail if the method operator's parent class is different from the
type of the destination object that this framework recognizes.
---
clang/lib/Analysis/FlowSensitive/Transfer.cpp | 7 ++
.../Analysis/FlowSensitive/TransferTest.cpp | 24 +++
2 files changed, 31 insertions(+)
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index b46c947c691b9b9..b510114a7a355eb 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -531,6 +531,13 @@ class TransferVisitor : public
ConstStmtVisitor {
auto *LocDst =
cast_or_null(Env.getStorageLocation(*Arg0));
+ // The assignment operators are different from the type of the
destination
+ // in this model (i.e. in one of their base classes). This must be very
rare
+ // and we just bail.
+ if (Method->getThisObjectType().getCanonicalType().getUnqualifiedType()
!=
+ LocDst->getType().getCanonicalType().getUnqualifiedType())
+return;
+
if (LocSrc != nullptr && LocDst != nullptr) {
copyRecord(*LocSrc, *LocDst, Env);
Env.setStorageLocation(*S, *LocDst);
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 0abd171f1d0b7cb..e0e3b71503d2176 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -2124,6 +2124,30 @@ TEST(TransferTest, AssignmentOperator) {
});
}
+TEST(TransferTest, AssignmentOperatorFromBase) {
+ // This is a crash repro. We don't model the copy this case, so no
+ // expectations on the copied field of the base class are checked.
+ std::string Code = R"(
+struct Base {
+ int base;
+};
+struct Derived : public Base {
+ using Base::operator=;
+ int derived;
+};
+void target(Base B, Derived D) {
+ D.base = 1;
+ D.derived = 1;
+ D = B;
+ // [[p]]
+}
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {});
+}
+
TEST(TransferTest, AssignmentOperatorFromCallResult) {
std::string Code = R"(
struct A {};
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Ignore assignment where base class's operator is used (PR #66364)
kinu wrote: @martinboehme @sam-mccall @ymand appreciated if any of you (or other owners) can review https://github.com/llvm/llvm-project/pull/66364 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Model the fields that are accessed via inline accessors (PR #66368)
https://github.com/kinu created https://github.com/llvm/llvm-project/pull/66368:
So that the values that are accessed via such accessors can be analyzed as a
limited version of context-sensitive analysis. We can potentially do this only
when some option is set, but doing additional modeling like this won't be
expensive and intrusive, so we do it by default for now.
>From d3f686a9f342d3cf38ed0cf742fd0377bb998891 Mon Sep 17 00:00:00 2001
From: Kinuko Yasuda
Date: Thu, 14 Sep 2023 12:45:04 +
Subject: [PATCH] [clang][dataflow] Model the fields that are accessed via
inline accessors
So that the values that are accessed via such accessors can be analyzed
as a limited version of context-sensitive analysis.
We can potentially do this only when some option is set, but doing
additional modeling like this won't be expensive and intrusive, so
we do it by default for now.
---
.../FlowSensitive/DataflowEnvironment.cpp | 18
.../Analysis/FlowSensitive/TransferTest.cpp | 44 +++
2 files changed, 62 insertions(+)
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index e13f880896fc071..713df62e5009336 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -288,6 +288,18 @@ static void insertIfFunction(const Decl &D,
Funcs.insert(FD);
}
+static Expr *getRetValueFromSingleReturnStmtMethod(const CXXMemberCallExpr &C)
{
+ auto *D = cast_or_null(C.getMethodDecl()->getDefinition());
+ if (!D)
+return nullptr;
+ auto *S = cast(D->getBody());
+ if (S->size() != 1)
+return nullptr;
+ if (auto *RS = dyn_cast(*S->body_begin()))
+return RS->getRetValue()->IgnoreParenImpCasts();
+ return nullptr;
+}
+
static void
getFieldsGlobalsAndFuncs(const Decl &D, FieldSet &Fields,
llvm::DenseSet &Vars,
@@ -324,6 +336,12 @@ getFieldsGlobalsAndFuncs(const Stmt &S, FieldSet &Fields,
} else if (auto *E = dyn_cast(&S)) {
insertIfGlobal(*E->getDecl(), Vars);
insertIfFunction(*E->getDecl(), Funcs);
+ } else if (const auto *C = dyn_cast(&S)) {
+// If this is a method that returns a member variable but does nothing
else,
+// model the field of the return value.
+if (MemberExpr *E = dyn_cast_or_null(
+getRetValueFromSingleReturnStmtMethod(*C)))
+ getFieldsGlobalsAndFuncs(*E, Fields, Vars, Funcs);
} else if (auto *E = dyn_cast(&S)) {
// FIXME: should we be using `E->getFoundDecl()`?
const ValueDecl *VD = E->getMemberDecl();
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 0abd171f1d0b7cb..d52433b14da142a 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -1446,6 +1446,50 @@ TEST(TransferTest, BaseClassInitializer) {
llvm::Succeeded());
}
+TEST(TransferTest, StructModeledFieldsWithConstAccessor) {
+ std::string Code = R"(
+class S {
+ int *P;
+ int *Q;
+ int X;
+ int Y;
+ int Z;
+public:
+ int *getPtr() const { return P; }
+ int *getPtrNonConst() { return Q; }
+ int getInt() const { return X; }
+ int getInt(int i) const { return Y; }
+ int getIntNonConst() { return Z; }
+ int getIntNoDefinition() const;
+};
+
+void target() {
+ S s;
+ int *p = s.getPtr();
+ int *q = s.getPtrNonConst();
+ int x = s.getInt();
+ int y = s.getIntNonConst();
+ int z = s.getIntNoDefinition();
+ // [[p]]
+}
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env =
+ getEnvironmentAtAnnotation(Results, "p");
+auto &SLoc = getLocForDecl(ASTCtx, Env, "s");
+std::vector Fields;
+for (auto [Field, _] : SLoc.children())
+ Fields.push_back(Field);
+// Only the fields that have corresponding const accessor methods
+// should be modeled.
+ASSERT_THAT(Fields, UnorderedElementsAre(
+findValueDecl(ASTCtx, "P"), findValueDecl(ASTCtx, "X")));
+ });
+}
+
TEST(TransferTest, StructModeledFieldsWithComplicatedInheritance) {
std::string Code = R"(
struct Base1 {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Model the fields that are accessed via inline accessors (PR #66368)
kinu wrote: /cc @martinboehme @ymand https://github.com/llvm/llvm-project/pull/66368 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Model the fields that are accessed via inline accessors (PR #66368)
https://github.com/kinu updated https://github.com/llvm/llvm-project/pull/66368
>From d311f12fe3ca0d30a40e659236ba7eaccda24a8b Mon Sep 17 00:00:00 2001
From: Kinuko Yasuda
Date: Thu, 14 Sep 2023 12:45:04 +
Subject: [PATCH 1/4] [clang][dataflow] Model the fields that are accessed via
inline accessors
So that the values that are accessed via such accessors can be analyzed
as a limited version of context-sensitive analysis.
We can potentially do this only when some option is set, but doing
additional modeling like this won't be expensive and intrusive, so
we do it by default for now.
---
.../FlowSensitive/DataflowEnvironment.cpp | 18
.../Analysis/FlowSensitive/TransferTest.cpp | 44 +++
2 files changed, 62 insertions(+)
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index e13f880896fc071..713df62e5009336 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -288,6 +288,18 @@ static void insertIfFunction(const Decl &D,
Funcs.insert(FD);
}
+static Expr *getRetValueFromSingleReturnStmtMethod(const CXXMemberCallExpr &C)
{
+ auto *D = cast_or_null(C.getMethodDecl()->getDefinition());
+ if (!D)
+return nullptr;
+ auto *S = cast(D->getBody());
+ if (S->size() != 1)
+return nullptr;
+ if (auto *RS = dyn_cast(*S->body_begin()))
+return RS->getRetValue()->IgnoreParenImpCasts();
+ return nullptr;
+}
+
static void
getFieldsGlobalsAndFuncs(const Decl &D, FieldSet &Fields,
llvm::DenseSet &Vars,
@@ -324,6 +336,12 @@ getFieldsGlobalsAndFuncs(const Stmt &S, FieldSet &Fields,
} else if (auto *E = dyn_cast(&S)) {
insertIfGlobal(*E->getDecl(), Vars);
insertIfFunction(*E->getDecl(), Funcs);
+ } else if (const auto *C = dyn_cast(&S)) {
+// If this is a method that returns a member variable but does nothing
else,
+// model the field of the return value.
+if (MemberExpr *E = dyn_cast_or_null(
+getRetValueFromSingleReturnStmtMethod(*C)))
+ getFieldsGlobalsAndFuncs(*E, Fields, Vars, Funcs);
} else if (auto *E = dyn_cast(&S)) {
// FIXME: should we be using `E->getFoundDecl()`?
const ValueDecl *VD = E->getMemberDecl();
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index cdb1bc3cd16ac7b..355d18bc1fe55a6 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -1446,6 +1446,50 @@ TEST(TransferTest, BaseClassInitializer) {
llvm::Succeeded());
}
+TEST(TransferTest, StructModeledFieldsWithConstAccessor) {
+ std::string Code = R"(
+class S {
+ int *P;
+ int *Q;
+ int X;
+ int Y;
+ int Z;
+public:
+ int *getPtr() const { return P; }
+ int *getPtrNonConst() { return Q; }
+ int getInt() const { return X; }
+ int getInt(int i) const { return Y; }
+ int getIntNonConst() { return Z; }
+ int getIntNoDefinition() const;
+};
+
+void target() {
+ S s;
+ int *p = s.getPtr();
+ int *q = s.getPtrNonConst();
+ int x = s.getInt();
+ int y = s.getIntNonConst();
+ int z = s.getIntNoDefinition();
+ // [[p]]
+}
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env =
+ getEnvironmentAtAnnotation(Results, "p");
+auto &SLoc = getLocForDecl(ASTCtx, Env, "s");
+std::vector Fields;
+for (auto [Field, _] : SLoc.children())
+ Fields.push_back(Field);
+// Only the fields that have corresponding const accessor methods
+// should be modeled.
+ASSERT_THAT(Fields, UnorderedElementsAre(
+findValueDecl(ASTCtx, "P"), findValueDecl(ASTCtx, "X")));
+ });
+}
+
TEST(TransferTest, StructModeledFieldsWithComplicatedInheritance) {
std::string Code = R"(
struct Base1 {
>From ec7e051b7b0901b5776fe0c0fe751b3a1d2f995d Mon Sep 17 00:00:00 2001
From: Kinuko Yasuda
Date: Thu, 14 Sep 2023 15:09:29 +
Subject: [PATCH 2/4] Update the broken test
---
.../Analysis/FlowSensitive/TransferTest.cpp | 25 +++
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 355d18bc1fe55a6..bf3d002567cc06a 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -1446,7 +1446,7 @@ TEST(TransferTest, BaseClassInitializer) {
llvm::Succeeded());
}
-TEST(TransferTest, StructModeledFieldsWithConstAccessor) {
+TEST(TransferTest, StructModeledFieldsWithAccessor) {
std::string Code = R"(
class S {
int
[clang] [clang][dataflow] Model the fields that are accessed via inline accessors (PR #66368)
@@ -1446,6 +1446,51 @@ TEST(TransferTest, BaseClassInitializer) {
llvm::Succeeded());
}
+TEST(TransferTest, StructModeledFieldsWithAccessor) {
+ std::string Code = R"(
+class S {
+ int *P;
+ int *Q;
+ int X;
+ int Y;
+ int Z;
+public:
+ int *getPtr() const { return P; }
+ int *getPtrNonConst() { return Q; }
+ int getInt(int i) const { return X; }
kinu wrote:
Done.
https://github.com/llvm/llvm-project/pull/66368
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Model the fields that are accessed via inline accessors (PR #66368)
https://github.com/kinu resolved https://github.com/llvm/llvm-project/pull/66368 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Model the fields that are accessed via inline accessors (PR #66368)
https://github.com/kinu resolved https://github.com/llvm/llvm-project/pull/66368 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Model the fields that are accessed via inline accessors (PR #66368)
@@ -324,6 +336,12 @@ getFieldsGlobalsAndFuncs(const Stmt &S, FieldSet &Fields,
} else if (auto *E = dyn_cast(&S)) {
insertIfGlobal(*E->getDecl(), Vars);
insertIfFunction(*E->getDecl(), Funcs);
+ } else if (const auto *C = dyn_cast(&S)) {
+// If this is a method that returns a member variable but does nothing
else,
+// model the field of the return value.
+if (MemberExpr *E = dyn_cast_or_null(
+getRetValueFromSingleReturnStmtMethod(*C)))
+ getFieldsGlobalsAndFuncs(*E, Fields, Vars, Funcs);
kinu wrote:
Totally, done.
https://github.com/llvm/llvm-project/pull/66368
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Model the fields that are accessed via inline accessors (PR #66368)
https://github.com/kinu resolved https://github.com/llvm/llvm-project/pull/66368 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Model the fields that are accessed via inline accessors (PR #66368)
https://github.com/kinu resolved https://github.com/llvm/llvm-project/pull/66368 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Model the fields that are accessed via inline accessors (PR #66368)
@@ -288,6 +288,18 @@ static void insertIfFunction(const Decl &D,
Funcs.insert(FD);
}
+static Expr *getRetValueFromSingleReturnStmtMethod(const CXXMemberCallExpr &C)
{
+ auto *D = cast_or_null(C.getMethodDecl()->getDefinition());
+ if (!D)
+return nullptr;
+ auto *S = cast(D->getBody());
+ if (S->size() != 1)
+return nullptr;
+ if (auto *RS = dyn_cast(*S->body_begin()))
+return RS->getRetValue()->IgnoreParenImpCasts();
+ return nullptr;
kinu wrote:
Applied with slight modifications
https://github.com/llvm/llvm-project/pull/66368
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Model the fields that are accessed via inline accessors (PR #66368)
https://github.com/kinu resolved https://github.com/llvm/llvm-project/pull/66368 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Model the fields that are accessed via inline accessors (PR #66368)
kinu wrote: Thanks! Addressed comments. https://github.com/llvm/llvm-project/pull/66368 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Merge `RecordValue`s with different locations correctly. (PR #65319)
https://github.com/kinu edited https://github.com/llvm/llvm-project/pull/65319 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Merge `RecordValue`s with different locations correctly. (PR #65319)
https://github.com/kinu commented: Thanks, this LGTM (confirmed that it makes it go through with one of the real code I hit this) https://github.com/llvm/llvm-project/pull/65319 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Merge `RecordValue`s with different locations correctly. (PR #65319)
@@ -121,18 +121,18 @@ static Value *mergeDistinctValues(QualType Type, Value
&Val1,
Value *MergedVal = nullptr;
if (auto *RecordVal1 = dyn_cast(&Val1)) {
-[[maybe_unused]] auto *RecordVal2 = cast(&Val2);
-
-// Values to be merged are always associated with the same location in
-// `LocToVal`. The location stored in `RecordVal` should therefore also
-// be the same.
-assert(&RecordVal1->getLoc() == &RecordVal2->getLoc());
-
-// `RecordVal1` and `RecordVal2` may have different properties associated
-// with them. Create a new `RecordValue` without any properties so that we
-// soundly approximate both values. If a particular analysis needs to merge
-// properties, it should do so in `DataflowAnalysis::merge()`.
-MergedVal = &MergedEnv.create(RecordVal1->getLoc());
+auto *RecordVal2 = cast(&Val2);
+
+if (&RecordVal1->getLoc() == &RecordVal2->getLoc())
+ // `RecordVal1` and `RecordVal2` may have different properties associated
+ // with them. Create a new `RecordValue` without any properties so that
we
+ // soundly approximate both values. If a particular analysis needs to
+ // merge properties, it should do so in `DataflowAnalysis::merge()`.
kinu wrote:
'If a particular analysis needs to merge...'
this part can be out of the if / else because the model's merge may do
something for the else case too?
https://github.com/llvm/llvm-project/pull/65319
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Merge `RecordValue`s with different locations correctly. (PR #65319)
@@ -121,18 +121,18 @@ static Value *mergeDistinctValues(QualType Type, Value
&Val1,
Value *MergedVal = nullptr;
if (auto *RecordVal1 = dyn_cast(&Val1)) {
-[[maybe_unused]] auto *RecordVal2 = cast(&Val2);
-
-// Values to be merged are always associated with the same location in
-// `LocToVal`. The location stored in `RecordVal` should therefore also
-// be the same.
-assert(&RecordVal1->getLoc() == &RecordVal2->getLoc());
-
-// `RecordVal1` and `RecordVal2` may have different properties associated
-// with them. Create a new `RecordValue` without any properties so that we
-// soundly approximate both values. If a particular analysis needs to merge
-// properties, it should do so in `DataflowAnalysis::merge()`.
-MergedVal = &MergedEnv.create(RecordVal1->getLoc());
+auto *RecordVal2 = cast(&Val2);
+
+if (&RecordVal1->getLoc() == &RecordVal2->getLoc())
+ // `RecordVal1` and `RecordVal2` may have different properties associated
+ // with them. Create a new `RecordValue` without any properties so that
we
+ // soundly approximate both values. If a particular analysis needs to
+ // merge properties, it should do so in `DataflowAnalysis::merge()`.
+ MergedVal = &MergedEnv.create(RecordVal1->getLoc());
+else
+ // If the locations for the two records are different, need to create a
+ // completely new value.
kinu wrote:
Given that how cryptic when this could happen, it might be helpful to have a
brief comment that both could happen depending on a subtle order of CFG and
therefore how merge happens.
https://github.com/llvm/llvm-project/pull/65319
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Merge `RecordValue`s with different locations correctly. (PR #65319)
https://github.com/kinu edited https://github.com/llvm/llvm-project/pull/65319 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Eliminate `RecordValue::getChild()`. (PR #65586)
https://github.com/kinu commented: Nice! Actual changes are tests only, LGTM (though I'm not a reviewer) https://github.com/llvm/llvm-project/pull/65586 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [WIP][clang]: Implement a conditional lifetimebound_if builtin. (PR #125520)
kinu wrote: Would the alternative that is discussed here mean we want to forward some attributes only when they are applicable, something like `clang::forwad_lifetimebound`? Regardless, I think it'd be also good to agree on the use cases we want to support in a concrete code snippet (that should also answer to @Xazax-hun 's question). There could also be a question of whether we really should try to support variadic templates in the current lifetime-bound too, because we're aware that the current semantic itself has a limitation. https://github.com/llvm/llvm-project/pull/125520 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
