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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

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

wants to merge 5 commits into from

Conversation

radical
Copy link
Member

@radical radical commented Oct 24, 2021 •
edited by lambdageek
Loading

Fixes #60800

@ghost ghost added the area-Build-mono label Oct 24, 2021
@radical radical added the arch-wasm WebAssembly architecture label Oct 24, 2021
@ghost
Copy link

ghost commented Oct 24, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: radical
Assignees: -
Labels:

arch-wasm, area-Build-mono
Milestone: -

@radical radical changed the title [wasm] Fixup symbol names with invalid chars [wasm] PInvokeTableGenerator: Fixup symbol names with invalid chars Oct 24, 2021
lambdageek
lambdageek requested changes Oct 26, 2021
View reviewed changes
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

Maybe we should use an allow-list instead of a deny list. there's a lot of stuff you can technically put in an IL method name, as far as I can tell from ECMA-335.

src/tasks/WasmAppBuilder/PInvokeTableGenerator.cs
public string FileWrites { get; private set; } = string.Empty;

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

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)

src/tasks/WasmAppBuilder/PInvokeTableGenerator.cs
private static string SymbolNameForMethod(MethodInfo method)
{
string module_symbol = method.DeclaringType!.Module!.Assembly!.GetName()!.Name!;
string class_name = method.DeclaringType.Name;
Copy link
Member

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?

src/tasks/WasmAppBuilder/PInvokeTableGenerator.cs
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

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.

@runfoapp runfoapp bot mentioned this pull request Oct 26, 2021
Closed
lambdageek
lambdageek reviewed Oct 27, 2021
View reviewed changes
@lambdageek
Copy link
Member

The interpreter runs of the new test are unhappy for some reason.

wasm test]                  
[wasm test]                  
[wasm test] [00:03:37] fail: console.error: RuntimeError: function signature mismatch
[wasm test]                  
[wasm test] [00:03:37] fail: console.error: RuntimeError: function signature mismatch
[wasm test]                  
[wasm test] [00:03:37] fail:     at <anonymous>:wasm-function[109]:0x6031
[wasm test]                  
[wasm test] [00:03:37] fail:     at <anonymous>:wasm-function[207]:0x173bc
[wasm test]                  
[wasm test] [00:03:37] fail:     at <anonymous>:wasm-function[186]:0x16ca8
[wasm test]                  
[wasm test] [00:03:37] fail:     at <anonymous>:wasm-function[132]:0x78c7
[wasm test]                  
[wasm test] [00:03:37] fail:     at <anonymous>:wasm-function[131]:0x67f0
[wasm test]                  
[wasm test] [00:03:37] fail:     at <anonymous>:wasm-function[8069]:0x1a3a53
[wasm test]                  
[wasm test] [00:03:37] fail:     at <anonymous>:wasm-function[2282]:0x93c92
[wasm test]                  
[wasm test] [00:03:37] fail:     at <anonymous>:wasm-function[2286]:0x943c5
[wasm test]                  
[wasm test] [00:03:37] fail:     at mono_wasm_invoke_method (<anonymous>:wasm-function[9150]:0x1d7100)
[wasm test]                  
[wasm test] [00:03:37] fail:     at Module._mono_wasm_invoke_method (dotnet.js:1:189720)

unclear why

@ghost ghost closed this Nov 26, 2021
@ghost
Copy link

ghost commented Nov 26, 2021

Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it.

@radical radical mentioned this pull request Dec 2, 2021
Merged
radical added a commit that referenced this pull request Dec 4, 2021
@radical
[wasm] Build improvements, and fixes (#61581)
f179b76 
Includes:
- Better error logging, and handling
- Add @(NativeFileReference) to up-to-date check items for VS
- Add `WasmBuild.sln`
- Better fix up of symbol names for pinvokes, and callbacks, based on @lambdageek's suggestion in #60814 (comment)

Fixes #60862
@ghost ghost locked as resolved and limited conversation to collaborators Dec 26, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lambdageek lambdageek lambdageek requested changes

@maraf maraf Awaiting requested review from maraf maraf will be requested when the pull request is marked ready for review maraf is a code owner

@akoeplinger akoeplinger Awaiting requested review from akoeplinger akoeplinger will be requested when the pull request is marked ready for review akoeplinger is a code owner

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

+1 more reviewer

@vargaz vargaz vargaz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

arch-wasm WebAssembly architecture area-Build-mono

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[UnmanagedCallersOnly] is not supported on inner functions in Blazor

3 participants

@radical @lambdageek @vargaz