[PATCH] D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

Thanks for creating this commit, it's a useful improvement!

I added some inline comments on minor implementation details; moreover, note that "Releted checkers:" (instead of "related") is a typo in the commit message.

I also have a less concrete question about the root cause of these bugs. Does this commit fix the "root" of the issue by eliminating some misuse of correctly implemented (but perhaps surprising) `SVal` / `APSInt` arithmetic; or is there an underlying bug in the `SVal` / `APSInt` arithmetic which is just avoided (and not eliminated) by this commit? In the latter case, what obstacles prevent us from fixing the root cause?



================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:202-203
   // CHECK UPPER BOUND
-  DefinedOrUnknownSVal Size = getDynamicExtent(state, Reg, svalBuilder);
+  SVal Size = svalBuilder.convertToArrayIndex(
+      getDynamicExtent(state, Reg, svalBuilder));
   if (auto KnownSize = Size.getAs<NonLoc>()) {
----------------
I wonder whether it would be better to move this conversion into the definition of `getDynamicExtent` to ensure that it has a consistent return type. Are there any situations where it's useful that `getDynamicExtent` can return something that's not an `ArrayIndexTy`?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:180-183
+      SVal CountReached =
+          SVB.evalBinOp(State, BO_GE, Idx, Count, ASTCtx.BoolTy);
+      if (!CountReached.isUndef() &&
+          State->assume(*CountReached.getAs<DefinedOrUnknownSVal>(), true))
----------------
I think checking the nullness of `getAs()` is more elegant than using a separate `isUndef()` check.

On a longer term / as a separate improvement, I'd also think about allowing `UndefinedVal` as the argument of the `assert()`-like functions, because the `evalBinOp` -> `assert` combination is very common in checkers and IIRC in most checkers the branch of `UndefinedVal` will produce the same result as `UnknownVal`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:186-192
       const ElementRegion *const ER = RegionManager.getElementRegion(
-          CE.getArgExpr(1)->getType()->getPointeeType(), Idx, SuperRegion,
-          Ctx.getASTContext());
+          ElemType,
+          SVB.evalBinOp(State, BO_Add, Idx, MROffset, SVB.getArrayIndexType())
+              .castAs<NonLoc>(),
+          SuperRegion, Ctx.getASTContext());
 
       ReqRegions.push_back(ER->getAs<MemRegion>());
----------------
I'd use `getAs<NonLoc>()` and a conditional to avoid crashes in the (theoretical) case that `evalBinOp` returns `UnknownVal`; and I suspect that `getAs<MemRegion>()` is superfluous because `MemRegion` is a base class of `ElementRegion`.


================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicExtent.cpp:92
+
+  return ElementCount.castAs<DefinedOrUnknownSVal>();
+}
----------------
Are you sure that this cannot cause crashes? (E.g. did you check that there is no corner case when `getElementExtent` returns 0?)

I can accept this cast, especially if you have a clear proof that it's valid, but I'd prefer a more defensive solution that turns `UndefinedVal` into `UnknownVal` either here or preferably in the `assert()` function family that consumes the results from functions like this. 


================
Comment at: clang/test/Analysis/array-bound-v2-constraint-check.c:1
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.security.ArrayBoundV2,debug.ExprInspection \
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,alpha.security.ArrayBoundV2,debug.ExprInspection \
 // RUN:   -analyzer-config eagerly-assume=false -verify %s
----------------
Perhaps only enable `unix.Malloc` to ensure that this test is not affected by changes to other checkers in the `unix` group. (It's enough for the testcase that you added.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158707