unique_ptrify ownership of ModuleFiles in the ModuleManager

[dblaikie]

Summary

We could maintain ownership in 'Chain' rather than 'Modules', though that'll
require a little more reordering in removeModules (if it's OK to just move the
Chain loop/remove_if to after the victim loop, then that's easy)

The main uncertainty I have about this change is whether it's safe to move the
Modules.erase from the start of the victim loop (in removeModules) to the end.
Is that OK? Or do some of the operations in the mody of that loop rely on the
entry to have already been removed from 'Modules'?)

[rsmith]

I think this is correct. I'm on the fence as to whether it's an improvement, due to the sort-of dual ownership of these pointers.

include/clang/Serialization/ModuleManager.h

34–37

Storing pointers to ModuleFiles that point into values owned by a unique_ptr worries me a little. Seems like there's a risk here of a dangling pointer getting left in Chain; I think it'd be more obvious this happened if the delete were explicit.

[dblaikie]

include/clang/Serialization/ModuleManager.h

34–37

Not sure how much more obvious it'd be - as it stands the likely problem is a memory leak (such as 220569 ) rather than an inconsistency between these two data structures.

At least if something's dangling it'll likely fail pretty quick, rather than silently leak... but yeah, tradeoffs.

This certainly isn't the only case where we have owning and non-owning data structures at the same scope (including member scope), so I don't think it's all that surprising, but I could be wrong. Certainly we could comment the members more clearly to indicate that the poniters of one point to the same objects as the unique_ptrs of the other and that these need to be kept in step (this latter property is probably worth describing in more detail regardless)

[klimek]

include/clang/Serialization/ModuleManager.h

34–37

I'd vote for putting the unique_ptr into Chain, and having Modules point to that. I find that to be a pattern I'm using quite a bit in the new world, and at least for me personally having an explicit delete in there will help less than than the nicely visible ownership we get through unique_ptr.

[dblaikie]

include/clang/Serialization/ModuleManager.h

34–37

Yeah, I'm totally OK with having Chains as the owner if that's any more compelling. I forget why I picked Modules - but all evidence points to Chains being the "owner" in this code (its the data structure iterated over in the dtor, iterators to Chains are what're handled in removeModules, etc).

[dblaikie]

include/clang/Serialization/ModuleManager.h

34–37

Found some old git branches I had lying around including this one.

Turns out one reason making Chain the owner is a bit harder is that it's exposed in more places (ModuleManager exposes iterators to Chain through begin/end/etc). This goes into many places, including llvm Graph APIs (llvm/Support/GraphWriter, et al), other places like ModuleManager.cpp:186: llvm::SmallPtrSet<ModuleFile *, 4> victimSet(first, last);

So to have Chain as the owner, we'd probably need to implement an iterator adapter from iterator over unique_ptr<T> to iterator over T* rvalue (since there's no underlying T* to point to/reference). Not sure if that's worth it here, but we'll probably eventually want such an abstraction, so maybe this can wait until I find the time/inclination to implement such a thing.

[dblaikie]

Looks like @dexonsmith got this (putting the ownership in Chain & using some iterator adapters to narrow the cleanup scope/fallout) in rGa897f7cd40b12f693acbb04b81ef928244637aae. Thanks @dexonsmith!

[dblaikie]

In D5980#3253378, @dblaikie wrote:

Looks like @dexonsmith got this (putting the ownership in Chain & using some iterator adapters to narrow the cleanup scope/fallout) in rGa897f7cd40b12f693acbb04b81ef928244637aae. Thanks @dexonsmith!

And just because I was looking at the code anyway, I migrated the FirstVisitState linked list to unique_ptr (thought about forward_list but given the handling of raw nodes, it seemed simpler not to do that) in rGbaa9b7c3c83ab6e4dfb15b8d7815a9958d5b5810

