[clang] [clang][dataflow] Fix handling of cyclical data structures in HTMLLogger. (PR #66887)
@@ -88,10 +88,12 @@ class ModelDumper {
void dump(Value &V) {
JOS.attribute("value_id", llvm::to_string(&V));
-if (!Visited.insert(&V).second)
- return;
-
JOS.attribute("kind", debugString(V.getKind()));
+if (!Visited.insert(&V).second) {
+ JOS.attribute("[in_cycle]", " ");
+ return;
+}
+auto EraseVisited = llvm::make_scope_exit([&] { Visited.erase(&V); });
sam-mccall wrote:
(Less invasive ideas that might be useful either way: "undefined" is a bug
which can be fixed. Duplicate nodes could/should also be made a different color)
I think showing a subtree twice is a pretty serious misrepresentation of the
data, probably more so than pruning children from a duplicated node. There's no
easy + perfect way to show a DAG in a tree-browser. It'd be possible to make
this more explicit (e.g. have a "duplicate node" box contain a link to an
anchor on the original node). But it's complexity, and if you *don't* care
about the DAG structure then it's still not ideal.
> I assume your concern is that we could have data structures with lots and
> lots of repeated values, and this would bloat the JSON? Do we actually know
> that this is a problem though?
Yes, I believe I saw this. I don't remember the details though, and it might
have involved the old BoolValue subclasses that bloated the tree.
https://github.com/llvm/llvm-project/pull/66887
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Fix handling of cyclical data structures in HTMLLogger. (PR #66887)
https://github.com/sam-mccall approved this pull request. The code looks good for the changes you want to make. Personally I'd keep the existing pruning behavior for all duplicated nodes, and use cosmetic/navigation changes to clarify. But I think you probably know better than me what's useful, and at this point it isn't my call either way :-) https://github.com/llvm/llvm-project/pull/66887 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Fix handling of cyclical data structures in HTMLLogger. (PR #66887)
@@ -88,10 +88,12 @@ class ModelDumper {
void dump(Value &V) {
JOS.attribute("value_id", llvm::to_string(&V));
-if (!Visited.insert(&V).second)
- return;
-
JOS.attribute("kind", debugString(V.getKind()));
+if (!Visited.insert(&V).second) {
+ JOS.attribute("[in_cycle]", " ");
+ return;
+}
+auto EraseVisited = llvm::make_scope_exit([&] { Visited.erase(&V); });
martinboehme wrote:
Sorry, this review has gone very stale because I had other things that grabbed
my attention at the time and then forgot to get back to this PR.
Responding only to this comment first as it's about what we want the behavior
to be (rather than the details of how we implement it).
> But I'm not sure the distinction is worth the code: the idea "we've seen this
> node before, and won't print its details again" applies whether the reason is
> a cycle or just multiple paths to a node, and they both benefit from some
> explicit hint.
Well, part of the motivation of this PR is that I _do_ also want to change this
existing behavior. Here's how repeated values are displayed today:
I see this very case pretty regularly; it was very confusing the first time
(the "undefined" made me think I had a bug), and I still do a double-take when
I see it now.
Even if this was displayed better (e.g. as an `AtomicBool` value with a
"previously dumped" annotation), that still requires me to go looking for the
previous value. In this case, that's easy, because the value is directly above,
but I still need to compare the hex address to be sure.
Why do this work if I can have the computer do it for me?
I assume your concern is that we could have data structures with lots and lots
of repeated values, and this would bloat the JSON? Do we actually know that
this is a problem though?
https://github.com/llvm/llvm-project/pull/66887
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Fix handling of cyclical data structures in HTMLLogger. (PR #66887)
https://github.com/martinboehme created
https://github.com/llvm/llvm-project/pull/66887
* Include more information. We can include the "kind" field for values and
"type" for storage locations without risking infinite recursion. Previously,
we would display these as "undefined" (simply because this is the JavaScript
value for a variable that isn't set), which is potentially confusing.
Similarly, `dump(StorageLocation&)` can unconditionally call `dump(Value&)`
without risking infinite recursion, and this ensures that the "value_id" and
"kind" fields are populated.
* Remove entries from `Visited` when we exit `dump()`. This ensures that we
correctly display values and storage locations that occur multiple times in a
dump (without being nested).
* If we detect a cycle, add an "[in_cycle]" atttribute to make it clear why
the display is truncated.
>From 329a7f9c1b6c0056a83d640c074468fe53bc54bb Mon Sep 17 00:00:00 2001
From: Martin Braenne
Date: Wed, 20 Sep 2023 10:43:39 +
Subject: [PATCH] [clang][dataflow] Fix handling of cyclical data structures in
HTMLLogger.
* Include more information. We can include the "kind" field for values and
"type" for storage locations without risking infinite recursion. Previously,
we would display these as "undefined" (simply because this is the JavaScript
value for a variable that isn't set), which is potentially confusing.
Similarly, `dump(StorageLocation&)` can unconditionally call `dump(Value&)`
without risking infinite recursion, and this ensures that the "value_id" and
"kind" fields are populated.
* Remove entries from `Visited` when we exit `dump()`. This ensures that we
correctly display values and storage locations that occur multiple times in a
dump (without being nested).
* If we detect a cycle, add an "[in_cycle]" atttribute to make it clear why
the display is truncated.
---
clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
index a5f64021eb6ba4b..48a48d86fce43a7 100644
--- a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
+++ b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
@@ -88,10 +88,12 @@ class ModelDumper {
void dump(Value &V) {
JOS.attribute("value_id", llvm::to_string(&V));
-if (!Visited.insert(&V).second)
- return;
-
JOS.attribute("kind", debugString(V.getKind()));
+if (!Visited.insert(&V).second) {
+ JOS.attribute("[in_cycle]", " ");
+ return;
+}
+auto EraseVisited = llvm::make_scope_exit([&] { Visited.erase(&V); });
switch (V.getKind()) {
case Value::Kind::Integer:
@@ -123,13 +125,16 @@ class ModelDumper {
}
void dump(const StorageLocation &L) {
JOS.attribute("location", llvm::to_string(&L));
-if (!Visited.insert(&L).second)
- return;
-
JOS.attribute("type", L.getType().getAsString());
if (auto *V = Env.getValue(L))
dump(*V);
+if (!Visited.insert(&L).second) {
+ JOS.attribute("[in_cycle]", " ");
+ return;
+}
+auto EraseVisited = llvm::make_scope_exit([&] { Visited.erase(&L); });
+
if (auto *RLoc = dyn_cast(&L)) {
for (const auto &Child : RLoc->children())
JOS.attributeObject("f:" + Child.first->getNameAsString(), [&] {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Fix handling of cyclical data structures in HTMLLogger. (PR #66887)
llvmbot wrote:
@llvm/pr-subscribers-clang
Changes
* Include more information. We can include the "kind" field for values and
"type" for storage locations without risking infinite recursion. Previously,
we would display these as "undefined" (simply because this is the JavaScript
value for a variable that isn't set), which is potentially confusing.
Similarly, `dump(StorageLocation&)` can unconditionally call
`dump(Value&)`
without risking infinite recursion, and this ensures that the "value_id" and
"kind" fields are populated.
* Remove entries from `Visited` when we exit `dump()`. This ensures that we
correctly display values and storage locations that occur multiple times in a
dump (without being nested).
* If we detect a cycle, add an "[in_cycle]" atttribute to make it clear why
the display is truncated.
---
Full diff: https://github.com/llvm/llvm-project/pull/66887.diff
1 Files Affected:
- (modified) clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp (+11-6)
``diff
diff --git a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
index a5f64021eb6ba4b..48a48d86fce43a7 100644
--- a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
+++ b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
@@ -88,10 +88,12 @@ class ModelDumper {
void dump(Value &V) {
JOS.attribute("value_id", llvm::to_string(&V));
-if (!Visited.insert(&V).second)
- return;
-
JOS.attribute("kind", debugString(V.getKind()));
+if (!Visited.insert(&V).second) {
+ JOS.attribute("[in_cycle]", " ");
+ return;
+}
+auto EraseVisited = llvm::make_scope_exit([&] { Visited.erase(&V); });
switch (V.getKind()) {
case Value::Kind::Integer:
@@ -123,13 +125,16 @@ class ModelDumper {
}
void dump(const StorageLocation &L) {
JOS.attribute("location", llvm::to_string(&L));
-if (!Visited.insert(&L).second)
- return;
-
JOS.attribute("type", L.getType().getAsString());
if (auto *V = Env.getValue(L))
dump(*V);
+if (!Visited.insert(&L).second) {
+ JOS.attribute("[in_cycle]", " ");
+ return;
+}
+auto EraseVisited = llvm::make_scope_exit([&] { Visited.erase(&L); });
+
if (auto *RLoc = dyn_cast(&L)) {
for (const auto &Child : RLoc->children())
JOS.attributeObject("f:" + Child.first->getNameAsString(), [&] {
``
https://github.com/llvm/llvm-project/pull/66887
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Fix handling of cyclical data structures in HTMLLogger. (PR #66887)
https://github.com/martinboehme edited https://github.com/llvm/llvm-project/pull/66887 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Fix handling of cyclical data structures in HTMLLogger. (PR #66887)
@@ -88,10 +88,12 @@ class ModelDumper {
void dump(Value &V) {
JOS.attribute("value_id", llvm::to_string(&V));
-if (!Visited.insert(&V).second)
- return;
-
JOS.attribute("kind", debugString(V.getKind()));
+if (!Visited.insert(&V).second) {
+ JOS.attribute("[in_cycle]", " ");
+ return;
+}
+auto EraseVisited = llvm::make_scope_exit([&] { Visited.erase(&V); });
sam-mccall wrote:
This breaks the existing use of `Visited`, which is to ensure we don't
recursively dump a value's structure multiple times, even in acyclic cases.
(e.g. a `pair` where the two pointers are the same).
if you want to detect cycles specifically, then we should use a different set
to track the values currently on the stack, with Visited still used to track
everything that we've seen.
But I'm not sure the distinction is worth the code: the idea "we've seen this
node before, and won't print its details again" applies whether the reason is a
cycle or just multiple paths to a node, and they both benefit from some
explicit hint.
So I'd probably rather keep the existing meaning of "Visited" and replacing
"in_cycle" with "already dumped" or so. WDYT?
https://github.com/llvm/llvm-project/pull/66887
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Fix handling of cyclical data structures in HTMLLogger. (PR #66887)
@@ -123,13 +125,16 @@ class ModelDumper {
}
void dump(const StorageLocation &L) {
JOS.attribute("location", llvm::to_string(&L));
-if (!Visited.insert(&L).second)
- return;
-
JOS.attribute("type", L.getType().getAsString());
if (auto *V = Env.getValue(L))
dump(*V);
+if (!Visited.insert(&L).second) {
+ JOS.attribute("[in_cycle]", " ");
sam-mccall wrote:
"[in_cycle]" => " " isn't a clear KV representation for this data!
what about "details" => "pruned, previously dumped", or so?
https://github.com/llvm/llvm-project/pull/66887
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Fix handling of cyclical data structures in HTMLLogger. (PR #66887)
@@ -88,10 +88,12 @@ class ModelDumper {
void dump(Value &V) {
JOS.attribute("value_id", llvm::to_string(&V));
-if (!Visited.insert(&V).second)
- return;
-
JOS.attribute("kind", debugString(V.getKind()));
+if (!Visited.insert(&V).second) {
sam-mccall wrote:
the reordering here looks good, including `kind` is safe & useful
https://github.com/llvm/llvm-project/pull/66887
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
