From 9a5350d3e20a8c68591cadaba12796e9dc5d8274 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Fri, 10 Mar 2023 13:33:23 -0500 Subject: [PATCH 1/6] [mono] Use `unsigned char` when computing UTF8 string hashes The C standard does not specify whether `char` is signed or unsigned, it is implementation defined. Apparently Android aarch64 makes a different choice than other platforms (at least macOS arm64 and Windows x64 give different results). Mono uses `mono_metadata_str_hash` in the AOT compiler and AOT runtime to optimize class name lookup. As a result, classes whose names include UTF-8 continuation bytes (with the high bit = 1) will hash differently in the AOT compiler and on the device. Fixes https://github.com/dotnet/runtime/issues/82187 Fixes https://github.com/dotnet/runtime/issues/78638 --- src/mono/mono/eglib/ghashtable.c | 2 +- src/mono/mono/metadata/metadata.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/mono/mono/eglib/ghashtable.c b/src/mono/mono/eglib/ghashtable.c index 8b5c29a1a9044c..79b24603a560f8 100644 --- a/src/mono/mono/eglib/ghashtable.c +++ b/src/mono/mono/eglib/ghashtable.c @@ -673,7 +673,7 @@ guint g_str_hash (gconstpointer v1) { guint hash = 0; - char *p = (char *) v1; + unsigned char *p = (unsigned char *) v1; while (*p++) hash = (hash << 5) - (hash + *p); diff --git a/src/mono/mono/metadata/metadata.c b/src/mono/mono/metadata/metadata.c index eedf2653b89e26..c2821d49961583 100644 --- a/src/mono/mono/metadata/metadata.c +++ b/src/mono/mono/metadata/metadata.c @@ -5524,7 +5524,8 @@ guint mono_metadata_str_hash (gconstpointer v1) { /* Same as g_str_hash () in glib */ - char *p = (char *) v1; + /* note: signed/unsigned char matters - we feed UTF-8 to this function, so the high bit will give diferent results if we don't match. */ + unsigned char *p = (unsigned char *) v1; guint hash = *p; while (*p++) { From b687b4d6e0f75d3f04f764c5a2caf6c7f72af572 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Fri, 10 Mar 2023 13:39:31 -0500 Subject: [PATCH 2/6] [aot] add DEBUG_AOT_NAME_TABLE code for debugging the class names AOT compiler: Emits a second "class_name_table_debug" symbol that has all the class names and hashes as strings. AOT runtime: warns if a class is not found in the name cache --- src/mono/mono/mini/aot-compiler.c | 52 ++++++++++++++++++++++++++++--- src/mono/mono/mini/aot-runtime.c | 18 +++++++++++ src/mono/mono/mini/aot-runtime.h | 5 +++ 3 files changed, 71 insertions(+), 4 deletions(-) diff --git a/src/mono/mono/mini/aot-compiler.c b/src/mono/mono/mini/aot-compiler.c index cfe88c2fb410b2..f89bee249dbe7b 100644 --- a/src/mono/mono/mini/aot-compiler.c +++ b/src/mono/mono/mini/aot-compiler.c @@ -11417,8 +11417,27 @@ emit_class_info (MonoAotCompile *acfg) typedef struct ClassNameTableEntry { guint32 token, index; struct ClassNameTableEntry *next; +#ifdef DEBUG_AOT_NAME_TABLE + char *full_name; + uint32_t hash; +#endif } ClassNameTableEntry; +static char* +get_class_full_name_for_hash (MonoClass *klass) +{ + return mono_type_get_name_full (m_class_get_byval_arg (klass), MONO_TYPE_NAME_FORMAT_FULL_NAME); +} + +static uint32_t +hash_for_class (MonoClass *klass) +{ + char *full_name = get_class_full_name_for_hash (klass); + uint32_t hash = mono_metadata_str_hash (full_name); + g_free (full_name); + return hash; +} + static void emit_class_name_table (MonoAotCompile *acfg) { @@ -11426,9 +11445,12 @@ emit_class_name_table (MonoAotCompile *acfg) guint32 token, hash; MonoClass *klass; GPtrArray *table; - char *full_name; guint8 *buf, *p; ClassNameTableEntry *entry, *new_entry; +#ifdef DEBUG_AOT_NAME_TABLE + int name_buf_size = 0; + guint8 *name_buf, *name_p; +#endif /* * Construct a chained hash table for mapping class names to typedef tokens. @@ -11446,13 +11468,17 @@ emit_class_name_table (MonoAotCompile *acfg) mono_error_cleanup (error); continue; } - full_name = mono_type_get_name_full (m_class_get_byval_arg (klass), MONO_TYPE_NAME_FORMAT_FULL_NAME); - hash = mono_metadata_str_hash (full_name) % table_size; - g_free (full_name); + hash = hash_for_class (klass) % table_size; /* FIXME: Allocate from the mempool */ new_entry = g_new0 (ClassNameTableEntry, 1); new_entry->token = token; +#ifdef DEBUG_AOT_NAME_TABLE + new_entry->full_name = get_class_full_name_for_hash (klass); + new_entry->hash = hash; + /* '%s'=%08x\n */ + name_buf_size += strlen (new_entry->full_name) + strlen("''=\n") + 8; +#endif entry = (ClassNameTableEntry *)g_ptr_array_index (table, hash); if (entry == NULL) { @@ -11471,6 +11497,10 @@ emit_class_name_table (MonoAotCompile *acfg) /* Emit the table */ buf_size = table->len * 4 + 4; p = buf = (guint8 *)g_malloc0 (buf_size); +#ifdef DEBUG_AOT_NAME_TABLE + name_buf_size ++; /* one extra trailing nul */ + name_p = name_buf = (guint8 *)g_malloc0 (name_buf_size); +#endif /* FIXME: Optimize memory usage */ g_assert (table_size < 65000); @@ -11488,14 +11518,28 @@ emit_class_name_table (MonoAotCompile *acfg) else encode_int16 (0, p, &p); } +#ifdef DEBUG_AOT_NAME_TABLE + if (entry != NULL) { + name_p += sprintf ((char*)name_p, "'%s'=%08x\n", entry->full_name, entry->hash); + g_free (entry->full_name); + } +#endif g_free (entry); } +#ifdef DEBUG_AOT_NAME_TABLE + g_assert (name_p - name_buf <= name_buf_size); +#endif g_assert (p - buf <= buf_size); g_ptr_array_free (table, TRUE); emit_aot_data (acfg, MONO_AOT_TABLE_CLASS_NAME, "class_name_table", buf, GPTRDIFF_TO_INT (p - buf)); g_free (buf); + +#ifdef DEBUG_AOT_NAME_TABLE + emit_aot_data (acfg, MONO_AOT_TABLE_CLASS_NAME_DEBUG, "class_name_table_debug", name_buf, GPTRDIFF_TO_INT (name_p - name_buf)); + g_free (name_buf); +#endif } static void diff --git a/src/mono/mono/mini/aot-runtime.c b/src/mono/mono/mini/aot-runtime.c index 4d92418cb74d1c..142e93b0fe8c1f 100644 --- a/src/mono/mono/mini/aot-runtime.c +++ b/src/mono/mono/mini/aot-runtime.c @@ -2601,6 +2601,10 @@ mono_aot_get_class_from_name (MonoImage *image, const char *name_space, const ch MonoTableInfo *t; guint32 cols [MONO_TYPEDEF_SIZE]; GHashTable *nspace_table; +#ifdef DEBUG_AOT_NAME_TABLE + char *debug_full_name; + uint32_t debug_hash; +#endif if (!amodule || !amodule->class_name_table) return FALSE; @@ -2634,6 +2638,10 @@ mono_aot_get_class_from_name (MonoImage *image, const char *name_space, const ch full_name = g_strdup_printf ("%s.%s", name_space, name); } } +#ifdef DEBUG_AOT_NAME_TABLE + debug_full_name = g_strdup (full_name); + debug_hash = mono_metadata_str_hash (full_name) % table_size; +#endif hash = mono_metadata_str_hash (full_name) % table_size; if (full_name != full_name_buf) g_free (full_name); @@ -2673,6 +2681,9 @@ mono_aot_get_class_from_name (MonoImage *image, const char *name_space, const ch g_hash_table_insert (nspace_table, (char*)name2, *klass); amodule_unlock (amodule); } +#ifdef DEBUG_AOT_NAME_TABLE + g_free (debug_full_name); +#endif return TRUE; } @@ -2686,6 +2697,13 @@ mono_aot_get_class_from_name (MonoImage *image, const char *name_space, const ch amodule_unlock (amodule); +#ifdef DEBUG_AOT_NAME_TABLE + if (*klass == NULL) { + g_warning ("AOT class name cache '%s'=%08x not found\n", debug_full_name, debug_hash); + } + g_free (debug_full_name); +#endif + return TRUE; } diff --git a/src/mono/mono/mini/aot-runtime.h b/src/mono/mono/mini/aot-runtime.h index 77f227f6c65950..bd666593536276 100644 --- a/src/mono/mono/mini/aot-runtime.h +++ b/src/mono/mono/mini/aot-runtime.h @@ -86,6 +86,8 @@ typedef enum { MONO_AOT_METHOD_FLAG_INTERP_ENTRY_ONLY = 16, } MonoAotMethodFlags; +#undef DEBUG_AOT_NAME_TABLE + typedef enum { MONO_AOT_TABLE_BLOB, MONO_AOT_TABLE_CLASS_NAME, @@ -99,6 +101,9 @@ typedef enum { MONO_AOT_TABLE_IMAGE_TABLE, MONO_AOT_TABLE_WEAK_FIELD_INDEXES, MONO_AOT_TABLE_METHOD_FLAGS_TABLE, +#ifdef DEBUG_AOT_NAME_TABLE + MONO_AOT_TABLE_CLASS_NAME_DEBUG, +#endif MONO_AOT_TABLE_NUM } MonoAotFileTable; From ff378f702b66369df9a2f03b831897272dbaf2a4 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Fri, 10 Mar 2023 14:35:44 -0500 Subject: [PATCH 3/6] Add regression test --- .../GitHub_82187/GitHub_82187.csproj | 9 ++++++ .../regressions/GitHub_82187/repro.cs | 31 +++++++++++++++++++ 2 files changed, 40 insertions(+) create mode 100644 src/tests/Loader/classloader/regressions/GitHub_82187/GitHub_82187.csproj create mode 100644 src/tests/Loader/classloader/regressions/GitHub_82187/repro.cs diff --git a/src/tests/Loader/classloader/regressions/GitHub_82187/GitHub_82187.csproj b/src/tests/Loader/classloader/regressions/GitHub_82187/GitHub_82187.csproj new file mode 100644 index 00000000000000..32355f272f9085 --- /dev/null +++ b/src/tests/Loader/classloader/regressions/GitHub_82187/GitHub_82187.csproj @@ -0,0 +1,9 @@ + + + true + Exe + + + + + diff --git a/src/tests/Loader/classloader/regressions/GitHub_82187/repro.cs b/src/tests/Loader/classloader/regressions/GitHub_82187/repro.cs new file mode 100644 index 00000000000000..ca4ca5782267da --- /dev/null +++ b/src/tests/Loader/classloader/regressions/GitHub_82187/repro.cs @@ -0,0 +1,31 @@ +using System; + +/* Regression test for https://github.com/dotnet/runtime/issues/78638 + * and https://github.com/dotnet/runtime/issues/82187 ensure AOT + * cross-compiler and AOT runtime use the same name hashing for names + * that include UTF-8 continuation bytes. + */ + +[MySpecial(typeof(MeineTüre))] +public class Program +{ + public static int Main() + { + var attr = typeof (Program).GetCustomAttribute(typeof(MySpecialAttribute), false); + if (attr == null) + return 101; + if (attr.Type == null) + return 102; + if (attr.Type.FullName != "MeineTüre") + return 103; + return 100; + } +} + +public class MySpecialAttribute : Attribute +{ + public Type Type {get; private set; } + public MySpecialAttribute(Type t) { Type = t; } +} + +public class MeineTüre {} From 80f645672db7ed6a7f6a101c3d67598dc0cacd70 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Fri, 10 Mar 2023 16:49:09 -0500 Subject: [PATCH 4/6] fixup test --- src/tests/Loader/classloader/regressions/GitHub_82187/repro.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tests/Loader/classloader/regressions/GitHub_82187/repro.cs b/src/tests/Loader/classloader/regressions/GitHub_82187/repro.cs index ca4ca5782267da..1ab3e5476a66b0 100644 --- a/src/tests/Loader/classloader/regressions/GitHub_82187/repro.cs +++ b/src/tests/Loader/classloader/regressions/GitHub_82187/repro.cs @@ -11,7 +11,7 @@ public class Program { public static int Main() { - var attr = typeof (Program).GetCustomAttribute(typeof(MySpecialAttribute), false); + var attr = (MySpecialAttribute)Attribute.GetCustomAttribute(typeof (Program), typeof(MySpecialAttribute), false); if (attr == null) return 101; if (attr.Type == null) From 275aea21182dc0fd0f8f0245a91cb1d734778fbc Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Fri, 10 Mar 2023 15:10:14 -0500 Subject: [PATCH 5/6] temporarily run Android device AOT runtime tests --- .../runtime-extra-platforms-android.yml | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/eng/pipelines/extra-platforms/runtime-extra-platforms-android.yml b/eng/pipelines/extra-platforms/runtime-extra-platforms-android.yml index 2b1b26964ba40e..cab1b62b5ee74e 100644 --- a/eng/pipelines/extra-platforms/runtime-extra-platforms-android.yml +++ b/eng/pipelines/extra-platforms/runtime-extra-platforms-android.yml @@ -38,18 +38,14 @@ jobs: testGroup: innerloop nameSuffix: AllSubsets_Mono_RuntimeTests runtimeVariant: minijit - buildArgs: -s mono+libs -c $(_BuildConfig) + buildArgs: -s mono+libs -c $(_BuildConfig) /p:RunAOTCompilation=true timeoutInMinutes: 240 - # don't run tests on PRs until we can get significantly more devices - # Turn off the testing for now, until https://github.com/dotnet/runtime/issues/60128 gets resolved - # ${{ if eq(variables['isRollingBuild'], true) }}: - # # extra steps, run tests - # extraStepsTemplate: /eng/pipelines/common/templates/runtimes/build-runtime-tests-and-send-to-helix.yml - # extraStepsParameters: - # creator: dotnet-bot - # testRunNamePrefixSuffix: Mono_$(_BuildConfig) - # extraVariablesTemplates: - # - template: /eng/pipelines/common/templates/runtimes/test-variables.yml + extraStepsTemplate: /eng/pipelines/common/templates/runtimes/build-runtime-tests-and-send-to-helix.yml + extraStepsParameters: + creator: dotnet-bot + testRunNamePrefixSuffix: Mono_$(_BuildConfig) + extraVariablesTemplates: + - template: /eng/pipelines/common/templates/runtimes/test-variables.yml # # Android devices From 4021ef81bdb701c75ac24e2ba6ce94b80a7c35dc Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Fri, 10 Mar 2023 21:41:43 -0500 Subject: [PATCH 6/6] Revert "temporarily run Android device AOT runtime tests" This reverts commit 275aea21182dc0fd0f8f0245a91cb1d734778fbc. --- .../runtime-extra-platforms-android.yml | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/eng/pipelines/extra-platforms/runtime-extra-platforms-android.yml b/eng/pipelines/extra-platforms/runtime-extra-platforms-android.yml index cab1b62b5ee74e..2b1b26964ba40e 100644 --- a/eng/pipelines/extra-platforms/runtime-extra-platforms-android.yml +++ b/eng/pipelines/extra-platforms/runtime-extra-platforms-android.yml @@ -38,14 +38,18 @@ jobs: testGroup: innerloop nameSuffix: AllSubsets_Mono_RuntimeTests runtimeVariant: minijit - buildArgs: -s mono+libs -c $(_BuildConfig) /p:RunAOTCompilation=true + buildArgs: -s mono+libs -c $(_BuildConfig) timeoutInMinutes: 240 - extraStepsTemplate: /eng/pipelines/common/templates/runtimes/build-runtime-tests-and-send-to-helix.yml - extraStepsParameters: - creator: dotnet-bot - testRunNamePrefixSuffix: Mono_$(_BuildConfig) - extraVariablesTemplates: - - template: /eng/pipelines/common/templates/runtimes/test-variables.yml + # don't run tests on PRs until we can get significantly more devices + # Turn off the testing for now, until https://github.com/dotnet/runtime/issues/60128 gets resolved + # ${{ if eq(variables['isRollingBuild'], true) }}: + # # extra steps, run tests + # extraStepsTemplate: /eng/pipelines/common/templates/runtimes/build-runtime-tests-and-send-to-helix.yml + # extraStepsParameters: + # creator: dotnet-bot + # testRunNamePrefixSuffix: Mono_$(_BuildConfig) + # extraVariablesTemplates: + # - template: /eng/pipelines/common/templates/runtimes/test-variables.yml # # Android devices