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

[Coleen Phillimore]

On 12/19/2013 12:24 PM, Daniel D. Daugherty wrote:
> On 12/19/13 9: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/
>
> src/share/vm/classfile/defaultMethods.cpp
>
> 1047 // original_ordering might be empty.
> 1048 if (JvmtiExport::can_maintain_original_method_order() || 
> DumpSharedSpaces) {
>     The original code didn't allocate a non-empty merged_ordering
>     if original_ordering was NULL or empty. 

Right.  In this case it should have.   I added a bit to the comment why.

> This new code and other
>     existing uses of original_ordering make the assumption that
>     original_ordering is !NULL.

Yes, it is never null because it's Universe::the_empty_int_array() but 
this code shouldn't know that.
>
>     I think you need to assert that original_ordering is !NULL
>     after this line:
>
>     1039   Array<int>* original_ordering = klass->method_ordering();
>            assert(original_ordering != NULL, "sanity check");

I added original_ordering != NULL in the two if statements that next get 
the length() instead.  I can be paranoid too!
>
>     I also think that you should only allocate a non-empty
>     merged_ordering "if (original_ordering->length() > 0_".

No, I can't do that.   Because the merged_ordering has to be equal to 
the number of methods, including AME overpasses which we add.
>
> 1075 assert(original_ordering->length() > 0, "should have ordered this 
> method");
>     I'm have a little trouble with the assertion message.
>     Maybe: "should have original order info for these methods"
>

Ok, that comment reads better.

Thanks!
Coleen

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