Checks for indexing situations, where an array is indexed with a
variable of type not sufficiently large to cover the whole range of the
arrays possible index set. This check is uses path-sensitivity in order
to reason about the possible size of the array being checked.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I suspect that with this check by choosing path-sensitivity you gain less than you lose. That is, checking how the array pointer is passed around may be nice, but if you sacrificed that, you could have turned this check into a compiler warning which would have had an effect on much larger audience.
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td | ||
---|---|---|
749 | What makes this checker C++-specific? All your tests are in plain C. | |
clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp | ||
94 | This is always true (we should probably tell SVal.getAsRegion() to return a const SubRegion *). | |
101 | This never happens. The chain of superregions never has loops; it is always finite. | |
104 | This deserves comments on what kinds of regions do you expect to see here. Do i understand correctly that you expect BaseMemRegion to be an ElementRegion and its superregion to be the whole array? 'Cause the former is super unobvious and most likely shouldn't be that way. | |
154–156 | I recommend also adding trackExpressionValue() for the array expression. In your "symbolic index handling" tests it would hopefully* allow you to see the execution path within f() as part of the report, which is essential. __ |
clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp | ||
---|---|---|
43 | The whole point of bug types is to remain the same regardless of the specific message. They're more like a category. Please use only one bug type for the whole checker (unless you really find different categories of bugs). |
clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp | ||
---|---|---|
76 | Shoudn't we use isa<> if we don't use the resulting pointer? | |
127 | I think its better to stay consistent regards naming. |
clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp | ||
---|---|---|
43 | I have removed the whole BugType factory think, that was really stupid in retrospect. | |
104 | I feel a bit more familiar now with the infrastructure now, and I used getDynamicElementCount as it now an available utility. Thanks for the comments, they were helpful for discovering how things work! :D |
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td | ||
---|---|---|
749 | Very true, I have moved to core.alpha. |
The checker has been updated, the comments and the logic polished. I would still take a stab at this being a ClangSa checker (as opposed to being a Tidy check). What do you think?
clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp | ||
---|---|---|
27 | The bug type can be initialized here: | |
50 | if (isa<IntegerLiteral>(Index)) should be used. IntegerLiteral is a subclass of Expr, not a QualType. | |
72 | I would use SVB.getBasicValueFactory().getMaxValue(IndexType) to get the max value. | |
82 | SValBuilder::getArrayIndexType should be better than the UnsignedLongLongTy. | |
86 | I think SVB.getConditionType() should be used for condition result. | |
121 | I am not sure if this markInteresting call is correct. | |
clang/test/Analysis/sufficient-size-array-indexing-32bit.c | ||
37 | I do not know if it is safe to make such assumptions about sizeof. |
clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp | ||
---|---|---|
27 | 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. | |
50 | 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. | |
72 | Good point, thanks for pointing out such a method existed in BasicValueFactory! | |
82 | Seems reasonable :) | |
86 | I will use that, thanks. | |
121 | 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. | |
clang/test/Analysis/sufficient-size-array-indexing-32bit.c | ||
37 | You are definitely right! However it is common as per: 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. |
clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp | ||
---|---|---|
27 | I had even a plan to remove the BuiltinBug because it looks confusing and does not add real value. New checkers should use BugType. | |
50 | Probably function SVB.getConstantVal can be used to test if there is a (compile-time) constant passed to the index. But it may be value of a (const) variable. | |
clang/test/Analysis/sufficient-size-array-indexing-32bit.c | ||
37 | Still it would be good to check if the test passes on all the buildbots. | |
49 | Does this work in [32 + 1] case? | |
107 | choice can be here only 1. If it could be 1 or 2 we should get no warning because the array size may be good or bad. But to test that it is enough that choice can have any value, like in test_symbolic_index_handling4. | |
121 | Here "is a chance that indexing is correct". So no warning should occur? | |
clang/test/Analysis/sufficient-size-array-indexing-64bit.c | ||
2 | I could not find difference between this and the previous test file (except the multi-dimensional arrays are omitted). It would be better to have a single test file without repetition. (Multiple RUN lines in a single file should work). |
I am now experimenting with the suggestions. The other issues are on the worklist. Thanks for these so far.
clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp | ||
---|---|---|
50 | Yes, that is a design question, should the checker warn if the index is not a literal but a const variable like: constexpr char SpecialIndex = 12; int Array[300] = make_array(); Array[SpecialIndex]; // should we warn in this case? because now we do, // as the indexing expression is not a literal. | |
clang/test/Analysis/sufficient-size-array-indexing-32bit.c | ||
37 | I'm afraid we can only test that by committing, as there is no other way to submit a build-job ( not that I am aware of, any information in that regard is welcome :) ). | |
49 | It should, I will add a case for it. | |
107 | Yes, this test is a bit redundant, should I remove it in your opinion? |
clang/test/Analysis/sufficient-size-array-indexing-32bit.c | ||
---|---|---|
121 | I think as there is a branch where it is provably wrong, this is correct. It is just not obvious from the diag message which branch is taken, and consequently which array is being indexed. I think there is no way to address this from inside the checker. |
What makes this checker C++-specific? All your tests are in plain C.