[PATCH] Fix memory leak for APValues that do memory allocation.

[Rafael Espíndola]

is it possible to test it? Even if the test only fails when run in a
leak checker?

On 3 May 2013 05:15, Manuel Klimek <klimek at google.com> wrote:
> This patch ensures that APValues are deallocated with the ASTContext
> by registering a deallocation function for APValues to the ASTContext.
>
> This patch was written by James Dennett, and I'm taking it over
> as this currently provides problems for tools.
>
> http://llvm-reviews.chandlerc.com/D736
>
> Files:
>   include/clang/AST/ASTContext.h
>   lib/AST/ASTContext.cpp
>   lib/AST/Decl.cpp
>
> Index: include/clang/AST/ASTContext.h
> ===================================================================
> --- include/clang/AST/ASTContext.h
> +++ include/clang/AST/ASTContext.h
> @@ -2186,10 +2186,12 @@
>                  const ObjCImplementationDecl *Impl) const;
>
>  private:
> -  /// \brief A set of deallocations that should be performed when the
> +  /// \brief A set of deallocations that should be performed when the
>    /// ASTContext is destroyed.
> -  SmallVector<std::pair<void (*)(void*), void *>, 16> Deallocations;
> -
> +  typedef llvm::SmallDenseMap<void(*)(void*), llvm::SmallVector<void*, 16> >
> +    DeallocationMap;
> +  DeallocationMap Deallocations;
> +
>    // FIXME: This currently contains the set of StoredDeclMaps used
>    // by DeclContext objects.  This probably should not be in ASTContext,
>    // but we include it here so that ASTContext can quickly deallocate them.
> Index: lib/AST/ASTContext.cpp
> ===================================================================
> --- lib/AST/ASTContext.cpp
> +++ lib/AST/ASTContext.cpp
> @@ -726,10 +726,12 @@
>    // FIXME: Is this the ideal solution?
>    ReleaseDeclContextMaps();
>
> -  // Call all of the deallocation functions.
> -  for (unsigned I = 0, N = Deallocations.size(); I != N; ++I)
> -    Deallocations[I].first(Deallocations[I].second);
> -
> +  // Call all of the deallocation functions on all of their targets.
> +  for (DeallocationMap::const_iterator I = Deallocations.begin(),
> +           E = Deallocations.end(); I != E; ++I)
> +    for (unsigned J = 0, N = I->second.size(); J != N; ++J)
> +      (I->first)((I->second)[J]);
> +
>    // ASTRecordLayout objects in ASTRecordLayouts must always be destroyed
>    // because they can contain DenseMaps.
>    for (llvm::DenseMap<const ObjCContainerDecl*,
> @@ -753,7 +755,16 @@
>  }
>
>  void ASTContext::AddDeallocation(void (*Callback)(void*), void *Data) {
> -  Deallocations.push_back(std::make_pair(Callback, Data));
> +  // TODO(jdennett): Optimize the naive code to avoid temporary vectors
> +  // for the common case where the Callback is already present.  The
> +  // assumption is that few distinct callbacks are used.
> +  // SmallDenseMap<void (*)(void*), void*>::const_iterator I =
> +  //   Deallocations.find(Callback);
> +  // if (I == Deallocations.end())
> +  //   Deallocations[Callback].push_back(Data);
> +  // else
> +  //   I->second.push_back(Data);
> +  Deallocations[Callback].push_back(Data);
>  }
>
>  void
> Index: lib/AST/Decl.cpp
> ===================================================================
> --- lib/AST/Decl.cpp
> +++ lib/AST/Decl.cpp
> @@ -1742,6 +1742,10 @@
>    EvaluatedStmt *Eval = Init.dyn_cast<EvaluatedStmt *>();
>    if (!Eval) {
>      Stmt *S = Init.get<Stmt *>();
> +    // Note: EvaluatedStmt contains an APValue, which usually holds
> +    // resources not allocated from the ASTContext.  We need to do some
> +    // work to avoid leaking those, but we do so in VarDecl::evaluateValue
> +    // where we can detect whether there's anything to clean up or not.
>      Eval = new (getASTContext()) EvaluatedStmt;
>      Eval->Value = S;
>      Init = Eval;
> @@ -1754,6 +1758,13 @@
>    return evaluateValue(Notes);
>  }
>
> +namespace {
> +// Destroy an APValue that was allocated in an ASTContext.
> +void DestroyAPValue(void* UntypedValue) {
> +  static_cast<APValue*>(UntypedValue)->~APValue();
> +}
> +} // namespace
> +
>  APValue *VarDecl::evaluateValue(
>      SmallVectorImpl<PartialDiagnosticAt> &Notes) const {
>    EvaluatedStmt *Eval = ensureEvaluatedStmt();
> @@ -1779,8 +1790,12 @@
>    bool Result = Init->EvaluateAsInitializer(Eval->Evaluated, getASTContext(),
>                                              this, Notes);
>
> -  // Ensure the result is an uninitialized APValue if evaluation fails.
> -  if (!Result)
> +  // Ensure the computed APValue is cleaned up later if evaluation succeeded,
> +  // or that it's empty (so that there's nothing to clean up) if evaluation
> +  // failed.
> +  if (Result)
> +    getASTContext().AddDeallocation(DestroyAPValue, &Eval->Evaluated);
> +  else
>      Eval->Evaluated = APValue();
>
>    Eval->IsEvaluating = false;
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>