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
