[PATCH] D79072: [Analyzer][VLASizeChecker] Check VLA size in typedef and sizeof.

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

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

I do not know what other than `typedef` or `sizeof` (and variable declaration) can contain VLA. To my knowledge VLA is not normally supported in C++ and should not be used anyway (there are better ways for doing it) so some funny C++ things with VLA may not work. Anyway the patch is about "Check VLA size in typedef and sizeof", later other things can be added if required. The CERT rule ARR32-C (this is to be supported by the change) mentions only `typedef` and `sizeof`.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp:65-69
+  // Walk over the VLAs for every dimension until a non-VLA is found.
+  // Collect the sizes in VLASizes, put the most inner VLA to `VLALast`.
+  // In "vla[x][2][y][3]" this will be the array for index "y".
+  // There is a VariableArrayType for every dimension (here "x", "2", "y")
+  // until a non-vla is found.
----------------
Szelethus wrote:
> This, or at least parts of this comment should be placed for the function declaration rather :) Also, you explain the "how", but not the "why". You remove the `VLALast` argument in the followup patch, why do we add it here?
It explains "how" because it is a comment inside the function. I can add comment to the function too. `VLALast` is used in this change, the next change contains additional refactoring that makes it unneeded, because the evaluation of size expressions (that uses `VLA` and `VLALast`) is moved into this function.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp:182-183
 void VLASizeChecker::checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {
   if (!DS->isSingleDecl())
     return;
 
----------------
Szelethus wrote:
> Should this ever happen? We seem to assert in ExprEngine.
This was part of the old code, it may be wrong but not related to this change.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:576
                                ExplodedNodeSet &Dst) {
+  if (isa<TypedefNameDecl>(*DS->decl_begin())) {
+    ExplodedNodeSet DstPre;
----------------
Szelethus wrote:
> xazax.hun wrote:
> > At first, it might be confusing why do we need to handle typedefs at all. I think it might worth adding a comment about VLAs. 
> Definitely. If there is a standard out there, it would be great to quote it. Also, it might be worth going on a bit of a tangent:
> 
> Although we're mostly looking for variable declarations here, the size of variable-length arrays are evaluated at the typedef, as specified by §x.x.x.x.
> 
> Also, whats up with `using`? VLAs can occur in C++ as well, can they not?
There is `TypedefNameDecl` used and not `TypedefDecl`, so this should work for `using` too. (But main intent is to work on C source code, as VLA is not C++ friendly.) I found the `typedef` in a C standard draft at 6.7.7 (that tells about VLA size). The size expression must be evaluated when the `typedef` is encountered.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79072