Make ExternFunctionSymbolNode implement INodeWithTypeSignature by giving them all signatures. - #134811
Open
jtschuster wants to merge 10 commits into
Open
Make ExternFunctionSymbolNode implement INodeWithTypeSignature by giving them all signatures.#134811jtschuster wants to merge 10 commits into
jtschuster wants to merge 10 commits into
Conversation
On Wasm, calls to extern functions (native runtime helpers, direct P/Invoke targets) need a typed function import, so extern function symbols now carry a signature: - ExternFunctionSymbolNode implements INodeWithTypeSignature by forwarding to an ExternalTypeSignature passed to its constructor (null when the function has no standard-ABI signature). - NodeFactory keeps one ExternFunctionSymbolNode per symbol name, created by DirectPInvokeTarget (the P/Invoke's own signature) or KnownExternFunction. All creators must agree on a name's signature (asserted), so a JIT helper and a direct P/Invoke to the same function (e.g. memset) share one node. - JitHelper and CorInfoImpl use a KnownExternFunction enum instead of string helper names. KnownExternFunctions maps each to its (unchanged, architecture-dependent) symbol name and its unmanaged signature. - On Wasm, each extern function depends on a WasmFunctionImportNode (one per name) that writes its own import-section entry, with its type index as a WASM_TYPE_INDEX_LEB relocation; the object writer assigns the import's function index when it records the node. - Remove INodeWithWasmSignature.
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib |
Source-sharing creates pinvokes that are for the same method but have different managed identities for their enum parameters. This causes the assert in NodeFactory to fire for these methods. In cases where the signatures don't match, check the Wasm lowered signature as a fallback. Also, limit signature checking to WASM compilations.
| /// and the mapping must produce, byte-for-byte, the strings of the exports that they correspond to | ||
| /// (including their architecture-dependent variants). | ||
| /// </summary> | ||
| public enum KnownExternFunction |
Member
There was a problem hiding this comment.
This enum looks like the existing ReadyToRunHelper enum with some additions. Can we just use ReadyToRunHelper enum? We can add things that are not part of the ReadyToRun format at the end of the enum, we already do that.
Comment on lines
+195
to
+197
| KnownExternFunction? knownFunction; | ||
| MethodDesc methodDesc; | ||
| JitHelper.GetEntryPoint(_compilation.TypeSystemContext, key, out mangledName, out methodDesc); | ||
| Debug.Assert(mangledName != null || methodDesc != null); | ||
| JitHelper.GetEntryPoint(_compilation.TypeSystemContext, key, out knownFunction, out methodDesc); |
Member
There was a problem hiding this comment.
We could then change GetEntryPoint to return a MethodDesc; if it returns null, the helper is referred to using the ReadyToRunHelper.
jtschuster
marked this pull request as ready for review
October 1, 2026 22:42
`ldvirtftn` that is not part of a verifiable delegate creation sequence is currently not implemented. We need to give RyuJIT something while it's asking `getCallInfo` before it detects delegate creation sequence. We used to give out a symbol that doesn't resolve so this would be a linking failure if it's indeed not replaced with a delegate creation. This is kind of getting in the way for WASM so instead give out an invalid handle. Now it will be a compile failure. Somewhat harder to root cause, but still not terrible.
Separate method declarations from definitions and prepare typed imports for unresolved symbols before layout. Remove WasmFunctionImportNode and its factory plumbing, and correct the signature interfaces' definition semantics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5e8796d9-2e40-4222-9807-7c290cd0cd6b
Member
|
I have a couple suggestions over at jtschuster#106. (I wasn't sure if they would work out and I didn't want to send you on a hike, so I just tried it myself.)
|
The NYI_LDVIRTFTN is a hack (we need to give RyuJIT something so we were just giving something that won't link. I replaced it with something that won't compile, it doesn't matter) KnownExternFunction is what we call ReadyToRunHelper, so let's just name it like that. Also, use the helper ID as the dictionary key because it's a lot more efficient than anything else we could do. WasmFunctionImportNode was externalizing something that is really a WASM file format detail. And the object writer already knows how to do imports. So moved it there. ExternalTypeSignature was a new concept. New concepts need to pay for themselves. This one didn't seem to pay for itself.
Member
Author
|
Those changes look good to me, I'll merge into this PR. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On Wasm, calls to extern functions (native runtime helpers, direct P/Invoke targets) need a typed function import, so extern function symbols now carry a signature:
A follow up to this would be to create WasmImport nodes for each possible import kind in WASM and move WasmObjectWriter.CreateDefaultImports to the dependency graph. This would allow WasmImportSection to derive from WasmExternallyCountedSection and simplify RecordFunctionImport.