mjit_worker.c: promote mjit_copy_job from function

-local variable to global variable.

Consider this case:
1. MJIT worker: dequeue ISeq (stop_worker_p was still FALSE)
2. Ruby thread: call Kernel#exec, which calls mjit_finish(FALSE),
                sets `stop_worker_p = TRUE`, and fires RUBY_VM_CHECK_INTS() once
3. MJIT worker: register copy job, but found stop_worker_p is TRUE.
                set `worker_stopped = TRUE` and the thread stops.
4. Function-local job variable expires by the thread stop (this is eliminated by this commit)
5. Ruby thread: find `worker_stopped` becamse TRUE, start Kernel#exec.
                Kernel#exec fails but exception is rescued.
6. Ruby thread: call RUBY_VM_CHECK_INTS. copy job is dispatched but job variable
                is already expired.

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@66035 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
This commit is contained in:
k0kubun 2018-11-27 12:00:51 +00:00
parent c3fe307808
commit 86cb3d319f
2 changed files with 24 additions and 20 deletions

12
mjit.c
View File

@ -24,17 +24,17 @@
static void static void
mjit_copy_job_handler(void *data) mjit_copy_job_handler(void *data)
{ {
struct mjit_copy_job *job = data; mjit_copy_job_t *job = data;
const struct rb_iseq_constant_body *body; const struct rb_iseq_constant_body *body;
if (stop_worker_p) { if (stop_worker_p) { /* check if mutex is still alive, before calling CRITICAL_SECTION_START. */
/* `copy_cache_from_main_thread()` stops to wait for this job. Then job data which is
allocated by `alloca()` could be expired and we might not be able to access that.
Also this should be checked before CRITICAL_SECTION_START to ensure that mutex is alive. */
return; return;
} }
CRITICAL_SECTION_START(3, "in mjit_copy_job_handler"); CRITICAL_SECTION_START(3, "in mjit_copy_job_handler");
/* Make sure that this job is never executed while job is being modified or ISeq is GC-ed */ /* Make sure that this job is never executed when:
1. job is being modified
2. alloca memory inside job is expired
3. ISeq is GC-ed */
if (job->finish_p || job->unit->iseq == NULL) { if (job->finish_p || job->unit->iseq == NULL) {
CRITICAL_SECTION_FINISH(3, "in mjit_copy_job_handler"); CRITICAL_SECTION_FINISH(3, "in mjit_copy_job_handler");
return; return;

View File

@ -1120,12 +1120,16 @@ convert_unit_to_func(struct rb_mjit_unit *unit, struct rb_call_cache *cc_entries
return (mjit_func_t)func; return (mjit_func_t)func;
} }
struct mjit_copy_job { typedef struct {
struct rb_mjit_unit *unit; struct rb_mjit_unit *unit;
struct rb_call_cache *cc_entries; struct rb_call_cache *cc_entries;
union iseq_inline_storage_entry *is_entries; union iseq_inline_storage_entry *is_entries;
int finish_p; int finish_p;
}; } mjit_copy_job_t;
/* Singleton MJIT copy job. This is made global since it needs to be durable even when MJIT worker thread is stopped.
(ex: register job -> MJIT pause -> MJIT resume -> dispatch job. Actually this should be just cancelled by finish_p check) */
static mjit_copy_job_t mjit_copy_job;
static void mjit_copy_job_handler(void *data); static void mjit_copy_job_handler(void *data);
@ -1133,7 +1137,7 @@ static void mjit_copy_job_handler(void *data);
could be different between ones on enqueue timing and ones on dequeue timing. could be different between ones on enqueue timing and ones on dequeue timing.
Return TRUE if copy succeeds. */ Return TRUE if copy succeeds. */
static int static int
copy_cache_from_main_thread(struct mjit_copy_job *job) copy_cache_from_main_thread(mjit_copy_job_t *job)
{ {
CRITICAL_SECTION_START(3, "in copy_cache_from_main_thread"); CRITICAL_SECTION_START(3, "in copy_cache_from_main_thread");
job->finish_p = FALSE; /* allow dispatching this job in mjit_copy_job_handler */ job->finish_p = FALSE; /* allow dispatching this job in mjit_copy_job_handler */
@ -1164,7 +1168,7 @@ copy_cache_from_main_thread(struct mjit_copy_job *job)
void void
mjit_worker(void) mjit_worker(void)
{ {
struct mjit_copy_job job; mjit_copy_job_t *job = &mjit_copy_job; /* just a shorthand */
#ifndef _MSC_VER #ifndef _MSC_VER
if (pch_status == PCH_NOT_READY) { if (pch_status == PCH_NOT_READY) {
@ -1192,30 +1196,30 @@ mjit_worker(void)
verbose(3, "Getting wakeup from client"); verbose(3, "Getting wakeup from client");
} }
unit = get_from_list(&unit_queue); unit = get_from_list(&unit_queue);
job.finish_p = TRUE; /* disable dispatching this job in mjit_copy_job_handler while it's being modified */ job->finish_p = TRUE; /* disable dispatching this job in mjit_copy_job_handler while it's being modified */
CRITICAL_SECTION_FINISH(3, "in worker dequeue"); CRITICAL_SECTION_FINISH(3, "in worker dequeue");
if (unit) { if (unit) {
mjit_func_t func; mjit_func_t func;
const struct rb_iseq_constant_body *body = unit->iseq->body; const struct rb_iseq_constant_body *body = unit->iseq->body;
job.unit = unit; job->unit = unit;
job.cc_entries = NULL; job->cc_entries = NULL;
if (body->ci_size > 0 || body->ci_kw_size > 0) if (body->ci_size > 0 || body->ci_kw_size > 0)
job.cc_entries = alloca(sizeof(struct rb_call_cache) * (body->ci_size + body->ci_kw_size)); job->cc_entries = alloca(sizeof(struct rb_call_cache) * (body->ci_size + body->ci_kw_size));
job.is_entries = NULL; job->is_entries = NULL;
if (body->is_size > 0) if (body->is_size > 0)
job.is_entries = alloca(sizeof(union iseq_inline_storage_entry) * body->is_size); job->is_entries = alloca(sizeof(union iseq_inline_storage_entry) * body->is_size);
/* Copy ISeq's inline caches values to avoid race condition. */ /* Copy ISeq's inline caches values to avoid race condition. */
if (job.cc_entries != NULL || job.is_entries != NULL) { if (job->cc_entries != NULL || job->is_entries != NULL) {
if (copy_cache_from_main_thread(&job) == FALSE) { if (copy_cache_from_main_thread(job) == FALSE) {
continue; /* retry postponed_job failure, or stop worker */ continue; /* retry postponed_job failure, or stop worker */
} }
} }
/* JIT compile */ /* JIT compile */
func = convert_unit_to_func(unit, job.cc_entries, job.is_entries); func = convert_unit_to_func(unit, job->cc_entries, job->is_entries);
CRITICAL_SECTION_START(3, "in jit func replace"); CRITICAL_SECTION_START(3, "in jit func replace");
while (in_gc) { /* Make sure we're not GC-ing when touching ISeq */ while (in_gc) { /* Make sure we're not GC-ing when touching ISeq */
@ -1240,7 +1244,7 @@ mjit_worker(void)
/* Disable dispatching this job in mjit_copy_job_handler while memory allocated by alloca /* Disable dispatching this job in mjit_copy_job_handler while memory allocated by alloca
could be expired after finishing this function. */ could be expired after finishing this function. */
job.finish_p = TRUE; job->finish_p = TRUE;
/* To keep mutex unlocked when it is destroyed by mjit_finish, don't wrap CRITICAL_SECTION here. */ /* To keep mutex unlocked when it is destroyed by mjit_finish, don't wrap CRITICAL_SECTION here. */
worker_stopped = TRUE; worker_stopped = TRUE;