balazske added inline comments.
================
Comment at:
clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:27
+ : public Checker<check::PreStmt<ArraySubscriptExpr>> {
+ mutable std::unique_ptr<BugType> BT;
+
----------------
gamesh411 wrote:
> 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.
I had even a plan to remove the `BuiltinBug` because it looks confusing and
does not add real value. New checkers should use `BugType`.
================
Comment at:
clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:50
+ if (isa<IntegerLiteral>(Index->IgnoreParenCasts()))
+ return;
+
----------------
gamesh411 wrote:
> 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.
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.
================
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
+
----------------
gamesh411 wrote:
> 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.
Still it would be good to check if the test passes on all the buildbots.
================
Comment at: clang/test/Analysis/sufficient-size-array-indexing-32bit.c:48
+void ignore_literal_indexing_with_parens() {
+ char a = exactly_4byte_signed_range[(32)]; // nowarning
+}
----------------
Does this work in `[32 + 1]` case?
================
Comment at: clang/test/Analysis/sufficient-size-array-indexing-32bit.c:106
+ if (choice >= 1) {
+ c = f(choice)[four_byte_signed_index]; // nowarnining // the value is
one or two, f returns an array that is correct in size
+ }
----------------
`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`.
================
Comment at: clang/test/Analysis/sufficient-size-array-indexing-32bit.c:120
+
+void test_symbolic_index_handling4(int choice) {
+ char c;
----------------
Here "is a chance that indexing is correct". So no warning should occur?
================
Comment at: clang/test/Analysis/sufficient-size-array-indexing-64bit.c:1
+// RUN: %clang_analyze_cc1 -triple x86_64
-analyzer-checker=core,alpha.core.SufficientSizeArrayIndexing %s -verify
+
----------------
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).
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
