[clang][extract-api] Stop allocating APIRecords via BumpPtrAllocator

[dang]

Summary

Using a BumpPtrAllocator introduced memory leaks for APIRecords as they
contain a std::vector. This meant that we needed to always keep a
reference to the records in APISet and arrange for their destructor to
get called appropriately. This was further complicated by the need for
records to own sub-records as these subrecords would still need to be
allocated via the BumpPtrAllocator and the owning record would now need
to arrange for the destructor of its subrecords to be called
appropriately.

Since APIRecords contain a std::vector so whenever elements get added to
that there is an associated heap allocation regardless. Since
performance isn't currently our main priority it makes sense to use
regular unique_ptr to keep track of APIRecords, this way we don't need
to arrange for destructors to get called.

The BumpPtrAllocator is still used for strings such as USRs so that we
can easily de-duplicate them as necessary.

[zixuw]

This is much cleaner now. Thanks Daniel!

clang/include/clang/ExtractAPI/API.h

323–326

Should we just rename the allocator along the lines of StringAllocator or StringPool so that the code is self-explanatory?

323–326

Also now that the allocator is used exclusively for strings, should we use SpecificBumpPtrAllocator?

[dang]

I have checked locally against San to make sure this doesn't reintroduce the memory leak issues we were seeing initially that lead to the addition of APIRecordUniquePtr

[dang]

Address review feedback: Rename APISet::Allocator to APISet::StringAllocator

clang/include/clang/ExtractAPI/API.h

323–326

After thinking about it, we shouldn't because we are storing strings that shouldn't be modified as const char * so we shouldn't need to call destructors.

clang/include/clang/ExtractAPI/API.h

 #include "clang/ExtractAPI/DeclarationFragments.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Casting.h"
 #include <memory>
 
