RFR 8030633: nsk/jvmti/RedefineClasses/StressRedefine failed invalid method ordering, length on Solaris

[Dmitry Samersoff]

Coleen,

I'm ok to push the fix as it is.

-Dmitry

On 2013-12-19 23:57, Coleen Phillimore wrote:
> 
> Hi Dmitry,
> 
> Thank you for looking at the code.  I agree with you about the nits, but
> since I didn't change those lines, I want to leave them as is. They seem
> safe to change but then again, it's the end of the release and any
> changes could be risky.
> 
> On 12/19/2013 02:42 PM, Dmitry Samersoff wrote:
>> Coleen,
>>
>> Looks good for me.
>>
>> Few cosmetic nits to mention:
>>
>> defaultMethods.cpp:
>>
>> ll.1042. 1052
>>
>> It might be more readable if we replace
>> klass->methods()->length() to original_methods->length();
>>
>> ll. 1054
>>
>> It might be good to call
>> sort(new_methods) only if (new_methods->length() > 0)
> 
> new_methods->length() will always be >0 because we only get here if
> there are overpass methods to add to the methods list.  It would be nice
> to encode that assumption but not just now.
> 
> Thanks,
> Coleen
>> -Dmitry
>>
>>
>> On 2013-12-19 22:59, Coleen Phillimore wrote:
>>> On 12/19/2013 12:42 PM, Karen Kinnear wrote:
>>>> Coleen,
>>>>
>>>> Code looks good.
>>>> In particular I was checking we handle both the null and
>>>> empty_int_array cases - I assume
>>>> that we need to handle both due to bootstrapping issues.
>>> The original method array is the_empty_int_array() and not null but I
>>> added null checks in it because Dan had the same comment too.
>>>
>>> See:
>>>
>>> http://cr.openjdk.java.net/~coleenp/8030633_2/
>>>
>>> Thanks,
>>> Coleen
>>>
>>>> thanks,
>>>> Karen
>>>>
>>>> On Dec 19, 2013, at 11:40 AM, Coleen Phillimore wrote:
>>>>
>>>>> Summary: A method with no declared methods was getting an AME
>>>>> overpass method with the latest change.  The method_ordering array
>>>>> was not updated for the new methods.
>>>>>
>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8030633/
>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8030633
>>>>>
>>>>> Tested with existing tests
>>>>> nsk/jvmti/RedefineClasses/StressRedefine{WithoutBytecodeCorruption}.
>>>>> Needs native code to create new test in jtreg, so I didn't add one.
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.