Skip to content

Commit

Permalink
[native] Split up "regular" and AOT DSO caches (#9117)
Browse files Browse the repository at this point in the history
Fixes: #9081

Context: dotnet/runtime#104397
Context: dotnet/runtime#106026
Context: #9081 (comment)
Context: c227042

("Compatibility is fun")

Consider a P/Invoke method declaration:

	[DllImport("libSkiaSharp")]
	static extern void gr_backendrendertarget_delete(IntPtr rendertarget);

Historically, when attempting to resolve this method, Mono would try
loading the following native libraries:

  * `libSkiaSharp`          (original name)
  * `libSkiaSharp.so`       (add `.so` suffix)
  * `liblibSkiaSharp`       (add `lib` prefix)
  * `liblibSkiaSharp.so`    (`lib` prefix + `.so` suffix)

.NET for Android would further permute these names, *removing* the
`lib` prefix, for attempted compatibility in case there is a P/Invoke
into `"SkiaSharp"`.

The unfortunate occasional result would be an *ambiguity*: when told
to resolve "SkiaSharp", what should we return?  The information for
`libSkiaSharp.so`, or for the *AOT'd image* of the assembly
`SkiaSharp.dll`, by way of `libaot-SkiaSharp.dll.so`?

        %struct.DSOCacheEntry {
                i64 u0x12e73d483788709d, ; from name: SkiaSharp.so
                i64 u0x3cb282562b838c95, ; uint64_t real_name_hash
                i1 false, ; bool ignore
                ptr @.DSOCacheEntry.23_name, ; name: libaot-SkiaSharp.dll.so
                ptr null; void* handle
        }, ; 71
        %struct.DSOCacheEntry {
                i64 u0x12e73d483788709d, ; from name: SkiaSharp.so
                i64 u0x43db119dcc3147fa, ; uint64_t real_name_hash
                i1 false, ; bool ignore
                ptr @.DSOCacheEntry.7_name, ; name: libSkiaSharp.so
                ptr null; void* handle
        }, ; 72

If we return the wrong thing, then the app may crash or otherwise
behave incorrectly.

Fix this by:

  * Splitting up the DSO cache into AOT-related `.so` files and
    everything else.
  * Updating `PinvokeOverride::load_library_symbol()` so that the AOT
    files are *not* consulted when resolving P/Invoke libraries.
  * Updating `MonodroidDl::monodroid_dlopen()` -- which is called by
    MonoVM via `mono_dl_fallback_register()` -- so that the AOT files
    *are* consulted *first* when resolving AOT images.

When dotnet/runtime#104397 is fixed, it will make the AOT side of the
split more efficient as we won't have to permute the shared library
name as many times as now.
  • Loading branch information
grendello authored Aug 27, 2024
1 parent 73f948e commit 47a3b33
Show file tree
Hide file tree
Showing 9 changed files with 130 additions and 42 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ public sealed class ApplicationConfig
public uint bundled_assembly_name_width;
public uint number_of_assembly_store_files;
public uint number_of_dso_cache_entries;
public uint number_of_aot_cache_entries;
public uint android_runtime_jnienv_class_token;
public uint jnienv_initialize_method_token;
public uint jnienv_registerjninatives_method_token;
Expand All @@ -67,7 +68,7 @@ public sealed class ApplicationConfig
public string android_package_name = String.Empty;
}

const uint ApplicationConfigFieldCount = 26;
const uint ApplicationConfigFieldCount = 27;

const string ApplicationConfigSymbolName = "application_config";
const string AppEnvironmentVariablesSymbolName = "app_environment_variables";
Expand Down Expand Up @@ -301,37 +302,42 @@ static ApplicationConfig ReadApplicationConfig (EnvironmentFile envFile)
ret.number_of_dso_cache_entries = ConvertFieldToUInt32 ("number_of_dso_cache_entries", envFile.Path, parser.SourceFilePath, item.LineNumber, field [1]);
break;

case 19: // android_runtime_jnienv_class_token: uint32_t / .word | .long
case 19: // number_of_aot_cache_entries: uint32_t / .word | .long
Assert.IsTrue (expectedUInt32Types.Contains (field [0]), $"Unexpected uint32_t field type in '{envFile.Path}:{item.LineNumber}': {field [0]}");
ret.number_of_dso_cache_entries = ConvertFieldToUInt32 ("android_runtime_jnienv_class_token", envFile.Path, parser.SourceFilePath, item.LineNumber, field [1]);
ret.number_of_aot_cache_entries = ConvertFieldToUInt32 ("number_of_aot_cache_entries", envFile.Path, parser.SourceFilePath, item.LineNumber, field [1]);
break;

