Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

JOS.attribute("[in_cycle]", " ");
return;
}
auto EraseVisited = llvm::make_scope_exit([&] { Visited.erase(&V); });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


switch (V.getKind()) {
case Value::Kind::Integer:
Expand Down Expand Up @@ -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]", " ");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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(), [&] {
Expand Down