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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
