-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix regression caused by special marker type optimization #123413
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
Conversation
- My previous work here apparently missed this entire category of code
- There was another issue where the rehydration code for interfaces could only handle up to MethodTable::MaxGenericParametersForSpecialMarkerType, but we didn't take that into account in the creation of special marker types.
…aths in the new code
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
This pull request fixes two critical regressions introduced by the special marker type optimization in .NET 10.0.2. The first issue (#123254) affects VB.NET programs that use interface hierarchies, causing an EntryPointNotFoundException at runtime. The second issue (#123318) causes ExecutionEngineException when instantiating types with 9 or more generic parameters.
Changes:
- Implements interface map logic for handling special marker types in different scenarios (special marker types nested within other special marker types or exact types)
- Introduces a centralized helper method
EligibleForSpecialMarkerTypeUsageto validate whether an instantiation can use special marker types, including the crucial check againstMaxGenericParametersForSpecialMarkerType - Adds comprehensive regression tests for both VB.NET interface hierarchy scenarios and high-arity generic types
- Fixes VB.NET build system integration by properly handling DefineConstants delimiter differences between VB and C#
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/methodtablebuilder.cpp | Implements comprehensive interface map logic for 4 cases of special marker type handling during type loading |
| src/coreclr/vm/methodtable.cpp | Adds EligibleForSpecialMarkerTypeUsage helper method with generic parameter count validation |
| src/coreclr/vm/typehandle.h | Declares new helper method for special marker type eligibility checking |
| src/coreclr/vm/siginfo.cpp | Replaces inline logic with centralized helper method call |
| src/tests/Loader/classloader/regressions/GitHub_123318/GitHub_123318.cs | Regression test for 9-parameter generic type instantiation |
| src/tests/Loader/classloader/regressions/GitHub_123318/GitHub_123318.csproj | Test project configuration |
| src/tests/Loader/classloader/regressions/GitHub_123254/GitHub_123254.vb | VB.NET regression tests for interface hierarchy scenarios |
| src/tests/Loader/classloader/regressions/GitHub_123254/GitHub_123254.vbproj | VB test project configuration |
| src/tests/Directory.Build.props | Fixes VB.NET DefineConstants handling to use comma delimiters |
src/tests/Loader/classloader/regressions/GitHub_123318/GitHub_123318.csproj
Outdated
Show resolved
Hide resolved
src/tests/Loader/classloader/regressions/GitHub_123254/GitHub_123254.vb
Outdated
Show resolved
Hide resolved
src/tests/Loader/classloader/regressions/GitHub_123254/GitHub_123254.vb
Outdated
Show resolved
Hide resolved
src/tests/Loader/classloader/regressions/GitHub_123254/GitHub_123254.vb
Outdated
Show resolved
Hide resolved
src/tests/Loader/classloader/regressions/GitHub_123254/GitHub_123254.vb
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
src/tests/Loader/classloader/regressions/GitHub_123254/GitHub_123254.vb
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…123254.vb Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
src/tests/Loader/classloader/regressions/GitHub_123254/GitHub_123254.vb
Outdated
Show resolved
Hide resolved
src/tests/Loader/classloader/regressions/GitHub_123254/GitHub_123254.vb
Outdated
Show resolved
Hide resolved
src/tests/Loader/classloader/regressions/GitHub_123254/GitHub_123254.vb
Outdated
Show resolved
Hide resolved
src/tests/Loader/classloader/regressions/GitHub_123254/GitHub_123254.vb
Outdated
Show resolved
Hide resolved
src/tests/Loader/classloader/regressions/GitHub_123254/GitHub_123254.vb
Outdated
Show resolved
Hide resolved
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
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
src/tests/Loader/classloader/regressions/GitHub_123254/GitHub_123254.vb
Outdated
Show resolved
Hide resolved
src/tests/Loader/classloader/regressions/GitHub_123254/GitHub_123254.vb
Outdated
Show resolved
Hide resolved
src/tests/Loader/classloader/regressions/GitHub_123318/GitHub_123318.csproj
Outdated
Show resolved
Hide resolved
jkotas
left a comment
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.
LGTM, it is very subtle maze of corner-cases, so it is easy to miss something.
|
/backport to release/10.0 |
|
Started backporting to |
|
@davidwrighton backporting to git am output$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Patch format detection failed.
Error: The process '/usr/bin/git' failed with exit code 128 |
) There are 2 fixes here 1. For issue dotnet#123254, the problem is that the implementation of the optimized path interface map logic for interfaces which can be expanded by just manually expanding the first 2 layers of interfaces was not wired up at all. It turns out that this path is almost exclusively only available to VB.NET programs due to differences in the metadata generation algorithms between the VB and C# compilers. Notably, the VB.NET compiler does not expand the full set of interfaces into the outermost type, and C# compilation does. The fix is to implement handling for all of the various combinations of type that can appear at that point. See the newly added regression test for details on how to get into those situations. 2. For issue dotnet#123318 there was an issue where if there are more than MaxGenericParametersForSpecialMarkerType but we may have attempted to use a special marker type to optimize type loading. The fix is to place the logic for determining if a type can be represented in an interface map via a special marker type into a common location, and to not forget the check against MaxGenericParametersForSpecialMarkerType. Fixes dotnet#123254 and dotnet#123318 --------- Co-authored-by: Jan Kotas <jkotas@microsoft.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ion (#123413) (#123520) Backport of #123413 to release/10.0 /cc @Copilot ## Customer Impact - [x] Customer reported - [ ] Found internally [Select one or both of the boxes. Describe how this issue impacts customers, citing the expected and actual behaviors and scope of the issue. If customer-reported, provide the issue number.] There are 2 fixes here 1. For issue #123254, the problem is that the implementation of the optimized path interface map logic for interfaces which can be expanded by just manually expanding the first 2 layers of interfaces was not wired up at all. It turns out that this path is almost exclusively only available to VB.NET programs due to differences in the metadata generation algorithms between the VB and C# compilers. Notably, the VB.NET compiler does not expand the full set of interfaces into the outermost type, and C# compilation does. The fix is to implement handling for all of the various combinations of type that can appear at that point. See the newly added regression test for details on how to get into those situations. All known customers hitting this issue are using VB.NET, although it is theoretically possible to encounter this problem in C# if complex binary assembly compatibility scenarios are in play. (e.g Assembly A is built against assembly B version 1 which has a significantly less complex implied interface hierarchy, but is run against assembly B version 2 which has a more complex hierarchy.) However, this is a relatively unusual scenario. 2. For issue #123318 there was an issue where if there are more than MaxGenericParametersForSpecialMarkerType but we may have attempted to use a special marker type to optimize type loading. The fix is to place the logic for determining if a type can be represented in an interface map via a special marker type into a common location, and to not forget the check against MaxGenericParametersForSpecialMarkerType. This is known to impact customers with large numbers of generic parameters on their types where all the generic parameters are the same. This issue is impacting customers using C# as well as VB. Fixes #123254 and #123318 ## Regression - [x] Yes - [ ] No This regressed with the change to improve the performance of process startup. See PR #120712 . That PR had several bugs which were noticed within days of 10.0.2 releasing. ## Testing The fix was verified by running the customer repro cases, and additional exploratory tests. New tests were added which stressed the problematic behavior. Notably, the new tests are written in VB.NET which due to an implementation detail of the VB.NET compiler has significantly different interface loading behavior compared to C#. The issue #123318 was simply an issue of insufficient boundary condition testing. ## Risk Low compared to risk of fix remaining in the product. This fix fixes issues in a codepath which was severely misbehaving. At worst this fix misses some special cases, but it's certainly better than it was before. --------- --------- Co-authored-by: Jan Kotas <jkotas@microsoft.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There are 2 fixes here
Fixes #123254 and #123318