[NativeAOT] Missing memory fence before bulk move of objects #90890

VSadov · 2023-08-21T20:25:05Z

Memory model requires that the modifications to an object are observable by other threads no later than the reference to the object becomes observable.
It is possible that RhBulkMoveWithWriteBarrier is making the objects shared, thus needs a fence.

See similar fence in CoreCLR InlinedMemmoveGCRefsHelper

runtime/src/coreclr/classlibnative/bcltype/arraynative.inl

Line 310 in e18e4aa

GCHeapMemoryBarrier();

ghost · 2023-08-21T20:25:19Z

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

Issue Details

Memory model requires that the object modifications are observable before the reference itself is observable.
It is possible that RhBulkMoveWithWriteBarrier is making the objects shared, thus needs a fence.

See similar fence in CoreCLR InlinedMemmoveGCRefsHelper

runtime/src/coreclr/classlibnative/bcltype/arraynative.inl

Line 310 in e18e4aa

GCHeapMemoryBarrier();

Author:	VSadov
Assignees:	-
Labels:	`area-NativeAOT-coreclr`
Milestone:	-

VSadov · 2023-08-21T20:30:23Z

In theory the fence could be done conditionally on whether destination is a heap location. Only a "publising" write needs a fence. Also a store fence should be sufficient. (as in dmb ishst).

I think we may want to backport this to 8.0, thus just doing the same unconditional full fence as in CoreCLR seems less risky.

VSadov · 2023-08-21T20:32:31Z

src/coreclr/nativeaot/Runtime/GCMemoryHelpers.cpp

    if (pDest <= pSrc || pSrc + cbDest <= pDest)
        InlineForwardGCSafeCopy(pDest, pSrc, cbDest);
    else
        InlineBackwardGCSafeCopy(pDest, pSrc, cbDest);

    InlinedBulkWriteBarrier(pDest, cbDest);
 }
-
-void REDHAWK_CALLCONV RhpBulkWriteBarrier(void* pMemStart, uint32_t cbMemSize)


This helper was unused.

jkotas · 2023-08-21T20:41:57Z

src/coreclr/nativeaot/Runtime/GCMemoryHelpers.cpp

