RFR: 8222841: Incorrect static call stub interactions with class unloading

[Erik Österlund]

Hi Coleen,

Thank you for the review.

On 2019-04-23 23:32, coleen.phillimore at oracle.com wrote:
>
> http://cr.openjdk.java.net/~eosterlund/8222841/webrev.00/src/hotspot/cpu/x86/gc/shared/barrierSetAssembler_x86.cpp.frames.html 
>
>
> Can you mention in a comment that incoming rbx is the method?

Fixed.

> You can't do a JRT_LEAF function from here, can you to some 
> Method::is_alive() function?  Or even a straight up call ?  This 
> doesn't seem that it should care about the extra overhead of the call 
> instruction.  The assembly code looks correct but yuck not more 
> assembly code.

I'm not sure I know how I feel about that. I would have to save and 
restore all registers then to go into the runtime just to check if the 
method is alive. Would you be okay with keeping the assembly variant 
instead until I find a better solution to this problem? (I'm on it)

> http://cr.openjdk.java.net/~eosterlund/8222841/webrev.00/src/hotspot/share/code/compiledMethod.cpp.frames.html 
>
>
> 578 case relocInfo::static_stub_type: {
> 579 is_in_static_stub = true;
> 580 break;
> 581 }
> 582
> 583 case relocInfo::metadata_type: {
> 584 if (!is_in_static_stub) {
> 585 continue;
> 586 }
>
>
> This was mystifying to me.  Can you put a comment that after 
> static_stub_type relocations are generated, the next metadata 
> relocation contains the address that is patched at runtime, so this is 
> the metadata that can be stale and should be cleared by unloading.

Sure, I updated the comments, and added a better description about what 
is going on here.

Webrev:
http://cr.openjdk.java.net/~eosterlund/8222841/webrev.01/

Incremental:
http://cr.openjdk.java.net/~eosterlund/8222841/webrev.00_01/

Thanks,
/Erik

> The clean_ic_stubs() name is my fault.  I didn't know what they were 
> and just wanted this code out of my way.
>
> Thank you for explaining how redefinition is broken by this.  I didn't 
> know these metadata_reloc guys were patched at runtime.
>
> Thanks,
> Coleen
>
>
>
> On 4/23/19 4:41 AM, Erik Österlund wrote:
>> Hi,
>>
>> Static call stubs are cleared in debug builds before an assert 
>> checking for dead metadata. This is done in the function 
>> "clean_ic_stubs()" (which does not clean ICStubs... but 
>> static/optimized virtual call stubs for calling c2i adapters from 
>> compiled code...) The reason is that the static call stub has 
>> metadata, and it complains about dead metadata being left after 
>> inline cache cleaning of is_alive nmethods.
>>
>> ...except class redefinition looks at this metadata to determine if 
>> there is old metadata in subsequent safepoints, and they could have 
>> been unloaded then. Ouch. So maybe we shouldn't squelch that assert 
>> in debug builds, because it actually shows there is a real problem.
>>
>> ...and with concurrent class unloading (ZGC and seemingly soon 
>> Shenandoah), we can't just clear the call stub of a concurrently 
>> running JavaThread; they can racingly call in to them before they get 
>> cleared, and then get stuck in an infinite loop, because clearing 
>> also involves updating the branch target of the static call stub to a 
>> self-loop. Or call a dead method, which also seems like a scary 
>> scenario.
>>
>> All things considered, clearing static call stubs when unloading the 
>> code cache seems like a bad idea.
>>
>> My proposal is the following:
>> 1) Make sure that code cache unloading *always* clears out the 
>> metadata part (but not the branch part) of static call stubs, so that 
>> subsequent operations such as class redefinition will not blow up 
>> when looking at the embedded metadata, but it won't get stuck in 
>> infinite loops with concurrent code cache unloading.
>> 2) Make a c2i entry barrier for concurrently unloading GCs that will 
>> reresolve the call when calling into a stale static call stub (iff 
>> method is NULL or dead).
>> 3) Relax MT-safety asserts to allow the previous medatada to be 
>> concurrently unloading when setting the new target.
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8222841
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8222841/webrev.00/
>>
>> Thanks,
>> /Erik
>