Skip to content

[wasm] PInvokeTableGenerator: Fixup symbol names with invalid chars #60814

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.

Sign up for GitHub

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

Closed
radical wants to merge 5 commits into from
Closed

[wasm] PInvokeTableGenerator: Fixup symbol names with invalid chars #60814

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1cfeab3
[wasm] Fixup symbol names with invalid chars
radical Oct 24, 2021
68192c0
Add UnmanagedCallersOnly testcase using local functions and nested cl…
lambdageek Oct 26, 2021
45c60ae
fix whitespace
lambdageek Oct 26, 2021
10dca6c
Run the new test
lambdageek Oct 26, 2021
582d3b7
Fix string interpolation
lambdageek Oct 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 29 additions & 29 deletions src/tasks/WasmAppBuilder/PInvokeTableGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public class PInvokeTableGenerator : Task
[Output]
public string FileWrites { get; private set; } = string.Empty;

private static char[] s_charsToReplace = new[] { '.', '-', };
private static char[] s_charsToReplace = new[] { '.', '-', ',', '|', '<', '>' };
Copy link
Member

@lambdageek lambdageek Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of this.

For one thing it's incomplete - for nested classes we also need to replace '/' when it's in a class name. For another thing, C# allows many unicode characters in an identifier (https://docs.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/identifier-names). And IL metadata actually doesn't seem to have any restrictions on identifiers - it's just sequences of bytes. I think we should do something like: convert the name to utf8; look at the bytes; for any bytes not in [0-9A-Za-z] replace them by "" or underscore and the byte hex code (ie F☃ will become "F_E2_98_83" https://www.compart.com/en/unicode/U+2603)


