Request for review (URGENT!!) 7033141: assert(has_cp_cache(i)) failed: oob

[Coleen Phillimore]

thanks!
Coleen

On 5/20/2011 11:08 AM, Tom Rodriguez wrote:
> Looks good.
>
> tom
>
> On May 19, 2011, at 3:01 PM, Coleen Phillimore wrote:
>
>> Yes, these should be reversed.  You are right and it makes a difference on x86.  Fixed and retested:
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/7033141_3/
>> bug link at http://bugs.sun.com/view_bug.do?bug_id=7033141_3
>>
>> I also fixed the null assert strings and comments that Dan and Karen pointed out.
>>
>> Thank you!
>> Coleen
>>
>> On 5/19/2011 1:15 PM, Tom Rodriguez wrote:
>>> Shouldn't the reversed version of the code swap the get_native/get_Java calls?
>>>
>>>   144 // Rewrite a classfile-order CP index into a native-order CPC index.
>>>   145 void Rewriter::rewrite_member_reference(address bcp, int offset, bool reverse) {
>>>   146   address p = bcp + offset;
>>>   147   if (!reverse) {
>>>   148     int  cp_index    = Bytes::get_Java_u2(p);
>>>   149     int  cache_index = cp_entry_to_cp_cache(cp_index);
>>>   150     Bytes::put_native_u2(p, cache_index);
>>>   151   } else {
>>>   152     int cache_index = Bytes::get_Java_u2(p);
>>>   153     int pool_index = cp_cache_entry_pool_index(cache_index);
>>>   154     Bytes::put_native_u2(p, pool_index);
>>>   155   }
>>>   156 }
>>>
>>>
>>> tom
>>>
>>> On May 18, 2011, at 1:14 PM, Coleen Phillimore wrote:
>>>
>>>> The last code change I sent out was not complete, and would not work if there was an exception in the relocator.  The code was structured as:
>>>>
>>>>    if klass->is_rewritten() flag {
>>>>        verify - exit if exception
>>>>        rewrite bytecodes to point to (to be created) cpCache rather than constant pool
>>>>        create constant pool cache  - (added restore_bytecodes and) exit if exception
>>>>        run relocator for jsr/ret    - exit if exception
>>>>        link method entry points  - exit if exception
>>>>        set rewritten flag
>>>>   }
>>>>
>>>> Any exceptions will cause the whole block to be retried if the exception is handled and class loading is attempted again for this class.  This latest code effectively changes this ordering to:
>>>>
>>>>    if klass->is_rewritten() flag {
>>>>      verify - exit if exception
>>>>      rewrite bytecodes to point to cpCache rather than constant pool entries
>>>>       create constant pool  {
>>>>           if exception restore bytecodes to point to constant pool
>>>>           exit }
>>>>       set rewritten flag
>>>>   }
>>>>   run relocator for jsr/ret - exit if exception
>>>>   link method entry points - exit if exception
>>>>
>>>> If there's an exception in relocator and linking method stages, these two actions are restartable if the VM handles the exception and has to retry loading that class.   The actions under the rewritten flag are not restartable.
>>>>
>>>> This is tested with invokedynamic tests, RedefineClasses tests and class data sharing.  There was special testing to throw OOM at each part and testing to rerun the relocator.
>>>>
>>>> This didn't make the deadline, but please review (again) for the next stage of the jdk7 release.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/7033141_2/
>>>> bug link at http://bugs.sun.com/view_bug.do?bug_id=7033141_2
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>>
>>>> On 5/17/2011 4:44 PM, Coleen Phillimore wrote:
>>>>> Summary: Unrewrite bytecodes for OOM error allocating the constant pool cache.
>>>>>
>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/7033141/
>>>>> bug link at http://bugs.sun.com/view_bug.do?bug_id=7033141
>>>>>
>>>>> This bug fix also moves linking methods outside of the rewriting code so that when the code cache is full, the vm can attempt to link the methods again.   (also fixes bug 6947901)
>>>>>
>>>>> Tested with oom errors inserted for random cpCache allocation and also tested with invokedynamic test.
>>>>>
>>>>> This is somewhat urgent.  If acceptable, I would like to put it back today for code freeze (if not, it'll wait).
>>>>>
>>>>> Thanks,
>>>>> Coleen