k0kubun	2018-11-06 16:22:25 +0900 (Tue, 06 Nov 2018)

  New Revision: 65569

  https://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=revision&revision=65569

  Log:
    mjit_worker.c: strictly control MJIT copy job
    
    -available region. reducing risk of SEGV in mjit_copy_job_handler() like
    http://ci.rvm.jp/results/trunk-mjit@silicon-docker/1446117
    
    I'm not sure which exact part is causing "[BUG] Segmentation fault at 0x0000000000000008"
    on `(mjit_copy_job_handler+0x12) [0x564a6c4ce632] /home/ko1/ruby/src/trunk-mjit/mjit.c:26`...
    
    mjit.c: ditto

  Modified files:
    trunk/mjit.c
    trunk/mjit_worker.c
Index: mjit.c
===================================================================
--- mjit.c	(revision 65568)
+++ mjit.c	(revision 65569)
@@ -24,15 +24,20 @@ https://github.com/ruby/ruby/blob/trunk/mjit.c#L24
 static void
 mjit_copy_job_handler(void *data)
 {
-    struct mjit_copy_job *job;
-    if (stop_worker_p) {
-        /* `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. */
+    struct mjit_copy_job *job = data;
+    int finish_p;
+    CRITICAL_SECTION_START(3, "in mjit_copy_job_handler");
+    finish_p = job->finish_p;
+    CRITICAL_SECTION_FINISH(3, "in mjit_copy_job_handler");
+
+    if (stop_worker_p || finish_p) {
+        /* `stop_worker_p`: `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.
+           `finish_p`: make sure that this job is never executed while job is being modified. */
         return;
     }
 
-    job = (struct mjit_copy_job *)data;
     if (job->cc_entries) {
         memcpy(job->cc_entries, job->body->cc_entries, sizeof(struct rb_call_cache) * (job->body->ci_size + job->body->ci_kw_size));
     }
@@ -40,10 +45,10 @@ mjit_copy_job_handler(void *data) https://github.com/ruby/ruby/blob/trunk/mjit.c#L45
         memcpy(job->is_entries, job->body->is_entries, sizeof(union iseq_inline_storage_entry) * job->body->is_size);
     }
 
-    CRITICAL_SECTION_START(3, "in MJIT copy job wait");
+    CRITICAL_SECTION_START(3, "in mjit_copy_job_handler");
     job->finish_p = TRUE;
     rb_native_cond_broadcast(&mjit_worker_wakeup);
-    CRITICAL_SECTION_FINISH(3, "in MJIT copy job wait");
+    CRITICAL_SECTION_FINISH(3, "in mjit_copy_job_handler");
 }
 
 extern int rb_thread_create_mjit_thread(void (*worker_func)(void));
Index: mjit_worker.c
===================================================================
--- mjit_worker.c	(revision 65568)
+++ mjit_worker.c	(revision 65569)
@@ -1136,11 +1136,17 @@ static void mjit_copy_job_handler(void * https://github.com/ruby/ruby/blob/trunk/mjit_worker.c#L1136
 static int
 copy_cache_from_main_thread(struct mjit_copy_job *job)
 {
-    job->finish_p = FALSE;
+    CRITICAL_SECTION_START(3, "in copy_cache_from_main_thread");
+    job->finish_p = FALSE; /* allow dispatching this job in mjit_copy_job_handler */
+    CRITICAL_SECTION_FINISH(3, "in copy_cache_from_main_thread");
+
+    if (UNLIKELY(mjit_opts.wait)) {
+        mjit_copy_job_handler((void *)job);
+        return job->finish_p;
+    }
 
-    if (!rb_postponed_job_register(0, mjit_copy_job_handler, (void *)job))
+    if (!rb_postponed_job_register_one(0, mjit_copy_job_handler, (void *)job))
         return FALSE;
-
     CRITICAL_SECTION_START(3, "in MJIT copy job wait");
     /* checking `stop_worker_p` too because `RUBY_VM_CHECK_INTS(ec)` may not
        lush mjit_copy_job_handler when EC_EXEC_TAG() is not TAG_NONE, and then
@@ -1159,6 +1165,8 @@ copy_cache_from_main_thread(struct mjit_ https://github.com/ruby/ruby/blob/trunk/mjit_worker.c#L1165
 void
 mjit_worker(void)
 {
+    struct mjit_copy_job job;
+
 #ifndef _MSC_VER
     if (pch_status == PCH_NOT_READY) {
         make_pch();
@@ -1185,11 +1193,11 @@ mjit_worker(void) https://github.com/ruby/ruby/blob/trunk/mjit_worker.c#L1193
             verbose(3, "Getting wakeup from client");
         }
         unit = get_from_list(&unit_queue);
+        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");
 
         if (unit) {
             mjit_func_t func;
-            struct mjit_copy_job job;
 
             job.body = unit->iseq->body;
             job.cc_entries = NULL;
@@ -1201,10 +1209,7 @@ mjit_worker(void) https://github.com/ruby/ruby/blob/trunk/mjit_worker.c#L1209
 
             /* Copy ISeq's inline caches values to avoid race condition. */
             if (job.cc_entries != NULL || job.is_entries != NULL) {
-                if (UNLIKELY(mjit_opts.wait)) {
-                    mjit_copy_job_handler((void *)&job); /* main thread is waiting in mjit_wait_call() and doesn't race */
-                }
-                else if (copy_cache_from_main_thread(&job) == FALSE) {
+                if (copy_cache_from_main_thread(&job) == FALSE) {
                     continue; /* retry postponed_job failure, or stop worker */
                 }
             }

--
ML: ruby-changes@q...
Info: http://www.atdot.net/~ko1/quickml/