public override bool Execute()
{
Expand Down Expand Up @@ -157,7 +157,7 @@ private void EmitPInvokeTable(StreamWriter w, Dictionary<string, string> modules

foreach (var module in modules.Keys)
{
string symbol = ModuleNameToId(module) + "_imports";
string symbol = FixupSymbolName(module) + "_imports";
w.WriteLine("static PinvokeImport " + symbol + " [] = {");

var assemblies_pinvokes = pinvokes.
Expand All @@ -176,7 +176,7 @@ private void EmitPInvokeTable(StreamWriter w, Dictionary<string, string> modules
w.Write("static void *pinvoke_tables[] = { ");
foreach (var module in modules.Keys)
{
string symbol = ModuleNameToId(module) + "_imports";
string symbol = FixupSymbolName(module) + "_imports";
w.Write(symbol + ",");
}
w.WriteLine("};");
Expand All @@ -187,18 +187,6 @@ private void EmitPInvokeTable(StreamWriter w, Dictionary<string, string> modules
}
w.WriteLine("};");

static string ModuleNameToId(string name)
{
if (name.IndexOfAny(s_charsToReplace) < 0)
return name;

string fixedName = name;
foreach (char c in s_charsToReplace)
fixedName = fixedName.Replace(c, '_');

return fixedName;
}

static bool ShouldTreatAsVariadic(PInvoke[] candidates)
{
if (candidates.Length < 2)
Expand All @@ -216,6 +204,26 @@ static bool ShouldTreatAsVariadic(PInvoke[] candidates)
}
}

private static string FixupSymbolName(string name)
{
if (name.IndexOfAny(s_charsToReplace) < 0)
return name;

string fixedName = name;
foreach (char c in s_charsToReplace)
fixedName = fixedName.Replace(c, '_');

return fixedName;
}

private static string SymbolNameForMethod(MethodInfo method)
{
string module_symbol = method.DeclaringType!.Module!.Assembly!.GetName()!.Name!;
string class_name = method.DeclaringType.Name;
Copy link
Member

@lambdageek lambdageek Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check method.DeclaringType.IsNested and include the enclosing types' names?

string method_name = method.Name;
return FixupSymbolName($"{module_symbol}_{class_name}_{method_name}");
}

private string MapType (Type t)
{
string name = t.Name;
Expand Down Expand Up @@ -343,15 +351,12 @@ private void EmitNativeToInterp(StreamWriter w, List<PInvokeCallback> callbacks)

bool is_void = method.ReturnType.Name == "Void";

string module_symbol = method.DeclaringType!.Module!.Assembly!.GetName()!.Name!.Replace(".", "_");
uint token = (uint)method.MetadataToken;
string class_name = method.DeclaringType.Name;
string method_name = method.Name;
string entry_name = $"wasm_native_to_interp_{module_symbol}_{class_name}_{method_name}";
string entry_name = $"wasm_native_to_interp_{SymbolNameForMethod(method)}";
Copy link
Member

@lambdageek lambdageek Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're compiling for !aot, we should check if the method UnmanagedCallersOnlyAttribute has an EntryPoint and emit the wasm_native_to_interp callback with the supplied name.

if (callbackNames.Contains (entry_name))
{
Error($"Two callbacks with the same name '{method_name}' are not supported.");
Error($"Two callbacks with the same name '{entry_name}' are not supported.");
}

callbackNames.Add (entry_name);
cb.EntryName = entry_name;
sb.Append(MapType(method.ReturnType));
Expand Down Expand Up @@ -395,21 +400,16 @@ private void EmitNativeToInterp(StreamWriter w, List<PInvokeCallback> callbacks)
// Array of function pointers
w.Write ("static void *wasm_native_to_interp_funcs[] = { ");
foreach (var cb in callbacks) {
w.Write (cb.EntryName + ",");
w.Write ($"\t{cb.EntryName},{Environment.NewLine}");
}
w.WriteLine ("};");

// Lookup table from method->interp entry
// The key is a string of the form <assembly name>_<method token>
// FIXME: Use a better encoding
w.Write ("static const char *wasm_native_to_interp_map[] = { ");
foreach (var cb in callbacks) {
var method = cb.Method;
string module_symbol = method.DeclaringType!.Module!.Assembly!.GetName()!.Name!.Replace(".", "_");
string class_name = method.DeclaringType.Name;
string method_name = method.Name;
w.WriteLine ($"\"{module_symbol}_{class_name}_{method_name}\",");
}
foreach (var cb in callbacks)
w.WriteLine ($"\"{SymbolNameForMethod(cb.Method)}\",");
w.WriteLine ("};");
}

Expand Down
1 change: 1 addition & 0 deletions src/tests/BuildWasmApps/Wasm.Build.Tests/BuildTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ protected static void InitProjectDir(string dir)
<PropertyGroup>
<TargetFramework>{s_targetFramework}</TargetFramework>
<OutputType>Exe</OutputType>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<WasmGenerateRunV8Script>true</WasmGenerateRunV8Script>
<WasmMainJSPath>runtime-test.js</WasmMainJSPath>
##EXTRA_PROPERTIES##
Expand Down
6 changes: 3 additions & 3 deletions src/tests/BuildWasmApps/Wasm.Build.Tests/NativeLibraryTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@ public NativeLibraryTests(ITestOutputHelper output, SharedBuildPerTestClassFixtu
{
}

[ConditionalTheory(typeof(BuildTestBase), nameof(IsUsingWorkloads))]
[Theory]
[BuildAndRun(aot: false)]
[BuildAndRun(aot: true)]
public void ProjectWithNativeReference(BuildArgs buildArgs, RunHost host, string id)
{
string projectName = $"AppUsingNativeLib-a";
buildArgs = buildArgs with { ProjectName = projectName };
buildArgs = ExpandBuildArgs(buildArgs, extraItems: "<NativeFileReference Include=\"native-lib.o\" />");
buildArgs = ExpandBuildArgs(buildArgs, extraItems: "<NativeFileReference Include=\"native-lib.cpp\" />");

if (!_buildContext.TryGetBuildFor(buildArgs, out BuildProduct? _))
{
Expand All @@ -33,7 +33,6 @@ public void ProjectWithNativeReference(BuildArgs buildArgs, RunHost host, string
Directory.Delete(_projectDir, recursive: true);

Utils.DirectoryCopy(Path.Combine(BuildEnvironment.TestAssetsPath, "AppUsingNativeLib"), _projectDir);
File.Copy(Path.Combine(BuildEnvironment.TestAssetsPath, "native-libs", "native-lib.o"), Path.Combine(_projectDir, "native-lib.o"));
}

BuildProject(buildArgs,
Expand All @@ -45,6 +44,7 @@ public void ProjectWithNativeReference(BuildArgs buildArgs, RunHost host, string
host: host, id: id);

Assert.Contains("print_line: 100", output);
Assert.Contains("total in helper: 253", output);
Assert.Contains("from pinvoke: 142", output);
}

Expand Down
31 changes: 31 additions & 0 deletions src/tests/BuildWasmApps/testassets/AppUsingNativeLib/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,41 @@ public class Test
public static int Main(string[] args)
{
Console.WriteLine ($"from pinvoke: {SimpleConsole.Test.print_line(100)}");
new Helper();
return 0;
}

class Helper {
public Helper () {
int t = 0;
unsafe {
delegate *unmanaged<int,int> fn = &nested_helper;
t += native_intint_callback_acceptor ((IntPtr)fn, 1);

fn = &member_helper;

t += native_intint_callback_acceptor ((IntPtr)fn, 2);
}

Console.WriteLine ($"total in helper: {t}");

// local function inside a nested class ctor. Mangled name will be something like
// int32 SimpleConsole.Test/Helper::'<.ctor>g__Helper|1_0'
[UnmanagedCallersOnly]
static int nested_helper (int j) => j + 20;

}
}

[UnmanagedCallersOnly]
private static int member_helper(int x) => x + 30;


[DllImport("native-lib")]
public static extern int print_line(int x);

// FIXME: support function pointers in pinvoke arguments
[DllImport("native-lib")]
public static unsafe extern int native_intint_callback_acceptor(/*delegate *unmanaged<int,int>*/ IntPtr fn, int i);
}
}
5 changes: 5 additions & 0 deletions src/tests/BuildWasmApps/testassets/AppUsingNativeLib/native-lib.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,8 @@ int print_line(int x)
printf("print_line: %d\n", x);
return 42 + x;
}

int native_intint_callback_acceptor(ManagedIntIntCallback fn, int i)
{
return fn(i + 100);
}
4 changes: 4 additions & 0 deletions src/tests/BuildWasmApps/testassets/AppUsingNativeLib/native-lib.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ extern "C" {

int print_line(int x);

typedef int (*ManagedIntIntCallback)(int x);

int native_intint_callback_acceptor(ManagedIntIntCallback fn, int i);

#ifdef __cplusplus
}
#endif
Expand Down
Binary file removed src/tests/BuildWasmApps/testassets/native-libs/native-lib.o
View file
Open in desktop
Binary file not shown.