Address feedback

dotnet · Aug 26, 2024 · 7efda5c · 7efda5c
1 parent 7cba241
commit 7efda5c
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 30 deletions.
diff --git a/src/native/pinvoke-override/pinvoke-override-api-impl.hh b/src/native/pinvoke-override/pinvoke-override-api-impl.hh
@@ -20,8 +20,8 @@ namespace xamarin::android {
 
 		if (lib_handle == nullptr) {
 			// We're being called as part of the p/invoke mechanism, we don't need to look in the AOT cache
-			constexpr bool USE_AOT_CACHE = false;
-			lib_handle = internal::MonodroidDl::monodroid_dlopen (library_name, MONO_DL_LOCAL, nullptr, USE_AOT_CACHE);
+			constexpr bool PREFER_AOT_CACHE = false;
+			lib_handle = internal::MonodroidDl::monodroid_dlopen (library_name, MONO_DL_LOCAL, nullptr, PREFER_AOT_CACHE);
 			if (lib_handle == nullptr) {
 				log_warn (LOG_ASSEMBLY, "Shared library '%s' not loaded, p/invoke '%s' may fail", library_name, symbol_name);
 				return nullptr;

diff --git a/src/native/runtime-base/monodroid-dl.hh b/src/native/runtime-base/monodroid-dl.hh
@@ -19,6 +19,15 @@ namespace xamarin::android::internal
 {
 	class MonodroidDl
 	{
+		enum class CacheKind
+		{
+			// Access AOT cache
+			AOT,
+
+			// Access DSO cache
+			DSO,
+		};
+
 		static inline xamarin::android::mutex   dso_handle_write_lock;
 
 		static unsigned int convert_dl_flags (int flags) noexcept
@@ -29,18 +38,20 @@ namespace xamarin::android::internal
 			return lflags;
 		}
 
-		template<bool AotCache>
+		template<CacheKind WhichCache>
 		[[gnu::always_inline, gnu::flatten]]
 		static DSOCacheEntry* find_dso_cache_entry_common (hash_t hash) noexcept
 		{
+			static_assert (WhichCache == CacheKind::AOT || WhichCache == CacheKind::DSO, "Unknown cache type specified");
+
 			DSOCacheEntry *arr;
 			size_t arr_size;
 
-			if constexpr (AotCache) {
+			if constexpr (WhichCache == CacheKind::AOT) {
 				log_debug (LOG_ASSEMBLY, "Looking for hash 0x%x in AOT cache", hash);
 				arr = aot_dso_cache;
 				arr_size = application_config.number_of_aot_cache_entries;
-			} else {
+			} else if constexpr (WhichCache == CacheKind::DSO) {
 				log_debug (LOG_ASSEMBLY, "Looking for hash 0x%x in DSO cache", hash);
 				arr = dso_cache;
 				arr_size = application_config.number_of_dso_cache_entries;
@@ -60,32 +71,13 @@ namespace xamarin::android::internal
 		[[gnu::always_inline, gnu::flatten]]
 		static DSOCacheEntry* find_only_aot_cache_entry (hash_t hash) noexcept
 		{
-			constexpr bool IsAotCache = true;
-			return find_dso_cache_entry_common<IsAotCache> (hash);
+			return find_dso_cache_entry_common<CacheKind::AOT> (hash);
 		}
 
 		[[gnu::always_inline, gnu::flatten]]
 		static DSOCacheEntry* find_only_dso_cache_entry (hash_t hash) noexcept
 		{
-			constexpr bool IsAotCache = false;
-			return find_dso_cache_entry_common<IsAotCache> (hash);
-		}
-
-		[[gnu::always_inline, gnu::flatten]]
-		static DSOCacheEntry* find_any_dso_cache_entry (hash_t hash) noexcept
-		{
-			// If we're asked to look in the AOT DSO cache, do it first.  This is because we're likely called from the
-			// MonoVM's dlopen fallback handler and it will not be a request to resolved a p/invoke, but most likely to
-			// find and load an AOT image for a managed assembly.  Since there might be naming/hash conflicts in this
-			// scenario, we look at the AOT cache first.
-			//
-			// See: https://github.com/dotnet/android/issues/9081
-			DSOCacheEntry *ret = find_only_aot_cache_entry (hash);
-			if (ret != nullptr) {
-				return ret;
-			}
-
-			return find_only_dso_cache_entry (hash);
+			return find_dso_cache_entry_common<CacheKind::DSO> (hash);
 		}
 
 		static void* monodroid_dlopen_log_and_return (void *handle, char **err, const char *full_name, bool free_memory)
@@ -150,7 +142,7 @@ namespace xamarin::android::internal
 
 	public:
 		[[gnu::flatten]]
-		static void* monodroid_dlopen (const char *name, int flags, char **err, bool use_aot_cache) noexcept
+		static void* monodroid_dlopen (const char *name, int flags, char **err, bool prefer_aot_cache) noexcept
 		{
 			if (name == nullptr) {
 				log_warn (LOG_ASSEMBLY, "monodroid_dlopen got a null name. This is not supported in NET+");
@@ -159,7 +151,22 @@ namespace xamarin::android::internal
 
 			hash_t name_hash = xxhash::hash (name, strlen (name));
 			log_debug (LOG_ASSEMBLY, "monodroid_dlopen: hash for name '%s' is 0x%zx", name, name_hash);
-			DSOCacheEntry *dso = use_aot_cache ? find_any_dso_cache_entry (name_hash) : find_only_dso_cache_entry (name_hash);
+
+			DSOCacheEntry *dso = nullptr;
+			if (prefer_aot_cache) {
+				// If we're asked to look in the AOT DSO cache, do it first.  This is because we're likely called from the
+				// MonoVM's dlopen fallback handler and it will not be a request to resolved a p/invoke, but most likely to
+				// find and load an AOT image for a managed assembly.  Since there might be naming/hash conflicts in this
+				// scenario, we look at the AOT cache first.
+				//
+				// See: https://github.com/dotnet/android/issues/9081
+				dso = find_only_aot_cache_entry (name_hash);
+			}
+
+			if (dso == nullptr) {
+				dso = find_only_dso_cache_entry (name_hash);
+			}
+
 			log_debug (LOG_ASSEMBLY, "monodroid_dlopen: hash match %sfound, DSO name is '%s'", dso == nullptr ? "not " : "", dso == nullptr ? "<unknown>" : dso->name);
 
 			if (dso == nullptr) {
@@ -213,8 +220,8 @@ namespace xamarin::android::internal
 		{
 			// We're called by MonoVM via a callback, we might need to return an AOT DSO.
 			// See: https://github.com/dotnet/android/issues/9081
-			constexpr bool USE_AOT_CACHE = true;
-			return monodroid_dlopen (name, flags, err, USE_AOT_CACHE);
+			constexpr bool PREFER_AOT_CACHE = true;
+			return monodroid_dlopen (name, flags, err, PREFER_AOT_CACHE);
 		}
 
 		[[gnu::flatten]]