Merge pull request #26816 from jrose-apple/cant-stand-the-rain

Fix a pair of leaks related to FileUnit destructors not being run

Author: jrose-apple

include/swift/AST/Module.h

@@ -644,7 +644,10 @@ static inline unsigned alignOfFileUnit();
 /// FileUnit is an abstract base class; its subclasses represent different
 /// sorts of containers that can each provide a set of decls, e.g. a source
 /// file. A module can contain several file-units.
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wnon-virtual-dtor"
 class FileUnit : public DeclContext {
+#pragma clang diagnostic pop
   virtual void anchor();
 
   // FIXME: Stick this in a PointerIntPair.
@@ -655,8 +658,6 @@ class FileUnit : public DeclContext {
     : DeclContext(DeclContextKind::FileUnit, &M), Kind(kind) {
   }
 
-  virtual ~FileUnit() = default;
-
 public:
   FileUnitKind getKind() const {
     return Kind;
@@ -874,10 +875,6 @@ class FileUnit : public DeclContext {
     return getParentModule()->getName().str();
   }
 
-  /// If this is a module imported from a parseable interface, return the path
-  /// to the interface file, otherwise an empty StringRef.
-  virtual StringRef getParseableInterface() const { return {}; }
-
   /// Traverse the decls within this file.
   ///
   /// \returns true if traversal was aborted, false if it completed
@@ -897,13 +894,7 @@ class FileUnit : public DeclContext {
   // Make placement new and vanilla new/delete illegal for FileUnits.
   void *operator new(size_t Bytes) throw() = delete;
   void *operator new(size_t Bytes, void *Mem) throw() = delete;
-
-protected:
-  // Unfortunately we can't remove this altogether because the virtual
-  // destructor requires it to be accessible.
-  void operator delete(void *Data) throw() {
-    llvm_unreachable("Don't use operator delete on a SourceFile");
-  }
+  void operator delete(void *Data) throw() = delete;
 
 public:
   // Only allow allocation of FileUnits using the allocator in ASTContext
@@ -1386,35 +1377,23 @@ class BuiltinUnit final : public FileUnit {
 };
 
 /// Represents an externally-loaded file of some kind.
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wnon-virtual-dtor"
 class LoadedFile : public FileUnit {
+#pragma clang diagnostic pop
 protected:
-  ~LoadedFile() = default;
   LoadedFile(FileUnitKind Kind, ModuleDecl &M) noexcept
     : FileUnit(Kind, M) {
     assert(classof(this) && "invalid kind");
   }
-
-  /// A map from private/fileprivate decls to the file they were defined in.
-  llvm::DenseMap<const ValueDecl *, Identifier> FilenameForPrivateDecls;
-
 public:
-
   /// Returns an arbitrary string representing the storage backing this file.
   ///
   /// This is usually a filesystem path.
   virtual StringRef getFilename() const;
 
-  void addFilenameForPrivateDecl(const ValueDecl *decl, Identifier id) {
-    assert(!FilenameForPrivateDecls.count(decl) ||
-           FilenameForPrivateDecls[decl] == id);
-    FilenameForPrivateDecls[decl] = id;
-  }
-
-  StringRef getFilenameForPrivateDecl(const ValueDecl *decl) {
-    auto it = FilenameForPrivateDecls.find(decl);
-    if (it == FilenameForPrivateDecls.end())
-      return StringRef();
-    return it->second.str();
+  virtual StringRef getFilenameForPrivateDecl(const ValueDecl *decl) const {
+    return StringRef();
   }
 
   /// Look up an operator declaration.

include/swift/ClangImporter/ClangModule.h

@@ -39,8 +39,6 @@ class ClangModuleUnit final : public LoadedFile {
   /// The metadata of the underlying Clang module.
   clang::ExternalASTSource::ASTSourceDescriptor ASTSourceDescriptor;
 
-  ~ClangModuleUnit() = default;
-
 public:
   /// True if the given Module contains an imported Clang module unit.
   static bool hasClangModule(ModuleDecl *M);

include/swift/Serialization/ModuleFile.h

@@ -78,6 +78,11 @@ class ModuleFile
   /// The target the module was built for.
   StringRef TargetTriple;
 
+  /// The name of the module interface this module was compiled from.
+  ///
+  /// Empty if this module didn't come from an interface file.
+  StringRef ModuleInterfacePath;
+
   /// The Swift compatibility version in use when this module was built.
   version::Version CompatibilityVersion;
 
@@ -391,6 +396,7 @@ class ModuleFile
   std::unique_ptr<SerializedObjCMethodTable> ObjCMethods;
 
   llvm::DenseMap<const ValueDecl *, Identifier> PrivateDiscriminatorsByValue;
+  llvm::DenseMap<const ValueDecl *, StringRef> FilenamesForPrivateValues;
 
   TinyPtrVector<Decl *> ImportDecls;
 
@@ -762,6 +768,8 @@ class ModuleFile
   void getDisplayDecls(SmallVectorImpl<Decl*> &results);
 
   StringRef getModuleFilename() const {
+    if (!ModuleInterfacePath.empty())
+      return ModuleInterfacePath;
     // FIXME: This seems fragile, maybe store the filename separately ?
     return ModuleInputBuffer->getBufferIdentifier();
   }

include/swift/Serialization/SerializedModuleLoader.h

@@ -251,14 +251,8 @@ class SerializedASTFile final : public LoadedFile {
 
   ModuleFile &File;
 
-  /// The parseable interface this module was generated from if any.
-  /// Used for debug info.
-  std::string ParseableInterface;
-
   bool IsSIB;
 
-  ~SerializedASTFile() = default;
-
   SerializedASTFile(ModuleDecl &M, ModuleFile &file, bool isSIB = false)
     : LoadedFile(FileUnitKind::SerializedAST, M), File(file), IsSIB(isSIB) {}
 
@@ -278,6 +272,9 @@ class SerializedASTFile final : public LoadedFile {
                            DeclName name, NLKind lookupKind,
                            SmallVectorImpl<ValueDecl*> &results) const override;
 
+  virtual StringRef
+  getFilenameForPrivateDecl(const ValueDecl *decl) const override;
+
   virtual TypeDecl *lookupLocalType(StringRef MangledName) const override;
 
   virtual OpaqueTypeDecl *
@@ -351,13 +348,6 @@ class SerializedASTFile final : public LoadedFile {
 
   virtual const clang::Module *getUnderlyingClangModule() const override;
 
-  /// If this is a module imported from a parseable interface, return the path
-  /// to the interface file, otherwise an empty StringRef.
-  virtual StringRef getParseableInterface() const override {
-    return ParseableInterface;
-  }
-  void setParseableInterface(StringRef PI) { ParseableInterface = PI; }
-  
   virtual bool getAllGenericSignatures(
                    SmallVectorImpl<GenericSignature*> &genericSignatures)
                 override;

include/swift/Serialization/Validation.h

@@ -92,7 +92,6 @@ struct ValidationInfo {
 class ExtendedValidationInfo {
   SmallVector<StringRef, 4> ExtraClangImporterOpts;
   StringRef SDKPath;
-  StringRef ParseableInterface;
   struct {
     unsigned ArePrivateImportsEnabled : 1;
     unsigned IsSIB : 1;
@@ -114,8 +113,6 @@ class ExtendedValidationInfo {
   void addExtraClangImporterOption(StringRef option) {
     ExtraClangImporterOpts.push_back(option);
   }
-  StringRef getParseableInterface() const { return ParseableInterface; }
-  void setParseableInterface(StringRef PI) { ParseableInterface = PI; }
 
   bool isSIB() const { return Bits.IsSIB; }
   void setIsSIB(bool val) {

lib/AST/Module.cpp

@@ -55,6 +55,11 @@
 
 using namespace swift;
 
+static_assert(IsTriviallyDestructible<FileUnit>::value,
+              "FileUnits are BumpPtrAllocated; the d'tor may not be called");
+static_assert(IsTriviallyDestructible<LoadedFile>::value,
+              "LoadedFiles are BumpPtrAllocated; the d'tor may not be called");
+
 //===----------------------------------------------------------------------===//
 // Builtin Module Name lookup
 //===----------------------------------------------------------------------===//
@@ -1249,10 +1254,6 @@ StringRef ModuleDecl::getModuleFilename() const {
   // per-file names. Modules can consist of more than one file.
   StringRef Result;
   for (auto F : getFiles()) {
-    Result = F->getParseableInterface();
-    if (!Result.empty())
-      return Result;
-
     if (auto SF = dyn_cast<SourceFile>(F)) {
       if (!Result.empty())
         return StringRef();

lib/ClangImporter/ClangImporter.cpp

@@ -3168,6 +3168,9 @@ void ClangImporter::verifyAllModules() {
 // ClangModule Implementation
 //===----------------------------------------------------------------------===//
 
+static_assert(IsTriviallyDestructible<ClangModuleUnit>::value,
+              "ClangModuleUnits are BumpPtrAllocated; the d'tor is not called");
+
 ClangModuleUnit::ClangModuleUnit(ModuleDecl &M,
                                  ClangImporter::Implementation &owner,
                                  const clang::Module *clangModule)

lib/ClangImporter/DWARFImporter.cpp

@@ -20,7 +20,6 @@ void DWARFImporterDelegate::anchor() {}
 /// Represents a Clang module that was "imported" from debug info. Since all the
 /// loading of types is done on demand, this class is effectively empty.
 class DWARFModuleUnit final : public LoadedFile {
-  ~DWARFModuleUnit() = default;
   ClangImporter::Implementation &Owner;
 
 public:
@@ -96,6 +95,9 @@ class DWARFModuleUnit final : public LoadedFile {
   }
 };
 
+static_assert(IsTriviallyDestructible<DWARFModuleUnit>::value,
+              "DWARFModuleUnits are BumpPtrAllocated; the d'tor is not called");
+
 ModuleDecl *ClangImporter::Implementation::loadModuleDWARF(
     SourceLoc importLoc, ArrayRef<std::pair<Identifier, SourceLoc>> path) {
   // There's no importing from debug info if no importer is installed.

lib/FrontendTool/FrontendTool.cpp

@@ -268,6 +268,8 @@ static bool emitLoadedModuleTraceIfNeeded(ModuleDecl *mainModule,
   for (auto &module : ctxt.LoadedModules) {
     ModuleDecl *loadedDecl = module.second;
     assert(loadedDecl && "Expected loaded module to be non-null.");
+    if (loadedDecl == mainModule)
+      continue;
     assert(!loadedDecl->getModuleFilename().empty()
            && "Don't know how to handle modules with empty names.");
     pathToModuleDecl.insert(

lib/Serialization/Deserialization.cpp

@@ -2254,7 +2254,7 @@ class swift::DeclDeserializer {
 
   Identifier privateDiscriminator;
   unsigned localDiscriminator = 0;
-  Identifier filenameForPrivate;
+  StringRef filenameForPrivate;
 
   void AddAttribute(DeclAttribute *Attr) {
     // Advance the linked list.
@@ -2308,10 +2308,8 @@ class swift::DeclDeserializer {
       if (localDiscriminator != 0)
         value->setLocalDiscriminator(localDiscriminator);
 
-      if (!filenameForPrivate.empty()) {
-        auto *loadedFile = cast<LoadedFile>(MF.getFile());
-        loadedFile->addFilenameForPrivateDecl(value, filenameForPrivate);
-      }
+      if (!filenameForPrivate.empty())
+        MF.FilenamesForPrivateValues[value] = filenameForPrivate;
     }
 
     decl->setValidationToChecked();
@@ -4202,7 +4200,7 @@ llvm::Error DeclDeserializer::deserializeDeclAttributes() {
     } else if (recordID == decls_block::FILENAME_FOR_PRIVATE) {
       IdentifierID filenameID;
       decls_block::FilenameForPrivateLayout::readRecord(scratch, filenameID);
-      filenameForPrivate = MF.getIdentifier(filenameID);
+      filenameForPrivate = MF.getIdentifierText(filenameID);
     } else {
       return llvm::Error::success();
     }

lib/Serialization/ModuleFile.cpp

@@ -38,6 +38,9 @@ using namespace swift::serialization;
 using namespace llvm::support;
 using llvm::Expected;
 
+static_assert(IsTriviallyDestructible<SerializedASTFile>::value,
+              "SerializedASTFiles are BumpPtrAllocated; d'tors are not called");
+
 static bool checkModuleSignature(llvm::BitstreamCursor &cursor,
                                  ArrayRef<unsigned char> signature) {
   for (unsigned char byte : signature)
@@ -1352,8 +1355,7 @@ ModuleFile::ModuleFile(
           break;
         }
         case input_block::PARSEABLE_INTERFACE_PATH: {
-          if (extInfo)
-            extInfo->setParseableInterface(blobData);
+          ModuleInterfacePath = blobData;
           break;
         }
         default:

lib/Serialization/SerializedModuleLoader.cpp

@@ -548,7 +548,6 @@ FileUnit *SerializedModuleLoaderBase::loadAST(
     // We've loaded the file. Now try to bring it into the AST.
     auto fileUnit = new (Ctx) SerializedASTFile(M, *loadedModuleFile,
                                                 extendedInfo.isSIB());
-    fileUnit->setParseableInterface(extendedInfo.getParseableInterface());
     M.addFile(*fileUnit);
     if (extendedInfo.isTestable())
       M.setTestingEnabled();
@@ -956,6 +955,11 @@ void SerializedASTFile::lookupValue(ModuleDecl::AccessPathTy accessPath,
   File.lookupValue(name, results);
 }
 
+StringRef
+SerializedASTFile::getFilenameForPrivateDecl(const ValueDecl *decl) const {
+  return File.FilenamesForPrivateValues.lookup(decl);
+}
+
 TypeDecl *SerializedASTFile::lookupLocalType(llvm::StringRef MangledName) const{
   return File.lookupLocalType(MangledName);
 }