[PATCH] D79072: [Analyzer][VLASizeChecker] Check VLA size in typedef and sizeof.
Balázs Kéri via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu May 7 06:45:47 PDT 2020
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
More information about the cfe-commits
mailing list