-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[clr-interp] Fix switch statement stack slot handling #120756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
davidwrighton
merged 4 commits into
dotnet:main
from
davidwrighton:switch_incoming_stack_slots
Oct 17, 2025
Merged
[clr-interp] Fix switch statement stack slot handling #120756
davidwrighton
merged 4 commits into
dotnet:main
from
davidwrighton:switch_incoming_stack_slots
Oct 17, 2025
Conversation
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
…dle dispatch to blocks which already had defined their incoming stack slots
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Fixes incorrect stack slot handling for switch instructions when multiple case labels branch to the same target basic block. Key changes ensure per-target stack state initialization and variable move emission are performed only once per unique target.
- Added collection of target offsets and deduplication via sorting before emitting per-target setup.
- Moved INTOP_SWITCH instruction emission until after target block preparation.
janvorli
approved these changes
Oct 15, 2025
BrzVlad
reviewed
Oct 16, 2025
…e-initialized stack state
…dwrighton/runtime into switch_incoming_stack_slots
BrzVlad
approved these changes
Oct 17, 2025
stephentoub
added a commit
that referenced
this pull request
Oct 17, 2025
This PR ports functional regex tests from the <a href="https://github.com/google/re2/tree/main/re2/testing">RE2 test suite</a> to improve .NET's regex test coverage, as requested in #120756. ## Changes ### Test Suite Additions - **Added `RegexRe2Tests.cs`**: New test file containing 85 unique test cases ported from RE2's `re2_test.cc` and `search_test.cc` - **Test coverage**: The ported tests execute ~340 times across all available regex engines (Interpreter, Compiled, NonBacktracking, SourceGenerated) - **Removed duplicates**: Analyzed overlap with existing PCRE, Rust, and core regex tests; removed 57 duplicative test cases to avoid redundancy ### Test Categories Covered The ported tests validate: - Complex matching patterns and alternations - Anchors (`^`, `$`) in single-line and multiline modes with non-trivial cases - Word boundaries (`\b`, `\B`) with ASCII and special characters - UTF-8/Unicode character handling - Escape sequences (octal `\141`, hexadecimal `\x61`, unicode `\u0061`) - Case-insensitive matching (`(?i)`) - Non-trivial quantifier combinations (`{n}`, `{n,}`, `{n,m}`) - Backreferences (excluded for NonBacktracking engine) - Edge cases and historical bug patterns from RE2 ### Compatibility Adjustments Several RE2-specific patterns were excluded or adapted for .NET compatibility: - **Removed `\C` patterns**: RE2's byte-matching construct is not supported in .NET - **Removed `\Q...\E` patterns**: Quote meta syntax is not supported in .NET - **Converted `\x{...}` escapes**: Changed to .NET-compatible `\x` (2 digits) or `\u` (4 digits) format - **Excluded backreferences for NonBacktracking**: Uses `RegexHelpers.IsNonBacktracking()` check - **Removed Unicode word boundary test**: .NET treats Unicode letters as word characters (unlike RE2/PCRE which use ASCII-only) - **Removed duplicative tests**: Excluded basic patterns like "a", "a*", "a+", "a?", "^$" that are already well-covered in existing test files ### License Attribution - **Updated `THIRD-PARTY-NOTICES.TXT`**: Added RE2 BSD 3-Clause license to properly attribute the ported tests - **Updated `.csproj`**: Added `RegexRe2Tests.cs` to the compilation ## Testing All 29,621 tests pass successfully, including the newly ported RE2 tests across all regex engines. Fixes #120756 <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>Port re2 regex tests</issue_title> > <issue_description>re2 has lots of regex functional tests we should port over to improve our coverage: > https://github.com/google/re2/blob/d826d9fcb68c62996c1b7c0a45d604e22d814952/re2/testing/</issue_description> > > <agent_instructions>You can see in https://github.com/dotnet/runtime/tree/main/src/libraries/System.Text.RegularExpressions/tests/FunctionalTests we have a file of tests ported from PCRE and a file of tests ported from Rust. Please similarly port the cited RE2 tests (or a newer version if there is one); any that don't pass but should need to be marked as [ActiveIssue(...)]... any that don't pass but that's expected should be removed.</agent_instructions> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > <comment_new><author>@</author><body> > Tagging subscribers to this area: @eerhardt, @dotnet/area-system-text-regularexpressions > See info in [area-owners.md](https://github.com/dotnet/runtime/blob/main/docs/area-owners.md) if you want to be subscribed. > <details> > <summary>Issue Details</summary> > <hr /> > > re2 has lots of regex functional tests we should port over to improve our coverage: > https://github.com/google/re2/blob/d826d9fcb68c62996c1b7c0a45d604e22d814952/re2/testing/ > > <table> > <tr> > <th align="left">Author:</th> > <td>stephentoub</td> > </tr> > <tr> > <th align="left">Assignees:</th> > <td>-</td> > </tr> > <tr> > <th align="left">Labels:</th> > <td> > > `area-System.Text.RegularExpressions` > > </td> > </tr> > <tr> > <th align="left">Milestone:</th> > <td>.NET 7.0</td> > </tr> > </table> > </details></body></comment_new> > <comment_new><author>@joperezr</author><body> > This will likely miss ZBB but will still make it before 7.0. Adjusting the milestone just because this is not technically a blocker for 7.0.</body></comment_new> > </comments> > </details> Fixes #61896 <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
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.
Fix issue where switch instructions did not properly handle dispatch to blocks which already had defined their incoming stack slots