[llvm] 7d67a1e - [dsymutil] Fix memory issue in the BinaryHolder

[Jonas Devlieghere via llvm-commits]

Author: Jonas Devlieghere
Date: 2022-04-27T16:35:48-07:00
New Revision: 7d67a1e45a04854506b1d14f219c643489d6d78e

URL: https://github.com/llvm/llvm-project/commit/7d67a1e45a04854506b1d14f219c643489d6d78e
DIFF: https://github.com/llvm/llvm-project/commit/7d67a1e45a04854506b1d14f219c643489d6d78e.diff

LOG: [dsymutil] Fix memory issue in the BinaryHolder

The BinaryHolder has two caches for object and archive entries. These
are implemented as StringMaps of ObjectEntry and ArchiveEntry
respectively. The fact that they're stored by value is problematic
because the BinaryHolder hands out references that become invalidate
when the data structure grows. This patch wraps those object instances
in unique pointers and changes the interface to hand out pointers. This
resulted in transient failures.

rdar://90412671

Differential revision: https://reviews.llvm.org/D124567

Added: 
    

Modified: 
    llvm/tools/dsymutil/BinaryHolder.cpp
    llvm/tools/dsymutil/BinaryHolder.h

Removed: 
    


################################################################################
diff  --git a/llvm/tools/dsymutil/BinaryHolder.cpp b/llvm/tools/dsymutil/BinaryHolder.cpp
index f83521346c5b7..d16e1d5de22aa 100644
--- a/llvm/tools/dsymutil/BinaryHolder.cpp
+++ b/llvm/tools/dsymutil/BinaryHolder.cpp
@@ -168,20 +168,17 @@ BinaryHolder::ArchiveEntry::getObjectEntry(StringRef Filename,
   StringRef ArchiveFilename;
   StringRef ObjectFilename;
   std::tie(ArchiveFilename, ObjectFilename) = getArchiveAndObjectName(Filename);
-
-  // Try the cache first.
   KeyTy Key = {ObjectFilename, Timestamp};
 
-  {
-    std::lock_guard<std::mutex> Lock(MemberCacheMutex);
-    if (MemberCache.count(Key))
-      return MemberCache[Key];
-  }
+  // Try the cache first.
+  std::lock_guard<std::mutex> Lock(MemberCacheMutex);
+  if (MemberCache.count(Key))
+    return *MemberCache[Key].get();
 
   // Create a new ObjectEntry, but don't add it to the cache yet. Loading of
   // the archive members might fail and we don't want to lock the whole archive
   // during this operation.
-  ObjectEntry OE;
+  auto OE = std::make_unique<ObjectEntry>();
 
   for (const auto &Archive : Archives) {
     Error Err = Error::success();
@@ -216,7 +213,7 @@ BinaryHolder::ArchiveEntry::getObjectEntry(StringRef Filename,
           if (!ErrOrObjectFile)
             return ErrOrObjectFile.takeError();
 
-          OE.Objects.push_back(std::move(*ErrOrObjectFile));
+          OE->Objects.push_back(std::move(*ErrOrObjectFile));
         }
       }
     }
@@ -224,12 +221,11 @@ BinaryHolder::ArchiveEntry::getObjectEntry(StringRef Filename,
       return std::move(Err);
   }
 
-  if (OE.Objects.empty())
+  if (OE->Objects.empty())
     return errorCodeToError(errc::no_such_file_or_directory);
 
-  std::lock_guard<std::mutex> Lock(MemberCacheMutex);
-  MemberCache.try_emplace(Key, std::move(OE));
-  return MemberCache[Key];
+  MemberCache[Key] = std::move(OE);
+  return *MemberCache[Key];
 }
 
 Expected<const BinaryHolder::ObjectEntry &>
@@ -243,18 +239,18 @@ BinaryHolder::getObjectEntry(StringRef Filename, TimestampTy Timestamp) {
     StringRef ArchiveFilename = getArchiveAndObjectName(Filename).first;
     std::lock_guard<std::mutex> Lock(ArchiveCacheMutex);
     if (ArchiveCache.count(ArchiveFilename)) {
-      return ArchiveCache[ArchiveFilename].getObjectEntry(Filename, Timestamp,
-                                                          Verbose);
+      return ArchiveCache[ArchiveFilename]->getObjectEntry(Filename, Timestamp,
+                                                           Verbose);
     } else {
-      ArchiveEntry &AE = ArchiveCache[ArchiveFilename];
-      auto Err = AE.load(VFS, Filename, Timestamp, Verbose);
+      auto AE = std::make_unique<ArchiveEntry>();
+      auto Err = AE->load(VFS, Filename, Timestamp, Verbose);
       if (Err) {
-        ArchiveCache.erase(ArchiveFilename);
         // Don't return the error here: maybe the file wasn't an archive.
         llvm::consumeError(std::move(Err));
       } else {
-        return ArchiveCache[ArchiveFilename].getObjectEntry(Filename, Timestamp,
-                                                            Verbose);
+        ArchiveCache[ArchiveFilename] = std::move(AE);
+        return ArchiveCache[ArchiveFilename]->getObjectEntry(
+            Filename, Timestamp, Verbose);
       }
     }
   }
@@ -263,15 +259,14 @@ BinaryHolder::getObjectEntry(StringRef Filename, TimestampTy Timestamp) {
   // it from the file system and cache it now.
   std::lock_guard<std::mutex> Lock(ObjectCacheMutex);
   if (!ObjectCache.count(Filename)) {
-    ObjectEntry &OE = ObjectCache[Filename];
-    auto Err = OE.load(VFS, Filename, Timestamp, Verbose);
-    if (Err) {
-      ObjectCache.erase(Filename);
+    auto OE = std::make_unique<ObjectEntry>();
+    auto Err = OE->load(VFS, Filename, Timestamp, Verbose);
+    if (Err)
       return std::move(Err);
-    }
+    ObjectCache[Filename] = std::move(OE);
   }
 
-  return ObjectCache[Filename];
+  return *ObjectCache[Filename];
 }
 
 void BinaryHolder::clear() {

diff  --git a/llvm/tools/dsymutil/BinaryHolder.h b/llvm/tools/dsymutil/BinaryHolder.h
index 6245e49247337..687f0e327111e 100644
--- a/llvm/tools/dsymutil/BinaryHolder.h
+++ b/llvm/tools/dsymutil/BinaryHolder.h
@@ -118,7 +118,7 @@ class BinaryHolder {
 
   private:
     std::vector<std::unique_ptr<object::Archive>> Archives;
-    DenseMap<KeyTy, ObjectEntry> MemberCache;
+    DenseMap<KeyTy, std::unique_ptr<ObjectEntry>> MemberCache;
     std::mutex MemberCacheMutex;
   };
 
@@ -130,11 +130,11 @@ class BinaryHolder {
 private:
   /// Cache of static archives. Objects that are part of a static archive are
   /// stored under this object, rather than in the map below.
-  StringMap<ArchiveEntry> ArchiveCache;
+  StringMap<std::unique_ptr<ArchiveEntry>> ArchiveCache;
   std::mutex ArchiveCacheMutex;
 
   /// Object entries for objects that are not in a static archive.
-  StringMap<ObjectEntry> ObjectCache;
+  StringMap<std::unique_ptr<ObjectEntry>> ObjectCache;
   std::mutex ObjectCacheMutex;
 
   /// Virtual File System instance.