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

[NativeAOT] Delegate bug fixes #99185

Merged
merged 18 commits into from
Mar 9, 2024
Merged

[NativeAOT] Delegate bug fixes #99185

merged 18 commits into from
Mar 9, 2024

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Mar 2, 2024

  • Fix Delegate.Method and Delegate.Target for marshalled delegates
  • Add tests and fixes for corner various delegate corner case behaviors
  • Delete runtime test for GetInvocationList since there is a better test coverage for this API under libraries

Fixes dotnet/runtimelab#164

@jkotas
[NativeAOT] Delegate bug fixes
fa3d1ec 
- Fix Delegate.Method and Delegate.Target for marshalled delegates
- Add tests and fixes for corner various delegate corner case behaviors
- Delete runtime test for GetInvocationList since there is a better test coverage for this API under libraries

Fixes dotnet/runtimelab#164
@jkotas jkotas requested a review from MichalStrehovsky as a code owner March 2, 2024 01:46
@ghost ghost assigned jkotas Mar 2, 2024
@ghost
Copy link

ghost commented Mar 2, 2024

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details
  • Fix Delegate.Method and Delegate.Target for marshalled delegates
  • Add tests and fixes for corner various delegate corner case behaviors
  • Delete runtime test for GetInvocationList since there is a better test coverage for this API under libraries

Fixes dotnet/runtimelab#164

Author: jkotas
Assignees: jkotas
Labels:

area-Meta
Milestone: -

@ghost
Copy link

ghost commented Mar 2, 2024

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

Issue Details
  • Fix Delegate.Method and Delegate.Target for marshalled delegates
  • Add tests and fixes for corner various delegate corner case behaviors
  • Delete runtime test for GetInvocationList since there is a better test coverage for this API under libraries

Fixes dotnet/runtimelab#164

Author: jkotas
Assignees: jkotas
Labels:

area-Meta, area-NativeAOT-coreclr
Milestone: -

@jkotas jkotas marked this pull request as draft March 2, 2024 01:47
This was referenced Mar 2, 2024
Closed
Closed
@AustinWise
Copy link
Contributor

While reviewing I noticed the PInvokeDelegateThunk class may be finalized before the delegate is fully collected. This causes this program to crash on NativeAOT:

https://gist.github.com/AustinWise/87eb03c4ec0cd06564c79e9b33bcfc07

One possible fix: make GCHandle in PInvokeDelegateThunk be WeakTrackResurrection. In the PInvokeDelegateThunk finalizer check to see if the object pointed to by the handle is still alive. If so, call ReRegisterForFinalize to defer finalization.

This was referenced Mar 3, 2024
Closed
Open
Closed
@jkotas jkotas force-pushed the DelegateMethodInfo branch from 99a99e9 to 7a2df9b Compare March 5, 2024 05:41
This was referenced Mar 5, 2024
Closed
Closed
@jkotas
Copy link
Member Author

jkotas commented Mar 6, 2024

/azp run runtime-nativeaot-outerloop

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Mar 6, 2024
Open
Closed
@build-analysis build-analysis bot mentioned this pull request Mar 6, 2024
Closed
@jkotas
Copy link
Member Author

jkotas commented Mar 7, 2024

While reviewing I noticed the PInvokeDelegateThunk class may be finalized before the delegate is fully collected

@AustinWise Thank you! I have included the fix and the test in this PR.

@jkotas
Copy link
Member Author

jkotas commented Mar 7, 2024

/azp run runtime-nativeaot-outerloop

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas jkotas marked this pull request as ready for review March 7, 2024 05:10
@jkotas jkotas requested a review from marek-safar as a code owner March 7, 2024 05:10
@jkotas
Copy link
Member Author

jkotas commented Mar 7, 2024

This is ready for review

@jkotas
Copy link
Member Author

jkotas commented Mar 7, 2024

/azp run runtime-nativeaot-outerloop

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

MichalStrehovsky
MichalStrehovsky approved these changes Mar 7, 2024
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.

:shipit:

This was referenced Mar 7, 2024
Open
Open
@jkotas jkotas mentioned this pull request Mar 9, 2024
Open
@jkotas jkotas mentioned this pull request Mar 9, 2024
Closed
@jkotas jkotas merged commit cc17166 into dotnet:main Mar 9, 2024
187 of 189 checks passed
@jkotas jkotas deleted the DelegateMethodInfo branch March 9, 2024 15:52
@AustinWise AustinWise mentioned this pull request Mar 11, 2024
Closed
4 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Apr 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MichalStrehovsky MichalStrehovsky MichalStrehovsky approved these changes

@marek-safar marek-safar Awaiting requested review from marek-safar

Assignees

@jkotas jkotas

Labels
area-NativeAOT-coreclr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Roundtripping delegates through a native function pointer
3 participants
@jkotas @AustinWise @MichalStrehovsky