[PATCH] D69318: [analyzer] Add SufficientSizeArrayIndexingChecker

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:27
+    : public Checker<check::PreStmt<ArraySubscriptExpr>> {
+  mutable std::unique_ptr<BugType> BT;
+
----------------
The bug type can be initialized here:
`BT{this, "...", "..."}`
How did this compile with only one text argument to constructor? I think the first is a short name of the bug, second is a category that is not omittable. The text that is used here should be passed to the `BugReport`. `BugType::getDescription` returns the "name" of the bug that is the first argument. But I am not sure what matters from these texts, the code design looks confusing.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:50
+  if (isa<IntegerLiteral>(Index->IgnoreParenCasts()))
+    return;
+
----------------
`if (isa<IntegerLiteral>(Index))` should be used. `IntegerLiteral` is a subclass of `Expr`, not a `QualType`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:72
+      llvm::APSInt::getMaxValue(C.getASTContext().getIntWidth(IndexType),
+                                IndexType->isUnsignedIntegerType());
+  const nonloc::ConcreteInt MaxIndexValue{MaxIndex};
----------------
I would use `SVB.getBasicValueFactory().getMaxValue(IndexType)` to get the max value.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:82
+      SVB.evalBinOp(State, BO_Sub, SizeInElements, ConstantOne,
+                    C.getASTContext().UnsignedLongLongTy);
+
----------------
`SValBuilder::getArrayIndexType` should be better than the `UnsignedLongLongTy`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:86
+  const SVal TypeCanIndexEveryElement = SVB.evalBinOp(
+      State, BO_LE, SizeInElementsMinusOne, MaxIndexValue, IndexType);
+
----------------
I think `SVB.getConditionType()` should be used for condition result.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:121
+  // Mark the array expression interesting.
+  R->markInteresting(FirstElement->getSuperRegion());
+  C.emitReport(std::move(R));
----------------
I am not sure if this `markInteresting` call is correct. 


================
Comment at: clang/test/Analysis/sufficient-size-array-indexing-32bit.c:37
+const short two_byte_signed_index = 1; // sizeof(short) == 2
+const int four_byte_signed_index = 1;  // sizeof(int) == 4
+
----------------
I do not know if it is safe to make such assumptions about `sizeof`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69318