-namespace {
-
-/// \brief A custom deleter used for ``std::unique_ptr`` to APIRecords stored
-/// in the BumpPtrAllocator.
-///
-/// \tparam T the exact type of the APIRecord subclass.
-template <typename T> struct UniquePtrBumpPtrAllocatorDeleter {
-  void operator()(T *Instance) { Instance->~T(); }
-};
-
-/// A unique pointer to an APIRecord stored in the BumpPtrAllocator.
-///
-/// \tparam T the exact type of the APIRecord subclass.
-template <typename T>
-using APIRecordUniquePtr =
-    std::unique_ptr<T, UniquePtrBumpPtrAllocatorDeleter<T>>;
-
-} // anonymous namespace
-
 namespace clang {
 namespace extractapi {
 
 /// DocComment is a vector of RawComment::CommentLine.
 ///
 /// Each line represents one line of striped documentation comment,
 /// with source range information. This simplifies calculating the source
 /// location of a character in the doc comment for pointing back to the source
struct EnumConstantRecord : APIRecord {
 
   static bool classof(const APIRecord *Record) {
     return Record->getKind() == RK_EnumConstant;
   }
 };
 
 /// This holds information associated with enums.
 struct EnumRecord : APIRecord {
-  SmallVector<APIRecordUniquePtr<EnumConstantRecord>> Constants;
+  SmallVector<std::unique_ptr<EnumConstantRecord>> Constants;
 
   EnumRecord(StringRef Name, StringRef USR, PresumedLoc Loc,
              const AvailabilityInfo &Availability, const DocComment &Comment,
              DeclarationFragments Declaration, DeclarationFragments SubHeading)
       : APIRecord(RK_Enum, Name, USR, Loc, Availability, LinkageInfo::none(),
                   Comment, Declaration, SubHeading) {}
 
   static bool classof(const APIRecord *Record) {
struct StructFieldRecord : APIRecord {
 
   static bool classof(const APIRecord *Record) {
     return Record->getKind() == RK_StructField;
   }
 };
 
 /// This holds information associated with structs.
 struct StructRecord : APIRecord {
-  SmallVector<APIRecordUniquePtr<StructFieldRecord>> Fields;
+  SmallVector<std::unique_ptr<StructFieldRecord>> Fields;
 
   StructRecord(StringRef Name, StringRef USR, PresumedLoc Loc,
                const AvailabilityInfo &Availability, const DocComment &Comment,
                DeclarationFragments Declaration,
                DeclarationFragments SubHeading)
       : APIRecord(RK_Struct, Name, USR, Loc, Availability, LinkageInfo::none(),
                   Comment, Declaration, SubHeading) {}
 
StructRecord *addStruct(StringRef Name, StringRef USR, PresumedLoc Loc,
                           const AvailabilityInfo &Availability,
                           const DocComment &Comment,
                           DeclarationFragments Declaration,
                           DeclarationFragments SubHeading);
 
   /// A map to store the set of GlobalRecord%s with the declaration name as the
   /// key.
   using GlobalRecordMap =
-      llvm::MapVector<StringRef, APIRecordUniquePtr<GlobalRecord>>;
+      llvm::MapVector<StringRef, std::unique_ptr<GlobalRecord>>;
 
   /// A map to store the set of EnumRecord%s with the declaration name as the
   /// key.
-  using EnumRecordMap =
-      llvm::MapVector<StringRef, APIRecordUniquePtr<EnumRecord>>;
+  using EnumRecordMap = llvm::MapVector<StringRef, std::unique_ptr<EnumRecord>>;
 
   /// A map to store the set of StructRecord%s with the declaration name as the
   /// key.
   using StructRecordMap =
-      llvm::MapVector<StringRef, APIRecordUniquePtr<StructRecord>>;
+      llvm::MapVector<StringRef, std::unique_ptr<StructRecord>>;
 
   /// Get the target triple for the ExtractAPI invocation.
   const llvm::Triple &getTarget() const { return Target; }
 
   /// Get the language options used to parse the APIs.
   const LangOptions &getLangOpts() const { return LangOpts; }
 
   const GlobalRecordMap &getGlobals() const { return Globals; }
public:
   ///
   /// \returns a StringRef of the copied string in APISet::Allocator.
   StringRef copyString(StringRef String);
 
   APISet(const llvm::Triple &Target, const LangOptions &LangOpts)
       : Target(Target), LangOpts(LangOpts) {}
 
 private:
-  /// BumpPtrAllocator to store APIRecord%s and generated/copied strings.
-  llvm::BumpPtrAllocator Allocator;
+  /// BumpPtrAllocator to store generated/copied strings.
+  ///
+  /// Note: The main use for this is being able to deduplicate strings.
+  llvm::BumpPtrAllocator StringAllocator;
 
   const llvm::Triple Target;
   const LangOptions LangOpts;
 
   GlobalRecordMap Globals;
   EnumRecordMap Enums;
   StructRecordMap Structs;
 };
 
 } // namespace extractapi
 } // namespace clang
 
 #endif // LLVM_CLANG_EXTRACTAPI_API_H

clang/lib/ExtractAPI/API.cpp

 ///
 //===----------------------------------------------------------------------===//
 
 #include "clang/ExtractAPI/API.h"
 #include "clang/AST/CommentCommandTraits.h"
 #include "clang/AST/CommentLexer.h"
 #include "clang/AST/RawCommentList.h"
 #include "clang/Index/USRGeneration.h"
-#include "llvm/Support/Allocator.h"
+#include <memory>
 
 using namespace clang::extractapi;
 using namespace llvm;
 
 GlobalRecord *APISet::addGlobal(GVKind Kind, StringRef Name, StringRef USR,
                                 PresumedLoc Loc,
                                 const AvailabilityInfo &Availability,
                                 LinkageInfo Linkage, const DocComment &Comment,
                                 DeclarationFragments Fragments,
                                 DeclarationFragments SubHeading,
                                 FunctionSignature Signature) {
   auto Result = Globals.insert({Name, nullptr});
   if (Result.second) {
     // Create the record if it does not already exist.
-    auto Record = APIRecordUniquePtr<GlobalRecord>(new (Allocator) GlobalRecord{
+    auto Record = std::make_unique<GlobalRecord>(
         Kind, Name, USR, Loc, Availability, Linkage, Comment, Fragments,
-        SubHeading, Signature});
+        SubHeading, Signature);
     Result.first->second = std::move(Record);
   }
   return Result.first->second.get();
 }
 
 GlobalRecord *
 APISet::addGlobalVar(StringRef Name, StringRef USR, PresumedLoc Loc,
                      const AvailabilityInfo &Availability, LinkageInfo Linkage,
APISet::addFunction(StringRef Name, StringRef USR, PresumedLoc Loc,
   return addGlobal(GVKind::Function, Name, USR, Loc, Availability, Linkage,
                    Comment, Fragments, SubHeading, Signature);
 }
 
 EnumConstantRecord *APISet::addEnumConstant(
     EnumRecord *Enum, StringRef Name, StringRef USR, PresumedLoc Loc,
     const AvailabilityInfo &Availability, const DocComment &Comment,
     DeclarationFragments Declaration, DeclarationFragments SubHeading) {
-  auto Record =
-      APIRecordUniquePtr<EnumConstantRecord>(new (Allocator) EnumConstantRecord{
-          Name, USR, Loc, Availability, Comment, Declaration, SubHeading});
+  auto Record = std::make_unique<EnumConstantRecord>(
+      Name, USR, Loc, Availability, Comment, Declaration, SubHeading);
   return Enum->Constants.emplace_back(std::move(Record)).get();
 }
 
 EnumRecord *APISet::addEnum(StringRef Name, StringRef USR, PresumedLoc Loc,
                             const AvailabilityInfo &Availability,
                             const DocComment &Comment,
                             DeclarationFragments Declaration,
                             DeclarationFragments SubHeading) {
   auto Result = Enums.insert({Name, nullptr});
   if (Result.second) {
     // Create the record if it does not already exist.
-    auto Record = APIRecordUniquePtr<EnumRecord>(new (Allocator) EnumRecord{
-        Name, USR, Loc, Availability, Comment, Declaration, SubHeading});
+    auto Record = std::make_unique<EnumRecord>(
+        Name, USR, Loc, Availability, Comment, Declaration, SubHeading);
     Result.first->second = std::move(Record);
   }
   return Result.first->second.get();
 }
 
 StructFieldRecord *APISet::addStructField(StructRecord *Struct, StringRef Name,
                                           StringRef USR, PresumedLoc Loc,
                                           const AvailabilityInfo &Availability,
                                           const DocComment &Comment,
                                           DeclarationFragments Declaration,
                                           DeclarationFragments SubHeading) {
-  auto Record =
-      APIRecordUniquePtr<StructFieldRecord>(new (Allocator) StructFieldRecord{
-          Name, USR, Loc, Availability, Comment, Declaration, SubHeading});
+  auto Record = std::make_unique<StructFieldRecord>(
+      Name, USR, Loc, Availability, Comment, Declaration, SubHeading);
   return Struct->Fields.emplace_back(std::move(Record)).get();
 }
 
 StructRecord *APISet::addStruct(StringRef Name, StringRef USR, PresumedLoc Loc,
                                 const AvailabilityInfo &Availability,
                                 const DocComment &Comment,
                                 DeclarationFragments Declaration,
                                 DeclarationFragments SubHeading) {
   auto Result = Structs.insert({Name, nullptr});
   if (Result.second) {
     // Create the record if it does not already exist.
-    auto Record = APIRecordUniquePtr<StructRecord>(new (Allocator) StructRecord{
-        Name, USR, Loc, Availability, Comment, Declaration, SubHeading});
+    auto Record = std::make_unique<StructRecord>(
+        Name, USR, Loc, Availability, Comment, Declaration, SubHeading);
     Result.first->second = std::move(Record);
   }
   return Result.first->second.get();
 }
 
 StringRef APISet::recordUSR(const Decl *D) {
   SmallString<128> USR;
   index::generateUSRForDecl(D, USR);
   return copyString(USR);
 }
 
 StringRef APISet::copyString(StringRef String) {
   if (String.empty())
     return {};
 
   // No need to allocate memory and copy if the string has already been stored.
-  if (Allocator.identifyObject(String.data()))
+  if (StringAllocator.identifyObject(String.data()))
     return String;
 
-  void *Ptr = Allocator.Allocate(String.size(), 1);
+  void *Ptr = StringAllocator.Allocate(String.size(), 1);
   memcpy(Ptr, String.data(), String.size());
   return StringRef(reinterpret_cast<const char *>(Ptr), String.size());
 }
 
 APIRecord::~APIRecord() {}
 
 void GlobalRecord::anchor() {}