Page MenuHomePhabricator

[analyzer][solver] Handle UnarySymExpr in RangeConstraintSolver
ClosedPublic

Authored by martong on May 11 2022, 8:41 AM.

Diff Detail

Event Timeline

martong created this revision.May 11 2022, 8:41 AM
Herald added a project: Restricted Project. · View Herald Transcript
martong requested review of this revision.May 11 2022, 8:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2022, 8:41 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
martong updated this revision to Diff 428683.May 11 2022, 8:50 AM
  • Add false positive test

Great content!
I've got a long list of nits though. Nothing personal :D

clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
1462–1463

This SymMgr is acquired multiple times. We could hoist it and use it everywhere.

1463
1464–1466

The comment says that it would work with both - and ~.
Why do you check against only -?

1467–1468
clang/test/Analysis/constraint_manager_negate.c
17–21

We can probably spare the unimportant stuff.

24–25

Why don't you use assert(-a > 0) here? It should be equivalent. Same for the rest.

28–30

You can also query if the bound is sharp, by asking the same question but with a slightly different value and expect UNKNOWN.

clang_analyzer_eval(a < 0); // expected-warning{{TRUE}}
clang_analyzer_eval(a < -1); // expected-warning{{UNKNOWN}}

I also believe that a >= INT_MIN is TRUE for all a anyway, thus it can be omitted.
Checking against equality won't help much either, because we had no equality check before, thus it should result in FALSE unconditionally.

47

Let's also assert a == INT_MIN just for the symmetry.

70–74

Sweet!!

77

Well, this is the third form of expressing constraints.

  1. Using asserts.
  2. Use ifs but with early returns.
  3. Use ifs but embed the code into the true branch.

Please, settle on one method.

88–89

Add an equality comparison which results in TRUE.

93–99

Leave a comment here that this is the only value besides zero where the negated is equal to the original value in the unsigned int domain.

101–106

I believe the eval query can be sharper than this.

martong marked 13 inline comments as done.May 12 2022, 6:55 AM

Great content!
I've got a long list of nits though. Nothing personal :D

No worries, thank you for being assiduous.

clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
1464–1466

Unfortunately, the complement operation is not implemented on the RangeSet. So, yes, in this sense the comment is misleading, I've changed it.

clang/test/Analysis/constraint_manager_negate.c
28–30

Okay, I changed it to something very obvious:

clang_analyzer_eval(a < 0); // expected-warning{{TRUE}}
clang_analyzer_eval(a > INT_MIN); // expected-warning{{TRUE}}
47

That would be superfluous in my opinion, since I have changed the if to an

assert(a == INT_MIN);
77

Ok, I changed to assert everywhere.

88–89

Sure.

101–106

Ok.

martong updated this revision to Diff 428932.May 12 2022, 6:55 AM
martong marked 6 inline comments as done.
  • Address steakhal review comments
martong updated this revision to Diff 429208.May 13 2022, 5:24 AM
  • Add a test case for casts related crash
This revision is now accepted and ready to land.May 13 2022, 7:43 AM
This revision was landed with ongoing or failed builds.Thu, May 26, 5:17 AM
This revision was automatically updated to reflect the committed changes.