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

[Coleen Phillimore]

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