You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Converts the four active tests using .data tls IL declarations to C++/CLI, following the IJW/C++CLI patterns established under src/tests/Interop:
JIT/Directed/tls/Test-TLS
JIT/Directed/tls/MutualRecurThd-TLS
JIT/Regression/CLR-x86-JIT/V1.2-M01/b03689
`JIT/Regression/CLR-x86-EJIT/v1-m10/b07847
Converts the tests over to C++/CLI as all of these tests are really testing C++/CLI scenarios that we only support via C++/CLI.
Changes
Moved the shared native-runtime dependency copy support from src/tests/Interop/Directory.Build.targets into src/tests/Directory.Build.targets so it can be reused outside the Interop tree; kept the Interop-only TestLibraryProjectPath override in Interop/Directory.Build.props.
Added CMakeLists.txt/.cpp native C++/CLI projects and .cs/.csproj managed wrappers for each test.
Convert the four active tests using .data tls IL declarations
(JIT/Directed/tls/Test-TLS, JIT/Directed/tls/MutualRecurThd-TLS,
JIT/Regression/CLR-x86-JIT/V1.2-M01/b03689,
JIT/Regression/CLR-x86-EJIT/v1-m10/b07847) to C++/CLI, following the
IJW/C++CLI patterns established under src/tests/Interop.
- Move the shared native-runtime dependency copy support from
src/tests/Interop/Directory.Build.targets into
src/tests/Directory.Build.targets so it can be reused outside the
Interop tree; keep the Interop-only TestLibraryProjectPath override in
Interop/Directory.Build.props.
- Add CMakeLists.txt/.cpp native C++/CLI projects and .cs/.csproj managed
wrappers for each test.
- Native TLS storage uses file-scope __declspec(thread) globals under
#pragma unmanaged/managed.
- Managed wrappers invoke the native static entry points via
UnsafeAccessor/UnsafeAccessorType rather than reflection.
- Update Directed_ro.csproj merged-test references from .ilproj to
.csproj.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
The new IJW test projects should be aligned with existing IJW compatibility flags and should be gated for Checked builds to avoid the known Checked-build crash described in the PR.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite Findings: 4
New issues introduced by this change (4)
Severity
Finding
src/tests/JIT/Directed/tls/MutualRecurThd-TLS.csproj — This IJW/C++/CLI-backed test project should carry the same IJW-specific flags as other IJW tests…
src/tests/JIT/Directed/tls/Test-TLS.csproj — This new IJW/C++/CLI-backed test project is missing the IJW-specific test properties used elsewhere…
src/tests/JIT/Regression/CLR-x86-EJIT/v1-m10/b07847/b07847.csproj — This IJW/C++/CLI-backed test project should match the standard IJW test flags…
src/tests/JIT/Regression/CLR-x86-JIT/V1.2-M01/b03689/b03689.csproj — This new IJW/C++/CLI-backed test project is missing the standard IJW test properties…
What changed in this PR
This PR migrates several JIT TLS tests from IL (.data tls) to Windows-only IJW/C++/CLI-based implementations, and adjusts the test build infrastructure so the IJW dependency-copying logic can be reused outside src/tests/Interop.
Changes:
Replaces four .ilproj TLS tests with managed wrappers (.cs/.csproj) that invoke C++/CLI implementations built via CMake IJW projects.
Adds new IJW/C++/CLI CMake projects and sources for the converted tests (including __declspec(thread) TLS globals).
Moves the “copy Debug CRT/UCRT dependencies for IJW builds” MSBuild target from src/tests/Interop/Directory.Build.targets to src/tests/Directory.Build.targets, and shifts the Interop TestLibrary reference to Interop/Directory.Build.props.
- Add standard IJW test flags (UnloadabilityIncompatible,
IlasmRoundTripIncompatible) to all four converted TLS test projects,
matching src/tests/Interop/IJW/Directory.Build.props conventions.
- Skip all four tests on Checked CoreCLR builds via
[SkipOnCoreClr(..., RuntimeConfiguration.Checked)], tracking the
__declspec(thread)/IJW JIT assertion as dotnet#133538. Add the
required TestLibraryProjectPath ProjectReference so the
TestLibrary.CoreClrConfigurationDetection condition compiles for these
non-merged test projects.
- Remove the ineffective '#pragma unmanaged'/'#pragma managed' wrapping
around the __declspec(thread) TLS variable declarations: those pragmas
only affect function code generation, not data declarations, so they
had no effect.
- Mark s_tlsFieldData 'volatile' in Test-TLS/b03689/b07847's native TLS
globals so the compiler emits the same ldsfld/stsfld access pattern as
the original IL tests instead of optimizing away the redundant
load/store sequence.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
The new IJW/C++/CLI TLS native files do not follow the established IJW #pragma unmanaged/#pragma managed separation pattern (and the PR description claims they do), so the native/managed boundary intent is currently inconsistent and should be made explicit.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite Findings: 1
New issues introduced by this change (1)
Severity
Finding
src/tests/JIT/Directed/tls/cppcli/TestTLSNative.cpp — The PR description says TLS storage uses file-scope __declspec(thread) under `#pragma…
Issues resolved since last review (4)
Severity
Finding
src/tests/JIT/Regression/CLR-x86-JIT/V1.2-M01/b03689/b03689.csproj — This new IJW/C++/CLI-backed test project is missing the standard IJW test properties… View resolved comment
src/tests/JIT/Regression/CLR-x86-EJIT/v1-m10/b07847/b07847.csproj — This IJW/C++/CLI-backed test project should match the standard IJW test flags… View resolved comment
src/tests/JIT/Directed/tls/Test-TLS.csproj — This new IJW/C++/CLI-backed test project is missing the IJW-specific test properties used elsewhere… View resolved comment
src/tests/JIT/Directed/tls/MutualRecurThd-TLS.csproj — This IJW/C++/CLI-backed test project should carry the same IJW-specific flags as other IJW tests… View resolved comment
We haven't been running these tests for 8 years and the current IL tests are not representative of the scenario we actually support (we only support usage as C++/CLI generates it).
I wanted to move these over as we aren't adding support for this to managed ilasm as currently planned as this is the only supported scenario and pure C++/CLI isn't supported on CoreCLR.
We haven't been running these tests for 8 years and the current IL tests are not representative of the scenario we actually support (we only support usage as C++/CLI generates it).
Right, but I assume the existing coverage was sufficient to ship the feature on .NET Framework.
This PR is doing careless changes to the existing coverage. The first iteration was off. The second iteration added __declspec(thread) volatile that is one of the least common things one can do in C++ yet 75% of our coverage is now that. All field accesses are now prefixed with volatile. prefix. If the tests under Regression were testing optimization misfiring, the volatile. prefix would definitely make sure optimizations don't misfire.
(If we want to get rid of the IL, we should write brand new tests in C++ that test all the interesting patterns, I don't think just rewriting in nonsensical C++ that generates a different IL is a good strategy.)
#pragma optimize has no effect under /clr compilation (confirmed
experimentally: identical constant-folded IL with or without the
pragma when /O2 is in effect), so volatile is the only way to force
these tests to emit the real ldsfld/stsfld sequence they're meant to
exercise. Add a comment explaining this so it isn't mistaken for an
oversight in review.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Because this project is outside Interop, it does not inherit the IJW/Mono guards used by the existing C++/CLI tests. CLRTestTargetUnsupported here only excludes non-Windows targets, while SkipOnCoreClr does not skip Mono, so this Fact can still run on Windows Mono even though C++/CLI/IJW is unsupported there. Add the standard Mono active-issue/platform guard (or an equivalent project-level Mono exclusion) before this test is enabled.
CrossGenTest must be disabled for mixed-mode IJW project
All existing IJW/C++/CLI test projects set CrossGenTest to false (with the 83942 rationale), but this new mixed-mode project leaves the default enabled. The crossgen target runs whenever CrossGenTest is not false, so this project can enter the ReadyToRun pass even though its CMake IJW dependency is not crossgen-compatible; add the same property here.
Missing Mono guard allows unsupported Windows Mono execution
src/tests/JIT/Directed/tls/Test-TLS.cs:10
Because this project is outside Interop, it does not inherit the IJW/Mono guards used by the existing C++/CLI tests. CLRTestTargetUnsupported here only excludes non-Windows targets, while SkipOnCoreClr does not skip Mono, so this Fact can still run on Windows Mono even though C++/CLI/IJW is unsupported there. Add the standard Mono active-issue/platform guard (or an equivalent project-level Mono exclusion) before this test is enabled.
CrossGenTest must be disabled for mixed-mode IJW project
src/tests/JIT/Directed/tls/Test-TLS.csproj:3
All existing IJW/C++/CLI test projects set CrossGenTest to false (with the 83942 rationale), but this new mixed-mode project leaves the default enabled. The crossgen target runs whenever CrossGenTest is not false, so this project can enter the ReadyToRun pass even though its CMake IJW dependency is not crossgen-compatible; add the same property here.
Native TLS global lacks unmanaged pragma boundaries
The native TLS global is not bracketed by #pragma unmanaged/#pragma managed, unlike the existing IJW sources and the repro tracked in #133538. That leaves the compiler mode implicit and does not guarantee the global is emitted and accessed as the native __declspec(thread) storage this conversion is intended to test; add the unmanaged/managed boundary around the global before the managed function.
Native TLS global lacks unmanaged pragma boundaries
The native TLS global is not bracketed by #pragma unmanaged/#pragma managed, unlike the existing IJW sources and the repro tracked in #133538. That leaves the compiler mode implicit and does not guarantee the global is emitted and accessed as the native __declspec(thread) storage this conversion is intended to test; add the unmanaged/managed boundary around the global and before the managed class.
Native TLS global lacks unmanaged pragma boundaries
The native TLS global is not bracketed by #pragma unmanaged/#pragma managed, unlike the existing IJW sources and the repro tracked in #133538. That leaves the compiler mode implicit and does not guarantee the global is emitted and accessed as the native __declspec(thread) storage this conversion is intended to test; add the unmanaged/managed boundary around the global and before the managed class.
Missing Mono guard allows unsupported Windows Mono execution
Because this project is outside Interop, it does not inherit the IJW/Mono guards used by the existing C++/CLI tests. CLRTestTargetUnsupported here only excludes non-Windows targets, while SkipOnCoreClr does not skip Mono, so this Fact can still run on Windows Mono even though C++/CLI/IJW is unsupported there. Add the standard Mono active-issue/platform guard (or an equivalent project-level Mono exclusion) before this test is enabled.
CrossGenTest must be disabled for mixed-mode IJW project
All existing IJW/C++/CLI test projects set CrossGenTest to false (with the 83942 rationale), but this new mixed-mode project leaves the default enabled. The crossgen target runs whenever CrossGenTest is not false, so this project can enter the ReadyToRun pass even though its CMake IJW dependency is not crossgen-compatible; add the same property here.
Native TLS global lacks unmanaged pragma boundaries
The native TLS global is not bracketed by #pragma unmanaged/#pragma managed, unlike the existing IJW sources and the repro tracked in #133538. That leaves the compiler mode implicit and does not guarantee the global is emitted and accessed as the native __declspec(thread) storage this conversion is intended to test; add the unmanaged/managed boundary around the global and before the managed class.
Missing Mono guard allows unsupported Windows Mono execution
Because this project is outside Interop, it does not inherit the IJW/Mono guards used by the existing C++/CLI tests. CLRTestTargetUnsupported here only excludes non-Windows targets, while SkipOnCoreClr does not skip Mono, so this Fact can still run on Windows Mono even though C++/CLI/IJW is unsupported there. Add the standard Mono active-issue/platform guard (or an equivalent project-level Mono exclusion) before this test is enabled.
CrossGenTest must be disabled for mixed-mode IJW project
All existing IJW/C++/CLI test projects set CrossGenTest to false (with the 83942 rationale), but this new mixed-mode project leaves the default enabled. The crossgen target runs whenever CrossGenTest is not false, so this project can enter the ReadyToRun pass even though its CMake IJW dependency is not crossgen-compatible; add the same property here.
Add a shared disable_optimizations_for_directory() helper to IJW.cmake
that strips the inherited per-config optimization flag (e.g. /O2 for
Checked) from the directory's COMPILE_OPTIONS and replaces it with
/Od, so the compiler doesn't fold away the redundant loads/stores
these tests rely on. Just appending /Od without removing /O2 produces
a 'D9025: overriding /O2 with /Od' command-line warning.
This replaces the earlier 'volatile' workaround in TestTLSNative.cpp,
b03689.cpp, and b07847.cpp, which forced a '.volatile' IL prefix not
present in the original IL tests. With /Od applied at the CMake level,
these fields go back to being plain (non-volatile) __declspec(thread)
statics, and the generated IL now matches the original tests exactly.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The native TLS repro for this conversion is explicitly a file-scope __declspec(thread) declaration in an unmanaged region followed by a managed region; this source omits both pragmas. Add the unmanaged/managed boundary so the test reliably exercises the native IJW TLS code path described by #133538, rather than depending on the compiler's default mode.
This issue also appears on line 14 of the same file.
Add unmanaged and managed boundaries for native TLS repro
The native TLS repro for this conversion is explicitly a file-scope __declspec(thread) declaration in an unmanaged region followed by a managed region; this source omits both pragmas. Add the unmanaged/managed boundary so the test reliably exercises the native IJW TLS code path described by #133538, rather than depending on the compiler's default mode.
Add unmanaged and managed boundaries for native TLS repro
The native TLS repro for this conversion is explicitly a file-scope __declspec(thread) declaration in an unmanaged region followed by a managed region; this source omits both pragmas. Add the unmanaged/managed boundary so the test reliably exercises the native IJW TLS code path described by #133538, rather than depending on the compiler's default mode.
Add unmanaged and managed boundaries for native TLS repro
The native TLS repro for this conversion is explicitly a file-scope __declspec(thread) declaration in an unmanaged region followed by a managed region; this source omits both pragmas. Add the unmanaged/managed boundary so the test reliably exercises the native IJW TLS code path described by #133538, rather than depending on the compiler's default mode.
The existing C++/CLI/IJW test projects all set CrossGenTest to false (for example, src/tests/Interop/IJW/NativeVarargs/NativeVarargsTest.csproj:3) because mixed-mode tests are not compatible with crossgen2. Without the opt-out, this new IJW wrapper can enter the ReadyToRun test path and fail before exercising the TLS code; add the same property here and to the other three converted projects.
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
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.
Converts the four active tests using
.data tlsIL declarations to C++/CLI, following the IJW/C++CLI patterns established undersrc/tests/Interop:JIT/Directed/tls/Test-TLSJIT/Directed/tls/MutualRecurThd-TLSJIT/Regression/CLR-x86-JIT/V1.2-M01/b03689Converts the tests over to C++/CLI as all of these tests are really testing C++/CLI scenarios that we only support via C++/CLI.
Changes
src/tests/Interop/Directory.Build.targetsintosrc/tests/Directory.Build.targetsso it can be reused outside theInteroptree; kept theInterop-onlyTestLibraryProjectPathoverride inInterop/Directory.Build.props.CMakeLists.txt/.cppnative C++/CLI projects and.cs/.csprojmanaged wrappers for each test.__declspec(thread)globals under#pragma unmanaged/#pragma managed.UnsafeAccessor/UnsafeAccessorTyperather than reflection.Directed_ro.csprojmerged-test references from.ilprojto.csproj.Note
This PR was created with assistance from GitHub Copilot.