[SharedCache] Misc cleanup

- Make `SharedCache::m_entries` a simple std::vector we never lookup based off the entry ID
- Remove some old comments
- Rename some old variables so that they are accurate
- Fix compiler warnings
- Remove some unneeded copying

Author: emesare

view/sharedcache/core/MachO.cpp

@@ -490,7 +490,7 @@ std::vector<CacheSymbol> SharedCacheMachOHeader::ReadSymbolTable(VirtualMemory&
 		{
 			// TODO: where logger?
 			LogError(
-				"Symbol entry at index %llu has a string offset of %u which is outside the strings buffer of size %u "
+				"Symbol entry at index %llu has a string offset of %u which is outside the strings buffer of size %llu "
 			    "for symbol table %x",
 				entryIndex, nlist.n_strx, stringInfo.address, stringInfo.entries);
 			continue;

view/sharedcache/core/ObjC.cpp

@@ -111,7 +111,7 @@ std::optional<ObjCOptimizationHeader> GetObjCOptimizationHeader(SharedCache& cac
 	// Find the first primary entry and use that header to read the obj opt header.
 	// Don't ask me why this is done like this...
 	std::optional<dyld_cache_header> primaryCacheHeader = std::nullopt;
-	for (const auto& [_, entry] : cache.GetEntries())
+	for (const auto& entry : cache.GetEntries())
 	{
 		if (entry.GetType() == CacheEntryType::Primary)
 		{

view/sharedcache/core/SharedCache.cpp

@@ -8,9 +8,6 @@
 
 using namespace BinaryNinja;
 
-// The next id to use when calling Cache::AddEntry
-static CacheEntryId nextId = 1;
-
 std::pair<std::string, Ref<Type>> CacheSymbol::DemangledName(BinaryView &view) const
 {
 	QualifiedName qname;
@@ -230,13 +227,8 @@ void SharedCache::AddSymbols(std::vector<CacheSymbol>&& symbols)
 		m_symbols.insert({symbol.address, std::move(symbol)});
 }
 
-CacheEntryId SharedCache::AddEntry(CacheEntry entry)
+void SharedCache::AddEntry(CacheEntry entry)
 {
-	// TODO: Maybe check to see if we already added the file?
-	// TODO: I doubt we will ever accidentally call this for the same entry...
-	// This is monotonically increasing so you can tell how many times we have called this function :)
-	CacheEntryId id = nextId++;
-
 	// Get the file accessor to associate with the virtual memory region.
 	auto fileAccessor = FileAccessorCache::Global().Open(entry.GetFilePath());
 
@@ -253,8 +245,7 @@ CacheEntryId SharedCache::AddEntry(CacheEntry entry)
 	}
 
 	// We are done and can make the entry visible to the entire cache.
-	m_entries.insert({id, std::move(entry)});
-	return id;
+	m_entries.push_back(std::move(entry));
 }
 
 bool SharedCache::ProcessEntryImage(const std::string& path, const dyld_cache_image_info& info)
@@ -330,7 +321,7 @@ void SharedCache::ProcessEntryImages(const CacheEntry& entry)
 // At this point all relevant mapping should be loaded in the virtual memory.
 void SharedCache::ProcessEntryRegions(const CacheEntry& entry)
 {
-	auto entryHeader = entry.GetHeader();
+	const auto& entryHeader = entry.GetHeader();
 
 	// Collect pool addresses as non image memory regions.
 	for (size_t i = 0; i < entryHeader.branchPoolsCount; i++)
@@ -468,7 +459,7 @@ void SharedCache::ProcessSymbols()
 
 std::optional<CacheEntry> SharedCache::GetEntryContaining(const uint64_t address) const
 {
-	for (const auto& [_, entry] : m_entries)
+	for (const auto& entry : m_entries)
 	{
 		for (const auto& mapping : entry.GetMappings())
 		{
@@ -482,7 +473,7 @@ std::optional<CacheEntry> SharedCache::GetEntryContaining(const uint64_t address
 
 std::optional<CacheEntry> SharedCache::GetEntryWithImage(const CacheImage& image) const
 {
-	for (const auto& [_, entry] : m_entries)
+	for (const auto& entry : m_entries)
 	{
 		for (const auto& [_, currentImage] : entry.GetImages())
 		{

view/sharedcache/core/SharedCache.h

@@ -106,7 +106,6 @@ enum class CacheEntryType
 	Primary,
 	Secondary,
 	// A special entry that holds symbols for other cache entries.
-	// TODO: We dont need this i think.
 	Symbols,
 	// If the type is marked as this then all mappings will be marked as such.
 	DyldData,
@@ -164,8 +163,6 @@ class CacheEntry
 // The ID for a given CacheEntry, use this instead of passing a pointer around to avoid complexity :V
 typedef uint32_t CacheEntryId;
 
-// TODO: Add a "ViewCache" that keeps track of what has been added to the view.
-
 // The C in DSC.
 // This represents the entire cache, all regions and images are visible from here.
 // This is the dump for all the information, and what the workflow activities and the UI want.
@@ -175,10 +172,9 @@ class SharedCache
 {
 	// Calculated within `AddEntry`, this indicates where the shared cache image is based at.
 	uint64_t m_baseAddress = 0;
-	// TODO: Figure out when to lock the mutex on this shit lmfao
 	// The shared cache can own the virtual memory, this is fine...
 	std::shared_ptr<VirtualMemory> m_vm;
-	std::unordered_map<CacheEntryId, CacheEntry> m_entries {};
+	std::vector<CacheEntry> m_entries {};
 	// This information is used in tandem with the cache images to load memory regions into the binary view.
 	AddressRangeMap<CacheRegion> m_regions {};
 	// Describes the images of the cache.
@@ -210,7 +206,7 @@ class SharedCache
 
 	uint64_t GetBaseAddress() const { return m_baseAddress; }
 	std::shared_ptr<VirtualMemory> GetVirtualMemory() const { return m_vm; }
-	const std::unordered_map<CacheEntryId, CacheEntry>& GetEntries() const { return m_entries; }
+	const std::vector<CacheEntry>& GetEntries() const { return m_entries; }
 	const AddressRangeMap<CacheRegion>& GetRegions() const { return m_regions; }
 	const std::unordered_map<uint64_t, CacheImage>& GetImages() const { return m_images; }
 	const std::unordered_map<uint64_t, CacheSymbol>& GetSymbols() const { return m_symbols; }
@@ -226,7 +222,7 @@ class SharedCache
 
 	// Adds the cache entry and populates the virtual memory using the mapping information.
 	// After being added the entry is read only, there is nothing that can modify it.
-	CacheEntryId AddEntry(CacheEntry entry);
+	void AddEntry(CacheEntry entry);
 
 	void ProcessEntryImages(const CacheEntry& entry);
 

view/sharedcache/core/SharedCacheController.cpp

@@ -66,17 +66,17 @@ DSCRef<SharedCacheController> SharedCacheController::Initialize(BinaryView& view
 	auto id = GetViewIdFromView(view);
 	std::unique_lock<std::shared_mutex> lock(GlobalControllersMutex);
 	auto logger = new Logger("SharedCache.Controller", view.GetFile()->GetSessionId());
-	DSCRef<SharedCacheController> dscView = new SharedCacheController(std::move(cache), logger);
+	DSCRef<SharedCacheController> controller = new SharedCacheController(std::move(cache), logger);
 
 	// Pull the settings from the view.
 	if (Ref<Settings> settings = view.GetLoadSettings(VIEW_NAME))
 	{
 		if (settings->Contains("loader.dsc.processObjC"))
-			dscView->m_processObjC = settings->Get<bool>("loader.dsc.processObjC", &view);
+			controller->m_processObjC = settings->Get<bool>("loader.dsc.processObjC", &view);
 		if (settings->Contains("loader.dsc.processCFStrings"))
-			dscView->m_processCFStrings = settings->Get<bool>("loader.dsc.processCFStrings", &view);
+			controller->m_processCFStrings = settings->Get<bool>("loader.dsc.processCFStrings", &view);
 		if (settings->Contains("loader.dsc.regionFilter"))
-			dscView->m_regionFilter = std::regex(settings->Get<std::string>("loader.dsc.regionFilter", &view));
+			controller->m_regionFilter = std::regex(settings->Get<std::string>("loader.dsc.regionFilter", &view));
 	}
 
 	// TODO: Support old shared cache metadata
@@ -88,10 +88,10 @@ DSCRef<SharedCacheController> SharedCacheController::Initialize(BinaryView& view
 	// This effectively restores the state of the opened database to when it was last saved.
 	// NOTE: We store on the parent view because hilariously, the metadata is not present until after view init.
 	if (const auto metadata = view.GetParentView()->QueryMetadata(METADATA_KEY))
-		dscView->LoadMetadata(*metadata);
+		controller->LoadMetadata(*metadata);
 
-	GlobalControllers().insert({id, dscView});
-	return dscView;
+	GlobalControllers().insert({id, controller});
+	return controller;
 }
 
 DSCRef<SharedCacheController> SharedCacheController::FromView(BinaryView& view)

view/sharedcache/core/SharedCacheView.cpp

@@ -912,7 +912,7 @@ bool SharedCacheView::InitController()
 		// Write all the slide info pointers to the virtual memory.
 		// This should be done before any other work begins, as the backing data will be altered by this process.
 		auto startTime = std::chrono::high_resolution_clock::now();
-		for (const auto& [_, entry] : sharedCache.GetEntries())
+		for (const auto& entry : sharedCache.GetEntries())
 			sharedCache.ProcessEntrySlideInfo(entry);
 		auto endTime = std::chrono::high_resolution_clock::now();
 		std::chrono::duration<double> elapsed = endTime - startTime;
@@ -923,7 +923,7 @@ bool SharedCacheView::InitController()
 		// Load up all images into the cache before adding extra regions.
 		// Currently, it is expected that a primary entry contain all relevant images, so we only check that one.
 		auto startTime = std::chrono::high_resolution_clock::now();
-		for (const auto& [_, entry] : sharedCache.GetEntries())
+		for (const auto& entry : sharedCache.GetEntries())
 			if (entry.GetType() == CacheEntryType::Primary)
 				sharedCache.ProcessEntryImages(entry);
 		auto endTime = std::chrono::high_resolution_clock::now();
@@ -936,10 +936,9 @@ bool SharedCacheView::InitController()
 	}
 
 	{
-		// TODO: Run this up on a separate thread maybe and have callback notifications?
 		// Load up all the regions into the cache.
 		auto startTime = std::chrono::high_resolution_clock::now();
-		for (const auto& [_, entry] : sharedCache.GetEntries())
+		for (const auto& entry : sharedCache.GetEntries())
 			sharedCache.ProcessEntryRegions(entry);
 		auto endTime = std::chrono::high_resolution_clock::now();
 		std::chrono::duration<double> elapsed = endTime - startTime;

view/sharedcache/core/SlideInfo.cpp

@@ -158,7 +158,7 @@ void ApplySlideInfoV2(MappedFileAccessor& accessor, const SlideMappingInfo& mapp
 
 std::vector<SlideMappingInfo> SlideInfoProcessor::ReadEntryInfo(const MappedFileAccessor& accessor, const CacheEntry& entry) const
 {
-	auto& baseHeader = entry.GetHeader();
+	const auto& baseHeader = entry.GetHeader();
 
 	// Handle legacy, single mapping slide info.
 	if (baseHeader.slideInfoOffsetUnused)