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.

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

[NativeAOT] Remove unused native memcopy helpers. #84314

Merged
merged 2 commits into from
Apr 4, 2023
Merged

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Apr 4, 2023

Managed memcopy nowdays calls RhBulkMoveWithWriteBarrier and only when the copy needs to set cards.

The native helpers to do memory copying are dead code and it looks like they have been dead code for a while and may not even function correctly. One example - it looks like EH knew how to handle AV in these, but the support was somehow lost with time.

@ghost
Copy link

ghost commented Apr 4, 2023

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

Issue Details

Managed memcopy nowdays only calls native helpers to set cards, if needed.

The native helpers to do memory copying are dead code and it looks like they have been dead code for a while and may not even function correctly. One example - it looks like EH knew how to handle AV in these, but the support was somehow lost with time.

Author: VSadov
Assignees: VSadov
Labels:

area-NativeAOT-coreclr

Milestone: -

}

InlineGCSafeFillMemory(mem, size, pv);
InlineGCSafeFillMemory(mem, size, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Rename this to InlineGCSafeZeroMemory and delete the extra argument?

@@ -21,91 +21,17 @@
//
// USAGE: The caller is responsible for hoisting any null reference exceptions to a place where the hardware exception
// can be properly translated to a managed exception.
COOP_PINVOKE_CDECL_HELPER(void *, RhpInitMultibyte, (void * mem, int c, size_t size))
COOP_PINVOKE_CDECL_HELPER(void *, RhpInitMultibyte, (void * mem, size_t size))
Copy link
Member

Choose a reason for hiding this comment

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

Change the comment and/or the name to indicate that the buffer can contain GC references?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. I am also tempted to move this to a managed helper as well, but I do not see a lot of reasons.
This is only used when unboxing a nullable null.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll move this to managed when/if we have a caller that could realistically need to zero something larger than BulkMoveWithWriteBarrierChunk.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

@VSadov
Copy link
Member Author

VSadov commented Apr 4, 2023

Thanks!!

@VSadov VSadov merged commit 70d00e4 into dotnet:main Apr 4, 2023
@VSadov VSadov deleted the memcpyHlp branch April 4, 2023 23:30
@ghost ghost locked as resolved and limited conversation to collaborators May 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants