Charusso added inline comments.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:25
-/// Get the stored dynamic size for the region \p MR.
+/// \returns The stored dynamic size for the region \p MR.
DefinedOrUnknownSVal getDynamicSize(ProgramStateRef State, const MemRegion *MR,
----------------
NoQ wrote:
> Naming: I believe we should keep using the word "Extent". There's no need to
> introduce a new term for the existing concept; it makes it harder to explain
> on the mailing list :) Let's make a follow-up patch to change the naming back
> (so that not to screw the review).
Since then I have changed my mind when I have read about Environment and Store
in a book. Sure, next up.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:41-44
+/// Set the dynamic size of a CXXNewExpr \p NE by its region \p MR.
+ProgramStateRef setDynamicSize(ProgramStateRef State, const MemRegion *MR,
+ const CXXNewExpr *NE,
+ const LocationContext *LCtx, SValBuilder &SVB);
----------------
NoQ wrote:
> This function is probably going to be used exactly once in the whole code.
> There's no need to turn it into a public API.
It is being used 3 times already, so I believe it is a cool API.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:99
+ .Case("clang_analyzer_region",
&ExprInspectionChecker::analyzerRegion)
+ .Case("clang_analyzer_size",
&ExprInspectionChecker::analyzerDumpSize)
+ .Case("clang_analyzer_elementCount",
----------------
NoQ wrote:
> `clang_analyzer_dump_extent()`? Or just
> `clang_analyzer_dump(clang_analyzer_getExtent())`.
I like the shorter version, but of course I have seen the longer version.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:283
+ llvm::raw_svector_ostream Out(Msg);
+ Out << MR;
+ reportBug(Out.str(), C);
----------------
NoQ wrote:
> So, how is it different from the existing `clang_analyzer_dump()`?
I wanted to make it for the derived regions, but then I have realized I am lazy
to dig into the buggier parts of the Analyzer. Removed.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:310-311
+
+ QualType Ty = TVR ? TVR->getDesugaredValueType(C.getASTContext())
+ : CE->getArg(0)->IgnoreParenImpCasts()->getType();
+
----------------
NoQ wrote:
> How is this better than `getValueType()`? Are you sure you're not getting a
> pointer type instead in the `!TVR` case? I.e., can you test this on a
> non-heap `SymRegion`?
> How is this better than getValueType()?
Consistency. We get the static ~~size~~ extent by getting the desugared type
which most likely just an extra overhead.
> Are you sure you're not getting a pointer type instead in the !TVR case?
> I.e., can you test this on a non-heap SymRegion?
The issue was the `var_region_simple_ptr` test: we need to use regex for
checking after the `}}` token where `/ 4` is the correct result. I did not
really get why the test pass.
================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:85
+ if (CI->getValue().isUnsigned())
+ Size = SVB.makeIntVal(CI->getValue(), /*IsUnsigned=*/false);
+
----------------
NoQ wrote:
> Charusso wrote:
> > That one is interesting. Some of the checkers / SValBuilder(?) generate
> > unsigned integers which should not happen, I believe. May we need a FIXME
> > and an assertion about signedness. What do you think?
> `SymbolExtent` has type `size_t` and both `malloc` and `operator new` accept
> `size_t` as a parameter. Therefore everything needs to be //unsigned// and we
> need to assert this.
>
> That said, array indexes are //signed// (as per implementation of
> `ElementRegion`).
It was a premature optimization for consistency. I will leave the signedness as
is, FIXME added.
================
Comment at: clang/test/Analysis/misc-ps-region-store.m:1190
+ tmp2[x] = am; // expected-warning \
+ {{Access out-of-bound array element (buffer overflow)}}
}
----------------
NoQ wrote:
> Charusso wrote:
> > That is the single regression which I do not get.
> Well, please debug. Like, look at the full report, dump egraph, see what
> changed. Try to `creduce` the example further under the condition "behavior
> changed with the patch", maybe that'll clear something up (though if it's a
> true positive after creduce it doesn't guarantee that it's a true positive
> before creduce).
Given that I have no access for `rdar` I did my hand-reduce and the solution is
the following:
```lang=c
// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin9 \
// RUN: -analyzer-checker=core,alpha.security.ArrayBound \
// RUN: -verify %s
static void RDar8424269_B(int unknown) {
unsigned char tmp2t[1][unknown];
tmp2t[1][13] = 0; // expected-warning \
{{Access out-of-bound array element (buffer overflow)}}
}
```
Looking at the master branch `core.VLASize` created the constraint
`extent_$1{tmp2} {[0, 0]}` without any state splits. The new behavior does not
create random constraints so that an alpha checker could detect we have no
information about the unknown ~~size~~ extent.
Given that the alpha checker cannot catch the following: `char foo[bar];
foo[13] = 0;` is assumed that the problem is the VLASizeChecker's modeling
which has a FIXME: `Handle multi-dimensional VLAs.` but does not handle this
case.
An easy and appropriate fix to detect the multi-dimensional array. Here is the
way to obtain the inner dimensions:
http://clang-developers.42468.n3.nabble.com/Multi-dimensional-arrays-td4028727.html
So that the final solution is:
```lang=diff
// FIXME: Handle multi-dimensional VLAs.
+ if (VLA->getElementType()->getAsArrayTypeUnsafe())
+ return;
```
I hope with that I have fixed even more Bugzilla issues. May it would be
interesting to see why the ~~size~~ extent of the array got a constraint of
being `0`, but I leave it as an exercise for the reader.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69726/new/
https://reviews.llvm.org/D69726
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
