RFR(one-liner): 8231321: compiler/codecache/stress/UnexpectedDeoptimizationAllTest.java failed assertion

[Robbin Ehn]

Hi,

On 9/25/19 10:58 AM, Tobias Hartmann wrote:
> Hi Robbin,
> 
> please note that we also hit the following assert (see comments in bug report):
> Crash: Internal Error ...nmethod.cpp...assert(!found) failed: osr nmethod should have been invalidated

Fixed both:
http://cr.openjdk.java.net/~rehn/8231321/v1/webrev/

The other problem was an osr method that is not_installed and gets marked for
deopt. We call make_not_entrant_or_zombie where we first remove the osr method
before changing state to not_entrant. Before we changed state but after we tried
to remove the osr method we make this osr method in use and install the code.
After we made it not entrant we check that there is no osr method, but due to
above it is and we assert.

I did a double fix by both moving the removal of osr into the same lock scope as
state change, since now we use same lock there is no issue with that.
Also we should not install a method marked for deopt.

It passes many iterations of UnexpectedDeoptimizationAllTest.java, locally and
in mach5. Started a pass with t3-5.

Thanks, Robbin

> 
> Thanks,
> Tobias
> 
> On 25.09.19 10:12, Robbin Ehn wrote:
>> Hi all, please review.
>>
>> We no longer allow state to ever go backwards.
>> So trying to make a zombie not-entrant is fine.
>> What happens is something like:
>> - make not-entrant
>> - safepoint
>> - deopt all => start iteration over nmethods
>> - picks up this not-entrant method, since it is alive
>> - sweeper turns it into a zombie
>> - we try making the nmethod not-entrant once more => assert
>>
>> If we remove the assert the method would just returns false instead, which is expected.
>>
>> Thanks, Robbin
>>
>> Issue:
>> https://bugs.openjdk.java.net/browse/JDK-8231321
>>
>> Changeset:
>> diff -r 448fe2bfd505 src/hotspot/share/code/nmethod.cpp
>> --- a/src/hotspot/share/code/nmethod.cpp    Tue Sep 24 08:54:39 2019 +0200
>> +++ b/src/hotspot/share/code/nmethod.cpp    Wed Sep 25 10:02:45 2019 +0200
>> @@ -1288,17 +1288,16 @@
>>     }
>>   }
>>
>>   /**
>>    * Common functionality for both make_not_entrant and make_zombie
>>    */
>>   bool nmethod::make_not_entrant_or_zombie(int state) {
>>     assert(state == zombie || state == not_entrant, "must be zombie or not_entrant");
>> -  assert(!is_zombie(), "should not already be a zombie");
>>
>>     if (Atomic::load(&_state) >= state) {
>>       // Avoid taking the lock if already in required state.
>>       // This is safe from races because the state is an end-state,
>>       // which the nmethod cannot back out of once entered.
>>       // No need for fencing either.
>>       return false;
>>     }