[do not review] test regression 3#124523
Conversation
Set `isTailcall` to `false`, when preparing the call. This is similar to how we set other state vars for the call, so I did it this way. Alternatively we can reset the `isTailcall` after updating the stack and frame for tailcalls. That would invalidate the meaning of the variable for the rest of the tailcall though, so we would need to rename it at least to signal, that it is valid only in the beginning of the tailcall.
Based on changes from the previous branch (https://github.com/radekdoulik/runtime/tree/clr-wasm-runtime-tests-with-node)
…s' into clr-wasm-runtime-tests-on-node-2
…rity 1 c… (dotnet#124303)" This reverts commit 3ea3efc.
|
Tagging subscribers to this area: @agocke |
There was a problem hiding this comment.
Pull request overview
This PR updates the test infrastructure and WASM/CoreCLR hosting to support running browser/wasm CoreCLR tests under NodeJS, adjusts Helix/pipeline wiring for new WASM CoreCLR scenarios, and applies a number of WASM- or threading-related test gating changes.
Changes:
- Add
--node/RunWithNodeJSplumbing acrossrun.sh/run.cmd/run.pyand test execution targets to enable NodeJS-based browser/wasm execution (including timeout handling). - Update browserhost/corerun shutdown and timer-abort behavior to stabilize exit/shutdown sequencing.
- Add/adjust WASM-related test conditions and exclusions, plus build/pipeline adjustments for browser/wasm CoreCLR testing.
Reviewed changes
Copilot reviewed 81 out of 83 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/run.sh | Adds --node switch and defaults NodeJS for wasm runs. |
| src/tests/run.py | Adds --node arg and sets RunWithNodeJS. |
| src/tests/run.cmd | Adds node argument plumbing for Windows test runner. |
| src/tests/baseservices/threading/coverage/OSThreadId/OSThreadId.cs | Switches to ConditionalFact based on threading support. |
| src/tests/Loader/classloader/StaticVirtualMethods/Reabstraction/Reabstraction.il | Adds ActiveIssue gating for WASM. |
| src/tests/Loader/classloader/StaticVirtualMethods/NegativeTestCases/AmbiguousImplementationException.il | Refactors/adds ActiveIssue gating (incl. WASM). |
| src/tests/Loader/classloader/StaticVirtualMethods/DiamondShape/svm_diamondshape.il | Refactors/adds ActiveIssue gating (incl. WASM). |
| src/tests/Loader/classloader/Casting/Functionpointer.cs | Adds ActiveIssue gating for WASM. |
| src/tests/JIT/opt/Regressions/Regression3_Regressions.cs | Adds ActiveIssue gating for WASM. |
| src/tests/JIT/jit64/regress/vsw/373472/test.il | Adds ActiveIssue gating for WASM. |
| src/tests/JIT/jit64/opt/rngchk/ArrayWithThread.cs | Switches to ConditionalFact based on threading support; reorder usings. |
| src/tests/JIT/SIMD/Sums.cs | Reduces workload size on WASM. |
| src/tests/JIT/Regression/JitBlue/Runtime_79354/Runtime_79354.cs | Adds ActiveIssue gating for WASM. |
| src/tests/JIT/Regression/JitBlue/Runtime_72265/Runtime_72265.cs | Adds ActiveIssue gating for WASM. |
| src/tests/JIT/Regression/JitBlue/Runtime_70259/Runtime_70259.il | Adds TestLibrary ref + ActiveIssue gating for WASM. |
| src/tests/JIT/Regression/JitBlue/Runtime_70259/Runtime_70259.cs | Adds ActiveIssue gating for WASM. |
| src/tests/JIT/Regression/JitBlue/Runtime_105619/Runtime_105619.cs | Adds ActiveIssue gating for WASM. |
| src/tests/JIT/Regression/JitBlue/GitHub_4044/GitHub_4044.cs | Adds ActiveIssue gating for WASM. |
| src/tests/JIT/Regression/JitBlue/GitHub_19438/GitHub_19438.cs | Reduces workload size on WASM. |
| src/tests/JIT/Regression/CLR-x86-JIT/V1-M12-Beta2/b68872/b68872.ilproj | Adds TestLibrary project reference for new attributes. |
| src/tests/JIT/Regression/CLR-x86-JIT/V1-M12-Beta2/b68872/b68872.il | Adds TestLibrary + ActiveIssue gating for WASM. |
| src/tests/JIT/Methodical/switch/switch6.il | Adds TestLibrary + ActiveIssue gating for WASM. |
| src/tests/JIT/Methodical/flowgraph/bug619534/moduleHandleCache.cs | Adds ActiveIssue gating for WASM. |
| src/tests/JIT/Methodical/eh/basics/throwinfilter_r.ilproj | Adds TestLibrary project reference. |
| src/tests/JIT/Methodical/eh/basics/throwinfilter_d.ilproj | Adds TestLibrary project reference. |
| src/tests/JIT/Methodical/eh/basics/throwinfilter.il | Adds ActiveIssue gating for WASM. |
| src/tests/JIT/Methodical/delegate/GSDelegate.cs | Adds ActiveIssue gating for WASM. |
| src/tests/JIT/Methodical/Methodical_others.csproj | Adds TestLibrary project reference. |
| src/tests/JIT/Methodical/Boxing/morph/sin3double.il | Adds TestLibrary + ActiveIssue gating for WASM. |
| src/tests/JIT/Directed/Misc/function_pointer/MutualThdRecur-fptr.il | Adds missing extern refs needed for new attributes/TestLibrary. |
| src/tests/JIT/CodeGenBringUpTests/LocallocLarge.cs | Switches to ConditionalFact based on threading support; reorder usings. |
| src/tests/Common/mergedrunner.targets | Adjusts mobile merged-runner import condition for wasm/CoreCLR. |
| src/tests/Common/helixpublishwitharcade.proj | Splits browser handling for Mono vs CoreCLR and sets NodeJS env for CoreCLR browser runs. |
| src/tests/Common/XUnitWrapperGenerator/XUnitWrapperGenerator.cs | Chooses XHarness runner generation based on OS + runtime flavor (Mono browser only). |
| src/tests/Common/CoreCLRTestLibrary/PlatformDetection.cs | Adds IsWasm helper and related comments. |
| src/tests/Common/CoreCLRTestLibrary/CoreClrConfigurationDetection.cs | Treats WASM as interpreter for now. |
| src/tests/Common/CLRTest.Execute.Batch.targets | Adds NodeJS execution path for browser on Windows + timeout helper; wasm build precommands gate on NodeJS. |
| src/tests/Common/CLRTest.Execute.Bash.targets | Adds NodeJS execution path for browser + timeout wrapper; wasm build precommands gate on NodeJS. |
| src/tasks/WasmAppBuilder/generate-coreclr-helpers.sh | New helper script to run generator for CoreCLR wasm helpers. |
| src/tasks/WasmAppBuilder/generate-coreclr-helpers.cmd | Improves quoting and argument passing for generator invocation. |
| src/tasks/WasmAppBuilder/coreclr/PInvokeTableGenerator.cs | Renames lookup import to LookupMethodByName. |
| src/tasks/WasmAppBuilder/coreclr/ManagedToNativeGenerator.cs | Expands supported signature “cookies” list. |
| src/native/libs/System.Native.Browser/utils/host.ts | Adds abort-state checks and marks runtime aborting to stop timer scheduling. |
| src/native/libs/System.Native.Browser/native/scheduling.ts | Avoids scheduling while aborting. |
| src/native/libs/System.Native.Browser/native/crypto.ts | Switches to typed DOTNET.cryptoWarnOnce field access. |
| src/native/libs/System.Native.Browser/libSystem.Native.Browser.Utils.footer.js | Exports abort dependency symbol. |
| src/native/libs/Common/JavaScript/types/ems-ambient.ts | Tightens typings for DOTNET/DOTNET_INTEROP and adds shutdown/exit hooks. |
| src/native/corehost/browserhost/libBrowserHost.footer.js | Exposes BrowserHost_ShutdownCoreCLR and __funcs_on_exit. |
| src/native/corehost/browserhost/host/index.ts | Hooks ___funcs_on_exit to abort timers and shut down CoreCLR. |
| src/native/corehost/browserhost/host/host.ts | Captures exit status from native execute and propagates via runMainAndExit. |
| src/native/corehost/browserhost/browserhost.cpp | Returns managed exit code from execute and adds shutdown export. |
| src/mono/sample/wasm/browser/wwwroot/main.js | Moves “exited normally” log after runMainAndExit. |
| src/libraries/System.Private.CoreLib/src/System/StartupHookProvider.cs | Removes UCO wrapper overload used by reverse-P/Invoke path. |
| src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs | Removes UCO wrapper overload used by reverse-P/Invoke path. |
| src/coreclr/vm/wasm/callhelpers-reverse.cpp | Switches lookup import name to LookupMethodByName. |
| src/coreclr/vm/wasm/callhelpers-pinvoke.cpp | Updates P/Invoke table exports/signatures (e.g., Dup, locking return type). |
| src/coreclr/vm/wasm/callhelpers-interp-to-managed.cpp | Adds additional thunk implementations and maps. |
| src/coreclr/vm/threads.h | Adds Thread::GetCulture/SetCulture declarations. |
| src/coreclr/vm/threads.cpp | Implements Thread::GetCulture/SetCulture. |
| src/coreclr/vm/metasig.h | Removes metasigs used by removed UCO wrappers. |
| src/coreclr/vm/loaderallocator.cpp | Replaces UCO caller with direct ctor invocation for LoaderAllocator. |
| src/coreclr/vm/interpexec.h | Renames lookup import to LookupMethodByName. |
| src/coreclr/vm/interoputil.h | Removes COM-interp culture helpers declarations. |
| src/coreclr/vm/interoputil.cpp | Replaces UCO marshaler calls with direct managed calls (CultureInfo + ColorMarshaler). |
| src/coreclr/vm/eventing/eventpipe/ds-rt-coreclr.h | Replaces UCO startup hook call with direct managed call. |
| src/coreclr/vm/dynamicmethod.h | Adjusts enum definition used by dynamic method resolver. |
| src/coreclr/vm/dynamicmethod.cpp | Replaces UCO resolver calls with direct managed calls via MethodDescCallSite. |
| src/coreclr/vm/dllimport.cpp | Renames and simplifies method lookup helper to LookupMethodByName. |
| src/coreclr/vm/dispatchinfo.cpp | Routes culture set/restore via Thread::GetCulture/SetCulture. |
| src/coreclr/vm/corhost.cpp | Replaces UCO EventSource init call with direct managed call. |
| src/coreclr/vm/corelib.h | Updates method bindings after removing UCO wrapper methods. |
| src/coreclr/vm/callhelpers.h | Removes CLR_BOOL_ARG macro (UCO-related). |
| src/coreclr/hosts/corerun/wasm/libCorerun.js | Hooks ___funcs_on_exit for shutdown sequencing and timer abort. |
| src/coreclr/hosts/corerun/corerun.cpp | Returns managed exit code under TARGET_BROWSER. |
| src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs | Removes UCO marshaler entrypoints (CultureInfoMarshaler, ColorMarshaler UCO stubs). |
| src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs | Removes UCO resolver entrypoints. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/LoaderAllocator.cs | Removes UCO factory method. |
| eng/pipelines/runtime.yml | Adds/adjusts browser_wasm CoreCLR build/test legs and artifact naming. |
| eng/pipelines/coreclr/templates/build-native-test-assets-step.yml | Adds extraBuildArgs passthrough. |
| eng/pipelines/common/templates/runtimes/test-variables.yml | Adjusts timeouts for browser CoreCLR test runs. |
| eng/pipelines/common/templates/runtimes/run-test-job.yml | Adds hostedOs and extraBuildArgs handling in job naming and build steps. |
| eng/pipelines/common/templates/browser-wasm-coreclr-build-tests.yml | Updates artifact/job name suffixes to match new CoreCLR naming. |
| eng/liveBuilds.targets | Moves LibrariesSharedFrameworkDir assignment to after LibrariesNativeArtifactsPath is set. |
| extern "C" int BrowserHost_ExecuteAssembly(const char* assemblyPath, int argc, const char** argv) | ||
| { | ||
| int ignore_exit_code = 0; | ||
| int retval = coreclr_execute_assembly(CurrentClrInstance, CurrentAppDomainId, argc, argv, assemblyPath, (uint32_t*)&ignore_exit_code); | ||
| int exit_code = 0; | ||
| int retval = coreclr_execute_assembly(CurrentClrInstance, CurrentAppDomainId, argc, argv, assemblyPath, (uint32_t*)&exit_code); | ||
|
|
||
| if (retval < 0) | ||
| { | ||
| std::fprintf(stderr, "coreclr_execute_assembly failed - Error: 0x%08x\n", retval); | ||
| return -1; | ||
| } | ||
| return 0; | ||
| return exit_code; |
There was a problem hiding this comment.
BrowserHost_ExecuteAssembly returns -1 on internal failure, but it also returns the managed exit code on success. This makes -1 an ambiguous value (managed code can legitimately set Environment.ExitCode = -1). Consider returning the coreclr_execute_assembly HRESULT/retval on failure (negative values) and the managed exit code otherwise, so JS can reliably treat < 0 as host failure and any int (including -1) as a valid managed exit code.
| if (_ems_.EXITSTATUS == -1) { | ||
| const reason = new Error("Failed to execute assembly"); | ||
| _ems_.dotnetApi.exit(res, reason); | ||
| _ems_.dotnetApi.exit(-1, reason); | ||
| throw reason; |
There was a problem hiding this comment.
if (_ems_.EXITSTATUS == -1) treats -1 as a sentinel for host failure, but BrowserHost_ExecuteAssembly returns the managed exit code on success, and managed code can legitimately return -1. Use a failure convention that can’t collide with valid exit codes (e.g., have native return negative HRESULTs on failure and check < 0 here), so valid managed exit codes aren’t misreported as “Failed to execute assembly”.
| void Thread::SetCulture(OBJECTREF *CultureObj, BOOL bUICulture) | ||
| { | ||
| CONTRACTL { | ||
| THROWS; | ||
| GC_TRIGGERS; | ||
| MODE_COOPERATIVE; | ||
| } | ||
| CONTRACTL_END; | ||
|
|
||
| MethodDescCallSite propSet(bUICulture | ||
| ? METHOD__CULTURE_INFO__SET_CURRENT_UI_CULTURE | ||
| : METHOD__CULTURE_INFO__SET_CURRENT_CULTURE); | ||
|
|
||
| // Set up the Stack. | ||
| ARG_SLOT pNewArgs[] = { | ||
| ObjToArgSlot(*CultureObj) | ||
| }; | ||
|
|
||
| // Make the actual call. | ||
| propSet.Call_RetArgSlot(pNewArgs); | ||
| } |
There was a problem hiding this comment.
Thread::GetCulture guards against SystemAssembly()==NULL, but Thread::SetCulture does not. If this is ever called during early startup (similar to the comment in GetCulture), it can attempt to bind/call CultureInfo methods before CoreLib/system assembly is ready. Consider adding the same guard (or otherwise ensuring callers can’t reach this path until the system assembly exists).
| ARG_SLOT args[] = { | ||
| ObjToArgSlot(*pKeepLoaderAllocatorAlive) | ||
| }; | ||
|
|
||
| initLoaderAllocator.Call(args); | ||
|
|
There was a problem hiding this comment.
MethodDescCallSite initLoaderAllocator(..., (OBJECTREF*)pKeepLoaderAllocatorAlive) already supplies the instance (this) for the call. Passing ObjToArgSlot(*pKeepLoaderAllocatorAlive) again in args means the ctor receives an extra unexpected argument, which will corrupt the call stack/signature at runtime. Remove the explicit this from args (for a parameterless ctor, call with NULL/empty args) or construct the callsite without a protected-this and keep this only in the args (but not both).
| ARG_SLOT args[] = { | |
| ObjToArgSlot(*pKeepLoaderAllocatorAlive) | |
| }; | |
| initLoaderAllocator.Call(args); | |
| initLoaderAllocator.Call(NULL); |
| propGet.InvokeThrowing(CLR_BOOL_ARG(bUICulture), &pCurrentCulture); | ||
| // Call the CultureInfo(int culture) constructor. | ||
| ARG_SLOT pNewArgs[] = { | ||
| ObjToArgSlot(CultureObj), |
There was a problem hiding this comment.
MethodDescCallSite cultureInfoCtor(..., &CultureObj) already provides the instance (this) for the constructor call. Including ObjToArgSlot(CultureObj) as the first element of pNewArgs passes this twice and will mismatch the constructor signature. The args array should contain only the ctor parameters (e.g., just lcid) when you pass &CultureObj to the callsite.
| ObjToArgSlot(CultureObj), |
| getJitContext.InvokeThrowing(&gc.Resolver, (int32_t*)securityControlFlags, &gc.ResultType); | ||
| ARG_SLOT args[] = | ||
| { | ||
| ObjToArgSlot(resolver), |
There was a problem hiding this comment.
MethodDescCallSite getJitContext(..., m_managedResolver) already binds the instance for this call. Adding ObjToArgSlot(resolver) as the first argument passes this again, which will break the call signature. Drop the explicit resolver argument and pass only the actual parameters (e.g., securityControlFlags).
| ObjToArgSlot(resolver), |
| MethodDescCallSite getCodeInfo(METHOD__RESOLVER__GET_CODE_INFO, m_managedResolver); | ||
|
|
||
| struct | ||
| { | ||
| OBJECTREF Resolver; | ||
| OBJECTREF DataArray; | ||
| } gc; | ||
| gc.Resolver = ObjectFromHandle(m_managedResolver); | ||
| gc.DataArray = NULL; | ||
| VALIDATEOBJECTREF(gc.Resolver); // gc root must be up the stack | ||
|
|
||
| GCPROTECT_BEGIN(gc); | ||
| OBJECTREF resolver = ObjectFromHandle(m_managedResolver); | ||
| VALIDATEOBJECTREF(resolver); // gc root must be up the stack | ||
|
|
||
| int32_t stackSize = 0, initLocals = 0, EHSize = 0; | ||
|
|
||
| UnmanagedCallersOnlyCaller getCodeInfo(METHOD__RESOLVER__GET_CODE_INFO); | ||
| getCodeInfo.InvokeThrowing(&gc.Resolver, &stackSize, &initLocals, &EHSize, &gc.DataArray); | ||
|
|
||
| U1ARRAYREF dataArray = (U1ARRAYREF)gc.DataArray; | ||
| ARG_SLOT args[] = | ||
| { | ||
| ObjToArgSlot(resolver), | ||
| PtrToArgSlot(&stackSize), | ||
| PtrToArgSlot(&initLocals), | ||
| PtrToArgSlot(&EHSize), | ||
| }; |
There was a problem hiding this comment.
MethodDescCallSite getCodeInfo(..., m_managedResolver) already supplies the instance. Including ObjToArgSlot(resolver) as the first element in args passes this twice and will misinvoke Resolver.GetCodeInfo(...). Remove the explicit resolver argument so args contains only the ref/out parameters.
| <TargetsBrowserOnMono>false</TargetsBrowserOnMono> | ||
| <TargetsBrowserOnMono Condition="'$(TargetsBrowser)' == 'true' and '$(RuntimeFlavor)' == 'mono'">true</TargetsBrowserOnMono> | ||
| <TargetsBrowserOnCoreCLR>false</TargetsBrowserOnCoreCLR> | ||
| <TargetsBrowserOnCoreCLR Condition="'$(TargetsBrowser)' == 'true' and '$(RuntimeFlavor)' == 'coreclr'">true</TargetsBrowserOnCoreCLR> | ||
| <TargetsMobile Condition="'$(TargetsBrowser)' == 'true' and '$(RuntimeFlavor)' == 'coreclr'">false</TargetsMobile> | ||
| <MobileAppBundleDirName Condition="'$(TargetsMobile)' == 'true' and '$(TargetsBrowser)' != 'true'">AppBundle</MobileAppBundleDirName> | ||
| <MobileAppBundleDirName Condition="'$(TargetsBrowser)' == 'true'">wwwroot</MobileAppBundleDirName> | ||
| <MobileAppBundleDirName Condition="'$(TargetsBrowserOnMono)' == 'true'">wwwroot</MobileAppBundleDirName> | ||
| </PropertyGroup> |
There was a problem hiding this comment.
This file compares $(RuntimeFlavor) against both Mono (Pascal case) and mono/coreclr (lowercase). MSBuild string comparisons are case-sensitive, and the runtime pipeline commonly passes /p:RuntimeFlavor=CoreCLR or Mono, so these lowercase comparisons can silently disable the new browser/CoreCLR logic (e.g., TargetsBrowserOnCoreCLR never becomes true). Normalize $(RuntimeFlavor) (e.g., ToLowerInvariant()) or compare against the canonical values consistently throughout the file.
No description provided.