Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR updates the WASM RyuJIT emitter to provide real call-indirect signatures by deriving argument lowering from method handles and by hard-coding signatures for certain runtime helpers, then passing those signatures to the JIT↔EE getWasmTypeSymbol API.
Changes:
- Build a
CorInfoWasmTypesignature array foremitIns_Callwhenparams.methHndis available. - Special-case a set of helper calls (detected via
eeGetHelperNum) with hard-coded signatures. - Use the computed signature to obtain a
CORINFO_WASM_TYPE_SYMBOL_HANDLEforcall_indirect.
| CorInfoWasmType *types = nullptr; | ||
| size_t typeCount = 0; |
There was a problem hiding this comment.
types and typeCount are initialized to nullptr and 0, but if the switch falls through to the default case with NYI_WASM (which may not abort in all build configurations), getWasmTypeSymbol at line 2304 would be called with a null pointer and zero count. However, a more immediate concern is that in non-debug/non-fatal NYI_WASM configurations, the unreached() after the NYI_WASM may also be compiled out, leading to undefined behavior. Consider initializing types and typeCount to safe defaults or restructuring so that the default case cannot fall through to the getWasmTypeSymbol call.
| #define _SIG(helper_id, ...) \ | ||
| case helper_id: \ | ||
| { \ | ||
| static CorInfoWasmType helper_id ## _types[] = {__VA_ARGS__}; \ | ||
| types = helper_id ## _types; \ | ||
| typeCount = sizeof(helper_id ## _types) / sizeof(CorInfoWasmType); \ | ||
| break; \ | ||
| } |
There was a problem hiding this comment.
The _SIG macro with token pasting creates static local arrays inside case blocks. While functional, this macro is fragile (e.g., it won't compose well if a helper ID contains special characters) and uses sizeof/sizeof instead of a safer ArrLen or std::size pattern that's used elsewhere in the codebase (see ArrLen(typecode_mapping) in emitwasm.cpp). Consider using ArrLen for consistency, or refactoring to a data-driven table lookup instead of a macro-based switch.
| _SIG(CORINFO_HELP_BULK_WRITEBARRIER, CORINFO_WASM_TYPE_VOID /* retval */, CORINFO_WASM_TYPE_I32, CORINFO_WASM_TYPE_I32, CORINFO_WASM_TYPE_I32); | ||
|
|
||
| default: | ||
| printf("Helper %d (%s) has no hard-coded signature\n", helper, GetCompiler()->eeGetMethodFullName(params.methHnd)); |
There was a problem hiding this comment.
Using raw printf for diagnostic output is inconsistent with the rest of the JIT, which typically uses JITDUMP or other compiler-provided logging macros that respect debug/verbosity settings. This printf will unconditionally write to stdout in release builds. Consider replacing it with JITDUMP or wrapping it in a #ifdef DEBUG guard.
| printf("Helper %d (%s) has no hard-coded signature\n", helper, GetCompiler()->eeGetMethodFullName(params.methHnd)); | |
| JITDUMP("Helper %d (%s) has no hard-coded signature\n", helper, GetCompiler()->eeGetMethodFullName(params.methHnd)); |
| if (wvt >= WasmValueType::Count) | ||
| { | ||
| NYI_WASM("Unrecognized register type in genCallInstruction"); | ||
| } | ||
| else | ||
| { | ||
| typeStack.Push((CorInfoWasmType)emitter::GetWasmValueTypeCode(wvt)); | ||
| } |
There was a problem hiding this comment.
When wvt >= WasmValueType::Count, the code hits NYI_WASM but then continues execution (skipping the Push), producing a signature with a missing argument type. This will result in a mismatched call signature at the Wasm level. If NYI_WASM is not fatal, the generated signature will be silently incorrect. Consider either making this unconditionally fatal or pushing a placeholder type to maintain signature arity.
| #if !TARGET_WASM | ||
| if (!hasIndirectionCell) | ||
| { | ||
| // Non-virtual direct calls to addresses accessed by |
There was a problem hiding this comment.
The #if !TARGET_WASM guard only wraps part of the logic within this case. If hasIndirectionCell is false on Wasm, result may be left uninitialized or in an unexpected state since the indirection load is skipped. Consider verifying that the fallthrough path correctly handles the Wasm case, or adding a comment explaining why skipping this block is safe.
No description provided.