case 20: // jnienv_initialize_method_token: uint32_t / .word | .long
case 20: // android_runtime_jnienv_class_token: uint32_t / .word | .long
Assert.IsTrue (expectedUInt32Types.Contains (field [0]), $"Unexpected uint32_t field type in '{envFile.Path}:{item.LineNumber}': {field [0]}");
ret.number_of_dso_cache_entries = ConvertFieldToUInt32 ("jnienv_initialize_method_token", envFile.Path, parser.SourceFilePath, item.LineNumber, field [1]);
ret.android_runtime_jnienv_class_token = ConvertFieldToUInt32 ("android_runtime_jnienv_class_token", envFile.Path, parser.SourceFilePath, item.LineNumber, field [1]);
break;

case 21: // jnienv_registerjninatives_method_token: uint32_t / .word | .long
case 21: // jnienv_initialize_method_token: uint32_t / .word | .long
Assert.IsTrue (expectedUInt32Types.Contains (field [0]), $"Unexpected uint32_t field type in '{envFile.Path}:{item.LineNumber}': {field [0]}");
ret.number_of_dso_cache_entries = ConvertFieldToUInt32 ("jnienv_registerjninatives_method_token", envFile.Path, parser.SourceFilePath, item.LineNumber, field [1]);
ret.jnienv_initialize_method_token = ConvertFieldToUInt32 ("jnienv_initialize_method_token", envFile.Path, parser.SourceFilePath, item.LineNumber, field [1]);
break;

case 22: // jni_remapping_replacement_type_count: uint32_t / .word | .long
case 22: // jnienv_registerjninatives_method_token: uint32_t / .word | .long
Assert.IsTrue (expectedUInt32Types.Contains (field [0]), $"Unexpected uint32_t field type in '{envFile.Path}:{item.LineNumber}': {field [0]}");
ret.jnienv_registerjninatives_method_token = ConvertFieldToUInt32 ("jnienv_registerjninatives_method_token", envFile.Path, parser.SourceFilePath, item.LineNumber, field [1]);
break;

case 23: // jni_remapping_replacement_type_count: uint32_t / .word | .long
Assert.IsTrue (expectedUInt32Types.Contains (field [0]), $"Unexpected uint32_t field type in '{envFile.Path}:{item.LineNumber}': {field [0]}");
ret.jni_remapping_replacement_type_count = ConvertFieldToUInt32 ("jni_remapping_replacement_type_count", envFile.Path, parser.SourceFilePath, item.LineNumber, field [1]);
break;

case 23: // jni_remapping_replacement_method_index_entry_count: uint32_t / .word | .long
case 24: // jni_remapping_replacement_method_index_entry_count: uint32_t / .word | .long
Assert.IsTrue (expectedUInt32Types.Contains (field [0]), $"Unexpected uint32_t field type in '{envFile.Path}:{item.LineNumber}': {field [0]}");
ret.jni_remapping_replacement_method_index_entry_count = ConvertFieldToUInt32 ("jni_remapping_replacement_method_index_entry_count", envFile.Path, parser.SourceFilePath, item.LineNumber, field [1]);
break;

case 24: // mono_components_mask: uint32_t / .word | .long
case 25: // mono_components_mask: uint32_t / .word | .long
Assert.IsTrue (expectedUInt32Types.Contains (field [0]), $"Unexpected uint32_t field type in '{envFile.Path}:{item.LineNumber}': {field [0]}");
ret.mono_components_mask = ConvertFieldToUInt32 ("mono_components_mask", envFile.Path, parser.SourceFilePath, item.LineNumber, field [1]);
break;

case 25: // android_package_name: string / [pointer type]
case 26: // android_package_name: string / [pointer type]
Assert.IsTrue (expectedPointerTypes.Contains (field [0]), $"Unexpected pointer field type in '{envFile.Path}:{item.LineNumber}': {field [0]}");
pointers.Add (field [1].Trim ());
break;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ sealed class ApplicationConfig
public uint number_of_assemblies_in_apk;
public uint bundled_assembly_name_width;
public uint number_of_dso_cache_entries;
public uint number_of_aot_cache_entries;
public uint number_of_shared_libraries;

[NativeAssembler (NumberFormat = LLVMIR.LlvmIrVariableNumberFormat.Hexadecimal)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ sealed class XamarinAndroidBundledAssembly
SortedDictionary <string, string>? systemProperties;
StructureInstance? application_config;
List<StructureInstance<DSOCacheEntry>>? dsoCache;
List<StructureInstance<DSOCacheEntry>>? aotDsoCache;
List<StructureInstance<XamarinAndroidBundledAssembly>>? xamarinAndroidBundledAssemblies;

StructureInfo? applicationConfigStructureInfo;
Expand Down Expand Up @@ -218,7 +219,7 @@ protected override void Construct (LlvmIrModule module)
};
module.Add (sysProps, stringGroupName: "sysprop", stringGroupComment: " System properties name:value pairs");

