-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[mono][wasm] Fix function signature mismatch in m2n invoke #101106
Changes from all commits
affbf56
f49a1c4
32358c0
d48ecb1
ca68d37
9159c02
ca36d4c
7634d53
022420a
5a3a58d
6e5beba
1bfd0f0
acd32e4
186a15f
899acbe
0b65450
9ba5a4a
46df3ec
b3d533f
a85f7c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1222,3 +1222,32 @@ mono_loader_install_pinvoke_override (PInvokeOverrideFn override_fn) | |
{ | ||
pinvoke_override = override_fn; | ||
} | ||
|
||
// Keep synced with FixupSymbolName from src/tasks/Common/Utils.cs | ||
char* mono_fixup_symbol_name (char *key) { | ||
char* fixedName = malloc(256); | ||
int sb_index = 0; | ||
int len = (int)strlen (key); | ||
|
||
for (int i = 0; i < len; ++i) { | ||
unsigned char b = key[i]; | ||
if ((b >= '0' && b <= '9') || | ||
(b >= 'a' && b <= 'z') || | ||
(b >= 'A' && b <= 'Z') || | ||
(b == '_')) { | ||
fixedName[sb_index++] = b; | ||
} | ||
else if (b == '.' || b == '-' || b == '+' || b == '<' || b == '>') { | ||
fixedName[sb_index++] = '_'; | ||
} | ||
else { | ||
// Append the hexadecimal representation of b between underscores | ||
sprintf(&fixedName[sb_index], "_%X_", b); | ||
sb_index += 4; // Move the index after the appended hexadecimal characters | ||
} | ||
} | ||
|
||
// Null-terminate the fixedName string | ||
fixedName[sb_index] = '\0'; | ||
return fixedName; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all of the call sites need to free this or we're leaking the memory. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks , will create a PR for these changes. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,6 +51,7 @@ | |
#include <mono/metadata/mempool-internals.h> | ||
#include <mono/metadata/mono-basic-block.h> | ||
#include <mono/metadata/mono-endian.h> | ||
#include <mono/metadata/native-library.h> | ||
#include <mono/metadata/threads-types.h> | ||
#include <mono/metadata/custom-attrs-internals.h> | ||
#include <mono/utils/mono-logger-internals.h> | ||
|
@@ -12407,22 +12408,24 @@ emit_file_info (MonoAotCompile *acfg) | |
|
||
if (acfg->aot_opts.static_link) { | ||
char symbol [MAX_SYMBOL_SIZE]; | ||
char *p; | ||
|
||
/* | ||
* Emit a global symbol which can be passed by an embedding app to | ||
* mono_aot_register_module (). The symbol points to a pointer to the file info | ||
* structure. | ||
*/ | ||
sprintf (symbol, "%smono_aot_module_%s_info", acfg->user_symbol_prefix, acfg->image->assembly->aname.name); | ||
|
||
#ifdef TARGET_WASM | ||
acfg->static_linking_symbol = g_strdup (mono_fixup_symbol_name(symbol)); | ||
#else | ||
/* Get rid of characters which cannot occur in symbols */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder why this needs to be wasm-specific, shouldn't we do the same logic everywhere? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It broke There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm that sounds concerning, I don't see why it should break iOS/tvOS There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is what error looked like (from https://dev.azure.com/dnceng-public/public/_build/results?buildId=652242&view=logs&j=88e1dda0-08c6-5d85-6ee4-8e42aeff31b6&t=b4bb5080-7a46-5114-c100-ade75b904ada) My guess is that for iOS/tvOS in other places it still expects the previous logic. And I'm not sure that for other platforms there is a need to apply the change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. then we need to find these places, the logic should be consistent |
||
p = symbol; | ||
char *p = symbol; | ||
for (p = symbol; *p; ++p) { | ||
if (!(isalnum (*p) || *p == '_')) | ||
*p = '_'; | ||
} | ||
acfg->static_linking_symbol = g_strdup (symbol); | ||
#endif | ||
} | ||
|
||
if (acfg->llvm) | ||
|
@@ -14860,7 +14863,6 @@ aot_assembly (MonoAssembly *ass, guint32 jit_opts, MonoAotOptions *aot_options) | |
{ | ||
MonoImage *image = ass->image; | ||
MonoAotCompile *acfg; | ||
char *p; | ||
int res; | ||
TV_DECLARE (atv); | ||
TV_DECLARE (btv); | ||
|
@@ -15120,13 +15122,17 @@ aot_assembly (MonoAssembly *ass, guint32 jit_opts, MonoAotOptions *aot_options) | |
acfg->flags = (MonoAotFileFlags)(acfg->flags | MONO_AOT_FILE_FLAG_LLVM_ONLY); | ||
|
||
acfg->assembly_name_sym = g_strdup (get_assembly_prefix (acfg->image)); | ||
/* Get rid of characters which cannot occur in symbols */ | ||
#ifdef TARGET_WASM | ||
acfg->global_prefix = g_strdup_printf ("mono_aot_%s", g_strdup(mono_fixup_symbol_name (acfg->assembly_name_sym))); | ||
#else | ||
char *p; | ||
/* Get rid of characters which cannot occur in symbols */ | ||
for (p = acfg->assembly_name_sym; *p; ++p) { | ||
if (!(isalnum (*p) || *p == '_')) | ||
*p = '_'; | ||
} | ||
|
||
acfg->global_prefix = g_strdup_printf ("mono_aot_%s", acfg->assembly_name_sym); | ||
#endif | ||
acfg->plt_symbol = g_strdup_printf ("%s_plt", acfg->global_prefix); | ||
acfg->got_symbol = g_strdup_printf ("%s_got", acfg->global_prefix); | ||
if (acfg->llvm) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
using System; | ||
using System.Runtime.InteropServices; | ||
|
||
public unsafe partial class Test | ||
{ | ||
public unsafe static int Main(string[] args) | ||
{ | ||
((IntPtr)(delegate* unmanaged<int,int>)&Interop.Managed8\u4F60Func).ToString(); | ||
|
||
Console.WriteLine($"main: {args.Length}"); | ||
Interop.UnmanagedFunc(); | ||
return 42; | ||
} | ||
} | ||
|
||
file partial class Interop | ||
{ | ||
[UnmanagedCallersOnly(EntryPoint = "ManagedFunc")] | ||
public static int Managed8\u4F60Func(int number) | ||
{ | ||
// called from UnmanagedFunc | ||
Console.WriteLine($"Managed8\u4F60Func({number}) -> 42"); | ||
return 42; | ||
} | ||
|
||
[DllImport("local", EntryPoint = "UnmanagedFunc")] | ||
public static extern void UnmanagedFunc(); // calls ManagedFunc | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
#include <stdio.h> | ||
int ManagedFunc(int number); | ||
|
||
void UnmanagedFunc() | ||
{ | ||
int ret = 0; | ||
printf("UnmanagedFunc calling ManagedFunc\n"); | ||
ret = ManagedFunc(123); | ||
printf("ManagedFunc returned %d\n", ret); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
using System.Reflection.Metadata; | ||
using System.Security.Cryptography; | ||
using System.Text; | ||
using System.Linq; | ||
using Microsoft.Build.Framework; | ||
using Microsoft.Build.Utilities; | ||
|
||
|
@@ -33,6 +34,8 @@ public enum HashEncodingType | |
|
||
private static readonly object s_SyncObj = new object(); | ||
|
||
private static readonly char[] s_charsToReplace = new[] { '.', '-', '+', '<', '>' }; | ||
|
||
public static string GetEmbeddedResource(string file) | ||
{ | ||
using Stream stream = typeof(Utils).Assembly | ||
|
@@ -411,4 +414,33 @@ private static bool IsManagedAssembly(PEReader peReader) | |
return false; | ||
} | ||
} | ||
|
||
// Keep synced with mono_fixup_symbol_name from src/mono/mono/metadata/native-library.c | ||
public static string FixupSymbolName(string name) | ||
{ | ||
UTF8Encoding utf8 = new(); | ||
byte[] bytes = utf8.GetBytes(name); | ||
StringBuilder sb = new(); | ||
|
||
foreach (byte b in bytes) | ||
{ | ||
if ((b >= (byte)'0' && b <= (byte)'9') || | ||
(b >= (byte)'a' && b <= (byte)'z') || | ||
(b >= (byte)'A' && b <= (byte)'Z') || | ||
(b == (byte)'_')) | ||
{ | ||
sb.Append((char)b); | ||
} | ||
else if (s_charsToReplace.Contains((char)b)) | ||
{ | ||
sb.Append('_'); | ||
} | ||
else | ||
{ | ||
sb.Append($"_{b:X}_"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure of the exact constraints for this name mapping algorithm but could we just replace all chars outside of a-z/0-9 with underscore? that would allow us to not allocate in the native side There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is needed when mapping two assemblies to the same name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah so e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure whether it'd be strictly better but we could also collect the special chars and append them at the end or return them separately. that'd have the benefit of not needing to allocate on the native side when the string doesn't have special chars @lambdageek for thoughts There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure how adding the characters in a suffix would make a difference. On the native side I guess it would be nice to have a function that precomputes the required space for the entire mangled name and just does a single allocation. (ie: get rid of the |
||
} | ||
} | ||
|
||
return sb.ToString(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to add a check that sb_index is < 252 (to account for the possible 4 hex bytes + 1 null terminate) in the loop so we don't write past the fixedName boundary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and ideally use snprintf to make it clear the code is doing this