From: Vlad Brezae Date: Wed, 12 Aug 2020 22:40:09 +0000 (+0300) Subject: [interp] Rework OBJREF and fix a GC issue (#40710) X-Git-Tag: submit/tizen/20210909.063632~6038 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=58bfab58278037d12dbce03a173ce0c9e9c4c828;p=platform%2Fupstream%2Fdotnet%2Fruntime.git [interp] Rework OBJREF and fix a GC issue (#40710) * [interp] Remove unnecessary uses of OBJREF No checkpoints are possible before the returned object is stored on interp stack. * [interp] Don't rely on scanning C stack in wasm In order to keep object references alive we were doing a volatile store to the address of a stack local. This would determine the compiler to store to the C stack, which we can scan on wasm. While I didn't see proof that this is not the case, this mechanism seems rather magical and fragile. This commit replaces this mechanism of keeping C locals alive with an explicit store into a handle that we allocate whenever we enter the interpreter. Since we don't require this too frequently (the interp stack is usually enough to keep required objects alive), a single handle is enough. The added benefit of this is that we can stop scanning the C stack/registers altogether on desktop interpreter in coop mode. This mode replicates the behavior of wasm, uncovering GC bugs that show up there while using desktop interpreter. * [interp] Make sure exception object is not collected during EH Most of the time, the exception object comes from a function call in the runtime so it is not kept alive by interp stack. --- diff --git a/src/mono/mono/mini/interp/interp-internals.h b/src/mono/mono/mini/interp/interp-internals.h index e3f3e04..a084671 100644 --- a/src/mono/mono/mini/interp/interp-internals.h +++ b/src/mono/mono/mini/interp/interp-internals.h @@ -51,50 +51,10 @@ typedef gint64 mono_i; #define MINT_TYPE_I MINT_TYPE_I8 #endif - -/* - * GC SAFETY: - * - * The interpreter executes in gc unsafe (non-preempt) mode. On wasm, the C stack is - * scannable but the wasm stack is not, so to make the code GC safe, the following rules - * should be followed: - * - every objref handled by the code needs to either be stored volatile or stored - * into a volatile; volatile stores are stack packable, volatile values are not. - * Use either OBJREF or stackval->data.o. - * This will ensure the objects are pinned. A volatile local - * is on the stack and not in registers. Volatile stores ditto. - * - minimize the number of MonoObject* locals/arguments (or make them volatile). - * - * Volatile on a type/local forces all reads and writes to go to memory/stack, - * and each such local to have a unique address. - * - * Volatile absence on a type/local allows multiple locals to share storage, - * if their lifetimes do not overlap. This is called "stack packing". - * - * Volatile absence on a type/local allows the variable to live in - * both stack and register, for fast reads and "write through". - */ #ifdef TARGET_WASM - -#define WASM_VOLATILE volatile - -static inline MonoObject * WASM_VOLATILE * -mono_interp_objref (MonoObject **o) -{ - return o; -} - -#define OBJREF(x) (*mono_interp_objref (&x)) - -#else - -#define WASM_VOLATILE /* nothing */ - -#define OBJREF(x) x - +#define INTERP_NO_STACK_SCAN 1 #endif - /* * Value types are represented on the eval stack as pointers to the * actual storage. A value type cannot be larger than 16 MB. @@ -109,7 +69,12 @@ typedef struct { } pair; float f_r4; double f; - MonoObject * WASM_VOLATILE o; +#ifdef INTERP_NO_STACK_SCAN + /* Ensure objref is always flushed to interp stack */ + MonoObject * volatile o; +#else + MonoObject *o; +#endif /* native size integer and pointer types */ gpointer p; mono_u nati; diff --git a/src/mono/mono/mini/interp/interp.c b/src/mono/mono/mini/interp/interp.c index 53d20e1..445da6d 100644 --- a/src/mono/mono/mini/interp/interp.c +++ b/src/mono/mono/mini/interp/interp.c @@ -1080,9 +1080,15 @@ interp_throw (ThreadContext *context, MonoException *ex, InterpFrame *frame, con g_assert (context->has_resume_state); } +// We conservatively pin exception object here to avoid tweaking the +// numerous call sites of this macro, even though, in a few cases, +// this is not needed. #define THROW_EX_GENERAL(exception,ex_ip, rethrow) \ do { \ - interp_throw (context, (exception), (frame), (ex_ip), (rethrow)); \ + MonoException *__ex = (exception); \ + MONO_HANDLE_ASSIGN_RAW (tmp_handle, (MonoObject*)__ex); \ + interp_throw (context, __ex, (frame), (ex_ip), (rethrow)); \ + MONO_HANDLE_ASSIGN_RAW (tmp_handle, (MonoObject*)NULL); \ goto resume; \ } while (0) @@ -3264,11 +3270,10 @@ mono_interp_box_nullable (InterpFrame* frame, const guint16* ip, stackval* sp, M } static int -mono_interp_box_vt (InterpFrame* frame, const guint16* ip, stackval* sp) +mono_interp_box_vt (InterpFrame* frame, const guint16* ip, stackval* sp, MonoObjectHandle tmp_handle) { InterpMethod* const imethod = frame->imethod; - MonoObject* o; // See the comment about GC safety. MonoVTable * const vtable = (MonoVTable*)imethod->data_items [ip [1]]; MonoClass* const c = vtable->klass; @@ -3277,26 +3282,27 @@ mono_interp_box_vt (InterpFrame* frame, const guint16* ip, stackval* sp) guint16 offset = ip [2]; guint16 pop_vt_sp = !ip [3]; - OBJREF (o) = mono_gc_alloc_obj (vtable, m_class_get_instance_size (vtable->klass)); + MonoObject* o = mono_gc_alloc_obj (vtable, m_class_get_instance_size (vtable->klass)); + MONO_HANDLE_ASSIGN_RAW (tmp_handle, o); mono_value_copy_internal (mono_object_get_data (o), sp [-1 - offset].data.p, c); + MONO_HANDLE_ASSIGN_RAW (tmp_handle, NULL); - sp [-1 - offset].data.p = o; + sp [-1 - offset].data.o = o; return pop_vt_sp ? ALIGN_TO (size, MINT_VT_ALIGNMENT) : 0; } static void -mono_interp_box (InterpFrame* frame, const guint16* ip, stackval* sp) +mono_interp_box (InterpFrame* frame, const guint16* ip, stackval* sp, MonoObjectHandle tmp_handle) { - MonoObject *o; // See the comment about GC safety. MonoVTable * const vtable = (MonoVTable*)frame->imethod->data_items [ip [1]]; - - OBJREF (o) = mono_gc_alloc_obj (vtable, m_class_get_instance_size (vtable->klass)); - guint16 const offset = ip [2]; + MonoObject *o = mono_gc_alloc_obj (vtable, m_class_get_instance_size (vtable->klass)); + MONO_HANDLE_ASSIGN_RAW (tmp_handle, o); stackval_to_data (m_class_get_byval_arg (vtable->klass), &sp [-1 - offset], mono_object_get_data (o), FALSE); + MONO_HANDLE_ASSIGN_RAW (tmp_handle, NULL); - sp [-1 - offset].data.p = o; + sp [-1 - offset].data.o = o; } static int @@ -3436,6 +3442,24 @@ interp_exec_method (InterpFrame *frame, ThreadContext *context, FrameClauseArgs }; #endif + HANDLE_FUNCTION_ENTER (); + /* + * GC SAFETY: + * + * The interpreter executes in gc unsafe (non-preempt) mode. On wasm, we cannot rely on + * scanning the stack or any registers. In order to make the code GC safe, every objref + * handled by the code needs to be kept alive and pinned in any of the following ways: + * - the object needs to be stored on the interpreter stack. In order to make sure the + * object actually gets stored on the interp stack and the store is not optimized out, + * the store/variable should be volatile. + * - if the execution of an opcode requires an object not coming from interp stack to be + * kept alive, the tmp_handle below can be used. This handle will keep only one object + * pinned by the GC. Ideally, once this object is no longer needed, the handle should be + * cleared. If we will need to have more objects pinned simultaneously, additional handles + * can be reserved here. + */ + MonoObjectHandle tmp_handle = MONO_HANDLE_NEW (MonoObject, NULL); + if (method_entry (context, frame, #if DEBUG_INTERP &tracing, @@ -5123,7 +5147,6 @@ call: MINT_IN_CASE(MINT_NEWOBJ_FAST) { MonoVTable *vtable = (MonoVTable*) frame->imethod->data_items [ip [3]]; INIT_VTABLE (vtable); - MonoObject *o; // See the comment about GC safety. guint16 param_count; guint16 imethod_index = ip [1]; @@ -5137,7 +5160,7 @@ call: memmove (sp + 2, sp, param_count * sizeof (stackval)); } - OBJREF (o) = mono_gc_alloc_obj (vtable, m_class_get_instance_size (vtable->klass)); + MonoObject *o = mono_gc_alloc_obj (vtable, m_class_get_instance_size (vtable->klass)); if (G_UNLIKELY (!o)) { mono_error_set_out_of_memory (error, "Could not allocate %i bytes", m_class_get_instance_size (vtable->klass)); THROW_EX (mono_error_convert_to_exception (error), ip); @@ -5241,13 +5264,13 @@ call_newobj: THROW_EX (exc, ip); } error_init_reuse (error); - MonoObject* o = NULL; // See the comment about GC safety. - OBJREF (o) = mono_object_new_checked (domain, newobj_class, error); + MonoObject* o = mono_object_new_checked (domain, newobj_class, error); + sp [0].data.o = o; // return value + sp [1].data.o = o; // first parameter + mono_error_cleanup (error); // FIXME: do not swallow the error error_init_reuse (error); EXCEPTION_CHECKPOINT; - sp [0].data.o = o; // return value - sp [1].data.o = o; // first parameter #ifndef DISABLE_REMOTING if (mono_object_is_transparent_proxy (o)) { MonoMethod *remoting_invoke_method = mono_marshal_get_remoting_invoke_with_check (cmethod->method, error); @@ -5946,12 +5969,12 @@ call_newobj: MINT_IN_BREAK; } MINT_IN_CASE(MINT_BOX) { - mono_interp_box (frame, ip, sp); + mono_interp_box (frame, ip, sp, tmp_handle); ip += 3; MINT_IN_BREAK; } MINT_IN_CASE(MINT_BOX_VT) { - vt_sp -= mono_interp_box_vt (frame, ip, sp); + vt_sp -= mono_interp_box_vt (frame, ip, sp, tmp_handle); ip += 4; MINT_IN_BREAK; } @@ -7381,6 +7404,8 @@ exit_clause: context->stack_pointer = (guchar*)frame->stack; DEBUG_LEAVE (); + + HANDLE_FUNCTION_RETURN (); } static void