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

[Mono] UnsafeAccessorAttribute non-generic support for field #88626

Merged
merged 35 commits into from
Jul 14, 2023
Merged

[Mono] UnsafeAccessorAttribute non-generic support for field #88626

merged 35 commits into from
Jul 14, 2023

Conversation

fanyang-mono
Copy link
Member

Contributes to #86040

fanyang-mono and others added 14 commits July 5, 2023 16:02
@fanyang-mono
Detect an UnsafeAccessorAttribute for method with interpreter
bc9d6ed
@fanyang-mono
Change field to property
1643710
@fanyang-mono
Get Kind from typed_args
b0def6c
@lambdageek @fanyang-mono
define MonoUnsafeAccessorKind enum
0c51e8a
@fanyang-mono
Add the frontend for JIT
94ce7d2
@lambdageek @fanyang-mono
Add mono_marshal_get_unsafe_accessor_wrapper and WRAPPER_SUBTYPE_UNSA…
a49a8e0 
…FE_ACCESSOR

And the associated AOT compiler/runtime and marshaling caching boilerplate.
@lambdageek @fanyang-mono
[interp] get the unsafe accessor wrapper
4701689
@lambdageek @fanyang-mono
fix: skip visibility in unsafe accessor wrappers
495ce38 
that is the whole point of them
@lambdageek @fanyang-mono
fix: decode the length and copy the name from UnsafeAccessorAttribute
35c4d45 
The name has a length as a prefix and doesn not have a null terminator
@lambdageek @fanyang-mono
[mini] compile wrapper
0c8f4da
@lambdageek @fanyang-mono
[aot] Emit unsafe accessor wrappers to the AOT image
8d039e6
@fanyang-mono
Add the method to emit wrapper for field
080559a
@fanyang-mono
Fix typo
2ec266b
@fanyang-mono
Remove assertion for interpreter
8571969
@ghost ghost assigned fanyang-mono Jul 10, 2023
@build-analysis build-analysis bot mentioned this pull request Jul 13, 2023
Closed
lambdageek
lambdageek reviewed Jul 13, 2023
View reviewed changes
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

a couple of nits for the new test and static modifier check

src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.cs Outdated Show resolved Hide resolved
src/mono/mono/metadata/marshal-lightweight.c Outdated Show resolved Hide resolved
lambdageek
lambdageek approved these changes Jul 13, 2023
View reviewed changes
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

LGTM. I think we're mostly focusing on nits now

@fanyang-mono fanyang-mono mentioned this pull request Jul 13, 2023
Closed
@fanyang-mono
Copy link
Member Author

The newly added tests for checking static modifier failed on CoreCLR. Disabled the test and created an issue(#88858) to track that.

fanyang-mono and others added 6 commits July 13, 2023 15:30
@fanyang-mono @AaronRobinsonMSFT
Update src/tests/baseservices/compilerservices/UnsafeAccessors/Unsafe…
8afb00a 
…AccessorsTests.cs

Co-authored-by: Aaron Robinson <arobins@microsoft.com>
@fanyang-mono @AaronRobinsonMSFT
Update src/tests/baseservices/compilerservices/UnsafeAccessors/Unsafe…
a3f9e77 
…AccessorsTests.cs

Co-authored-by: Aaron Robinson <arobins@microsoft.com>
@fanyang-mono @AaronRobinsonMSFT
Update src/tests/baseservices/compilerservices/UnsafeAccessors/Unsafe…
9f6b532 
…AccessorsTests.cs

Co-authored-by: Aaron Robinson <arobins@microsoft.com>
@fanyang-mono @AaronRobinsonMSFT
Update src/tests/baseservices/compilerservices/UnsafeAccessors/Unsafe…
0c15ccd 
…AccessorsTests.cs

Co-authored-by: Aaron Robinson <arobins@microsoft.com>
@fanyang-mono @AaronRobinsonMSFT
Update src/tests/baseservices/compilerservices/UnsafeAccessors/Unsafe…
f21b78e 
…AccessorsTests.cs

Co-authored-by: Aaron Robinson <arobins@microsoft.com>
@fanyang-mono
Address more review feedback
781f512
@fanyang-mono
Copy link
Member Author

The newly added tests for checking static modifier failed on CoreCLR. Disabled the test and created an issue(#88858) to track that.

Aaron updated the test, it is passing now.

This was referenced Jul 14, 2023
Open
Closed
@AaronRobinsonMSFT
Copy link
Member

The failures all appear to be known - #88867 and #82637.

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 45db21d into dotnet:main Jul 14, 2023
@lambdageek lambdageek mentioned this pull request Jul 14, 2023
Closed
7 tasks
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kotlarmilos kotlarmilos kotlarmilos left review comments

@lambdageek lambdageek lambdageek approved these changes

@vargaz vargaz vargaz left review comments

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT approved these changes

@BrzVlad BrzVlad Awaiting requested review from BrzVlad BrzVlad is a code owner

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

@thaystg thaystg Awaiting requested review from thaystg thaystg is a code owner

@jandupej jandupej Awaiting requested review from jandupej

Assignees

@fanyang-mono fanyang-mono

Labels
area-VM-meta-mono
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fanyang-mono @AaronRobinsonMSFT @vargaz @lambdageek @kotlarmilos