Skip to content
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

Fix issues with virtuals and mdarrays #53400

Merged
merged 3 commits into from
May 31, 2021
Merged

Fix issues with virtuals and mdarrays #53400

merged 3 commits into from
May 31, 2021

Conversation

davidwrighton
Copy link
Member

  • Change up the definition of an MDArray which doesn't need a special descriptor in the embedded signature data to match the type of mdarray thay everyone always uses. This fixes the virtual function handling behavior for all reasonable cases. (All cases where the MDArray is used for a base type in the expected fashion.)
  • Add a devirtualization test for this specific scenario

Fixes #52444

@davidwrighton
Add new test to cover override involving generics and multidimensiona…
f94b847 
…l arrays
@davidwrighton
Change up the definition of an MDArray which doesn't need a special d…
12720cf 
…escriptor in the embedded signature data to match the type of mdarray thay everyone always uses. This fixes the virtual function handling behavior for all reasonable cases.
MichalStrehovsky
MichalStrehovsky reviewed May 28, 2021
View reviewed changes
src/coreclr/tools/Common/TypeSystem/MetadataEmitter/TypeSystemMetadataEmitter.cs Outdated
}
}

static ImmutableArray<int>[] ImmutableArraysFilledWithZeroes = CreateStaticArrayOfImmutableArraysFilledWithZeroes(33); // The max rank of an array is 32
Copy link
Member

Choose a reason for hiding this comment

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

If this can come from user code, might not hurt to add a fallback that just creates a fresh immutable array if rank is more than 32. C# has no problem emitting such array.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, seems reasonable.

src/coreclr/tools/Common/TypeSystem/Ecma/EcmaSignatureParser.cs

if (boundsCount != 0 || lowerBoundsCount != 0)
if (boundsCount != 0 || lowerBoundsCount != rank || nonZeroLowerBounds)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why this is lowerBoundsCount != rank || nonZeroLowerBounds instead of just nonZeroLowerBounds? It seems natural that if we treat [0..,0..] same as [,], we should also treat [0..,] the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't want to treat [,] the same as [0...,0...] those are considered incompatible in a signature. Only [0...,0...] would be a valid scenario for a rank 2 mdarray to not carry the extra signature data.

trylek
trylek approved these changes May 28, 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.

LGTM modulo Michal's feedback that seems reasonable and might be worth adding one or two more test cases to the IL test. Thank you!

MichalStrehovsky
MichalStrehovsky approved these changes May 31, 2021
View reviewed changes
Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Thank you!

@MichalStrehovsky MichalStrehovsky merged commit fea7ff2 into dotnet:main May 31, 2021
thaystg added a commit to thaystg/runtime that referenced this pull request Jun 1, 2021
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_remove_mini_w…
c4bf8fc 
…asm_debugger_and_use_debugger_agent

* upstream/main: (597 commits)
  Fix42292 (dotnet#52463)
  [mono] Remove some obsolete emscripten flags. (dotnet#53486)
  Fixed path to projects (dotnet#53435)
  support ServerCertificateContext in quic (dotnet#53175)
  Socket: delete unix local endpoint filename on Close (dotnet#52103)
  [mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364)
  Refactor MsQuic's native IP address types. (dotnet#53461)
  Re-enabled optimizations for gtFoldExprConst (dotnet#53347)
  Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361)
  Fix issues with virtuals and mdarrays (dotnet#53400)
  Split Variant marshalling tests (dotnet#53035)
  Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470)
  remove WSL checks in tests (dotnet#53475)
  Always spawn message loop thread for SystemEvents (dotnet#53467)
  add AcceptAsync cancellation overloads (dotnet#53340)
  Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425)
  Add CookieContainer.GetAllCookies (dotnet#53441)
  Remove DynamicallyAccessedMembers on JsonSerializer  (dotnet#53235)
  [wasm] Re-enable Wasm.Build.Tests (dotnet#53433)
  [libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253)
  ...
@ghost ghost locked as resolved and limited conversation to collaborators Jun 30, 2021
@davidwrighton davidwrighton deleted the fix_issues_with_virtuals_and_mdarrays branch April 13, 2023 18:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@CertifiedRice CertifiedRice CertifiedRice approved these changes

@trylek trylek trylek approved these changes

@MichalStrehovsky MichalStrehovsky MichalStrehovsky approved these changes

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

Successfully merging this pull request may close these issues.

Managed type system has trouble resolving virtual methods
4 participants
@davidwrighton @MichalStrehovsky @trylek @CertifiedRice