[PATCH] D41938: [Analyzer] SValBuilder Comparison Rearrangement (with Restrictions and Analyzer Option)

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

George's style comments are usually super spot-on. Please feel free to improve my code. Also it was only written as a proof-of-concept because i failed to explain my approach with natural language, so it definitely needs polishing. I'd let you know when i disagree with anything :)



================
Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:472
+  if (BinaryOperator::isComparisonOp(Op) &&
+      Opts.shouldAggressivelySimplifyRelationalComparison()) {
+    if (ResultTy != SVB.getConditionType())
----------------
george.karpenkov wrote:
> I am confused why the option `shouldAggressivelySimplifyRelationalComparison` is checked here. Do we rewrite cases where `Op` is additive even if the option is not checked? It's generally better to check the flag outside of the rearranging function, so that the high-level logic can be seen.
> Do we rewrite cases where `Op` is additive even if the option is not checked?

Yes. Because for additive ops the presumably-expensive clamped-by-max/4 check is not necessary (and is indeed skipped below).

This should be documented though.


================
Comment at: test/Analysis/svalbuilder-rearrange-comparisons.c:19
+  if (x < -(((int)INT_MAX) / 4))
+    exit(1);
+  return x;
----------------
george.karpenkov wrote:
> assert would express intent better
Yeah, but it's also a bit more painful to write in tests (i.e. `__builtin_assert()` and macros).


================
Comment at: test/Analysis/svalbuilder-rearrange-comparisons.c:28
+  clang_analyzer_dump(x == y);
+  // expected-warning at -1{{((conj_$2{int}) - (conj_$5{int})) == 0}}
+}
----------------
george.karpenkov wrote:
> Can we also have integration-like tests where rearrangement would affect observable behavior?
> 
> Also, `clang_analyzer_dump` is a debugging function, and I am not sure whether we should rely on its output in tests.
Yeah, this test would be painful to maintain if we change the dump output. I guess we definitely need more integration tests.

I still wish there were ways to write "this" sort of tests without relying on dumps. Eg.:
```
clang_analyzer_denote(x, "$x");
clang_analyzer_denote(y, "$y");
clang_analyzer_express(x == y); // expected-warning{{($x - $y) == 0}}
```
It should be easy to make an `SValVisitor` that does that.

Given how painful this would be to actually update these numerous tests, i'm fine with leaving a comment explaining the above, as a bit of technical debt - for anyone who would need to update them in the future.

We still need more integration tests though.


================
Comment at: test/Analysis/svalbuilder-rearrange-comparisons.c:100
+  clang_analyzer_dump(x == y);
+  // expected-warning at -1{{1 S32b}}
+}
----------------
This one, for instance, is essentially an integration test. You can easily replace the last dump with clang_analyzer_eval(x == y) and the test would still test the same thing. All tests here that expect a concrete integer in the last check should probably be just converted to clang_analyzer_eval tests.


https://reviews.llvm.org/D41938