[clang][dataflow] Fix handling of cyclical data structures in HTMLLogger.

[martinboehme]

• 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.

Cyclical data structure before

Cyclical data structure after

Repeated value before

Note how the value for p:is_null is being displayed as "undefined" because it is the same Value as the previous p:from_nullable:

Repeated value after

p:is_null is now displayed correctly:

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@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 --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<RecordStorageLocation>(&L)) {
       for (const auto &Child : RLoc->children())
         JOS.attributeObject("f:" + Child.first->getNameAsString(), [&] {

[sam-mccall]

the reordering here looks good, including kind is safe & useful

[sam-mccall]

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<Struct*, Struct*> 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?

[martinboehme]

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?

[sam-mccall]

"[in_cycle]" => " " isn't a clear KV representation for this data!

what about "details" => "pruned, previously dumped", or so?