From 7cd5d236cb486d241a9408597587cae5676dbc4d Mon Sep 17 00:00:00 2001 From: Jay Krell Date: Thu, 25 Jul 2019 03:19:02 -0700 Subject: [PATCH] thread names: coop, constants, usually ignore-error. - Convert setting thread name to be coop-compatible. - For constant thread names, just retain the constant, not a copy. This includes producing and using constant unicode thread names on Windows. - Make setting a thread name often non-fatal, except where mandated by public API. - From earlier PR: mono_free (mono-publib.c) would no longer be referenced and therefore no longer exported. That broke profilers and maybe other externals. Choices: 1 Use it instead of g_free randomly sometimes. 2 Call it randomly sometimes. 3 include it in external-only.c or object.c 4 Mark it external only (breaks profiler). 5 Use a .def file or Unix equivalent. 5 is best, 2 is done here, the advantages/disadvantages among most choices are subtle. 3 is probably better than 2 but is slightly bigger change, to put off -- you'd stop compiling mono-publib.c and move it to include/extra_redist. The advantage of not-.def file is perhaps that it is exposed by the compiler, so maybe easy to port and work with. People are more comfortable with obscure C extensions than any linker options, and there are multiple linkers to contend with. Arguably there are fewer compilers. Commit migrated from https://github.com/mono/mono/commit/0c6cb9fbc3f97e15acf6be07eb46c19a288b3937 --- src/mono/configure.ac | 2 +- src/mono/mono/metadata/appdomain.c | 12 +------ src/mono/mono/metadata/attach.c | 10 ++---- src/mono/mono/metadata/gc.c | 6 +--- src/mono/mono/metadata/icall-def-netcore.h | 2 +- src/mono/mono/metadata/icall-def.h | 2 +- src/mono/mono/metadata/object-internals.h | 4 +-- src/mono/mono/metadata/threadpool-io.c | 8 ++--- src/mono/mono/metadata/threadpool.c | 11 +++--- src/mono/mono/metadata/threads-types.h | 9 +++-- src/mono/mono/metadata/threads.c | 41 ++++++++++------------ src/mono/mono/mini/aot-compiler.c | 7 +--- src/mono/mono/mini/debugger-agent.c | 6 +--- src/mono/mono/mini/mini-posix.c | 7 +--- src/mono/mono/profiler/aot.c | 9 +---- src/mono/mono/profiler/log.c | 12 +++---- .../src/System.Threading/Thread.cs | 8 ++++- 17 files changed, 59 insertions(+), 97 deletions(-) diff --git a/src/mono/configure.ac b/src/mono/configure.ac index fc6f354..7207324 100644 --- a/src/mono/configure.ac +++ b/src/mono/configure.ac @@ -62,7 +62,7 @@ MONO_VERSION_BUILD=`echo $VERSION | cut -d . -f 3` # This line is parsed by tools besides autoconf, such as msvc/mono.winconfig.targets. # It should remain in the format they expect. # -MONO_CORLIB_VERSION=1b5be199-6637-495c-9baa-a95056f2137f +MONO_CORLIB_VERSION=69f9feb5-e6ef-4d90-8722-17346c85efb6 # # Put a quoted #define in config.h. diff --git a/src/mono/mono/metadata/appdomain.c b/src/mono/mono/metadata/appdomain.c index a8e8b06..bd769ac 100644 --- a/src/mono/mono/metadata/appdomain.c +++ b/src/mono/mono/metadata/appdomain.c @@ -3093,22 +3093,13 @@ deregister_reflection_info_roots (MonoDomain *domain) static gsize WINAPI unload_thread_main (void *arg) { - ERROR_DECL (error); unload_data *data = (unload_data*)arg; MonoDomain *domain = data->domain; MonoInternalThread *internal; int i; gsize result = 1; // failure - internal = mono_thread_internal_current (); - - MonoString *thread_name_str = mono_string_new_checked (mono_domain_get (), "Domain unloader", error); - if (is_ok (error)) - mono_thread_set_name (internal, thread_name_str, MonoSetThreadNameFlag_Permanent, error); - if (!is_ok (error)) { - data->failure_reason = g_strdup (mono_error_get_message (error)); - goto failure; - } + mono_thread_set_name_constant_ignore_error (mono_thread_internal_current (), "Domain unloader", MonoSetThreadNameFlag_Permanent); /* * FIXME: Abort our parent thread last, so we can return a failure @@ -3171,7 +3162,6 @@ unload_thread_main (void *arg) result = 0; // success exit: - mono_error_cleanup (error); mono_atomic_store_release (&data->done, TRUE); unload_data_unref (data); return result; diff --git a/src/mono/mono/metadata/attach.c b/src/mono/mono/metadata/attach.c index af05525..d8453cc 100644 --- a/src/mono/mono/metadata/attach.c +++ b/src/mono/mono/metadata/attach.c @@ -502,17 +502,13 @@ transport_start_receive (void) static gsize WINAPI receiver_thread (void *arg) { - ERROR_DECL (error); int res, content_len; guint8 buffer [256]; guint8 *p, *p_end; - MonoInternalThread *internal; + MonoInternalThread *internal = mono_thread_internal_current (); + + mono_thread_set_name_constant_ignore_error (internal, "Attach receiver", MonoSetThreadNameFlag_Permanent); - internal = mono_thread_internal_current (); - MonoString *attach_str = mono_string_new_checked (mono_domain_get (), "Attach receiver", error); - mono_error_assert_ok (error); - mono_thread_set_name (internal, attach_str, MonoSetThreadNameFlag_Permanent, error); - mono_error_assert_ok (error); /* Ask the runtime to not abort this thread */ //internal->flags |= MONO_THREAD_FLAG_DONT_MANAGE; /* Ask the runtime to not wait for this thread */ diff --git a/src/mono/mono/metadata/gc.c b/src/mono/mono/metadata/gc.c index d88e961..da0a66b 100644 --- a/src/mono/mono/metadata/gc.c +++ b/src/mono/mono/metadata/gc.c @@ -943,13 +943,9 @@ mono_runtime_do_background_work (void) static gsize WINAPI finalizer_thread (gpointer unused) { - ERROR_DECL (error); gboolean wait = TRUE; - MonoString *finalizer = mono_string_new_checked (mono_get_root_domain (), "Finalizer", error); - mono_error_assert_ok (error); - mono_thread_set_name (mono_thread_internal_current (), finalizer, MonoSetThreadNameFlag_None, error); - mono_error_assert_ok (error); + mono_thread_set_name_constant_ignore_error (mono_thread_internal_current (), "Finalizer", MonoSetThreadNameFlag_None); /* Register a hazard free queue pump callback */ mono_hazard_pointer_install_free_queue_size_callback (hazard_free_queue_is_too_big); diff --git a/src/mono/mono/metadata/icall-def-netcore.h b/src/mono/mono/metadata/icall-def-netcore.h index 15007ec..9b5ecff 100644 --- a/src/mono/mono/metadata/icall-def-netcore.h +++ b/src/mono/mono/metadata/icall-def-netcore.h @@ -509,7 +509,7 @@ HANDLES(THREAD_4, "InitInternal", ves_icall_System_Threading_Thread_InitInternal HANDLES(THREAD_5, "InitializeCurrentThread", ves_icall_System_Threading_Thread_GetCurrentThread, MonoThreadObject, 0, ()) HANDLES(THREAD_6, "InterruptInternal", ves_icall_System_Threading_Thread_Interrupt_internal, void, 1, (MonoThreadObject)) HANDLES(THREAD_7, "JoinInternal", ves_icall_System_Threading_Thread_Join_internal, MonoBoolean, 2, (MonoThreadObject, int)) -ICALL(THREAD_8, "SetName", ves_icall_System_Threading_Thread_SetName_internal) +HANDLES(THREAD_8, "SetName_icall", ves_icall_System_Threading_Thread_SetName_icall, void, 3, (MonoInternalThread, const_gunichar2_ptr, gint32)) HANDLES(THREAD_9, "SetPriority", ves_icall_System_Threading_Thread_SetPriority, void, 2, (MonoThreadObject, int)) HANDLES(THREAD_10, "SetState", ves_icall_System_Threading_Thread_SetState, void, 2, (MonoInternalThread, guint32)) HANDLES(THREAD_11, "SleepInternal", ves_icall_System_Threading_Thread_Sleep_internal, void, 1, (gint32)) diff --git a/src/mono/mono/metadata/icall-def.h b/src/mono/mono/metadata/icall-def.h index 7486e00..f056cd1 100644 --- a/src/mono/mono/metadata/icall-def.h +++ b/src/mono/mono/metadata/icall-def.h @@ -1047,7 +1047,7 @@ HANDLES(THREAD_12, "JoinInternal", ves_icall_System_Threading_Thread_Join_intern NOHANDLES(ICALL(THREAD_13, "MemoryBarrier", ves_icall_System_Threading_Thread_MemoryBarrier)) HANDLES(THREAD_14, "ResetAbortNative", ves_icall_System_Threading_Thread_ResetAbort, void, 1, (MonoThreadObject)) HANDLES(THREAD_15, "ResumeInternal", ves_icall_System_Threading_Thread_Resume, void, 1, (MonoThreadObject)) -ICALL(THREAD_18, "SetName_internal(System.Threading.InternalThread,string)", ves_icall_System_Threading_Thread_SetName_internal) +HANDLES(THREAD_18, "SetName_icall", ves_icall_System_Threading_Thread_SetName_icall, void, 3, (MonoInternalThread, const_gunichar2_ptr, gint32)) HANDLES(THREAD_58, "SetPriorityNative", ves_icall_System_Threading_Thread_SetPriority, void, 2, (MonoThreadObject, int)) HANDLES(THREAD_21, "SetState(System.Threading.InternalThread,System.Threading.ThreadState)", ves_icall_System_Threading_Thread_SetState, void, 2, (MonoInternalThread, guint32)) HANDLES(THREAD_22, "SleepInternal", ves_icall_System_Threading_Thread_Sleep_internal, void, 1, (gint32)) diff --git a/src/mono/mono/metadata/object-internals.h b/src/mono/mono/metadata/object-internals.h index e4c3c5f..629b504 100644 --- a/src/mono/mono/metadata/object-internals.h +++ b/src/mono/mono/metadata/object-internals.h @@ -556,8 +556,8 @@ typedef enum { struct _MonoThreadInfo; typedef struct MonoThreadName { - char* chars; - volatile gsize generation; + char* volatile chars; // null check outside of lock + gsize volatile generation; // read outside of lock gint32 free; gint32 length; } MonoThreadName; diff --git a/src/mono/mono/metadata/threadpool-io.c b/src/mono/mono/metadata/threadpool-io.c index c2e622e..e1d9569 100644 --- a/src/mono/mono/metadata/threadpool-io.c +++ b/src/mono/mono/metadata/threadpool-io.c @@ -325,13 +325,11 @@ selector_thread_interrupt (gpointer unused) static gsize WINAPI selector_thread (gpointer data) { - ERROR_DECL (error); MonoGHashTable *states; - MonoString *thread_name = mono_string_new_checked (mono_get_root_domain (), "Thread Pool I/O Selector", error); - mono_error_assert_ok (error); - mono_thread_set_name (mono_thread_internal_current (), thread_name, MonoSetThreadNameFlag_Reset, error); - mono_error_assert_ok (error); + mono_thread_set_name_constant_ignore_error (mono_thread_internal_current (), "Thread Pool I/O Selector", MonoSetThreadNameFlag_Reset); + + ERROR_DECL (error); if (mono_runtime_is_shutting_down ()) { io_selector_running = FALSE; diff --git a/src/mono/mono/metadata/threadpool.c b/src/mono/mono/metadata/threadpool.c index 8c6db89..07754e1 100644 --- a/src/mono/mono/metadata/threadpool.c +++ b/src/mono/mono/metadata/threadpool.c @@ -288,7 +288,6 @@ try_invoke_perform_wait_callback (MonoObject** exc, MonoError *error) static void worker_callback (void) { - ERROR_DECL (error); ThreadPoolDomain *tpdomain, *previous_tpdomain; ThreadPoolCounter counter; MonoInternalThread *thread; @@ -359,12 +358,8 @@ worker_callback (void) // This only partly fights against that -- i.e. not atomic and not a loop. // It is reliable against the thread setting its own name, and somewhat // reliable against other threads setting this thread's name. - if (name_generation != thread->name.generation) { - MonoString *thread_name = mono_string_new_checked (mono_get_root_domain (), "Thread Pool Worker", error); - mono_error_assert_ok (error); - name_generation = mono_thread_set_name (thread, thread_name, MonoSetThreadNameFlag_Reset, error); - mono_error_assert_ok (error); - } + if (name_generation != thread->name.generation) + name_generation = mono_thread_set_name_constant_ignore_error (thread, "Thread Pool Worker", MonoSetThreadNameFlag_Reset); mono_thread_clear_and_set_state (thread, (MonoThreadState)~ThreadState_Background, @@ -374,6 +369,8 @@ worker_callback (void) if (mono_domain_set_fast (tpdomain->domain, FALSE)) { MonoObject *exc = NULL, *res; + ERROR_DECL (error); + res = try_invoke_perform_wait_callback (&exc, error); if (exc || !is_ok(error)) { if (exc == NULL) diff --git a/src/mono/mono/metadata/threads-types.h b/src/mono/mono/metadata/threads-types.h index eab0dc9..ca5be1c 100644 --- a/src/mono/mono/metadata/threads-types.h +++ b/src/mono/mono/metadata/threads-types.h @@ -344,7 +344,7 @@ typedef enum { MonoSetThreadNameFlag_None = 0x0000, MonoSetThreadNameFlag_Permanent = 0x0001, MonoSetThreadNameFlag_Reset = 0x0002, - //MonoSetThreadNameFlag_Constant = 0x0004, + MonoSetThreadNameFlag_Constant = 0x0004, } MonoSetThreadNameFlags; G_ENUM_FUNCTIONS (MonoSetThreadNameFlags) @@ -352,9 +352,14 @@ G_ENUM_FUNCTIONS (MonoSetThreadNameFlags) MONO_PROFILER_API gsize mono_thread_set_name (MonoInternalThread *thread, - MonoString *name, + const char* name8, size_t name8_length, const gunichar2* name16, MonoSetThreadNameFlags flags, MonoError *error); +#define mono_thread_set_name_constant_ignore_error(thread, name, flags) \ + mono_thread_set_name ((thread), name, G_N_ELEMENTS (name) - 1, \ + MONO_THREAD_NAME_WINDOWS_CONSTANT (name), \ + (flags) | MonoSetThreadNameFlag_Constant, NULL) + void mono_thread_suspend_all_other_threads (void); gboolean mono_threads_abort_appdomain_threads (MonoDomain *domain, int timeout); diff --git a/src/mono/mono/metadata/threads.c b/src/mono/mono/metadata/threads.c index 2516ddb..c3acfb1 100644 --- a/src/mono/mono/metadata/threads.c +++ b/src/mono/mono/metadata/threads.c @@ -1881,10 +1881,15 @@ ves_icall_System_Threading_Thread_GetName_internal (MonoInternalThreadHandle thr } #endif +// Unusal function: +// - MonoError is optional -- failure is usually not interesting, except the documented failure mode for managed callers. +// - name16 only used on Windows. +// - name8 is either constant, or g_free'able -- this function always takes ownership and never copies. +// gsize mono_thread_set_name (MonoInternalThread *this_obj, - MonoString *name, - MonoSetThreadNameFlags flags, MonoError *error) + const char* name8, size_t name8_length, const gunichar2* name16, + MonoSetThreadNameFlags flags, MonoError *error) { MonoNativeThreadId tid = 0; @@ -1892,21 +1897,11 @@ mono_thread_set_name (MonoInternalThread *this_obj, // It is not exactly thread safe but no use of it could be. gsize name_generation; - // FIXME lots of temporary stuff here. - // Callers will pass utf8 and on Windows utf16. - - const gunichar2* name16 = NULL; - gint32 name16_length = 0; - long name8_length = 0; - const char* name8 = NULL; - - if (name) { - name16 = mono_string_chars_internal (name); - name16_length = mono_string_length_internal (name); - name8 = name16 ? g_utf16_to_utf8 (name16, name16_length, NULL, &name8_length, NULL) : NULL; - } + const gboolean constant = !!(flags & MonoSetThreadNameFlag_Constant); - const gboolean constant = FALSE; // !!(flags & MonoSetThreadNameFlag_Constant) +#if HOST_WIN32 // On Windows, if name8 is supplied, then name16 must be also. + g_assert (!name8 || name16); +#endif LOCK_THREAD (this_obj); @@ -1917,7 +1912,8 @@ mono_thread_set_name (MonoInternalThread *this_obj, } else if (this_obj->flags & MONO_THREAD_FLAG_NAME_SET) { UNLOCK_THREAD (this_obj); - mono_error_set_invalid_operation (error, "%s", "Thread.Name can only be set once."); + if (error) + mono_error_set_invalid_operation (error, "%s", "Thread.Name can only be set once."); if (!constant) g_free ((char*)name8); @@ -1955,13 +1951,14 @@ mono_thread_set_name (MonoInternalThread *this_obj, } void -ves_icall_System_Threading_Thread_SetName_internal (MonoInternalThread *this_obj, MonoString *name) +ves_icall_System_Threading_Thread_SetName_icall (MonoInternalThreadHandle thread_handle, const gunichar2* name16, gint32 name16_length, MonoError* error) { - // FIXME This function churning. + long name8_length = 0; - ERROR_DECL (error); - mono_thread_set_name (this_obj, name, MonoSetThreadNameFlag_Permanent, error); - mono_error_set_pending_exception (error); + char* name8 = name16 ? g_utf16_to_utf8 (name16, name16_length, NULL, &name8_length, NULL) : NULL; + + mono_thread_set_name (mono_internal_thread_handle_ptr (thread_handle), + name8, (gint32)name8_length, name16, MonoSetThreadNameFlag_Permanent, error); } #ifndef ENABLE_NETCORE diff --git a/src/mono/mono/mini/aot-compiler.c b/src/mono/mono/mini/aot-compiler.c index a53be52..62a4d54 100644 --- a/src/mono/mono/mini/aot-compiler.c +++ b/src/mono/mono/mini/aot-compiler.c @@ -8782,12 +8782,7 @@ compile_thread_main (gpointer user_data) GPtrArray *methods = ((GPtrArray **)user_data) [1]; int i; - ERROR_DECL (error); - MonoInternalThread *internal = mono_thread_internal_current (); - MonoString *str = mono_string_new_checked (mono_domain_get (), "AOT compiler", error); - mono_error_assert_ok (error); - mono_thread_set_name (internal, str, MonoSetThreadNameFlag_Permanent, error); - mono_error_assert_ok (error); + mono_thread_set_name_constant_ignore_error (mono_thread_internal_current (), "AOT compiler", MonoSetThreadNameFlag_Permanent); for (i = 0; i < methods->len; ++i) compile_method (acfg, (MonoMethod *)g_ptr_array_index (methods, i)); diff --git a/src/mono/mono/mini/debugger-agent.c b/src/mono/mono/mini/debugger-agent.c index 5cecb4c..c2e0390 100644 --- a/src/mono/mono/mini/debugger-agent.c +++ b/src/mono/mono/mini/debugger-agent.c @@ -9853,7 +9853,6 @@ wait_for_attach (void) static gsize WINAPI debugger_thread (void *arg) { - ERROR_DECL (error); int res, len, id, flags, command = 0; CommandSet command_set = (CommandSet)0; guint8 header [HEADER_LENGTH]; @@ -9870,10 +9869,7 @@ debugger_thread (void *arg) debugger_thread_id = mono_native_thread_id_get (); MonoInternalThread *internal = mono_thread_internal_current (); - MonoString *str = mono_string_new_checked (mono_domain_get (), "Debugger agent", error); - mono_error_assert_ok (error); - mono_thread_set_name (internal, str, MonoSetThreadNameFlag_Permanent, error); - mono_error_assert_ok (error); + mono_thread_set_name_constant_ignore_error (internal, "Debugger agent", MonoSetThreadNameFlag_Permanent); internal->state |= ThreadState_Background; internal->flags |= MONO_THREAD_FLAG_DONT_MANAGE; diff --git a/src/mono/mono/mini/mini-posix.c b/src/mono/mono/mini/mini-posix.c index f88a5e3..cb61d3a 100644 --- a/src/mono/mono/mini/mini-posix.c +++ b/src/mono/mono/mini/mini-posix.c @@ -689,12 +689,7 @@ sampling_thread_func (gpointer unused) thread->flags |= MONO_THREAD_FLAG_DONT_MANAGE; - ERROR_DECL (error); - - MonoString *name = mono_string_new_checked (mono_get_root_domain (), "Profiler Sampler", error); - mono_error_assert_ok (error); - mono_thread_set_name (thread, name, MonoSetThreadNameFlag_None, error); - mono_error_assert_ok (error); + mono_thread_set_name_constant_ignore_error (thread, "Profiler Sampler", MonoSetThreadNameFlag_None); mono_thread_info_set_flags (MONO_THREAD_INFO_FLAGS_NO_GC | MONO_THREAD_INFO_FLAGS_NO_SAMPLE); diff --git a/src/mono/mono/profiler/aot.c b/src/mono/mono/profiler/aot.c index 199b7b0..61e102a 100644 --- a/src/mono/mono/profiler/aot.c +++ b/src/mono/mono/profiler/aot.c @@ -214,14 +214,7 @@ helper_thread (void *arg) { mono_thread_attach (mono_get_root_domain ()); - MonoInternalThread *internal = mono_thread_internal_current (); - - ERROR_DECL (error); - - MonoString *name_str = mono_string_new_checked (mono_get_root_domain (), "AOT Profiler Helper", error); - mono_error_assert_ok (error); - mono_thread_set_name (internal, name_str, MonoSetThreadNameFlag_None, error); - mono_error_assert_ok (error); + mono_thread_set_name_constant_ignore_error (mono_thread_internal_current (), "AOT Profiler Helper", MonoSetThreadNameFlag_None); mono_thread_info_set_flags (MONO_THREAD_INFO_FLAGS_NO_GC | MONO_THREAD_INFO_FLAGS_NO_SAMPLE); diff --git a/src/mono/mono/profiler/log.c b/src/mono/mono/profiler/log.c index 6c1f231..925ed1b 100644 --- a/src/mono/mono/profiler/log.c +++ b/src/mono/mono/profiler/log.c @@ -3144,7 +3144,7 @@ new_filename (const char* filename) } static MonoProfilerThread * -profiler_thread_begin (const char *name, gboolean send) +profiler_thread_begin_function (const char *name8, const gunichar2* name16, size_t name_length, gboolean send) { mono_thread_info_attach (); MonoProfilerThread *thread = init_thread (FALSE); @@ -3160,12 +3160,7 @@ profiler_thread_begin (const char *name, gboolean send) */ internal->flags |= MONO_THREAD_FLAG_DONT_MANAGE; - ERROR_DECL (error); - - MonoString *name_str = mono_string_new_checked (mono_get_root_domain (), name, error); - mono_error_assert_ok (error); - mono_thread_set_name (internal, name_str, MonoSetThreadNameFlag_None, error); - mono_error_assert_ok (error); + mono_thread_set_name (internal, name8, name_length, name16, MonoSetThreadNameFlag_Constant, NULL); mono_thread_info_set_flags (MONO_THREAD_INFO_FLAGS_NO_GC | MONO_THREAD_INFO_FLAGS_NO_SAMPLE); @@ -3180,6 +3175,9 @@ profiler_thread_begin (const char *name, gboolean send) return thread; } +#define profiler_thread_begin(name, send) \ + profiler_thread_begin_function (name, MONO_THREAD_NAME_WINDOWS_CONSTANT (name), G_N_ELEMENTS (name) - 1, (send)) + static void profiler_thread_end (MonoProfilerThread *thread, MonoOSEvent *event, gboolean send) { diff --git a/src/mono/netcore/System.Private.CoreLib/src/System.Threading/Thread.cs b/src/mono/netcore/System.Private.CoreLib/src/System.Threading/Thread.cs index 01a93b3..8937901 100644 --- a/src/mono/netcore/System.Private.CoreLib/src/System.Threading/Thread.cs +++ b/src/mono/netcore/System.Private.CoreLib/src/System.Threading/Thread.cs @@ -295,7 +295,13 @@ namespace System.Threading extern static string GetName (Thread thread); [MethodImplAttribute(MethodImplOptions.InternalCall)] - extern static void SetName (Thread thread, String name); + private static unsafe extern void SetName_icall (Thread thread, char *name, int nameLength); + + static unsafe void SetName (Thread thread, String name) + { + fixed (char* fixed_name = name) + SetName_icall (thread, fixed_name, name?.Length ?? 0); + } [MethodImplAttribute(MethodImplOptions.InternalCall)] extern static bool YieldInternal (); -- 2.7.4