On Aug 8, 2014, at 23:23 , Manuel Klimek <[email protected]> wrote:
> On Sat, Aug 9, 2014 at 3:16 AM, Jordan Rose <[email protected]> wrote:
> ================
> Comment at: include/clang/Analysis/CFG.h:285
> @@ -285,1 +284,3 @@
> + CFGTemporaryDtor(CXXBindTemporaryExpr *expr, bool BindsParameter)
> + : CFGImplicitDtor(TemporaryDtor, expr, BindsParameter ? T : nullptr) {}
>
> ----------------
> Why not just use "self"? Or &T? Why "T"? Why does T require a dynamic
> allocation?
>
> Because I hoped you would say something like the above :) and I didn't find a
> good way to add a bool (the PointerIntPair Data2 already uses the "int" part
> for the kind, and it doesn't work with pointers to static constants (throws a
> runtime error in PointerInPair for incorrectly aligned pointer)).
It doesn't actually matter what you put in here as long as it's aligned, since
you're just using it for its truth value.
>
> ================
> Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:688-693
> @@ +687,8 @@
> + const MemRegion *Region = nullptr;
> + // If the class does not have any members, there will not be a region
> + // for it bound in the environment.
> + if (Optional<nonloc::LazyCompoundVal> LCV =
> + Val.getAs<nonloc::LazyCompoundVal>()) {
> + Region = LCV->getRegion();
> + }
> +
> ----------------
> This is not how LCVs work. An LCV is a snapshot of a region at a particular
> "time" (state of the Store); the region may not exist in the current store.
> The "best" answer in the current scheme is to use
> createTemporaryRegionIfNeeded, but the right answer may end up being to
> disregard the rvalue-ness of temporaries (as we talked about some on cfe-dev).
>
> Can you give me test cases that fail so I can work against them?
I'll get back to you on this; it requires me to go think about when we use LCVs.
>
> ================
> Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:508-509
> @@ -507,13 +507,4 @@
>
> bool ScanReachableSymbols::scan(nonloc::LazyCompoundVal val) {
> - bool wasVisited = !visited.insert(val.getCVData()).second;
> - if (wasVisited)
> - return true;
> -
> - StoreManager &StoreMgr = state->getStateManager().getStoreManager();
> - // FIXME: We don't really want to use getBaseRegion() here because pointer
> - // arithmetic doesn't apply, but scanReachableSymbols only accepts base
> - // regions right now.
> - const MemRegion *R = val.getRegion()->getBaseRegion();
> - return StoreMgr.scanReachableSymbols(val.getStore(), R, *this);
> + return scan(val.getRegion());
> }
> ----------------
> This is not correct; see above.
>
> Can you explain that in more detail - especially for the scan part? What does
> this break?
It looks something like this, but I don't know if this case will actually break.
// This particular example is C, not C++.
struct Wrapper w = { malloc(4) };
struct Wrapper w2 = w; // uses a LazyCompoundVal
w.value = NULL;
return w2;
At the return statement, we scan for live symbols. We see that 'w2' is still
live and that its value is an LCV. The LCV's region is the 'w' region. In the
code as written, we look into the store at the time the LCV was created and see
that the malloc'd region is still live. With your patch, we look at the current
contents of 'w' and see no reference to the malloc'd region, and thus would
report a leak.
Jordan
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
