[clang] [clang][dataflow] Fix handling of cyclical data structures in HTMLLogger. (PR #66887)

via cfe-commits cfe-commits at lists.llvm.org
Thu May 23 05:12:29 PDT 2024


================
@@ -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:

![repeated_value_before](https://github.com/llvm/llvm-project/assets/29098113/b8af43ac-1a14-4b6f-aa2a-c9e88834d5c6)

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?

![repeated_value_after](https://github.com/llvm/llvm-project/assets/29098113/1fd5f753-2a2e-4442-b6e0-f3f40d4a5b5e)

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


More information about the cfe-commits mailing list