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

[Christian Thalinger]

Thank you, Tom and Vladimir.  -- Chris

On Nov 2, 2011, at 9:09 PM, Vladimir Kozlov wrote:

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