[PATCH] D110357: [Analyzer] Extend ConstraintAssignor to handle remainder op

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1603-1604
 
+  template <typename SymT>
+  bool handleRem(const SymT *Sym, RangeSet Constraint) {
+    // a % b != 0 implies that a != 0.
----------------
ASDenysPetrov wrote:
> steakhal wrote:
> > Why is this not a `const` member function?
> IMO it's better to rename the function `handleRemainderOp`.
> 
> Add a function description in comments above. 
> E.g. Handle expressions like: `a % b == 0`. ... Returns `true` when //bla-bla//, otherwise returns `false`.
> Why is this not a const member function?
Because `RCM.assumeSymNE` is not `const`.

> IMO it's better to rename the function handleRemainderOp.
I agree, changed it.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1605
+  bool handleRem(const SymT *Sym, RangeSet Constraint) {
+    // a % b != 0 implies that a != 0.
+    if (Sym->getOpcode() != BO_Rem)
----------------
steakhal wrote:
> I think you should also implement `a % b == 0 implies that a == 0`.
Good point, I've just added that.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1609-1612
+      const SymExpr *LHS = Sym->getLHS();
+      const llvm::APSInt &Zero =
+          Builder.getBasicValueFactory().getValue(0, LHS->getType());
+      State = RCM->assumeSymNE(State, LHS, Zero, Zero);
----------------
steakhal wrote:
> ASDenysPetrov wrote:
> > Maybe make some more complex assumptions to cover complex **LHS's**?
> Oh nice.
`State->assume` goes through many higher level abstractions and finally calls `assumeSymNE`, so I think calling that would be a pessimization in this case.


================
Comment at: clang/test/Analysis/constraint-assignor.c:30
+  (void)x; // keep the constraints alive.
+}
----------------
ASDenysPetrov wrote:
> Add some nested cases like `x % y % z == 0`.
Good point! I've added one such test case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110357/new/

https://reviews.llvm.org/D110357