dsoCache = InitDSOCache ();
(dsoCache, aotDsoCache) = InitDSOCache ();
var app_cfg = new ApplicationConfig {
uses_mono_llvm = UsesMonoLLVM,
uses_mono_aot = UsesMonoAOT,
Expand All @@ -239,6 +240,7 @@ protected override void Construct (LlvmIrModule module)
number_of_shared_libraries = (uint)NativeLibraries.Count,
bundled_assembly_name_width = (uint)BundledAssemblyNameWidth,
number_of_dso_cache_entries = (uint)dsoCache.Count,
number_of_aot_cache_entries = (uint)aotDsoCache.Count,
android_runtime_jnienv_class_token = (uint)AndroidRuntimeJNIEnvToken,
jnienv_initialize_method_token = (uint)JNIEnvInitializeToken,
jnienv_registerjninatives_method_token = (uint)JNIEnvRegisterJniNativesToken,
Expand All @@ -256,6 +258,12 @@ protected override void Construct (LlvmIrModule module)
};
module.Add (dso_cache);

var aot_dso_cache = new LlvmIrGlobalVariable (aotDsoCache, "aot_dso_cache", LlvmIrVariableOptions.GlobalWritable) {
Comment = " AOT DSO cache entries",
BeforeWriteCallback = HashAndSortDSOCache,
};
module.Add (aot_dso_cache);

var dso_apk_entries = new LlvmIrGlobalVariable (typeof(List<StructureInstance<DSOApkEntry>>), "dso_apk_entries") {
ArrayItemCount = (ulong)NativeLibraries.Count,
Options = LlvmIrVariableOptions.GlobalWritable,
Expand Down Expand Up @@ -337,7 +345,7 @@ void HashAndSortDSOCache (LlvmIrVariable variable, LlvmIrModuleTarget target, ob
cache.Sort ((StructureInstance<DSOCacheEntry> a, StructureInstance<DSOCacheEntry> b) => a.Instance.hash.CompareTo (b.Instance.hash));
}

List<StructureInstance<DSOCacheEntry>> InitDSOCache ()
(List<StructureInstance<DSOCacheEntry>> dsoCache, List<StructureInstance<DSOCacheEntry>> aotDsoCache) InitDSOCache ()
{
var dsos = new List<(string name, string nameLabel, bool ignore)> ();
var nameCache = new HashSet<string> (StringComparer.OrdinalIgnoreCase);
Expand All @@ -357,6 +365,7 @@ List<StructureInstance<DSOCacheEntry>> InitDSOCache ()
}

var dsoCache = new List<StructureInstance<DSOCacheEntry>> ();
var aotDsoCache = new List<StructureInstance<DSOCacheEntry>> ();
var nameMutations = new List<string> ();

for (int i = 0; i < dsos.Count; i++) {
Expand All @@ -372,11 +381,16 @@ List<StructureInstance<DSOCacheEntry>> InitDSOCache ()
name = name,
};

dsoCache.Add (new StructureInstance<DSOCacheEntry> (dsoCacheEntryStructureInfo, entry));
var item = new StructureInstance<DSOCacheEntry> (dsoCacheEntryStructureInfo, entry);
if (name.StartsWith ("libaot-", StringComparison.OrdinalIgnoreCase)) {
aotDsoCache.Add (item);
} else {
dsoCache.Add (item);
}
}
}

return dsoCache;
return (dsoCache, aotDsoCache);