@@ -36,15 +36,18 @@ COOP_PINVOKE_CDECL_HELPER(void *, RhpGcSafeZeroMemory, (void * mem, size_t size)

 COOP_PINVOKE_HELPER(void, RhBulkMoveWithWriteBarrier, (uint8_t* pDest, uint8_t* pSrc, size_t cbDest))
 {
+#if TARGET_ARM64


Can we define GCHeapMemoryBarrier macro that matches what we do in CoreCLR?

runtime/src/coreclr/vm/common.h

Lines 338 to 352 in e18e4aa

#if defined(TARGET_X86) || defined(TARGET_AMD64)

//

// Strong memory model. No memory barrier necessary before writing object references into GC heap.

//

#define GCHeapMemoryBarrier()

#else

//

// The weak memory model forces us to raise memory barriers before writing object references into GC heap. This is required

// for both security and to make most managed code written against strong memory model work. Under normal circumstances, this memory

// barrier is part of GC write barrier. However, there are a few places in the VM that set cards manually without going through

// regular GC write barrier. These places need to this macro. This macro is usually used before memcpy-like operation followed

// by SetCardsAfterBulkCopy.

//

#define GCHeapMemoryBarrier() MemoryBarrier()

#endif

I am not sure GCHeapMemoryBarrier should be a helper.
It sounds like something more general for use in GC, but it is used only in the bulk copy helper and that is the only place where it is applicable.

Also, ideally it should be something like

#if TARGET_ARM64 if (InHeap(dst)) { StoreMemoryBarrier(); } #endif

That is to not do fences when this is used to copy structs on stack.

I guess, since we are not doing that, we could match the CoreCLR pattern.

jkotas · 2023-08-21T20:42:28Z

src/coreclr/nativeaot/Runtime/GCMemoryHelpers.h

@@ -4,5 +4,3 @@
 //
 // Unmanaged GC memory helpers


Delete this file?

jkotas

Thanks

VSadov · 2023-08-22T03:00:40Z

Thanks!

VSadov · 2023-08-22T03:01:15Z

I think we want an 8.0 backport for this.

VSadov · 2023-08-22T18:18:50Z

/backport to release/8.0

github-actions · 2023-08-22T18:19:03Z

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/5942571566

Memory fence before bulk move of objects

4652bbe

dotnet-issue-labeler bot added the area-NativeAOT-coreclr label Aug 21, 2023

ghost assigned VSadov Aug 21, 2023

VSadov requested a review from jkotas August 21, 2023 20:31

VSadov commented Aug 21, 2023

View reviewed changes

VSadov marked this pull request as ready for review August 21, 2023 20:34

VSadov requested a review from MichalStrehovsky as a code owner August 21, 2023 20:34

jkotas reviewed Aug 21, 2023

View reviewed changes

VSadov added 2 commits August 21, 2023 14:09

deleted GCMemoryHelpers.h

837947a

Introduced a GCHeapMemoryBarrier helper.

43af4a1

jkotas approved these changes Aug 21, 2023

View reviewed changes

VSadov merged commit 3bda6e0 into dotnet:main Aug 22, 2023
108 checks passed

VSadov deleted the bulkBar branch August 22, 2023 03:00

github-actions bot mentioned this pull request Aug 22, 2023

[release/8.0] [NativeAOT] Missing memory fence before bulk move of objects #90941

Merged

ghost locked as resolved and limited conversation to collaborators Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NativeAOT] Missing memory fence before bulk move of objects #90890

[NativeAOT] Missing memory fence before bulk move of objects #90890

VSadov commented Aug 21, 2023 •

edited

Loading

ghost commented Aug 21, 2023

VSadov commented Aug 21, 2023 •

edited

Loading

VSadov Aug 21, 2023

jkotas Aug 21, 2023

VSadov Aug 21, 2023 •

edited

Loading

jkotas Aug 21, 2023

jkotas left a comment

VSadov commented Aug 22, 2023

VSadov commented Aug 22, 2023

VSadov commented Aug 22, 2023

github-actions bot commented Aug 22, 2023

	#if defined(TARGET_X86) \|\| defined(TARGET_AMD64)
	//
	// Strong memory model. No memory barrier necessary before writing object references into GC heap.
	//
	#define GCHeapMemoryBarrier()
	#else
	//
	// The weak memory model forces us to raise memory barriers before writing object references into GC heap. This is required
	// for both security and to make most managed code written against strong memory model work. Under normal circumstances, this memory
	// barrier is part of GC write barrier. However, there are a few places in the VM that set cards manually without going through
	// regular GC write barrier. These places need to this macro. This macro is usually used before memcpy-like operation followed
	// by SetCardsAfterBulkCopy.
	//
	#define GCHeapMemoryBarrier() MemoryBarrier()
	#endif

[NativeAOT] Missing memory fence before bulk move of objects #90890

[NativeAOT] Missing memory fence before bulk move of objects #90890

Conversation

VSadov commented Aug 21, 2023 • edited Loading

ghost commented Aug 21, 2023

VSadov commented Aug 21, 2023 • edited Loading

VSadov Aug 21, 2023

Choose a reason for hiding this comment

jkotas Aug 21, 2023

Choose a reason for hiding this comment

VSadov Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

jkotas Aug 21, 2023

Choose a reason for hiding this comment

jkotas left a comment

Choose a reason for hiding this comment

VSadov commented Aug 22, 2023

VSadov commented Aug 22, 2023

VSadov commented Aug 22, 2023

github-actions bot commented Aug 22, 2023

VSadov commented Aug 21, 2023 •

edited

Loading

VSadov commented Aug 21, 2023 •

edited

Loading

VSadov Aug 21, 2023 •

edited

Loading