RFR: 8212585: Clean up CompiledMethod::oops_reloc_begin()

[Doerr, Martin]

Hi Erik,

I still see crashes on SPARC and PPC64 when running jck tests:
V  [libjvm.so+0xef36a0]  void DependencyContext::remove_dependent_nmethod(nmethod*,bool)+0x90
V  [libjvm.so+0x1243688]  void InstanceKlass::remove_dependent_nmethod(nmethod*,bool)+0xa8
V  [libjvm.so+0x1b19978]  void nmethod::flush_dependencies(bool)+0x388
V  [libjvm.so+0x1b1842c]  void nmethod::make_unloaded()+0x8c
V  [libjvm.so+0x1c249cc]  void CodeCacheUnloadingTask::work(unsigned)+0x7c
V  [libjvm.so+0x1c24cf0]  void ParallelCleaningTask::work(unsigned)+0x10
V  [libjvm.so+0x21330b4]  void GangWorker::loop()+0xa4
V  [libjvm.so+0x1f94908]  void Thread::call_run()+0x128
V  [libjvm.so+0x1bc297c]  thread_native_entry+0x3ec

I haven't tried to debug, but I found a problem by reading code:
We shouldn't use NativeJump::instruction_size in CompiledMethod::oops_reloc_begin().
On SPARC and PPC64, this size is the size of a large instruction sequence, but NativeJump::patch_verified_entry only patches a 4 byte trapping instruction. AFAICS only 4 byte is guaranteed to contain to oop (on these 2 platforms, other ones look ok).
We could fix this constant on PPC64, but SPARC uses is for other purposes.
Do you have a good proposal to fix it?

Thanks and best regards,
Martin


-----Original Message-----
From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf Of Erik Österlund
Sent: Montag, 5. November 2018 11:14
To: Per Liden <per.liden at oracle.com>; hotspot-dev developers <hotspot-dev at openjdk.java.net>
Subject: Re: RFR: 8212585: Clean up CompiledMethod::oops_reloc_begin()

Hi Per,

Thanks for the review.

/Erik

On 2018-11-05 10:51, Per Liden wrote:
> Hi Erik,
>
> On 10/18/18 11:07 PM, Erik Österlund wrote:
>> Hi,
>>
>> I decided to make this less risky for platforms that do not yet need 
>> to be scanned concurrently, and hence don't really need to be "fixed".
>>
>> In this new webrev, I decided to check for the frame completed offset 
>> if available and further away than verified entry + native jump size. 
>> Then it is safe for me to go ahead and scan this stuff concurrently 
>> using nmethod entry barriers. Otherwise, the same good old behaviour 
>> we used to rely on applies so that we won't get any surprises where 
>> we do not want them.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8212585/webrev.01/
>
> Looks good!
>
> /Per
>
>>
>> Thanks,
>> /Erik
>>
>> On 2018-10-17 17:36, Erik Österlund wrote:
>>> Hi,
>>>
>>> In CompiledMethod::oops_reloc_begin() there is code to prevent 
>>> looking for oops at the verified entry of an nmethod that is not 
>>> entrant, as a native jump instruction has been overwritten there. 
>>> Except there would *never* be any immediate oops there for any 
>>> compiler, even before becoming not entrant, with or without OSR 
>>> compilation, so this special handling of not entrant vs entrant 
>>> nmethods is seemingly completely unnecessary.
>>>
>>> This gets increasingly awkward when oops_do is called concurrently 
>>> with concurrent class unloading, where an nmethod is racing to 
>>> become not entrant.
>>>
>>> To clean this up, I propose to change the boundary for immediate oop 
>>> scanning and start looking for oops only after the frame building is 
>>> completed, as there is absolutely no point in looking for oops 
>>> before that in the first place. This removes unnecessary workarounds 
>>> that exist today, and removes unnecessary races going forward.
>>>
>>> It seems like Graal as JIT also inlines oops into the code stream, 
>>> but never sets the CodeOffsets::Frame_Complete in the nmethod. So I 
>>> recognize such nmethods and unconditionally start scanning at the 
>>> verified entry + native jump instruction size. I spoke to the Graal 
>>> folks, and they said they do not put oops in there either. So I 
>>> think everyone involved should be happy with this solution.
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8212585
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8212585/webrev.00/
>>>
>>> Thanks,
>>> /Erik
>>