Merge pull request #244 from chaoticgd/symbol_map_fixes

Improve the API for retrieving handles associated with specific addresses/names and fix a buggy test
chaoticgd · Sep 18, 2024 · bb98913 · bb98913
2 parents 35bb62d + abf86fa
commit bb98913
Show file tree

Hide file tree

Showing 5 changed files with 60 additions and 52 deletions.
diff --git a/src/ccc/mdebug_importer.cpp b/src/ccc/mdebug_importer.cpp
@@ -348,7 +348,7 @@ static Result<void> resolve_type_name(
 		CCC_ASSERT(source_file);
 		auto handle = source_file->stabs_type_number_to_handle.find(unresolved_stabs->stabs_type_number);
 		if(handle != source_file->stabs_type_number_to_handle.end()) {
-			type_name.data_type_handle = handle->second.value;
+			type_name.data_type_handle = handle->second;
 			type_name.is_forward_declared = false;
 			type_name.unresolved_stabs.reset();
 			return Result<void>();
@@ -359,10 +359,10 @@ static Result<void> resolve_type_name(
 	// its name instead. This happens when a type is forward declared but not
 	// defined in a given translation unit.
 	if(!unresolved_stabs->type_name.empty()) {
-		for(auto& name_handle : database.data_types.handles_from_name(unresolved_stabs->type_name)) {
-			DataType* data_type = database.data_types.symbol_from_handle(name_handle.second);
+		for(DataTypeHandle& handle : database.data_types.handles_from_name(unresolved_stabs->type_name)) {
+			DataType* data_type = database.data_types.symbol_from_handle(handle);
 			if(data_type && group.is_in_group(*data_type)) {
-				type_name.data_type_handle = name_handle.second.value;
+				type_name.data_type_handle = handle;
 				type_name.is_forward_declared = true;
 				type_name.unresolved_stabs.reset();
 				return Result<void>();
@@ -412,7 +412,7 @@ static Result<void> resolve_type_name(
 		(*forward_declared_type)->set_type(std::move(forward_declared_node));
 		(*forward_declared_type)->not_defined_in_any_translation_unit = true;
 
-		type_name.data_type_handle = (*forward_declared_type)->handle().value;
+		type_name.data_type_handle = (*forward_declared_type)->handle();
 		type_name.is_forward_declared = true;
 		type_name.unresolved_stabs.reset();
 
@@ -545,18 +545,18 @@ static void detect_duplicate_functions(SymbolDatabase& database, const SymbolGro
 		// translation units.
 		FunctionHandle best_handle;
 		u32 best_offset = UINT32_MAX;
-		for(const auto& [address, handle] : functions_with_same_address) {
+		for(FunctionHandle handle : functions_with_same_address) {
 			ccc::Function* function = database.functions.symbol_from_handle(handle);
 			if(!function || !group.is_in_group(*function) || function->mangled_name() != test_function.mangled_name()) {
 				continue;
 			}
 
-			if(address - source_file_address < best_offset) {
+			if(test_function.address().value - source_file_address < best_offset) {
 				if(best_handle.valid()) {
 					duplicate_functions.emplace_back(best_handle);
 				}
 				best_handle = function->handle();
-				best_offset = address - source_file_address;
+				best_offset = test_function.address().value - source_file_address;
 			} else {
 				duplicate_functions.emplace_back(function->handle());
 			}
@@ -643,8 +643,8 @@ void fill_in_pointers_to_member_function_definitions(SymbolDatabase& database)
 			type_name = qualified_name.substr(0, name_separator_pos - 1);
 		}
 
-		for(const auto& name_handle : database.data_types.handles_from_name(type_name)) {
-			DataType* data_type = database.data_types.symbol_from_handle(name_handle.second);
+		for(DataTypeHandle handle : database.data_types.handles_from_name(type_name)) {
+			DataType* data_type = database.data_types.symbol_from_handle(handle);
 			if(!data_type || !data_type->type() || data_type->type()->descriptor != ast::STRUCT_OR_UNION) {
 				continue;
 			}

diff --git a/src/ccc/symbol_database.cpp b/src/ccc/symbol_database.cpp
@@ -104,22 +104,40 @@ typename SymbolList<SymbolType>::ConstIterator SymbolList<SymbolType>::end() con
 }
 
 template <typename SymbolType>
-typename SymbolList<SymbolType>::AddressToHandleMapIterators SymbolList<SymbolType>::handles_from_starting_address(Address address) const
+std::vector<SymbolHandle<SymbolType>> SymbolList<SymbolType>::handles_from_starting_address(Address address) const
 {
-	auto iterators = m_address_to_handle.equal_range(address.value);
-	return {iterators.first, iterators.second};
+	std::vector<SymbolHandle<SymbolType>> handles;
+
+	auto [begin, end] = m_address_to_handle.equal_range(address.value);
+	for(auto iterator = begin; iterator != end; iterator++) {
+		handles.emplace_back(iterator->second);
+	}
+
+	return handles;
 }
 
 template <typename SymbolType>
-typename SymbolList<SymbolType>::AddressToHandleMapIterators SymbolList<SymbolType>::handles_from_address_range(AddressRange range) const
+std::vector<SymbolHandle<SymbolType>> SymbolList<SymbolType>::handles_from_address_range(AddressRange range) const
 {
+	std::vector<SymbolHandle<SymbolType>> handles;
+
+	typename AddressToHandleMap::const_iterator begin, end;
 	if(range.low.valid()) {
-		return {m_address_to_handle.lower_bound(range.low.value), m_address_to_handle.lower_bound(range.high.value)};
+		begin = m_address_to_handle.lower_bound(range.low.value);
+		end = m_address_to_handle.lower_bound(range.high.value);
 	} else if(range.high.valid()) {
-		return {m_address_to_handle.begin(), m_address_to_handle.lower_bound(range.high.value)};
+		begin = m_address_to_handle.begin();
+		end = m_address_to_handle.lower_bound(range.high.value);
 	} else {
-		return {m_address_to_handle.end(), m_address_to_handle.end()};
+		begin = m_address_to_handle.end();
+		end = m_address_to_handle.end();
+	}
+
+	for(auto iterator = begin; iterator != end; iterator++) {
+		handles.emplace_back(iterator->second);
 	}
+
+	return handles;
 }
 
 template <typename SymbolType>
@@ -134,10 +152,16 @@ SymbolHandle<SymbolType> SymbolList<SymbolType>::first_handle_from_starting_addr
 }
 
 template <typename SymbolType>
-typename SymbolList<SymbolType>::NameToHandleMapIterators SymbolList<SymbolType>::handles_from_name(const std::string& name) const
+std::vector<SymbolHandle<SymbolType>> SymbolList<SymbolType>::handles_from_name(const std::string& name) const
 {
-	auto iterators = m_name_to_handle.equal_range(name);
-	return {iterators.first, iterators.second};
+	std::vector<SymbolHandle<SymbolType>> handles;
+
+	auto [begin, end] = m_name_to_handle.equal_range(name);
+	for(auto iterator = begin; iterator != end; iterator++) {
+		handles.emplace_back(iterator->second);
+	}
+
+	return handles;
 }
 
 template <typename SymbolType>
@@ -929,7 +953,7 @@ Result<DataType*> SymbolDatabase::create_data_type_if_unique(
 		// Types with this name have previously been processed, we need to
 		// figure out if this one matches any of the previous ones.
 		bool match = false;
-		for(auto [key, existing_type_handle] : types_with_same_name) {
+		for(DataTypeHandle existing_type_handle : types_with_same_name) {
 			DataType* existing_type = data_types.symbol_from_handle(existing_type_handle);
 			CCC_ASSERT(existing_type);
 

diff --git a/src/ccc/symbol_database.h b/src/ccc/symbol_database.h
@@ -107,32 +107,14 @@ class SymbolList {
 	Iterator end();
 	ConstIterator end() const;
 
-	using AddressToHandleMap = std::multimap<u32, SymbolHandle<SymbolType>>;
-	using NameToHandleMap = std::multimap<std::string, SymbolHandle<SymbolType>>;
-
-	template <typename Iterator>
-	class Iterators {
-	public:
-		Iterators(Iterator b, Iterator e)
-			: m_begin(b), m_end(e) {}
-		Iterator begin() const { return m_begin; }
-		Iterator end() const { return m_end; }
-	protected:
-		Iterator m_begin;
-		Iterator m_end;
-	};
-
-	using AddressToHandleMapIterators = Iterators<typename AddressToHandleMap::const_iterator>;
-	using NameToHandleMapIterators = Iterators<typename NameToHandleMap::const_iterator>;
-
 	// Lookup symbols by their address.
-	AddressToHandleMapIterators handles_from_starting_address(Address address) const;
-	AddressToHandleMapIterators handles_from_address_range(AddressRange range) const;
+	std::vector<SymbolHandle<SymbolType>> handles_from_starting_address(Address address) const;
+	std::vector<SymbolHandle<SymbolType>> handles_from_address_range(AddressRange range) const;
 	SymbolHandle<SymbolType> first_handle_from_starting_address(Address address) const;
 	SymbolHandle<SymbolType> first_handle_after_address(Address address) const;
 
 	// Lookup symbols by their name.
-	NameToHandleMapIterators handles_from_name(const std::string& name) const;
+	std::vector<SymbolHandle<SymbolType>> handles_from_name(const std::string& name) const;
 	SymbolHandle<SymbolType> first_handle_from_name(const std::string& name) const;
 
 	// Find a symbol with an address range that contains the provided address.
@@ -217,6 +199,9 @@ class SymbolList {
 	void link_name_map(SymbolType& symbol);
 	void unlink_name_map(SymbolType& symbol);
 
+	using AddressToHandleMap = std::multimap<u32, SymbolHandle<SymbolType>>;
+	using NameToHandleMap = std::multimap<std::string, SymbolHandle<SymbolType>>;
+
 	std::vector<SymbolType> m_symbols;
 	AddressToHandleMap m_address_to_handle;
 	NameToHandleMap m_name_to_handle;

diff --git a/src/ccc/util.h b/src/ccc/util.h
@@ -270,7 +270,6 @@ struct AddressRange {
 	Address high;
 
 	AddressRange() {}
-	AddressRange(Address address) : low(address), high(address) {}
 	AddressRange(Address l, Address h) : low(l), high(h) {}
 
 	friend auto operator<=>(const AddressRange& lhs, const AddressRange& rhs) = default;

diff --git a/test/ccc/symbol_database_tests.cpp b/test/ccc/symbol_database_tests.cpp
@@ -57,22 +57,22 @@ TEST(CCCSymbolDatabase, HandlesFromAddressRange)
 	EXPECT_EQ(empty_after_open.begin(), empty_after_open.end());
 
 	auto last = database.functions.handles_from_address_range(AddressRange(19, 30));
-	EXPECT_EQ(last.begin()->second, handles[19]);
+	EXPECT_EQ(*last.begin(), handles[19]);
 
 	auto last_open = database.functions.handles_from_address_range(AddressRange(19, Address()));
-	EXPECT_EQ(last_open.begin()->second, handles[19]);
+	EXPECT_EQ(*last_open.begin(), handles[19]);
 
 	auto first_half = database.functions.handles_from_address_range(AddressRange(5, 15));
-	EXPECT_EQ(first_half.begin()->second, handles[10]);
-	EXPECT_EQ(first_half.end()->second, handles[15]);
+	EXPECT_EQ(*first_half.begin(), handles[10]);
+	EXPECT_EQ(*(--first_half.end()), handles[14]);
 
 	auto second_half = database.functions.handles_from_address_range(AddressRange(15, 25));
-	EXPECT_EQ(second_half.begin()->second, handles[15]);
-	EXPECT_EQ((--second_half.end())->second, handles[19]);
+	EXPECT_EQ(*second_half.begin(), handles[15]);
+	EXPECT_EQ(*(--second_half.end()), handles[19]);
 
 	auto whole_range = database.functions.handles_from_address_range(AddressRange(10, 20));
-	EXPECT_EQ(whole_range.begin()->second, handles[10]);
-	EXPECT_EQ((--whole_range.end())->second, handles[19]);
+	EXPECT_EQ(*whole_range.begin(), handles[10]);
+	EXPECT_EQ(*(--whole_range.end()), handles[19]);
 }
 
 TEST(CCCSymbolDatabase, HandleFromStartingAddress)
@@ -424,7 +424,7 @@ TEST(CCCSymbolDatabase, DeduplicateWobblyTypedefs)
 	EXPECT_EQ(++handles.begin(), handles.end());
 
 	// Validate that we can lookup the struct and that it has a single field which is a type name.
-	DataType* chosen_type = database.data_types.symbol_from_handle(handles.begin()->second);
+	DataType* chosen_type = database.data_types.symbol_from_handle(*handles.begin());
 	ASSERT_TRUE(chosen_type && chosen_type->type() && chosen_type->type()->descriptor == ast::STRUCT_OR_UNION);
 	ast::StructOrUnion& chosen_struct = chosen_type->type()->as<ast::StructOrUnion>();
 	ASSERT_EQ(chosen_struct.fields.size(), 1);