RFR 8028347: Rewriter::scan_method asserts with array oob in RT_Baseline

[harold seigel]

Hi Coleen,

Thanks for retesting.  The changes look ok.

Harold
On 11/15/2013 5:03 PM, Coleen Phillimore wrote:
> On 11/15/2013 3:45 PM, John Rose wrote:
>> Yes, I think that's much better.  I'm especially glad to see 
>> scan_method lose the TRAPS.  Thanks, Coleen!  — John
>
> You are welcome.  Thanks for the quick code review.
> Coleen
>
>>
>> On Nov 15, 2013, at 10:45 AM, Coleen Phillimore 
>> <coleen.phillimore at oracle.com> wrote:
>>
>>> Here is a boolean that's passed back through the rewriter for the 
>>> invokespecial case, so that we can get a somewhat meaningful message.
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8028347_2/
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8028347
>>>
>>> I reran the testing.
>>> thanks,
>>> Coleen
>>>
>>> On 11/14/2013 9:48 PM, John Rose wrote:
>>>> On Nov 14, 2013, at 12:53 PM, Coleen Phillimore 
>>>> <coleen.phillimore at oracle.com> wrote:
>>>>
>>>>> On 11/14/2013 03:22 PM, John Rose wrote:
>>>>>> Rather than messing with the exceptions more, it would be better 
>>>>>> to have rewrite_invokespecial return a boolean for success, and 
>>>>>> have the caller (in the forward direction only) throw the exception.
>>>>> The caller in the forward direction is the only one that can throw 
>>>>> the exception.
>>>>>> The CATCH macro use makes me especially nervous.  It's almost 
>>>>>> always the wrong answer.
>>>>> Yes, it should make you nervous.
>>>>>> Also, exceptions are wrong here (in hindsight) because they cause 
>>>>>> rewrite_invokespecial to fail out halfway, and then it's probably 
>>>>>> wrong to reverse all the way.  I.e., suppose you have three 
>>>>>> invokespecials, of which the second fails:  You will then *not* 
>>>>>> rewrite the third, and when attempting to reverse all three of 
>>>>>> them (during failure processing) you will probably do something 
>>>>>> wrong trying to reverse the third which was *not* rewritten.
>>>>> If any of the invokespecials fail, this exception will propagate 
>>>>> to the caller and the exception is stored for the entire class and 
>>>>> the class is in error.   None of the reversing will happen in this 
>>>>> case.   The case where we were reversing the rewrite is for OOM 
>>>>> where we can reverse and try the rewriting again.
>>>> Those two exceptions are not fully separable.  The error throw 
>>>> could throw an OOM by accident, and then you will attempt to 
>>>> reverse the irreversible.
>>>>
>>>> Please use a boolean.
>>>>
>>>> — John
>>>>
>>>>> The simplest thing would be to crash the VM.   Returning an exit 
>>>>> code has the same effect as throwing the exception.   In this 
>>>>> case, you can't unrewrite.  The index has overflowed the field it 
>>>>> needs to be stored in.   It's really more complicated to store a 
>>>>> result than propagate the exception.
>>>>>
>>>>>> So (in hindsight) exceptions are the wrong tool for the job here; 
>>>>>> accumulate a "failing" boolean, finish the rewrite pass, and then 
>>>>>> un-rewrite it all cleanly before throwing the exception.
>>>>>>
>>>>>> Sorry I didn't catch this.  When I have worked on rewriter.cpp, I 
>>>>>> have stayed away from exceptions for these sorts of reasons, and 
>>>>>> I should have called it out on first review.
>>>>> Let's get rid of the rewriter for jdk9 instead, okay? This 
>>>>> regression is going to cause headaches for the promotion to jdk8 
>>>>> that we had to get some default methods changes in for.
>>>>>
>>>>> thanks,
>>>>> Coleen
>>>>>
>>>>>> — John
>>>>>>
>>>>>> On Nov 14, 2013, at 12:01 PM, Coleen Phillimore 
>>>>>> <coleen.phillimore at oracle.com> wrote:
>>>>>>
>>>>>>> Summary: Fix reversing rewriting for invokespecial (which I 
>>>>>>> broke with last checkin)
>>>>>>>
>>>>>>> Ran runtime jtreg tests (including failing tests). Rerunning 
>>>>>>> vm.mlvm.testlist.
>>>>>>>
>>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8028347/
>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8028347
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>>>>
>>>>>>>
>