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

[Tom Rodriguez]

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
>>>>>