Skip to content

Fix type loader not recognizing overridden method #56337

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 27, 2021
Merged

Fix type loader not recognizing overridden method #56337

merged 2 commits into from
Jul 27, 2021

Conversation

davidwrighton
Copy link
Member

  • Result of a bad change when removing support for full stub dispatch in .NET 4.0 timeframe (circa 2008)
  • Caused issue when the following set of conditions were all true
    • The type implements an interface
    • The interface has more virtual methods on it than the number of virtual methods on the base type of the type.
    • The base type implements the interface partially (and the partial implementation has a slot number greater than the number of virtual methods on the base type + its base types)
    • The type does not re-implement the interface methods implemented by the base type.
  • The comment referred to situations where stub dispatch was used to resolve non-virtual calls which is a very long time removed feature and is not applicable to today's codebase.
  • Not reachable with versions of C# that shipped before the default interfaces feature, but with that feature became easily reachable. Has been a bug since .NET 4 for handwritten IL.

Fixes #44533

@davidwrighton
Fix type loader not recognizing overriden method
d43754c 
- Result of a bad change when removing support for full stub dispatch in .NET 4.0 timeframe (circa 2008)
- Caused issue when the following set of conditions were all true
  - The type implements an interface
  - The interface has more virtual methods on it than the number of virtual methods on the base type of the type.
  - The base type implements the interface partially (and the partial implementation has a slot number greater than the number of virtual methods on the base type + its base types)
  - The type does not re-implement the interface methods implemented by the base type.
- The comment referred to situations where stub dispatch was used to resolve non-virtual calls which is a very long time removed feature and is not applicable to today's codebase.
- Not reachable with versions of C# that shipped before the default interfaces feature, but with that feature became easily reachable. Has been a bug since .NET 4 for handwritten IL.

Fixes dotnet#44533
@davidwrighton davidwrighton requested review from janvorli and trylek July 26, 2021 22:22
janvorli
janvorli approved these changes Jul 26, 2021
View reviewed changes
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

trylek
trylek approved these changes Jul 27, 2021
View reviewed changes
Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks for fixing!

@davidwrighton davidwrighton merged commit 3ce1168 into dotnet:main Jul 27, 2021
thaystg added a commit to thaystg/runtime that referenced this pull request Jul 28, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into improve_hot_reload_de…
1930315 
…bug_tests

* origin/main: (274 commits)
  Disable test ConnectWithCertificateForDifferentName_Throws (dotnet#56456)
  Update dependencies from https://github.com/mono/linker build 20210726.2 (dotnet#56374)
  Cleanup disabled test conditions (dotnet#56381)
  [mono] Add GC unsafe transition to mono_unhandled_exception  (dotnet#56380)
  don't fail the file extraction when we can't set last write time (dotnet#56370)
  Properly rebuild optimization data when it changes (dotnet#56397)
  Make open function calls in coreclr EINTR resilient on macOS (dotnet#56403)
  Fix dependency from EventLog to TraceSource ref (dotnet#56417)
  Fix comments in asm with JitDiffableDasm=1 (dotnet#56416)
  Catch TcpClient ctor exceptions in FtpWebRequest.CreateConnectionAsync (dotnet#56379)
  Add interop between serializer and DOMs (dotnet#56112)
  Fix type loader not recognizing overridden method (dotnet#56337)
  Prevent Segfault in EventPipe on disable (dotnet#56104)
  Update runtimeconfig.json and deps.json paths when these break past the MAX_PATH threshold  (dotnet#56224)
  Use native allocator instead of pinning when decompressing embedded PDB (dotnet#56336)
  Specify win-x64 as a valid platform in the microsoft-net-runtime-* workloads for iOS/tvOS/MacCatalyst (dotnet#56311)
  Fix FailFast message formatting race (dotnet#56388)
  Try to fix finalizer-based async tests (dotnet#56384)
  Fix MetricsEventSource tests (dotnet#56382)
  Remove invalid Castle.DynamicProxy.Internal.AbstractInvocation from ILLink descriptor files (dotnet#56392)
  ...
thaystg added a commit to thaystg/runtime that referenced this pull request Jul 28, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into thays_fix_js_tests
3f83a08 
* origin/main: (95 commits)
  Disable test ConnectWithCertificateForDifferentName_Throws (dotnet#56456)
  Update dependencies from https://github.com/mono/linker build 20210726.2 (dotnet#56374)
  Cleanup disabled test conditions (dotnet#56381)
  [mono] Add GC unsafe transition to mono_unhandled_exception  (dotnet#56380)
  don't fail the file extraction when we can't set last write time (dotnet#56370)
  Properly rebuild optimization data when it changes (dotnet#56397)
  Make open function calls in coreclr EINTR resilient on macOS (dotnet#56403)
  Fix dependency from EventLog to TraceSource ref (dotnet#56417)
  Fix comments in asm with JitDiffableDasm=1 (dotnet#56416)
  Catch TcpClient ctor exceptions in FtpWebRequest.CreateConnectionAsync (dotnet#56379)
  Add interop between serializer and DOMs (dotnet#56112)
  Fix type loader not recognizing overridden method (dotnet#56337)
  Prevent Segfault in EventPipe on disable (dotnet#56104)
  Update runtimeconfig.json and deps.json paths when these break past the MAX_PATH threshold  (dotnet#56224)
  Use native allocator instead of pinning when decompressing embedded PDB (dotnet#56336)
  Specify win-x64 as a valid platform in the microsoft-net-runtime-* workloads for iOS/tvOS/MacCatalyst (dotnet#56311)
  Fix FailFast message formatting race (dotnet#56388)
  Try to fix finalizer-based async tests (dotnet#56384)
  Fix MetricsEventSource tests (dotnet#56382)
  Remove invalid Castle.DynamicProxy.Internal.AbstractInvocation from ILLink descriptor files (dotnet#56392)
  ...
@jaredpar jaredpar mentioned this pull request Aug 17, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 27, 2021
@davidwrighton davidwrighton deleted the fix_44533 branch April 13, 2023 18:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas left review comments

@janvorli janvorli janvorli approved these changes

@trylek trylek trylek approved these changes

Assignees
No one assigned
Labels
area-TypeSystem-coreclr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Multiple default implementation properties in an interface, applied to an abstract hierarchy, causes TypeLoadException
4 participants
@davidwrighton @jkotas @janvorli @trylek