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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

We check now whether the argument of `typedef` and `sizeof` is an incorrect VLA, but what other examples are we potentially forgetting? The warning message states that "Declared variable-length array (VLA) has negative size", but we are not actually looking for declarations, but try to guess which statements may be declarations. Did we cover all realistic cases?

In D79072#2010142 <https://reviews.llvm.org/D79072#2010142>, @xazax.hun wrote:

> Overall the changes look good to me here. I had a small nit inline. But I wonder if we actually should add more code in the analyzer core to better model the sizes of memory regions corresponding to the VLAs.


I think this is that code :) I think the existing regions/values we have can explain VLAs well, don't you think?



================
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.
----------------
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?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp:182-183
 void VLASizeChecker::checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {
   if (!DS->isSingleDecl())
     return;
 
----------------
Should this ever happen? We seem to assert in ExprEngine.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:576
                                ExplodedNodeSet &Dst) {
+  if (isa<TypedefNameDecl>(*DS->decl_begin())) {
+    ExplodedNodeSet DstPre;
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79072