void AddNameMutations (string name)
{
Expand Down
1 change: 0 additions & 1 deletion src/native/monodroid/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ set(XAMARIN_MONODROID_SOURCES
monodroid-tracing.cc
monovm-properties.cc
osbridge.cc
pinvoke-override-api.cc
runtime-util.cc
timezones.cc
xamarin_getifaddrs.cc
Expand Down
20 changes: 0 additions & 20 deletions src/native/monodroid/pinvoke-override-api.cc

This file was deleted.

4 changes: 3 additions & 1 deletion src/native/pinvoke-override/pinvoke-override-api-impl.hh
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ namespace xamarin::android {
void *lib_handle = dso_handle == nullptr ? nullptr : *dso_handle;

if (lib_handle == nullptr) {
lib_handle = internal::MonodroidDl::monodroid_dlopen (library_name, MONO_DL_LOCAL, nullptr, nullptr);
// We're being called as part of the p/invoke mechanism, we don't need to look in the 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;
Expand Down
73 changes: 68 additions & 5 deletions src/native/runtime-base/monodroid-dl.hh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -29,18 +38,48 @@ namespace xamarin::android::internal
return lflags;
}

static DSOCacheEntry* find_dso_cache_entry (hash_t hash) noexcept
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 (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 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;
}

auto equal = [](DSOCacheEntry const& entry, hash_t key) -> bool { return entry.hash == key; };
auto less_than = [](DSOCacheEntry const& entry, hash_t key) -> bool { return entry.hash < key; };
ssize_t idx = Search::binary_search<DSOCacheEntry, equal, less_than> (hash, dso_cache, application_config.number_of_dso_cache_entries);
ssize_t idx = Search::binary_search<DSOCacheEntry, equal, less_than> (hash, arr, arr_size);

if (idx >= 0) {
return &dso_cache[idx];
return &arr[idx];
}

return nullptr;
}

[[gnu::always_inline, gnu::flatten]]
static DSOCacheEntry* find_only_aot_cache_entry (hash_t hash) noexcept
{
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
{
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)
{
if (handle == nullptr && err != nullptr) {
Expand Down Expand Up @@ -103,7 +142,7 @@ namespace xamarin::android::internal

public:
[[gnu::flatten]]
static void* monodroid_dlopen (const char *name, int flags, char **err, [[maybe_unused]] void *user_data) 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+");
Expand All @@ -112,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 = find_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) {
Expand Down Expand Up @@ -161,6 +215,15 @@ namespace xamarin::android::internal
return monodroid_dlopen_log_and_return (dso->handle, err, name, false /* name_needs_free */);
}

[[gnu::flatten]]
static void* monodroid_dlopen (const char *name, int flags, char **err, [[maybe_unused]] void *user_data) noexcept
{
// 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 PREFER_AOT_CACHE = true;
return monodroid_dlopen (name, flags, err, PREFER_AOT_CACHE);
}

[[gnu::flatten]]
static void* monodroid_dlsym (void *handle, const char *name, char **err, [[maybe_unused]] void *user_data)
{
Expand Down
18 changes: 18 additions & 0 deletions src/native/xamarin-app-stub/application_dso_stub.cc
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,24 @@ DSOCacheEntry dso_cache[] = {
},
};

DSOCacheEntry aot_dso_cache[] = {
{
.hash = xamarin::android::xxhash::hash (fake_dso_name, sizeof(fake_dso_name) - 1),
.real_name_hash = xamarin::android::xxhash::hash (fake_dso_name, sizeof(fake_dso_name) - 1),
.ignore = true,
.name = fake_dso_name,
.handle = nullptr,
},

{
.hash = xamarin::android::xxhash::hash (fake_dso_name2, sizeof(fake_dso_name2) - 1),
.real_name_hash = xamarin::android::xxhash::hash (fake_dso_name2, sizeof(fake_dso_name2) - 1),
.ignore = true,
.name = fake_dso_name2,
.handle = nullptr,
},
};

DSOApkEntry dso_apk_entries[2] {};

//
Expand Down
5 changes: 5 additions & 0 deletions src/native/xamarin-app-stub/xamarin-app.hh
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,9 @@ enum class MonoComponent : uint32_t
Tracing = 0x04,
};

// Keep in strict sync with:
// src/Xamarin.Android.Build.Tasks/Utilities/ApplicationConfig.cs
// src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/EnvironmentHelper.cs
struct ApplicationConfig
{
bool uses_mono_llvm;
Expand All @@ -247,6 +250,7 @@ struct ApplicationConfig
uint32_t number_of_assemblies_in_apk;
uint32_t bundled_assembly_name_width;
uint32_t number_of_dso_cache_entries;
uint32_t number_of_aot_cache_entries;
uint32_t number_of_shared_libraries;
uint32_t android_runtime_jnienv_class_token;
uint32_t jnienv_initialize_method_token;
Expand Down Expand Up @@ -336,6 +340,7 @@ MONO_API MONO_API_EXPORT AssemblyStoreSingleAssemblyRuntimeData assembly_store_b
MONO_API MONO_API_EXPORT AssemblyStoreRuntimeData assembly_store;

MONO_API MONO_API_EXPORT DSOCacheEntry dso_cache[];
MONO_API MONO_API_EXPORT DSOCacheEntry aot_dso_cache[];
MONO_API MONO_API_EXPORT DSOApkEntry dso_apk_entries[];

//
Expand Down

0 comments on commit 47a3b33

Please sign in to comment.