This proved to be very useful during debugging.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This is definitely useful! Thanks!
I was just wondering if we should add it into the state printer instead. @NoQ what's your take on this?
I suspect that the exploded-graph-rewriter should be updated as well so that the HTML dumps also carry this information.
I've put that into a separate patch because that change has its own complexity. Plus, I think this patch is meaningful and useful by itself (without the exploded-graph-rewriter changes).
https://reviews.llvm.org/D104917
This breaks tests on Windows: http://45.33.8.238/win/40864/step_7.txt
Please take a look, and please revert if it takes a while to fix.
Thanks for the report @thakis ! This seems to be b/c of the different newline handling in windows. I've committed a change that disables the tests on windows.
Thanks. Looks like it flakily fails on mac too every now and then: http://45.33.8.238/mac/33005/step_7.txt
Maybe you print something in nondeterministic iteration order?
Yes, I think that the problem is deeper. ClassMembers is llvm::ImmutableSet (i.e. sorted set), and it uses SymbolRef (e.g. pointer values) for sorting. So, it does look like the order can be nondeterministic.
However, when we print constraints, we should see similar problems, but we don't 🤔
Cool, can we revert this for now then? Flakily failing on my m1 bot too: http://45.33.8.238/macm1/12514/step_7.txt
(here's another flaky failure that was due to this: http://45.33.8.238/mac/33008/step_7.txt )
I have changed the dump methods to order the equivalence classes and the disequality info available before printing them out. The ordering is done based on their string representation. This comes necessarily with some performance penalty, though otherwise it is impossible (or practically way too cumbersome) to write deterministic tests. Also, the dump is used either from the debugger or from small test cases or when we create an exploded graph (which is again intended to be used during debugging), so I think that the penalty induced by the sorting won't be an issue.
- Change the dump methods to order the equivalence classes and the disequality info available before printing them out
clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp | ||
---|---|---|
2716 | Ah, SmallSet is not sorted :( I am gonna have to change this to either std::set or to a sorted vector :( |
AFAICT this patch does not introduce significant overhead to the exploded graph DOT dumps.
Before the patch, an exploded graph took 12G and about 55 secs for a single top-level function.
After the patch, it increased to 13G, and the runtime remained the same as expected.
Which is about a 1.5 % increase in size in total. TBH I expected slightly higher numbers.
This benchmark was done using a release build with dumps, asserts, and expensive checks.
Thanks Balázs for the benchmark, this makes me confident to commit. Hopefully this time the tests won't be flaky.
Ah, SmallSet is not sorted :( I am gonna have to change this to either std::set or to a sorted vector :(