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

[Tobias Hartmann]

Hi,

Mikael, thanks a lot for the detailed explanation. I was not aware of these macros.

I agree with David that this code is buggy and as Vladimir suggested, I will investigate if we can request the sweeping from the existing sweeper thread without creating a new one. We already have the functionality to notify the sweeper but need some further synchronization to block until a full sweep is done.

Thanks,
Tobias

On 25.03.2015 15:51, Mikael Gerdin wrote:
> 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
>>