[NativeAOT-LLVM] Implement WasmImportlinkage

[yowl]

This PR implements the new WasmImportlinkageAttribute and remove the old policy based mechanism. Added a test for the browser target as it is easier to fulfill the import in Javascript.

Added a key ExternSymbolKey to _externSymbolsWithAccessors to distinguish between Wasm imports and normal DllImports, Wasm imports of the same function from different modules, and imports of the same name but with different signatures.

When writing the object files, added a new dictionary _wasmImportLinkages to track emitted functions with the wasm import attributes. This allows us to remove duplicate imports as they behaviour for that is not defined.

[yowl]

cc @dotnet/nativeaot-llvm

[SingleAccretion]

Is all of the JS needed? It would be nice to trim it down to a more readable state.

[yowl]

Have removed most of it.

[SingleAccretion]

I think more is needed to implement the behavior in #2414 (comment).

WasmImportLinkage-attributed methods create what is effectively a second namespace to the static symbols, so, for example:

[DllImport("xyz", EntryPoint = "xyz_func"), WasmImportLinkage]
void func_1();

[DllImport("*", EntryPoint = "xyz_func")]
void func_2();

Here, we will link in func_2 statically, and func_1 as an xyz_func import from the xyz module. It means signature mismatch and AddFunctionAttribute("wasm-import-name") must be segregated for these two categories too. For example:

[DllImport("xyz", EntryPoint = "xyz_func"), WasmImportLinkage]
void func_2(int arg);
[DllImport("xyz", EntryPoint = "xyz_func"), WasmImportLinkage]
void func_3();

Should report a warning (and choose which signature to import? Or import none?).

While this:

[DllImport("not_xyz", EntryPoint = "xyz_func"), WasmImportLinkage]
void func_2(int arg);
[DllImport("xyz", EntryPoint = "xyz_func"), WasmImportLinkage]
void func_3();

Should not report a warning, etc.

Naturally, a number of tests is needed to verify all these interactions.

[yowl]

Or import none?

I think that would be safer.

[yowl]

must be segregated

I was thinking that I would change

runtimelab/src/coreclr/tools/aot/ILCompiler.LLVM/Compiler/DependencyAnalysis/LLVMCodegenNodeFactory.cs

Line 16 in 7c00d39

private readonly Dictionary<string, ExternMethodAccessorNode> _externSymbolsWithAccessors = new();

to have a key of a struct with the necessary fields to do this segregation. Does that sound correct?

[SingleAccretion]

SGTM.

[yowl]

Naturally, a number of tests is needed to verify all these interactions.

There are just 3 tests:

1. The original which just tests we place the import in the correct module
2. A test for not importing anything if there are duplciate module/func names
3. A test for the same function in different modules (a minor variation on 1 I suppose)

Do we want some others?

[yowl]

Thanks, I'd completely forgotten about the changes we were going to do for DirectPInvoke, sorry. I've added a few more tests but I don't think I've got the tests for PNSE, or signature warnings quite right. We suppress IL3049 for HelloWasm and I didn't really understand what is happening with the switch in TestNativeCallsWithMismatchedSignatures

[SingleAccretion]

We suppress IL3049 for HelloWasm and I didn't really understand what is happening with the switch in TestNativeCallsWithMismatchedSignatures

The basic idea of the test is that invalid PIs do not fail the compilation and do not fail the binding of valid PIs (the latter will not be true for WASM imports, but, oh well). So it's not really testing the warning directly in any special way.

[yowl]

• It is fine to defer the work of actually throwing the exception from this PR.

I'm missing something here. If we don't do the generation of the PNSE here how do we do the test for 3:

[DllImport("not_xyz", EntryPoint="func")]
void func_1(); // Throws PNSE

[SingleAccretion]

Just #if 0 it out for now?

[yowl]

ok, a few are like this that throw unmanaged exceptions in the wasm runtime.

[SingleAccretion]

WasmImportLinkage

Probably worth mentioning that:

`WasmImportLinkage` is only available when you build against the latest nightly SDK, although you can define it manually as well:
<copy paste minimal definition of WasmImportLinkage>

[yowl]

updated the docs with this, thanks

[SingleAccretion]

I think it's time for us to start using Microsoft.NETCore.Native.Wasm.props (which should be renamed to .targets), and add this there, similar to how it is for Unix.targets.

[yowl]

Done

[SingleAccretion]

EcmaMethod.cs is core type system code, representing only ECMA-defined concepts. The attribute checking will have be done elsewhere else.

[yowl]

Reverted

[SingleAccretion]

I just checked, DirectPInvokePolicy.cs is actually (unused) upstream code. So the conflict-free strategy here would be to not touch it in this change, merge it, and then delete it upstream.

[yowl]

ok, will do.

