[9] RFR(S): 8075214: SIGSEGV in nmethod sweeping

[Mikael Gerdin]

Hi Tobias,

Handling Java-level exceptions in the C++ code is fairly convoluted. 
Stare in horror at the macros in exceptions.hpp and see my comments below.

On 2015-03-25 14:53, Tobias Hartmann wrote:
> Hi,
>
> please review the following patch.
>
> https://bugs.openjdk.java.net/browse/JDK-8075214
> http://cr.openjdk.java.net/~thartmann/8075214/webrev.00/

  818   Klass* k = 
SystemDictionary::resolve_or_fail(vmSymbols::java_lang_Thread(), true, 
THREAD);

This THREAD and a lot of the following ones need to be CHECK_NULL
If any function throws a Java exception (such as OOME) then you cannot 
continue and must return a failure to the caller.

I realize that at this exact point resolving the Thread class should not 
fail, but if it does for some strange reason you will SEGV a few lines 
below. You could make this a CATCH, which will crash the VM on an 
unexpected exception, which may be ok for a white box test function.

  819   instanceKlassHandle klass(THREAD, k);
  820   instanceHandle thread_oop = klass->allocate_instance_handle(THREAD);

You can get OOME here and need to bail out if thread_oop is null due to 
an allocation failure, change this THREAD to CHECK_NULL

  821
  822   Handle name = java_lang_String::create_from_str("WB Sweeper 
thread", THREAD);

Allocating memory for the String can OOME, replace with CHECK_NULL

  823
  824   // Initialize thread_oop to put it into the system threadGroup
  825   Handle thread_group(THREAD, Universe::system_thread_group());
  826   JavaValue result(T_VOID);
  827   JavaCalls::call_special(&result,
  828                           thread_oop,
  829                           klass,
  830                           vmSymbols::object_initializer_name(),
  831 
vmSymbols::threadgroup_string_void_signature(),
  832                           thread_group,
  833                           name,
  834                           THREAD);

I'd pass CHECK_NULL here as well, since it's unclear if Thread::<init> 
can throw any exception.

  835
  836   KlassHandle group(THREAD, SystemDictionary::ThreadGroup_klass());
  837   JavaCalls::call_special(&result,
  838                           thread_group,
  839                           group,
  840                           vmSymbols::add_method_name(),
  841                           vmSymbols::thread_void_signature(),
  842                           thread_oop,             // ARG 1
  843                           THREAD);

Adding to the thread group can cause an OOME (the Arrays.copyOf 
allocates a new array), add CHECK_NULL.

  844
  845   {
  846     MutexLocker mu(Threads_lock);
  847
  848     // create sweeper thread w/ custom entry -- one iteration 
instead of loop
  849     CodeCacheSweeperThread* sweeper_thread = new 
CodeCacheSweeperThread();
  850     sweeper_thread->set_entry_point(&WhiteBox::sweeper_thread_entry);
  851
  852     // check that thread and osthread were created
  853     if (sweeper_thread == NULL || sweeper_thread->osthread() == 
NULL) {
  854       vm_exit_during_initialization("java.lang.OutOfMemoryError",
  855 
os::native_thread_creation_failed_msg());

This should be vm_exit_out_of_memory, not "during initialization".

  856     }
  857

/Mikael

>
> Problem:
> The test uses the Whitebox API to enforce sweeping by creating and starting a 'CodeCacheSweeperThread'. During creation of the thread, the interpreter crashes in j.l.ThreadGroup.add(Thread t) [1] while executing a subtype check to validate that 't' is a subtype of j.l.Thread [2]. The problem is that we pass 'JavaThread->threadObj()' to 'ThreadGroup.add' which is invalid due to a GC that moved the object. The GC does not know about the thread because it was not yet added to the threads list and therefore does not update the oop.
>
> Solution:
> Instead of calling 'JavaThread::allocate_threadObj', the initialization is moved to the caller to make sure that setting the thread oop is done together with adding the thread to the threads list. I also fixed the missing oom handling described as one of the problems in JDK-8072377 [3].
>
> Testing:
> - 1k runs of failing testcase
> - JPRT
>
> Thanks,
> Tobias
>
> [1] http://hg.openjdk.java.net/jdk9/hs-comp/jdk/file/tip/src/java.base/share/classes/java/lang/ThreadGroup.java#l896
> [2] see '__ gen_subtype_check' in 'TemplateTable::aastore'
> [3] https://bugs.openjdk.java.net/browse/JDK-8072377
>