Request for reviews (XS): 7106944: assert(_pc == *pc_addr) failed may be too strong

[Vladimir Kozlov]

I see, the only work frame::deoptimize() does is patching PC. OK then.

Thanks,
Vladimir

Tom Rodriguez wrote:
> On Nov 2, 2011, at 12:54 PM, Vladimir Kozlov wrote:
> 
>> Should we then check that the frame is deoptimized already and bailout after revoke_biases_of_monitors() call? I surprise we did not hit any problems before by deoptimizing frame twice.
> 
> We could but I'm not sure I see the point.  I think the existing guards are just about avoiding needless work more than anything.
> 
> tom
> 
>> Vladimir
>>
>> Tom Rodriguez wrote:
>>> On Nov 2, 2011, at 10:22 AM, Vladimir Kozlov wrote:
>>>> Yes, it looks good. But, Christian, can you show (call stack?) how we endup deoptimizing twice? How we enter deoptimization code twice?
>>> I think it's biased locking revocation.  Here's the assertion point:
>>> V  [libjvm.so+0x2701328]  void VMError::report(outputStream*)+0x8c8
>>> V  [libjvm.so+0x2702469]  void VMError::report_and_die()+0x4fd
>>> V  [libjvm.so+0xe869bf]  void report_vm_error(const char*,int,const char*,const char*)+0x55f
>>> V  [libjvm.so+0xff8666]  void frame::patch_pc(Thread*,unsigned char*)+0xe6
>>> V  [libjvm.so+0xfe6e8d]  void frame::deoptimize(JavaThread*)+0x361
>>> V  [libjvm.so+0xed9622]  void Deoptimization::deoptimize_frame(JavaThread*,long*)+0x55e
>>> V  [libjvm.so+0xa34674]  void deopt_caller()+0x154
>>> V  [libjvm.so+0xa35974]  void Runtime1::new_type_array(JavaThread*,klassOopDesc*,int)+0x3c4
>>> deopt_caller has a check for the caller already being deopted:
>>> static bool caller_is_deopted() {
>>>  JavaThread* thread = JavaThread::current();
>>>  RegisterMap reg_map(thread, false);
>>>  frame runtime_frame = thread->last_frame();
>>>  frame caller_frame = runtime_frame.sender(&reg_map);
>>>  assert(caller_frame.is_compiled_frame(), "must be compiled");
>>>  return caller_frame.is_deoptimized_frame();
>>> }
>>> // Stress deoptimization                                                                                                                                static void deopt_caller() {
>>>  if ( !caller_is_deopted()) {
>>>    JavaThread* thread = JavaThread::current();
>>>    RegisterMap reg_map(thread, false);
>>>    frame runtime_frame = thread->last_frame();
>>>    frame caller_frame = runtime_frame.sender(&reg_map);
>>>    Deoptimization::deoptimize_frame(thread, caller_frame.id());
>>>    assert(caller_is_deopted(), "Must be deoptimized");
>>>  }
>>> }
>>> but down in deoptimize we may revoke biases at a safepoint:
>>>  if (UseBiasedLocking) {
>>>    revoke_biases_of_monitors(thread, fr, map);
>>>  }
>>>  deoptimize_single_frame(thread, fr);
>>> This allows a deopt to occur during the safepoint, after the initial guard.
>>> tom
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 11/2/11 10:01 AM, Tom Rodriguez wrote:
>>>>> Looks good.
>>>>>
>>>>> tom
>>>>>
>>>>> On Nov 2, 2011, at 8:53 AM, Christian Thalinger wrote:
>>>>>
>>>>>> http://cr.openjdk.java.net/~twisti/7106944/
>>>>>>
>>>>>> 7106944: assert(_pc == *pc_addr) failed may be too strong
>>>>>> Reviewed-by:
>>>>>>
>>>>>> Sometimes we are already deoptimizing a frame when we request another
>>>>>> deoptimization of the same frame.  This makes the assert to fail.
>>>>>>
>>>>>> The fix is to add a check to the assert that the return address is the
>>>>>> same that we are going to patch.
>>>>>>
>>>>>> src/cpu/x86/vm/frame_x86.cpp
>>>>>>
>