Always do copy_stack_data on entering GC safe/unsafe mode. (mono/mono#17150)

Commit migrated from https://github.com/mono/mono/commit/2aa7adb269c795b721a9fcbac98969efb5b4a006

diff --git a/src/mono/configure.ac b/src/mono/configure.ac
index 92c6bf8..6ffe717 100644 (file)
--- a/src/mono/configure.ac
+++ b/src/mono/configure.ac
@@ -1352,7 +1352,6 @@ with_wasm_default=no
 with_bitcode_default=no
 enable_cooperative_suspend_default=no
 enable_hybrid_suspend_default=no
-enable_copy_stack_data_default=yes
 
 # For the sake of clearer error messages, these numbers should all be different from each other.
 INVARIANT_AOT_OPTIONS=nimt-trampolines=2000,ntrampolines=10000,nrgctx-fetch-trampolines=256,ngsharedvt-trampolines=4400,nftnptr-arg-trampolines=4000
@@ -5693,38 +5692,6 @@ AM_CONDITIONAL([ENABLE_HYBRID_SUSPEND], [test x$enable_hybrid_suspend != xno])
 
 dnl End of thread suspend policy
 
-dnl *******************************
-dnl *** Copy stack data support ***
-dnl *******************************
-
-dnl Disable copy_stack_data on platforms supporting full context and
-dnl having access to full stack of running threads.
-if test x$enable_copy_stack_data_default != xno; then
-       case $HOST in
-       X86 | AMD64)
-               if test x$target_osx = xyes; then
-                       dnl Some host/target confusion, there's no host_osx (and
-                       dnl host_darwin would be true on iOS not just macOS).
-                       enable_copy_stack_data_default=no
-               elif test x$host_linux = xyes -o x$host_win32 = xyes; then
-                       enable_copy_stack_data_default=no
-               fi
-               ;;
-       esac
-fi
-
-AC_ARG_ENABLE(copy_stack_data, [ --enable-copy-stack-data     Enable copy_stack_data feature for hybrid/cooperative suspend scenarios, (defaults to yes)], [], [enable_copy_stack_data=default])
-
-if test x$enable_copy_stack_data = xdefault; then
-   enable_copy_stack_data=$enable_copy_stack_data_default
-fi
-
-if test x$enable_copy_stack_data != xno ; then
-       AC_DEFINE(ENABLE_COPY_STACK_DATA,1,[Enable copy_stack_data feature for hybrid/cooperative suspend scenarios])
-fi
-
-dnl End of Copy stack data support
-
 dnl ***************************
 dnl *** feature experiments ***
 dnl ***************************

diff --git a/src/mono/mono/utils/mono-threads-coop.c b/src/mono/mono/utils/mono-threads-coop.c
index 90b8575..4114aaf 100644 (file)
--- a/src/mono/mono/utils/mono-threads-coop.c
+++ b/src/mono/mono/utils/mono-threads-coop.c
@@ -34,12 +34,6 @@
 #include <mono/utils/mach-support.h>
 #endif
 
-/* On platforms that doesn't have full context support (or doesn't do conservative stack scan), use copy stack data */
-/* when entering safe/unsafe GC regions. For platforms with full context support (doing conservative stack scan), */
-/* there is already logic in place to take context before getting in a state where thread could be conservative */
-/* scanned by GC. Avoiding doing additional stack copy will increse performance when entering safe/unsafe regions */
-/* when running in hybrid/cooperative supspend mode. */
-#if defined (ENABLE_COPY_STACK_DATA)
 #ifdef _MSC_VER
 // __builtin_unwind_init not available under MSVC but equivalent implementation is done using
 // copy_stack_data_internal_win32_wrapper.
@@ -50,9 +44,6 @@
 #else 
 #define SAVE_REGS_ON_STACK __builtin_unwind_init ();
 #endif
-#else
-#define SAVE_REGS_ON_STACK do {} while (0)
-#endif
 
 volatile size_t mono_polling_required;
 
@@ -207,7 +198,6 @@ copy_stack_data_internal (MonoThreadInfo *info, MonoStackData *stackdata_begin,
        state->gc_stackdata_size = stackdata_size;
 }
 
-#if defined (ENABLE_COPY_STACK_DATA)
 #ifdef _MSC_VER
 typedef void (*CopyStackDataFunc)(MonoThreadInfo *, MonoStackData *, gconstpointer, gconstpointer);
 
@@ -258,12 +248,6 @@ copy_stack_data (MonoThreadInfo *info, MonoStackData *stackdata_begin)
        copy_stack_data_internal (info, stackdata_begin, NULL, NULL);
 }
 #endif
-#else
-static void
-copy_stack_data (MonoThreadInfo *info, MonoStackData *stackdata_begin)
-{
-}
-#endif
 
 static gpointer
 mono_threads_enter_gc_safe_region_unbalanced_with_info (MonoThreadInfo *info, MonoStackData *stackdata);
@@ -326,6 +310,11 @@ mono_threads_enter_gc_safe_region_unbalanced_with_info (MonoThreadInfo *info, Mo
 
        check_info (info, "enter", "safe", function_name);
 
+       // NOTE, copy_stack_data needs to be done. One problem it solves is optimization taking place between stackdata snapshot and
+       // thread_state_init, storing changed register(s) on stack and if those register(s) include managed references
+       // (that are not previously stored anywhere on the stack), then GC won't detect that reference(s). Storing the stack
+       // and registers into a separate location makes sure we still see any registers temporary stored on stack due to optimizations
+       // done between stackdata snapshot and thread_state_init.
        copy_stack_data (info, stackdata);
 
 retry:

diff --git a/src/mono/netcore/CoreFX.issues.rsp b/src/mono/netcore/CoreFX.issues.rsp
index 6a8293a..d0d2911 100644 (file)
--- a/src/mono/netcore/CoreFX.issues.rsp
+++ b/src/mono/netcore/CoreFX.issues.rsp
@@ -983,9 +983,6 @@
 # Flaky test (FileSystemWatcher)
 -nomethod System.IO.Tests.Directory_NotifyFilter_Tests.FileSystemWatcher_Directory_NotifyFilter_LastWriteTime
 
-# Flaky test
--nomethod System.Security.Cryptography.Rsa.Tests.RSAKeyFileTests.ReadEncryptedRsa16384
-
 ####################################################################
 ##  System.Security.Cryptography.Encoding.Tests
 ####################################################################
@@ -996,7 +993,6 @@
 -nomethod System.Security.Cryptography.Encoding.Tests.OidTests.Oid_StringString_NullFriendlyName
 -nomethod System.Security.Cryptography.Encoding.Tests.OidTests.LookupOidByFriendlyName_Ctor
 
-
 # corefx tests need to be updated
 -nomethod System.Numerics.Tests.GenericVectorTests.GetHashCodeInt16
 -nomethod System.Numerics.Tests.GenericVectorTests.GetHashCodeInt32