[cfe-commits] r137309 - in /cfe/trunk: include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h lib/StaticAnalyzer/Core/SymbolManager.cpp

[Ted Kremenek]

This looks good.  I have a few comments/questions inline.

On Aug 11, 2011, at 9:43 AM, Anna Zaks wrote:

> Author: zaks
> Date: Thu Aug 11 11:43:28 2011
> New Revision: 137309
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=137309&view=rev
> Log:
> Analyzer Core: Adding support for user-defined symbol dependencies. (For example, the allocated resource symbol only needs to be freed if no error has been returned by the allocator, so a checker might want to make the lifespan of the error code symbol depend on the allocated resource symbol.) Note, by default, the map that holds the dependencies will get destroyed along with the SymbolManager at the end of function exploration.
> 
> Modified:
>    cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
>    cfe/trunk/lib/StaticAnalyzer/Core/SymbolManager.cpp
> 
> Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h?rev=137309&r1=137308&r2=137309&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h (original)
> +++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h Thu Aug 11 11:43:28 2011
> @@ -91,6 +91,7 @@
> };
> 
> typedef const SymbolData* SymbolRef;
> +typedef llvm::SmallVector<SymbolRef, 1> SymbolRefSmallVectorTy;
> 
> /// A symbol representing the value of a MemRegion.
> class SymbolRegionValue : public SymbolData {
> @@ -357,8 +358,12 @@
> 
> class SymbolManager {
>   typedef llvm::FoldingSet<SymExpr> DataSetTy;
> +  typedef llvm::DenseMap<SymbolRef, SymbolRefSmallVectorTy> SymbolDependTy;

I think having a DenseMap that directly maps from a symbol to a vector will have additional malloc traffic whenever the DenseMap needs to be resized.  This is because all the value types need to be copied, which in this case are vector objects.  Instead, having

  llvm::DenseMap<SymbolRef, SymbolRefSmallVectoryTy*>

would be a bit more efficient in this regard.  Whenever the DenseMap gets resized, only these pointer values get copied.  Of course the tradeoff is that we'd need to walk over the DenseMap and explicitly destroy these vectors in SymbolReaper's destructor.


> 
>   DataSetTy DataSet;
> +  /// Stores the extra dependencies between symbols: the data should be kept
> +  /// alive as long as the key is live.
> +  SymbolDependTy SymbolDependencies;
>   unsigned SymbolCounter;
>   llvm::BumpPtrAllocator& BPAlloc;
>   BasicValueFactory &BV;
> @@ -413,6 +418,16 @@
>     return SE->getType(Ctx);
>   }
> 
> +  /// \brief Add artificial symbol dependency.
> +  ///
> +  /// The dependent symbol should stay alive as long as the primary is alive.
> +  void addSymbolDependency(const SymbolRef Primary, const SymbolRef Dependent);
> +
> +  /// \brief Drop all user-added dependencies on the primary symbol.
> +  void removeSymbolDependencies(const SymbolRef Primary);
> +
> +  const SymbolRefSmallVectorTy *getDependentSymbols(const SymbolRef Primary);
> +
>   ASTContext &getContext() { return Ctx; }
>   BasicValueFactory &getBasicVals() { return BV; }
> };
> @@ -495,6 +510,10 @@
>   /// \brief Set to the value of the symbolic store after
>   /// StoreManager::removeDeadBindings has been called.
>   void setReapedStore(StoreRef st) { reapedStore = st; }
> +
> +private:
> +  /// Mark the symbols dependent on the input symbol as live.
> +  void markDependentsLive(SymbolRef sym);
> };
> 
> class SymbolVisitor {
> 
> Modified: cfe/trunk/lib/StaticAnalyzer/Core/SymbolManager.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/SymbolManager.cpp?rev=137309&r1=137308&r2=137309&view=diff
> ==============================================================================
> --- cfe/trunk/lib/StaticAnalyzer/Core/SymbolManager.cpp (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Core/SymbolManager.cpp Thu Aug 11 11:43:28 2011
> @@ -248,9 +248,36 @@
>   return false;
> }
> 
> +void SymbolManager::addSymbolDependency(const SymbolRef Primary,
> +                                        const SymbolRef Dependent) {
> +  SymbolDependencies[Primary].push_back(Dependent);
> +}
> +
> +void SymbolManager::removeSymbolDependencies(const SymbolRef Primary) {
> +  SymbolDependencies.erase(Primary);
> +}
> +
> +const SymbolRefSmallVectorTy *SymbolManager::getDependentSymbols(
> +                                                     const SymbolRef Primary) {
> +  SymbolDependTy::const_iterator I = SymbolDependencies.find(Primary);
> +  if (I == SymbolDependencies.end())
> +    return 0;
> +  return &I->second;
> +}
> +
> +void SymbolReaper::markDependentsLive(SymbolRef sym) {
> +  if (const SymbolRefSmallVectorTy *Deps = SymMgr.getDependentSymbols(sym)) {
> +    for (SymbolRefSmallVectorTy::const_iterator I = Deps->begin(),
> +                                                E = Deps->end(); I != E; ++I) {
> +      markLive(*I);
> +    }
> +  }
> +}
> +
> void SymbolReaper::markLive(SymbolRef sym) {
>   TheLiving.insert(sym);
>   TheDead.erase(sym);
> +  markDependentsLive(sym);
> }
> 
> void SymbolReaper::markLive(const MemRegion *region) {
> @@ -299,8 +326,10 @@
> }
> 
> bool SymbolReaper::isLive(SymbolRef sym) {
> -  if (TheLiving.count(sym))
> +  if (TheLiving.count(sym)) {
> +    markDependentsLive(sym);
>     return true;
> +  }

Hmm.  Is it necessary to call markDependents() every time that TheLiving.count() returns true?  That seems a bit expensive.  Once we call markDependents() the first time, we shouldn't need to do it again.

One thing we could do is change TheLiving to be a map from symbols to some integer or enum value that records whether or not we have already called markDependents.  For example:

TheLivingTy::iterator it = TheLiving.find(sym);
if (it != TheLiving.end()) {
  if (it->second != HaveMarkedDependents) {
    it->second = HaveMarkedDependents;
    markDependentsLive(sym);
  }
  return true;
}

Alternatively, markDependentsLive() could do this check.  This way we only do markDependentsLive() once when isLive()/markLive() is called on a given symbol.  That added benefit is that this also supports circular dependencies.  Right now if we have a circular dependency, markDependentsLive() might not terminate.

> 
>   if (const SymbolDerived *derived = dyn_cast<SymbolDerived>(sym)) {
>     if (isLive(derived->getParentSymbol())) {
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110811/89cdfa07/attachment.html>