From 9d51768adc562195ac1e767ddb13cdbff9056028 Mon Sep 17 00:00:00 2001 From: Chris Foster Date: Fri, 14 Sep 2018 23:43:16 +1000 Subject: [PATCH] Fix exc_stack GC by making exc stacks always belong to tasks Make exc_stack always belong to a task and never to the TLS, to correctly handle exception stacks as GC'd buffers. A downside is that we now allocate on the first throw within any given task. This should generally be ok, but could loose the root cause of an error if we coincidentally exhaust memory at the same time. Also simplify save and restore of exception stack state in codegen and interpreter by removing mention of this from jl_handler_t, and providing a runtime function jl_exc_stack_state(). --- src/cgutils.cpp | 11 ----------- src/codegen.cpp | 20 ++++++++++++++------ src/gc.c | 27 ++------------------------- src/interpreter.c | 10 ++++------ src/jlapi.c | 4 ++-- src/julia.h | 9 +++++---- src/julia_threads.h | 4 +--- src/rtutils.c | 21 +++++++++++++++------ src/stackwalk.c | 12 ++++++++---- src/task.c | 40 +++++++++++++--------------------------- src/threading.c | 2 -- 11 files changed, 64 insertions(+), 96 deletions(-) diff --git a/src/cgutils.cpp b/src/cgutils.cpp index 5df12e5cfaef0..83a1673eea9f9 100644 --- a/src/cgutils.cpp +++ b/src/cgutils.cpp @@ -2568,17 +2568,6 @@ static jl_cgval_t emit_new_struct(jl_codectx_t &ctx, jl_value_t *ty, size_t narg } } -static Value *emit_pexc_stack_top(jl_codectx_t &ctx) -{ - // Load the pointer ptls->exc_stack - // Types here are a bit of a lie, but we didn't declare jl_tls_states_t to LLVM. - Value *ptls_val = emit_bitcast(ctx, ctx.ptlsStates, PointerType::get(T_psize,0)); - Constant *offset = ConstantInt::getSigned(T_int32, - offsetof(jl_tls_states_t, exc_stack) / sizeof(size_t**)); - Value *ppexc_stack_top = ctx.builder.CreateInBoundsGEP(ptls_val, offset); - return ctx.builder.CreateLoad(ppexc_stack_top, true/*isvolatile*/); -} - static void emit_signal_fence(jl_codectx_t &ctx) { #if defined(_CPU_ARM_) || defined(_CPU_AARCH64_) diff --git a/src/codegen.cpp b/src/codegen.cpp index c0749acac8710..66fff5d2cc5d0 100644 --- a/src/codegen.cpp +++ b/src/codegen.cpp @@ -290,6 +290,7 @@ static Function *jlenter_func; static Function *jlcurrent_exception_func; static Function *jlleave_func; static Function *jl_restore_exc_stack_func; +static Function *jl_exc_stack_state_func; static Function *jlegal_func; static Function *jl_alloc_obj_func; static Function *jl_newbits_func; @@ -3762,9 +3763,9 @@ static void emit_stmtpos(jl_codectx_t &ctx, jl_value_t *expr, int ssaval_result) ConstantInt::get(T_int32, jl_unbox_long(args[0]))); } else if (head == pop_exc_sym) { - jl_cgval_t exc_stack_top = emit_expr(ctx, jl_exprarg(expr, 0)); - assert(exc_stack_top.V && exc_stack_top.V->getType() == T_size); - ctx.builder.CreateCall(prepare_call(jl_restore_exc_stack_func), exc_stack_top.V); + jl_cgval_t exc_stack_state = emit_expr(ctx, jl_exprarg(expr, 0)); + assert(exc_stack_state.V && exc_stack_state.V->getType() == T_size); + ctx.builder.CreateCall(prepare_call(jl_restore_exc_stack_func), exc_stack_state.V); return; } else { @@ -6182,10 +6183,11 @@ static std::unique_ptr emit_function( assert(jl_is_long(args[0])); int lname = jl_unbox_long(args[0]); // Save exception stack depth at enter for use in pop_exc - Value *exc_stack_top = ctx.builder.CreateLoad(emit_pexc_stack_top(ctx), - "exc_stack_top"); + + Value *exc_stack_state = + ctx.builder.CreateCall(prepare_call(jl_exc_stack_state_func)); assert(!ctx.ssavalue_assigned.at(cursor)); - ctx.SAvalues.at(cursor) = jl_cgval_t(exc_stack_top, NULL, false, + ctx.SAvalues.at(cursor) = jl_cgval_t(exc_stack_state, NULL, false, (jl_value_t*)jl_ulong_type, NULL); ctx.ssavalue_assigned.at(cursor) = true; CallInst *sj = ctx.builder.CreateCall(prepare_call(except_enter_func)); @@ -7139,6 +7141,12 @@ static void init_julia_llvm_env(Module *m) "jl_restore_exc_stack", m); add_named_global(jl_restore_exc_stack_func, &jl_restore_exc_stack); + jl_exc_stack_state_func = + Function::Create(FunctionType::get(T_size, false), + Function::ExternalLinkage, + "jl_exc_stack_state", m); + add_named_global(jl_exc_stack_state_func, &jl_exc_stack_state); + std::vector args_2vals_callee_rooted(0); args_2vals_callee_rooted.push_back(PointerType::get(T_jlvalue, AddressSpace::CalleeRooted)); args_2vals_callee_rooted.push_back(PointerType::get(T_jlvalue, AddressSpace::CalleeRooted)); diff --git a/src/gc.c b/src/gc.c index 39e4ba3077000..3b8bd8a718376 100644 --- a/src/gc.c +++ b/src/gc.c @@ -1472,26 +1472,6 @@ STATIC_INLINE int gc_mark_queue_obj(jl_gc_mark_cache_t *gc_cache, gc_mark_sp_t * return (int)nptr; } -// Mark and queue an exception stack to be scanned. -void gc_mark_queue_scan_exc_stack(jl_gc_mark_cache_t *gc_cache, gc_mark_sp_t *sp, - jl_exc_stack_t* exc_stack) -{ - if (gc_marked(jl_astaggedvalue(exc_stack)->header)) { - return; - } - // TODO: Should we be testing `if (gc_marked(jl_astaggedvalue(exc_stack)->header))`? - // FIXME: How does gc_try_setmark differ from gc_setmark_buf_ ? - // What's the difference between a buffer and object? - // if (!gc_try_setmark((jl_value_t*)exc_stack, &nptr, &tag, &bits)) { - gc_setmark_buf_(jl_get_ptls_states(), exc_stack, 0, - sizeof(exc_stack) + sizeof(uintptr_t)*exc_stack->reserved_size); - if (exc_stack->top == 0) - return; - gc_mark_exc_stack_t data = {exc_stack, exc_stack->top, 0}; - gc_mark_stack_push(gc_cache, sp, gc_mark_label_addrs[GC_MARK_L_excstack], - &data, sizeof(data), 1); -} - // Check if `nptr` is tagged for `old + refyoung`, // Push the object to the remset and update the `nptr` counter if necessary. STATIC_INLINE void gc_mark_push_remset(jl_ptls_t ptls, jl_value_t *obj, uintptr_t nptr) JL_NOTSAFEPOINT @@ -1720,10 +1700,9 @@ STATIC_INLINE int gc_mark_scan_obj32(jl_ptls_t ptls, gc_mark_sp_t *sp, gc_mark_o // (OTOH, the spill will likely make use of the stack engine which is otherwise idle so // the performance impact is minimum as long as it's not in the hottest path) -// There are four external entry points to the loop, corresponding to labels `marked_obj`, +// There are three external entry points to the loop, corresponding to labels `marked_obj`, // `scan_only`, `finlist` and `excstack` (see the corresponding functions -// `gc_mark_queue_obj`, `gc_mark_queue_scan_obj`, `gc_mark_queue_finlist` and -// `gc_mark_queue_scan_exc_stack` above). +// `gc_mark_queue_obj`, `gc_mark_queue_scan_obj` and `gc_mark_queue_finlist` above). // // The scanning of the object starts with `goto mark`, which updates the metadata and scans // the object whose information is stored in `new_obj`, `tag` and `bits`. @@ -2210,7 +2189,6 @@ mark: { &stackdata, sizeof(stackdata), 1); } if (ta->exc_stack) { - assert(ta->exc_stack->top > 0); // FIXME? gc_setmark_buf_(ptls, ta->exc_stack, bits, sizeof(jl_exc_stack_t) + sizeof(uintptr_t)*ta->exc_stack->reserved_size); gc_mark_exc_stack_t stackdata = {ta->exc_stack, ta->exc_stack->top, 0}; @@ -2315,7 +2293,6 @@ static void jl_gc_queue_thread_local(jl_gc_mark_cache_t *gc_cache, gc_mark_sp_t gc_mark_queue_obj(gc_cache, sp, ptls2->root_task); if (ptls2->previous_exception) gc_mark_queue_obj(gc_cache, sp, ptls2->previous_exception); - gc_mark_queue_scan_exc_stack(gc_cache, sp, ptls2->exc_stack); } // mark the initial root set diff --git a/src/interpreter.c b/src/interpreter.c index 0c509f6cc21bf..a22d14083d1a9 100644 --- a/src/interpreter.c +++ b/src/interpreter.c @@ -615,8 +615,8 @@ SECT_INTERP static jl_value_t *eval_body(jl_array_t *stmts, interpreter_state *s } catch_ip += 1; } - // store previous top of exception stack for use by pop_exc. - s->locals[jl_source_nslots(s->src) + s->ip] = jl_box_ulong(__eh.exc_stack_top); + // store current top of exception stack for restore in pop_exc. + s->locals[jl_source_nslots(s->src) + s->ip] = jl_box_ulong(jl_exc_stack_state()); if (!jl_setjmp(__eh.eh_ctx,1)) { eval_body(stmts, s, s->ip + 1, toplevel); // returns via continue_at } @@ -648,10 +648,8 @@ SECT_INTERP static jl_value_t *eval_body(jl_array_t *stmts, interpreter_state *s jl_longjmp(eh->eh_ctx, 1); } else if (head == pop_exc_sym) { - // note that `__eh->exc_stack_top` may be already overwritten - // at this point. - size_t prev_stack_top = jl_unbox_ulong(eval_value(jl_exprarg(stmt, 0), s)); - jl_restore_exc_stack(prev_stack_top); + size_t prev_state = jl_unbox_ulong(eval_value(jl_exprarg(stmt, 0), s)); + jl_restore_exc_stack(prev_state); } else if (head == const_sym) { jl_sym_t *sym = (jl_sym_t*)jl_exprarg(stmt, 0); diff --git a/src/jlapi.c b/src/jlapi.c index a351a9e1306a3..396d7b9b66ddc 100644 --- a/src/jlapi.c +++ b/src/jlapi.c @@ -106,8 +106,8 @@ JL_DLLEXPORT jl_value_t *jl_eval_string(const char *str) JL_DLLEXPORT jl_value_t *jl_current_exception(void) { - jl_exc_stack_t *s = jl_get_ptls_states()->exc_stack; - return s->top != 0 ? jl_exc_stack_exception(s, s->top) : jl_nothing; + jl_exc_stack_t *s = jl_get_ptls_states()->current_task->exc_stack; + return s && s->top != 0 ? jl_exc_stack_exception(s, s->top) : jl_nothing; } JL_DLLEXPORT jl_value_t *jl_exception_occurred(void) diff --git a/src/julia.h b/src/julia.h index 186de272fd125..53a661134529a 100644 --- a/src/julia.h +++ b/src/julia.h @@ -1589,7 +1589,6 @@ typedef struct _jl_handler_t { jl_jmp_buf eh_ctx; jl_gcframe_t *gcstack; struct _jl_handler_t *prev; - size_t exc_stack_top; int8_t gc_state; #ifdef JULIA_ENABLE_THREADING size_t locks_len; @@ -1624,7 +1623,7 @@ typedef struct _jl_task_t { jl_handler_t *eh; // saved gc stack top for context switches jl_gcframe_t *gcstack; - // saved exception stack (NULL if empty) + // saved exception stack jl_exc_stack_t *exc_stack; // current module, or NULL if this task has not set one jl_module_t *current_module; @@ -1654,7 +1653,8 @@ JL_DLLEXPORT void JL_NORETURN jl_no_exc_handler(jl_value_t *e); JL_DLLEXPORT void jl_enter_handler(jl_handler_t *eh); JL_DLLEXPORT void jl_eh_restore_state(jl_handler_t *eh); JL_DLLEXPORT void jl_pop_handler(int n); -JL_DLLEXPORT void jl_restore_exc_stack(size_t prev_top); +JL_DLLEXPORT size_t jl_exc_stack_state(void); +JL_DLLEXPORT void jl_restore_exc_stack(size_t state); #if defined(_OS_WINDOWS_) #if defined(_COMPILER_MINGW_) @@ -1694,13 +1694,14 @@ extern int had_exception; #define JL_TRY \ int i__tr, i__ca; jl_handler_t __eh; \ + size_t __exc_stack_state = jl_exc_stack_state(); \ jl_enter_handler(&__eh); \ if (!jl_setjmp(__eh.eh_ctx,0)) \ for (i__tr=1; i__tr; i__tr=0, jl_eh_restore_state(&__eh)) #define JL_CATCH \ else \ - for (i__ca=1, jl_eh_restore_state(&__eh); i__ca; i__ca=0, jl_restore_exc_stack(__eh.exc_stack_top)) + for (i__ca=1, jl_eh_restore_state(&__eh); i__ca; i__ca=0, jl_restore_exc_stack(__exc_stack_state)) #endif diff --git a/src/julia_threads.h b/src/julia_threads.h index ecfad604ff53e..dce3936a957ba 100644 --- a/src/julia_threads.h +++ b/src/julia_threads.h @@ -110,11 +110,9 @@ struct _jl_tls_states_t { int16_t tid; // Temp storage for exception thrown in signal handler. Not rooted. struct _jl_value_t *sig_exception; - // Temporary backtrace buffer. + // Temporary backtrace buffer. Scanned for gc roots when bt_size > 0. uintptr_t *bt_data; // JL_MAX_BT_SIZE + 1 elements long size_t bt_size; // Size for backtrace in transit in bt_data - // Stack of exceptions being handled by task. - jl_exc_stack_t *exc_stack; // Atomically set by the sender, reset by the handler. volatile sig_atomic_t signal_request; // Allow the sigint to be raised asynchronously diff --git a/src/rtutils.c b/src/rtutils.c index e3cb87e829c3e..bdfaea5da7190 100644 --- a/src/rtutils.c +++ b/src/rtutils.c @@ -219,7 +219,6 @@ JL_DLLEXPORT void jl_enter_handler(jl_handler_t *eh) // Must have no safepoint eh->prev = current_task->eh; eh->gcstack = ptls->pgcstack; - eh->exc_stack_top = ptls->exc_stack->top; #ifdef JULIA_ENABLE_THREADING eh->gc_state = ptls->gc_state; eh->locks_len = current_task->locks.len; @@ -286,11 +285,21 @@ JL_DLLEXPORT void jl_pop_handler(int n) jl_eh_restore_state(eh); } -JL_DLLEXPORT void jl_restore_exc_stack(size_t prev_top) +JL_DLLEXPORT size_t jl_exc_stack_state(void) { jl_ptls_t ptls = jl_get_ptls_states(); - assert(ptls->exc_stack->top >= prev_top); - ptls->exc_stack->top = prev_top; + jl_exc_stack_t *s = ptls->current_task->exc_stack; + return s ? s->top : 0; +} + +JL_DLLEXPORT void jl_restore_exc_stack(size_t state) +{ + jl_ptls_t ptls = jl_get_ptls_states(); + jl_exc_stack_t *s = ptls->current_task->exc_stack; + if (s) { + assert(s->top >= state); + s->top = state; + } } JL_DLLEXPORT jl_value_t *jl_apply_with_saved_exception_state(jl_value_t **args, uint32_t nargs, int drop_exceptions) @@ -326,17 +335,17 @@ void jl_reserve_exc_stack(jl_exc_stack_t **stack, size_t reserved_size) size_t bufsz = sizeof(jl_exc_stack_t) + sizeof(uintptr_t)*reserved_size; jl_exc_stack_t *new_s = (jl_exc_stack_t*)jl_gc_alloc_buf(jl_get_ptls_states(), bufsz); new_s->top = 0; + new_s->reserved_size = reserved_size; if (s) jl_copy_exc_stack(new_s, s); - new_s->reserved_size = reserved_size; *stack = new_s; } void jl_push_exc_stack(jl_exc_stack_t **stack, jl_value_t *exception, uintptr_t *bt_data, size_t bt_size) { + jl_reserve_exc_stack(stack, (*stack ? (*stack)->top : 0) + bt_size + 2); jl_exc_stack_t *s = *stack; - jl_reserve_exc_stack(stack, (s ? s->top : 0) + bt_size + 2); memcpy(jl_excstk_raw(s) + s->top, bt_data, sizeof(uintptr_t)*bt_size); s->top += bt_size + 2; jl_excstk_raw(s)[s->top-2] = bt_size; diff --git a/src/stackwalk.c b/src/stackwalk.c index 4165294aec9e0..8e94b3f361adf 100644 --- a/src/stackwalk.c +++ b/src/stackwalk.c @@ -167,10 +167,10 @@ void decode_backtrace(uintptr_t *bt_data, size_t bt_size, JL_DLLEXPORT void jl_get_backtrace(jl_array_t **btout, jl_array_t **bt2out) { - jl_exc_stack_t *s = jl_get_ptls_states()->exc_stack; + jl_exc_stack_t *s = jl_get_ptls_states()->current_task->exc_stack; uintptr_t *bt_data = NULL; size_t bt_size = 0; - if (s->top) { + if (s && s->top) { bt_data = jl_exc_stack_bt_data(s, s->top); bt_size = jl_exc_stack_bt_size(s, s->top); } @@ -187,7 +187,9 @@ JL_DLLEXPORT jl_value_t *jl_get_exc_stack(int include_bt, int max_entries) jl_array_t *bt2 = NULL; JL_GC_PUSH3(&stack, &bt, &bt2); stack = jl_alloc_array_1d(jl_array_any_type, 0); - jl_exc_stack_t *s = jl_get_ptls_states()->exc_stack; + jl_exc_stack_t *s = jl_get_ptls_states()->current_task->exc_stack; + if (!s) + return (jl_value_t*)stack; size_t itr = s->top; int i = 0; while (itr > 0 && i < max_entries) { @@ -514,7 +516,9 @@ JL_DLLEXPORT void jl_gdblookup(uintptr_t ip) JL_DLLEXPORT void jlbacktrace(void) { - jl_exc_stack_t *s = jl_get_ptls_states()->exc_stack; + jl_exc_stack_t *s = jl_get_ptls_states()->current_task->exc_stack; + if (!s) + return; size_t bt_size = jl_exc_stack_bt_size(s, s->top); uintptr_t *bt_data = jl_exc_stack_bt_data(s, s->top); for (size_t i = 0; i < bt_size; i++) diff --git a/src/task.c b/src/task.c index f92c63a5fa3e4..9d837b9d54457 100644 --- a/src/task.c +++ b/src/task.c @@ -252,7 +252,6 @@ static void NOINLINE JL_NORETURN JL_USED_FUNC start_task(void) t->started = 1; if (t->exception != jl_nothing) { size_t bt_size = rec_backtrace(ptls->bt_data, JL_MAX_BT_SIZE); - jl_reserve_exc_stack(&t->exc_stack, bt_size+2); jl_push_exc_stack(&t->exc_stack, t->exception, ptls->bt_data, bt_size); res = t->exception; } @@ -267,11 +266,12 @@ static void NOINLINE JL_NORETURN JL_USED_FUNC start_task(void) res = jl_apply(&t->start, 1); } JL_CATCH { - // TODO: Persist exception stack as above? res = jl_current_exception(); t->exception = res; jl_gc_wb(t, res); + goto skip_pop_exc; } +skip_pop_exc:; } finish_task(t, res); gc_debug_critical_error(); @@ -314,30 +314,11 @@ static void ctx_switch(jl_ptls_t ptls, jl_task_t **pt) if (!jl_setjmp(ptls->current_task->ctx, 0)) { jl_task_t *lastt = ptls->current_task; - // Swap exception stacks. Note that the state must be left consistent - // if allocations throw, so we reserve space before doing anything else. - if (ptls->exc_stack->top) { - // May throw - jl_reserve_exc_stack(&lastt->exc_stack, ptls->exc_stack->top); - } - if (t->exc_stack && t->exc_stack->top) { - // Only necessary if t->exc_stack was run on a different thread. - jl_reserve_exc_stack(&ptls->exc_stack, t->exc_stack->top); - } - #ifdef COPY_STACKS save_stack(ptls, lastt, pt); // allocates (gc-safepoint, and can also fail) #else *pt = lastt; // can't fail after here: clear the gc-root for the target task now #endif - // Cannot throw. - if (ptls->exc_stack->top) { - jl_copy_exc_stack(lastt->exc_stack, ptls->exc_stack); - } - if (t->exc_stack && t->exc_stack->top) { - jl_copy_exc_stack(ptls->exc_stack, t->exc_stack); - t->exc_stack->top = 0; - } // set up global state for new task lastt->gcstack = ptls->pgcstack; @@ -567,12 +548,16 @@ void JL_NORETURN throw_internal(jl_value_t *exception JL_MAYBE_UNROOTED, ptls->io_wait = 0; if (ptls->safe_restore) jl_longjmp(*ptls->safe_restore, 1); + jl_gc_unsafe_enter(ptls); if (exception) { // Persist exception in transit before a new one can be generated. - jl_push_exc_stack(&ptls->exc_stack, exception, bt_data, bt_size); + JL_GC_PUSH1(&exception); + // May allocate. FIXME: Ensure bt_data is rooted. + assert(ptls->current_task); + jl_push_exc_stack(&ptls->current_task->exc_stack, exception, bt_data, bt_size); + JL_GC_POP(); } - assert(ptls->exc_stack->top); - jl_gc_unsafe_enter(ptls); + assert(ptls->current_task->exc_stack && ptls->current_task->exc_stack->top); jl_handler_t *eh = ptls->current_task->eh; if (eh != NULL) { #ifdef ENABLE_TIMINGS @@ -612,6 +597,7 @@ JL_DLLEXPORT void jl_sig_throw(void) jl_ptls_t ptls = jl_get_ptls_states(); jl_value_t *e = ptls->sig_exception; assert(e); + ptls->sig_exception = NULL; throw_internal(e, ptls->bt_data, ptls->bt_size); } @@ -619,10 +605,10 @@ JL_DLLEXPORT void jl_rethrow_other(jl_value_t *e) { // TODO: Uses of this function could be replaced with jl_throw // if we rely on exc_stack for root cause analysis. - jl_ptls_t ptls = jl_get_ptls_states(); - assert(ptls->exc_stack->top != 0); + jl_exc_stack_t *exc_stack = jl_get_ptls_states()->current_task->exc_stack; + assert(exc_stack && exc_stack->top != 0); // overwrite exception on top of stack. see jl_exc_stack_exception - jl_excstk_raw(ptls->exc_stack)[ptls->exc_stack->top-1] = (uintptr_t)e; + jl_excstk_raw(exc_stack)[exc_stack->top-1] = (uintptr_t)e; jl_rethrow(); } diff --git a/src/threading.c b/src/threading.c index 36cf96f91e70e..4dd86663e7616 100644 --- a/src/threading.c +++ b/src/threading.c @@ -283,8 +283,6 @@ static void ti_initthread(int16_t tid) } memset(bt_data, 0, sizeof(uintptr_t) * (JL_MAX_BT_SIZE + 1)); ptls->bt_data = (uintptr_t*)bt_data; - ptls->exc_stack = NULL; - jl_reserve_exc_stack(&ptls->exc_stack, JL_MAX_BT_SIZE + 2); ptls->sig_exception = NULL; #ifdef _OS_WINDOWS_ ptls->needs_resetstkoflw = 0;