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