include/clang/Serialization/ModuleManager.h

 class ModuleMap;
 
 namespace serialization {
 
 /// \brief Manages the set of modules loaded by an AST reader.
 class ModuleManager {
   /// \brief The chain of AST files. The first entry is the one named by the
   /// user, the last one is the one that doesn't depend on anything further.
   SmallVector<ModuleFile *, 2> Chain;
   
   /// \brief All loaded modules, indexed by name.
-  llvm::DenseMap<const FileEntry *, ModuleFile *> Modules;
+  llvm::DenseMap<const FileEntry *, std::unique_ptr<ModuleFile>> Modules;
   
   /// \brief FileManager that handles translating between filenames and
   /// FileEntry *.
   FileManager &FileMgr;
   
   /// \brief A lookup of in-memory (virtual file) buffers
   llvm::DenseMap<const FileEntry *, std::unique_ptr<llvm::MemoryBuffer>>
       InMemoryBuffers;

lib/Serialization/ModuleManager.cpp

const FileEntry *Entry = FileMgr.getFile(Name, /*openFile=*/false,
                                            /*cacheFailure=*/false);
   if (Entry)
     return lookup(Entry);
 
   return nullptr;
 }
 
 ModuleFile *ModuleManager::lookup(const FileEntry *File) {
-  llvm::DenseMap<const FileEntry *, ModuleFile *>::iterator Known
-    = Modules.find(File);
+  auto Known = Modules.find(File);
   if (Known == Modules.end())
     return nullptr;
 
-  return Known->second;
+  return Known->second.get();
 }
 
 std::unique_ptr<llvm::MemoryBuffer>
 ModuleManager::lookupBuffer(StringRef Name) {
   const FileEntry *Entry = FileMgr.getFile(Name, /*openFile=*/false,
                                            /*cacheFailure=*/false);
   return std::move(InMemoryBuffers[Entry]);
 }
ModuleManager::addModule(StringRef FileName, ModuleKind Type,
   }
 
   if (!Entry && FileName != "-") {
     ErrorStr = "module file not found";
     return Missing;
   }
 
   // Check whether we already loaded this module, before
-  ModuleFile *&ModuleEntry = Modules[Entry];
+  std::unique_ptr<ModuleFile> &ModuleEntry = Modules[Entry];
   bool NewModule = false;
   if (!ModuleEntry) {
     // Allocate a new module.
-    ModuleFile *New = new ModuleFile(Type, Generation);
+    ModuleEntry = llvm::make_unique<ModuleFile>(Type, Generation);
+    ModuleFile *New = ModuleEntry.get();
     New->Index = Chain.size();
     New->FileName = FileName.str();
     New->File = Entry;
     New->ImportLoc = ImportLoc;
     Chain.push_back(New);
     NewModule = true;
-    ModuleEntry = New;
 
     New->InputFilesValidationTimestamp = 0;
     if (New->Kind == MK_ImplicitModule) {
       std::string TimestampFilename = New->getTimestampFilename();
       vfs::Status Status;
       // A cached stat value would be fine as well.
       if (!FileMgr.getNoncachedStatValue(TimestampFilename, Status))
         New->InputFilesValidationTimestamp =
if (ExpectedSignature) {
         ErrorStr = New->Signature ? "signature mismatch"
                                   : "could not read module signature";
 
         // Remove the module file immediately, since removeModules might try to
         // invalidate the file cache for Entry, and that is not safe if this
         // module is *itself* up to date, but has an out-of-date importer.
         Modules.erase(Entry);
         Chain.pop_back();
-        delete New;
         return OutOfDate;
       }
     }
   }
   
   if (ImportedBy) {
     ModuleEntry->ImportedBy.insert(ImportedBy);
-    ImportedBy->Imports.insert(ModuleEntry);
+    ImportedBy->Imports.insert(ModuleEntry.get());
   } else {
     if (!ModuleEntry->DirectlyImported)
       ModuleEntry->ImportLoc = ImportLoc;
     
     ModuleEntry->DirectlyImported = true;
   }
 
-  Module = ModuleEntry;
-  return NewModule? NewlyLoaded : AlreadyLoaded;
+  Module = ModuleEntry.get();
+  return NewModule ? NewlyLoaded : AlreadyLoaded;
 }
 
 void ModuleManager::removeModules(
     ModuleIterator first, ModuleIterator last,
     llvm::SmallPtrSetImpl<ModuleFile *> &LoadedSuccessfully,
     ModuleMap *modMap) {
   if (first == last)
     return;
 
   // Collect the set of module file pointers that we'll be removing.
   llvm::SmallPtrSet<ModuleFile *, 4> victimSet(first, last);
 
   // Remove any references to the now-destroyed modules.
   for (unsigned i = 0, n = Chain.size(); i != n; ++i) {
     Chain[i]->ImportedBy.remove_if([&](ModuleFile *MF) {
       return victimSet.count(MF);
     });
   }
 
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = first; victim != last; ++victim) {
-    Modules.erase((*victim)->File);
-
     if (modMap) {
       StringRef ModuleName = (*victim)->ModuleName;
       if (Module *mod = modMap->findModule(ModuleName)) {
         mod->setASTFile(nullptr);
       }
     }
 
     // Files that didn't make it through ReadASTCore successfully will be
     // rebuilt (or there was an error). Invalidate them so that we can load the
     // new files that will be renamed over the old ones.
     if (LoadedSuccessfully.count(*victim) == 0)
       FileMgr.invalidateCache((*victim)->File);
 
-    delete *victim;
+    Modules.erase((*victim)->File);
   }
 
   // Remove the modules from the chain.
   Chain.erase(first, last);
 }
 
 void
 ModuleManager::addInMemoryBuffer(StringRef FileName,
void ModuleManager::moduleFileAccepted(ModuleFile *MF) {
 
   ModulesInCommonWithGlobalIndex.push_back(MF);
 }
 
 ModuleManager::ModuleManager(FileManager &FileMgr)
   : FileMgr(FileMgr), GlobalIndex(), FirstVisitState(nullptr) {}
 
 ModuleManager::~ModuleManager() {
-  for (unsigned i = 0, e = Chain.size(); i != e; ++i)
-    delete Chain[e - i - 1];
   delete FirstVisitState;
 }
 
 void
 ModuleManager::visit(bool (*Visitor)(ModuleFile &M, void *UserData),
                      void *UserData,
                      llvm::SmallPtrSetImpl<ModuleFile *> *ModuleFilesHit) {
   // If the visitation order vector is the wrong size, recompute the order.