[SingleAccretion]

Wasm imports are different if any of module name, function name, ABI signature are different

The WASM file format does support "overloaded" imports, but we do not (since LLVM does not, I believe). So the signature is not part of the symbol resolution.

[yowl]

Ok, this was related to getting the warning out in LLVMObjectWriter, I'll redo that in line with the other comments

[yowl]

removed now.

[SingleAccretion]

This I am struggling to understand the need for _wasmImportLinkages. The node factory should unique accessor nodes for the same WASM import, so why would we be adding the same one twice?

[yowl]

You are right, it can all be done the same as direct P/Invokes.

[SingleAccretion]

The layering here no longer quite makes sense. Originally, this only served static symbols, so we would go like so:

codegen asks LLVMCodegenCompilation
  for an accessor to a static PInvoke (via 'GetExternalMethodAccessor')
    LLVMCodegenCompilation asks ConfigurablePInvokePolicy
      for a direct symbol name for that static PI
    LLVMCodegenCompilation asks LLVMCodegenNodeFactory
      to produce a node representing the accessor, for that symbol name

Somewhat non-obviously, but importantly, method/sig here are auxiliary information essentially only used for diagnostic purposes. Now we are using method to essentially "skip over" the PI policy, which controls symbol resolution.

I think we can use the PI policy mechanism to our advantage and make this a lot cleaner:

1. Create a new WasmConfigurablePInvokePolicy : ConfigurablePInvokePolicy that would do override GenerateDirectCall somewhat like so:

public override bool GenerateDirectCall(MethodDesc method, out string externName)
{
    if (IsWasmImport(method))
    {
        externName = "__wasm_import_" + moduleName + "_" + externName;
        return true;
    }
}

2. Leave all of the accessor code essentially unchanged.
3. In LLVM object writer, ask the policy whether the accessor node represents a WASM import. Could be simple as asking whether the symbol name starts with __wasm_import_.

[yowl]

sounds good, thanks.

[SingleAccretion]

It is surprising that this is needed. Isn't it already imported via *?

[yowl]

Removed.

[SingleAccretion]

Could you add the same kind of test to the WasmImport section above?

[yowl]

Done.

[SingleAccretion]

This should technically be a try { } catch (EntryPointNotFoundException) { } test.

[yowl]

Have implemented like this, but is #if out for now as we dont throw EntryPointNotFoundException yet, same situation as PNSE I suppose.

[SingleAccretion]

Could you also add the scenario 3 test?

[yowl]

Added the missing tests.

[yowl]

Think the last commit addresses the remaining review comments, thanks.

[SingleAccretion]

pInvokeMetadata.Name

Does this work correctly with PIs that don't specify EntryPoint (if not, it also needs a test)?

[yowl]

It seems to yes, added the test:

private static void TestNamedModuleCallWithoutEntryPoint()
{
    StartTest("Wasm import from named module test");
    EndTest(ModuleFuncWithoutEntryPoint(77) == 77);
}

[DllImport("ModuleName"), WasmImportLinkage]
private static extern int ModuleFuncWithoutEntryPoint(int x);

Generates

declare i32 @__wasm_import_ModuleName_ModuleFuncWithoutEntryPoint(i32) #4

define ptr @get.__wasm_import_ModuleName_ModuleFuncWithoutEntryPoint() {
GetExternFunc:
  ret ptr @__wasm_import_ModuleName_ModuleFuncWithoutEntryPoint
}

attributes #4 = { "wasm-import-module"="ModuleName" "wasm-import-name"="ModuleFuncWithoutEntryPoint" }

[SingleAccretion]

It would be better to consolidate this logic in the policy.

You can add a virtual bool GenerateWasmImportCall(MethodDesc method, out string name, out string moduleName) to PInvokeILEmitterConfiguration, and expose it through PInvokeILProvider as, e. g., GetWasmImportCallInfo

[yowl]

Thanks, yes that's cleaner.

[SingleAccretion]

Perf nit: adding a GetSingleMethod() to the accessor node, like:

_methods switch { MethodDesc methods => method, _ => /* foreach loop over HashSet<MethodDesc> */ }

will avoid allocating the enumerator.

[yowl]

Thanks, I did the switch more like the one earlier in the file as I couldn't see how to do the foreach with the other style as I think it needs an expression.

switch (_methods)
{
    case MethodDesc oneMethod:
        return oneMethod;

    default:
        foreach (MethodDesc method in (HashSet<MethodDesc>)_methods)
        {
            return method;
        }

        return null;
}

[SingleAccretion]

Thank you for making the changes, this LGTM!

Could you open an issue detailing what is still missing (PNSE/EPNF)?

[yowl]

Thanks for the extended review!

[jkotas]

Looks great! Thank you for adding a lot of tests.