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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to