[PATCH] Implement inlining of temporary destructors.

[Jordan Rose]

On Aug 8, 2014, at 23:23 , Manuel Klimek <klimek at google.com> wrote:

> On Sat, Aug 9, 2014 at 3:16 AM, Jordan Rose <jordan_rose at apple.com> 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



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140812/5a04b71c/attachment.html>