[PATCH] D69318: [analyzer] Add SufficientSizeArrayIndexingChecker

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 marked 4 inline comments as done.
gamesh411 added a comment.

I am now experimenting with the suggestions. The other issues are on the worklist. Thanks for these so far.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:50
+  if (isa<IntegerLiteral>(Index->IgnoreParenCasts()))
+    return;
+
----------------
balazske wrote:
> 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.
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.
```


================
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
+}
----------------
balazske wrote:
> Does this work in `[32 + 1]` case?
It should, I will add a case for it.


================
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
+    }
----------------
balazske wrote:
> `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`.
Yes, this test is a bit redundant, should I remove it in your opinion?


================
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:
> 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.
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 :) ).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69318