RFR (S) 8209889: RedefineStress tests crash

[Doerr, Martin]

Hi Coleen,

good catch. Looks good where you moved it.
I couldn’t find a place for a NoSafepointVerifier, either.

Maybe it’d be more clear if we replaced the “continue” in the NULL case by an else case in the loop so the CompileTaskWrapper is the first thing after the NULL check (see below).

Best regards,
Martin


--- a/src/hotspot/share/compiler/compileBroker.cpp      Thu Oct 04 14:03:13 2018 +0200
+++ b/src/hotspot/share/compiler/compileBroker.cpp      Thu Oct 04 16:29:45 2018 +0200
@@ -1781,30 +1781,31 @@
           return; // Stop this thread.
         }
       }
-      continue;
-    }
-
-    if (UseDynamicNumberOfCompilerThreads) {
-      possibly_add_compiler_threads();
-    }
+    } else {
+      // Assign the task to the current thread.  Mark this compilation
+      // thread as active for the profiler.
+      // CompileTaskWrapper also keeps the Method* from being deallocated if redefinition
+      // occurs after fetching the compile task off the queue.
+      CompileTaskWrapper ctw(task);
+      nmethodLocker result_handle;  // (handle for the nmethod produced by this task)
+      task->set_code_handle(&result_handle);
+      methodHandle method(thread, task->method());

-    // Assign the task to the current thread.  Mark this compilation
-    // thread as active for the profiler.
-    CompileTaskWrapper ctw(task);
-    nmethodLocker result_handle;  // (handle for the nmethod produced by this task)
-    task->set_code_handle(&result_handle);
-    methodHandle method(thread, task->method());
+      // Never compile a method if breakpoints are present in it
+      if (method()->number_of_breakpoints() == 0) {
+        // Compile the method.
+        if ((UseCompiler || AlwaysCompileLoopMethods) && CompileBroker::should_compile_new_jobs()) {
+          invoke_compiler_on_method(task);
+          thread->start_idle_timer();
+        } else {
+          // After compilation is disabled, remove remaining methods from queue
+          method->clear_queued_for_compilation();
+          task->set_failure_reason("compilation is disabled");
+        }
+      }

-    // Never compile a method if breakpoints are present in it
-    if (method()->number_of_breakpoints() == 0) {
-      // Compile the method.
-      if ((UseCompiler || AlwaysCompileLoopMethods) && CompileBroker::should_compile_new_jobs()) {
-        invoke_compiler_on_method(task);
-        thread->start_idle_timer();
-      } else {
-        // After compilation is disabled, remove remaining methods from queue
-        method->clear_queued_for_compilation();
-        task->set_failure_reason("compilation is disabled");
+      if (UseDynamicNumberOfCompilerThreads) {
+        possibly_add_compiler_threads();
       }
     }
   }


From: coleen.phillimore at oracle.com <coleen.phillimore at oracle.com>
Sent: Donnerstag, 4. Oktober 2018 13:45
To: hotspot-dev developers <hotspot-dev at openjdk.java.net>; Doerr, Martin <martin.doerr at sap.com>
Subject: RFR (S) 8209889: RedefineStress tests crash

Summary: Create CompileTaskWrapper before potential safepoint

open webrev at http://cr.openjdk.java.net/~coleenp/8209889.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8209889

MetadataOnStackMark needs to find the CompileTask in the current thread, or else the Method isn't marked as being alive, so if it's old/obsolete, it is freed.  There are checks to disable adding old methods to the compile queue but if the method is redefined after this point, there may be a preexisting bug that we compile an old method.  This change just fixes the crash.

Can some compiler person (Martin?) look at where I moved possibly_add_compiler_threads and let me know if that looks okay?  There's a comment in the change that says why you need to create the CompileTaskWrapper before that call.  I couldn't find a place to put a NoSafepointVerifier.

Tested with the test that failed, which doesn't reproduce the bug except rarely, and test/hotspot/jtreg/compiler tests.  tier1 an 2 in progress.

Thanks,
Coleen