Skip to content

Add tests for Volatile.ReadBarrier and Volatile.WriteBarrier#124558

Merged
stephentoub merged 10 commits intomainfrom
copilot/implement-volatile-intrinsics
Mar 4, 2026
Merged

Add tests for Volatile.ReadBarrier and Volatile.WriteBarrier#124558
stephentoub merged 10 commits intomainfrom
copilot/implement-volatile-intrinsics

Conversation

Copy link
Contributor

Copilot AI commented Feb 18, 2026

  • Add basic tests for Volatile.ReadBarrier and Volatile.WriteBarrier
  • Add reflection invocation test
  • Consolidate into focused test methods per review feedback
  • Combine smoke tests into single method, move reflection test after it
  • Remove cross-thread test (Task.Run and SpinOnce already involve barriers, making the test not meaningful for validating ReadBarrier/WriteBarrier)
Original prompt

This section details on the original issue you should resolve

<issue_title>[wasm] Volatile intrinsics not implemented</issue_title>
<issue_description>### Description

Some intrinsics like

public static void ReadBarrier() => ReadBarrier();

don't seem to be implemented on WebAssembly, leading to failures when referenced libraries call them.

Reproduction Steps

Create a minimal blazor wasm app, call Volatile.ReadBarrier() in it, run it.

Expected behavior

Runs. I think the runtime could simply use Interlocked.MemoryBarrier for correct (albeit perhaps suboptimal) behavior for Volatile.ReadBarrier and Volatile.WriteBarrier.

Actual behavior

Crashes with stack overflow (since the default implementation seems to be that the method is just calling itself).

Regression?

Probably not, these seem to be new APIs in .NET 10.

Known Workarounds

Don't use Volatile.ReadBarrier/WriteBarrier but that's difficult if it's used by referenced libraries whose source code I don't control.

Configuration

No response

Other information

No response</issue_description>

<agent_instructions>Add basic tests for Volatile/ReadBarrier and Volatile.WriteBarrier APIs</agent_instructions>

Comments on the Issue (you are @copilot in this section)

@jkotas #107843 that introduced these APIs did not come with any tests.

@hamarb123 @VSadov </comment_new>
<comment_new>@VSadov
If wasm is single-threaded, then ReadBarrier() could have a noop implementation.</comment_new>
<comment_new>@jkotas
Fixed by #124538. We still need to add some tests.</comment_new>


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Copilot AI changed the title [WIP] Implement volatile intrinsics for WebAssembly Add tests for Volatile.ReadBarrier and Volatile.WriteBarrier Feb 18, 2026
Copilot AI requested a review from jkotas February 18, 2026 15:20
…ction and delegates

Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
…ions test

Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request adds comprehensive test coverage for the Volatile.ReadBarrier() and Volatile.WriteBarrier() methods that were introduced in .NET 10 without tests. These intrinsic methods were causing stack overflow crashes on WebAssembly when called (as they were recursively calling themselves without proper implementation), which was fixed in PR #124538. This PR ensures those methods are properly tested to prevent similar issues in the future.

Changes:

  • Added VolatileTests.cs with four test methods covering smoke tests, functional usage patterns, and reflection invocation
  • Updated System.Threading.Tests.csproj to include the new test file

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/libraries/System.Threading/tests/VolatileTests.cs New test file with comprehensive coverage for Volatile.ReadBarrier and WriteBarrier methods including smoke tests, integration tests with Volatile.Read/Write, and reflection invocation tests
src/libraries/System.Threading/tests/System.Threading.Tests.csproj Added VolatileTests.cs to the project's compiled files list

@jkotas jkotas requested a review from eduardo-vp February 20, 2026 04:11
@stephentoub
Copy link
Member

@copilot please invoke the code-review skill and post the analysis/comments as a comment on this PR

Copy link
Contributor Author

Copilot AI commented Mar 4, 2026

@copilot please invoke the code-review skill and post the analysis/comments as a comment on this PR

🤖 Copilot Code Review

Holistic Assessment

Motivation: Justified. Volatile.ReadBarrier() and Volatile.WriteBarrier() are self-referencing [Intrinsic] methods that shipped in .NET 10 without tests. They are used in production code (CastCache.cs, GenericCache.cs), and the WASM stack overflow bug (#124538) demonstrated the need for coverage. Adding tests is the right thing to do.

Approach: The approach is appropriate — simple smoke tests, a combined functional test, and a separate reflection invocation test. The PR has been through multiple rounds of review with @jkotas and the test structure reflects his feedback.

Summary: ✅ LGTM. This is a clean, focused test-only PR that adds appropriate coverage for two intrinsic APIs. No blocking issues found.


Detailed Findings

✅ Test structure — Appropriate and well-organized

The 4 test methods cover the key scenarios:

  • ReadBarrier_DoesNotThrow / WriteBarrier_DoesNotThrow — Smoke tests that directly verify the original bug (stack overflow on WASM) won't regress.
  • BarriersAndVolatileOperations — Combined test exercising barriers alongside Volatile.Read/Volatile.Write with multiple types (int, long), sequential calls, and interleaved operations.
  • BarriersViaReflection — Separate method testing reflection invocation, which exercises a different code path (the JIT intrinsic expansion doesn't apply via MethodInfo.Invoke; the self-referencing body must work).

This structure is consistent with the existing InterlockedTests.MemoryBarrierIntrinsic pattern in the same test project.

✅ Csproj inclusion — Correct

VolatileTests.cs is properly added to System.Threading.Tests.csproj in alphabetical order among the other test file entries.

✅ File conventions — Correct

License header present, standard namespace, standard using directives, no unnecessary imports.

💡 Minor observation — BarriersAndVolatileOperations initializes locals to 0

Lines 27-29 initialize value1, value2, value3 to 0/0/0, which is technically redundant per CA1805 (default initialization). However, this is a test file and the explicit initialization improves readability by making the test's initial state clear. Not worth changing.

…latility test

Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Copilot AI requested a review from stephentoub March 4, 2026 15:42
…barriers

Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
@stephentoub stephentoub enabled auto-merge (squash) March 4, 2026 19:28
@stephentoub stephentoub merged commit 4d77205 into main Mar 4, 2026
89 of 91 checks passed
@stephentoub stephentoub deleted the copilot/implement-volatile-intrinsics branch March 4, 2026 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[wasm] Volatile intrinsics not implemented

4 participants