zaks.anna added inline comments.
================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1003
@@ +1002,3 @@
+//
+ProgramStateRef MallocChecker::addExtentSize(CheckerContext &C,
+ const CXXNewExpr *NE,
----------------
NoQ wrote:
> dkrupp wrote:
> > xazax.hun wrote:
> > > zaks.anna wrote:
> > > > I am not sure this code belongs to the malloc checker since it only
> > > > supports the array bounds checker. Is there a reason it's not part of
> > > > that checker?
> > > I think it is part of the malloc checker because it already does
> > > something very very similar to malloc, see the MallocMemAux function. So
> > > in fact, for the array bounds checker to work properly, the malloc
> > > checker should be turned on.
> > Extent size is used by ArrayBoundChecker, ArrayBoundCheckerV2 and
> > CStringChecker checkers currently. New expression in case of simple
> > allocations (0 allocation) was already handled in Malloc checker , that's
> > why I implemented it there. But I agree it feels odd that one has to switch
> > on unix.Malloc checker to get the size of new allocated heap regions.
> > Should I move this to ArrayBoundChecker or ArrayBoundCheckerV2?
> 1. Well, in my dreams, this code should be in the silent operator-new
> modelling checker, which would be separate from the stateless operator-new
> bug-finding checker. Then all the checkers that rely on extent would
> automatically load the modelling checker as a dependency.
>
> 2. Yeah, i think many checkers may consider extents, not just array bound;
> other checkers may appear in the future. This info is very useful, which is
> why we have the whole symbol class just for that (however, checker
> communication through constraints on this symbol class wasn't IMHO a good
> idea, because it's harder for the constraint manager to deal with symbols and
> constraints rather than with concrete values).
>
> //Just wanted to speak out, thoughts below might perhaps be more on-topic//
>
> 3. The MallocChecker is not just unix.Malloc, but also cplusplus.NewDelete,
> etc., which makes a lot more sense to leave it here.
>
> 4. Consider another part of MallocChecker's job - modeling the memory spaces
> for symbolic regions (heap vs. unknown). For malloc(), this is done in the
> checker. For operator-new, it is done in the ExprEngine(!), because this is
> part of the language rather than a library function. Perhaps ExprEngine would
> be the proper place for that code as well.
> Well, in my dreams, this code should be in the silent operator-new modelling
> checker, which would be
> separate from the stateless operator-new bug-finding checker. Then all the
> checkers that rely on extent
> would automatically load the modelling checker as a dependency.
I agree. This sounds like the best approach! (Though it's not a must have to
get the patch in.)
> Consider another part of MallocChecker's job - modeling the memory spaces for
> symbolic regions (heap vs.
> unknown). For malloc(), this is done in the checker. For operator-new, it is
> done in the ExprEngine(!), because
> this is part of the language rather than a library function. Perhaps
> ExprEngine would be the proper place for
> that code as well.
Interesting point. Can you clarify the last sentence?
================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1020
@@ +1019,3 @@
+ } else {
+ Size = svalBuilder.makeIntVal(1, true);
+ Region = (State->getSVal(NE, LCtx)).getAsRegion()->getAs<SubRegion>();
----------------
Please, be more explicit that this is not a size of allocation (as in not 1
byte)? Maybe call this ElementCount or something like that.
================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1021
@@ +1020,3 @@
+ Size = svalBuilder.makeIntVal(1, true);
+ Region = (State->getSVal(NE, LCtx)).getAsRegion()->getAs<SubRegion>();
+ }
----------------
A bit of code repetition from above. Please, add comments explaining why we
need subregion in one case and super region in the other.
Are there test cases for this branch?
================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1049
@@ +1048,3 @@
+ CharUnits Scaling, SValBuilder &Sb) {
+ return Sb.evalBinOpNN(State, BO_Mul, BaseVal,
+ Sb.makeArrayIndex(Scaling.getQuantity()),
----------------
This should probably be inline/have different name since it talks about
ArrayIndexType and is not reused elsewhere.
================
Comment at: test/Analysis/out-of-bounds.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -std=c++11 -Wno-array-bounds -analyze
-analyzer-checker=unix,core,alpha.security.ArrayBoundV2 -verify %s
+
----------------
Let's use a more specific file name.
https://reviews.llvm.org/D24307
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
