gamesh411 marked 7 inline comments as done.
gamesh411 added inline comments.
================
Comment at:
clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:27
+ : public Checker<check::PreStmt<ArraySubscriptExpr>> {
+ mutable std::unique_ptr<BugType> BT;
+
----------------
balazske wrote:
> 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.
I think because it is initialized with a `BuiltinBug`, which must be a subclass
of BugType. I don't really know what should be preferable nowadays, as this
code was actually written more than a year ago. Thanks for pointing out that it
can be initialized there, I think lazy initialization seems not that important
with this particular checker.
================
Comment at:
clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:50
+ if (isa<IntegerLiteral>(Index->IgnoreParenCasts()))
+ return;
+
----------------
balazske wrote:
> `if (isa<IntegerLiteral>(Index))` should be used. `IntegerLiteral` is a
> subclass of `Expr`, not a `QualType`.
The way I have structured the code is very misleading, sorry for that, I will
move the type extraction if lower, where it is actually used.
================
Comment at:
clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:72
+ llvm::APSInt::getMaxValue(C.getASTContext().getIntWidth(IndexType),
+ IndexType->isUnsignedIntegerType());
+ const nonloc::ConcreteInt MaxIndexValue{MaxIndex};
----------------
balazske wrote:
> I would use `SVB.getBasicValueFactory().getMaxValue(IndexType)` to get the
> max value.
Good point, thanks for pointing out such a method existed in
`BasicValueFactory`!
================
Comment at:
clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:82
+ SVB.evalBinOp(State, BO_Sub, SizeInElements, ConstantOne,
+ C.getASTContext().UnsignedLongLongTy);
+
----------------
balazske wrote:
> `SValBuilder::getArrayIndexType` should be better than the
> `UnsignedLongLongTy`.
Seems reasonable :)
================
Comment at:
clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:86
+ const SVal TypeCanIndexEveryElement = SVB.evalBinOp(
+ State, BO_LE, SizeInElementsMinusOne, MaxIndexValue, IndexType);
+
----------------
balazske wrote:
> I think `SVB.getConditionType()` should be used for condition result.
I will use that, thanks.
================
Comment at:
clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:121
+ // Mark the array expression interesting.
+ R->markInteresting(FirstElement->getSuperRegion());
+ C.emitReport(std::move(R));
----------------
balazske wrote:
> I am not sure if this `markInteresting` call is correct.
What I wanted to do conceptually is to mark the array itself interesting. I am
however not sure whether this is the best way. I will try this and
`FirstElement` itself and maybe look at whether there are some additional notes
emitted along the way.
================
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
+
----------------
balazske wrote:
> I do not know if it is safe to make such assumptions about `sizeof`.
You are definitely right! However it is common as per:
https://en.cppreference.com/w/cpp/language/types#Data_models
```
Data models
The choices made by each implementation about the sizes of the fundamental
types are collectively known as data model. Four data models found wide
acceptance:
32 bit systems:
LP32 or 2/4/4 (int is 16-bit, long and pointer are 32-bit)
Win16 API
ILP32 or 4/4/4 (int, long, and pointer are 32-bit);
Win32 API
Unix and Unix-like systems (Linux, macOS)
64 bit systems:
LLP64 or 4/4/8 (int and long are 32-bit, pointer is 64-bit)
Win64 API
LP64 or 4/8/8 (int is 32-bit, long and pointer are 64-bit)
Unix and Unix-like systems (Linux, macOS)
Other models are very rare. For example, ILP64 (8/8/8: int, long, and pointer
are 64-bit) only appeared in some early 64-bit Unix systems (e.g. Unicos on
Cray).
```
Only ILP32 has 16 bit ints.
Next idea would be to use fixed-width integer types from `cstdint`. But tests
should not use system headers, and there are mentions in test files to
`int32_t`, howevery they are just typedefs for int. And I think we maintaining
a whole standard library headers is a bit too much a hassle.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69318/new/
https://reviews.llvm.org/D69318
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
