[mono] Don't generate safepoints inside write barrier wrapper

[BrzVlad]

A ref store and its associated write barrier need to happen atomically relative to garbage collections. No GCs should happen between the ref store and the card mark.

[BrzVlad]

@imhameed @EgorBo I think I might be missing a place where safe points are added. I think there is some logic in llvm itself adding safepoints. Any observations ?

[lambdageek]

Do we have a list of LLVM passes that we use somewhere? It should be the https://llvm.org/docs/Statepoints.html#placesafepoints PlaceSafepoints pass that adds calls to gc.safepoint_poll.

I think the decision of which functions should get safepoints is controlled by PlaceSafepoints::shouldRewriteFunction https://github.com/dotnet/llvm-project/blob/release/11.x/llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp#L454-L456

I guess the way to mark a Function not to do a safepoint is to make sure that Function::getGC returns something other than "mono" (or "coreclr" or "safepoint-example"), right? Do we have that exposed to the aot-compiler?

@imhameed

[imhameed]

@imhameed @EgorBo I think I might be missing a place where safe points are added. I think there is some logic in llvm itself adding safepoints. Any observations ?

Calls to gc.safepoint_poll are automatically inserted by the PlaceSafepoints pass, which we always use with LLVM as an AOT backend and LLVM as a JIT backend.

[imhameed]

Do we have a list of LLVM passes that we use somewhere? It should be the https://llvm.org/docs/Statepoints.html#placesafepoints PlaceSafepoints pass that adds calls to gc.safepoint_poll.

We don't have a centralized list of all the LLVM passes we use, and the passes we use differ between LLVM AOT and LLVM as a JIT. For AOT, the flags we pass to opt and llc are here. For JIT, the passes we use are listed here and here.

I think the decision of which functions should get safepoints is controlled by PlaceSafepoints::shouldRewriteFunction https://github.com/dotnet/llvm-project/blob/release/11.x/llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp#L454-L456

I guess the way to mark a Function not to do a safepoint is to make sure that Function::getGC returns something other than "mono" (or "coreclr" or "safepoint-example"), right? Do we have that exposed to the aot-compiler?

You could call LLVMSetGC(method, NULL) to make PlaceSafepoints::runOnFunction leave your function alone (or simply not call LLVMSetGC at all).

[lambdageek]

Do we have a list of LLVM passes that we use somewhere? It should be the https://llvm.org/docs/Statepoints.html#placesafepoints PlaceSafepoints pass that adds calls to gc.safepoint_poll.

We don't have a centralized list of all the LLVM passes we use, and the passes we use differ between LLVM AOT and LLVM as a JIT. For AOT, the flags we pass to opt and llc are here. For JIT, the passes we use are listed here and here.

I think the decision of which functions should get safepoints is controlled by PlaceSafepoints::shouldRewriteFunction https://github.com/dotnet/llvm-project/blob/release/11.x/llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp#L454-L456
I guess the way to mark a Function not to do a safepoint is to make sure that Function::getGC returns something other than "mono" (or "coreclr" or "safepoint-example"), right? Do we have that exposed to the aot-compiler?

You could call LLVMSetGC(method, NULL) to make PlaceSafepoints::runOnFunction leave your function alone (or simply not call LLVMSetGC at all).

@BrzVlad So add the barrier wrapper here:

runtime/src/mono/mono/mini/mini-llvm.c

Lines 11464 to 11470 in b6cc822

	if (!cfg->llvm_only && mono_threads_are_safepoints_enabled () && requires_safepoint) {
	if (!cfg->compile_aot && cfg->method->wrapper_type != MONO_WRAPPER_ALLOC) {
	LLVMSetGC (method, "coreclr");
	emit_gc_safepoint_poll (ctx->module, ctx->lmodule, cfg);
	} else if (cfg->compile_aot) {
	LLVMSetGC (method, "coreclr");
	}

@imhameed Why do we bother with a "mono" GC type when the runtime actually uses "coreclr" 🤪

[BrzVlad]

Refactored the code a little bit so we disable safepoints in GC wrappers both on aot and !aot.

[lambdageek]

@BrzVlad do we want this for 6.0? How risky is it?

[BrzVlad]

I think the risk is low but not negligible.

I don't know if this issue ever lead to a crash, so I don't think it is a mandatory backport.

This change might have some noticeable perf gains on AOT since we avoid wrapper + safepoints for stores into ref fields, so that could be a motivating factor. Maybe if we see some crazy perf gains in the benchmark runs that could justify it.