[SharedCache] Prevent some unneeded copies when processing

Also swapped out CacheEntry::m_images to a vector as its never actually used as a lookup

Author: emesare

view/sharedcache/core/MachO.cpp

@@ -556,17 +556,17 @@ std::vector<CacheSymbol> SharedCacheMachOHeader::ReadSymbolTable(BinaryView& vie
 	return symbolList;
 }
 
-std::optional<CacheSymbol> SharedCacheMachOHeader::AddExportTerminalSymbol(
-	const std::string& symbolName, const uint8_t* current, const uint8_t* end) const
+bool SharedCacheMachOHeader::AddExportTerminalSymbol(
+	std::vector<CacheSymbol>& symbols, const std::string& symbolName, const uint8_t *current, const uint8_t *end) const
 {
 	uint64_t symbolFlags = readValidULEB128(current, end);
 	if (symbolFlags & EXPORT_SYMBOL_FLAGS_REEXPORT)
-		return std::nullopt;
+		return false;
 
 	uint64_t imageOffset = readValidULEB128(current, end);
 	uint64_t symbolAddress = textBase + imageOffset;
 	if (symbolName.empty() || symbolAddress == 0)
-		return std::nullopt;
+		return false;
 
 	// Tries to get the symbol type based off the section containing it.
 	auto sectionSymbolType = [&]() -> BNSymbolType {
@@ -595,13 +595,17 @@ std::optional<CacheSymbol> SharedCacheMachOHeader::AddExportTerminalSymbol(
 	{
 	case EXPORT_SYMBOL_FLAGS_KIND_REGULAR:
 	case EXPORT_SYMBOL_FLAGS_KIND_THREAD_LOCAL:
-		return CacheSymbol(sectionSymbolType(), symbolAddress, symbolName);
+		symbols.emplace_back(sectionSymbolType(), symbolAddress, symbolName);
+		break;
 	case EXPORT_SYMBOL_FLAGS_KIND_ABSOLUTE:
-		return CacheSymbol(DataSymbol, symbolAddress, symbolName);
+		symbols.emplace_back(DataSymbol, symbolAddress, symbolName);
+		break;
 	default:
 		LogWarn("Unhandled export symbol kind: %llx", symbolFlags & EXPORT_SYMBOL_FLAGS_KIND_MASK);
-		return std::nullopt;
+		return false;
 	}
+
+	return true;
 }
 
 // TODO: This is like 90% of the runtime.
@@ -616,12 +620,7 @@ bool SharedCacheMachOHeader::ProcessLinkEditTrie(std::vector<CacheSymbol>& symbo
 
 	// The terminal is an export symbol.
 	if (terminalSize != 0)
-	{
-		// Add the export symbol is applicable.
-		auto symbol = AddExportTerminalSymbol(currentText, current, end);
-		if (symbol.has_value())
-			symbols.push_back(*symbol);
-	}
+		AddExportTerminalSymbol(symbols, currentText, current, end);
 
 	// TODO: Make this look better
 	current = child;

view/sharedcache/core/MachO.h

@@ -64,8 +64,9 @@ struct SharedCacheMachOHeader
 	// TODO: Replace view with address size?
 	std::vector<CacheSymbol> ReadSymbolTable(BinaryNinja::BinaryView& view, VirtualMemory& vm) const;
 
-	std::optional<CacheSymbol> AddExportTerminalSymbol(
-		const std::string& symbolName, const uint8_t* current, const uint8_t* end) const;
+	bool AddExportTerminalSymbol(
+		std::vector<CacheSymbol>& symbols, const std::string& symbolName, const uint8_t* current,
+		const uint8_t* end) const;
 
 	bool ProcessLinkEditTrie(std::vector<CacheSymbol>& symbols, const std::string& currentText, const uint8_t* begin,
 		const uint8_t* current, const uint8_t* end) const;

view/sharedcache/core/SharedCache.cpp

@@ -29,7 +29,7 @@ std::vector<std::string> CacheImage::GetDependencies() const
 }
 
 CacheEntry::CacheEntry(std::string filePath, std::string fileName, CacheEntryType type, dyld_cache_header header,
-	std::vector<dyld_cache_mapping_info> mappings, std::unordered_map<std::string, dyld_cache_image_info> images)
+	std::vector<dyld_cache_mapping_info> mappings, std::vector<std::pair<std::string, dyld_cache_image_info>> images)
 {
 	m_filePath = std::move(filePath);
 	m_fileName = std::move(fileName);
@@ -88,14 +88,15 @@ std::optional<CacheEntry> CacheEntry::FromFile(const std::string& filePath, cons
 	}
 
 	// Gather all images for the entry.
-	std::unordered_map<std::string, dyld_cache_image_info> images;
+	std::vector<std::pair<std::string, dyld_cache_image_info>> images;
+	images.reserve(header.imagesCountOld ? header.imagesCountOld : header.imagesCount);
 	dyld_cache_image_info currentImg {};
 	for (size_t i = 0; i < header.imagesCount; i++)
 	{
 		file->Read(
 			&currentImg, header.imagesOffset + (i * sizeof(dyld_cache_image_info)), sizeof(dyld_cache_image_info));
 		auto imagePath = file->ReadNullTermString(currentImg.pathFileOffset);
-		images.insert_or_assign(imagePath, currentImg);
+		images.emplace_back(imagePath, currentImg);
 	}
 
 	// Handle old dyld format that uses old images field.
@@ -104,7 +105,7 @@ std::optional<CacheEntry> CacheEntry::FromFile(const std::string& filePath, cons
 		file->Read(
 			&currentImg, header.imagesOffsetOld + (i * sizeof(dyld_cache_image_info)), sizeof(dyld_cache_image_info));
 		auto imagePath = file->ReadNullTermString(currentImg.pathFileOffset);
-		images.insert_or_assign(imagePath, currentImg);
+		images.emplace_back(imagePath, currentImg);
 	}
 
 	// NOTE: I am not sure how the header type has changed over time but if apple is replacing fields with other ones
@@ -119,7 +120,7 @@ std::optional<CacheEntry> CacheEntry::FromFile(const std::string& filePath, cons
 		branchIslandImg.address = header.branchPoolsOffset + (i * sizeof(uint64_t));
 		// Mason: why such a long name for the image???
 		auto imageName = fmt::format("dyld_shared_cache_branch_islands_{}", i);
-		images.insert_or_assign(imageName, branchIslandImg);
+		images.emplace_back(imageName, branchIslandImg);
 	}
 
 	return CacheEntry(filePath, fileName, type, header, mappings, images);
@@ -151,12 +152,12 @@ SharedCache::SharedCache(uint64_t addressSize)
 }
 
 
-void SharedCache::AddImage(CacheImage image)
+void SharedCache::AddImage(CacheImage&& image)
 {
 	m_images.insert({image.headerAddress, std::move(image)});
 }
 
-void SharedCache::AddRegion(CacheRegion region)
+void SharedCache::AddRegion(CacheRegion&& region)
 {
 	// Handle overlapping regions here.
 	const auto regionRange = region.AsAddressRange();
@@ -203,8 +204,8 @@ void SharedCache::AddSymbol(CacheSymbol symbol)
 
 void SharedCache::AddSymbols(std::vector<CacheSymbol>&& symbols)
 {
-	for (auto&& symbol : symbols)
-		m_symbols.emplace(symbol.address, std::move(symbol));
+	for (auto& symbol : symbols)
+		m_symbols.insert({symbol.address, std::move(symbol)});
 }
 
 CacheEntryId SharedCache::AddEntry(CacheEntry entry)
@@ -281,18 +282,17 @@ bool SharedCache::ProcessEntryImage(const std::string& path, const dyld_cache_im
 				flags |= SegmentExecutable;
 		sectionRegion.flags = static_cast<BNSegmentFlag>(flags);
 
-		// Add the image section to the cache and also to the image region starts
-		AddRegion(sectionRegion);
 		image.regionStarts.push_back(sectionRegion.start);
+		// Add the image section to the cache and also to the image region starts
+		AddRegion(std::move(sectionRegion));
 	}
 
 	// Add the exported symbols to the available symbols.
 	std::vector<CacheSymbol> exportSymbols = imageHeader->ReadExportSymbolTrie(*m_vm);
 	AddSymbols(std::move(exportSymbols));
 
 	// This is behind a shared pointer as the header itself is very large.
-	// TODO: Make this a unique pointer? I think the image should own the header at this point?
-	image.header = std::make_shared<SharedCacheMachOHeader>(*imageHeader);
+	image.header = std::make_shared<SharedCacheMachOHeader>(std::move(*imageHeader));
 
 	AddImage(std::move(image));
 	return true;

view/sharedcache/core/SharedCache.h

@@ -136,11 +136,11 @@ class CacheEntry
 	std::vector<dyld_cache_mapping_info> m_mappings {};
 	// Mapping of image path to image info, used within ProcessImagesAndRegions to add them to the cache.
 	// Also used to retrieve the image dependencies.
-	std::unordered_map<std::string, dyld_cache_image_info> m_images {};
+	std::vector<std::pair<std::string, dyld_cache_image_info>> m_images {};
 
 public:
 	CacheEntry(std::string filePath, std::string fileName, CacheEntryType type, dyld_cache_header header,
-		std::vector<dyld_cache_mapping_info> mappings, std::unordered_map<std::string, dyld_cache_image_info> images);
+		std::vector<dyld_cache_mapping_info> mappings, std::vector<std::pair<std::string, dyld_cache_image_info>> images);
 
 	CacheEntry() = default;
 
@@ -173,7 +173,7 @@ class CacheEntry
 	const std::string GetFileName() const { return m_fileName; }
 	const dyld_cache_header& GetHeader() const { return m_header; }
 	const std::vector<dyld_cache_mapping_info>& GetMappings() const { return m_mappings; }
-	const std::unordered_map<std::string, dyld_cache_image_info>& GetImages() const { return m_images; }
+	const std::vector<std::pair<std::string, dyld_cache_image_info>>& GetImages() const { return m_images; }
 };
 
 // The ID for a given CacheEntry, use this instead of passing a pointer around to avoid complexity :V
@@ -227,10 +227,10 @@ class SharedCache
 	const std::unordered_map<uint64_t, CacheSymbol>& GetSymbols() const { return m_symbols; }
 	const std::unordered_map<std::string, uint64_t>& GetNamedSymbols() const { return m_namedSymbols; }
 
-	void AddImage(CacheImage image);
+	void AddImage(CacheImage&& image);
 
 	// Add a region that may overlap with another.
-	void AddRegion(CacheRegion region);
+	void AddRegion(CacheRegion&& region);
 
 	void AddSymbol(CacheSymbol symbol);