review for 7145024: Crashes in ucrypto related to C2

[Vladimir Kozlov]

Actually assembler from JNITest::nativeTest3 should be enough since first array 
copied as rsi -> rdi:rsi, the same as in nativeTest1.

Vladimir

Vladimir Kozlov wrote:
> First, can you show assembler moves code for an array?
> 
> For example, in next example you move "rsi -> rdi:rsi". I assume that 
> destination registers contains length in rdi and ptr to array base in 
> rsi. Correct?
> 
> rsi, rdx, rcx, r8, r9, rdi, [0], [8]
> rdi, rsi, rdx, rcx, r8, r9, [0], [8], [16]
> [8] -> [16], [0] -> [8], rdi -> [0], r9 -> r9, r8 -> r8, rcx -> rcx, rdx 
> -> rdx, rsi -> rdi:rsi
>    1744   16     n 0       JNITest::nativeTest1([BIIIIIII)V (0 bytes)   
> (static)
> 
> In next example first register is matched for second array : rdx -> 
> rdx:rcx. So what is assembler for it again?
> 
> rsi, rdx, rcx, r8, r9, rdi, [0], [8]
> rdi, rsi, rdx, rcx, r8, r9, [0], [8], [16], [24]
> [8] -> [24], [0] -> [16], rdi -> [8], r9 -> [0], r8 -> r9, rcx -> r8, 
> rdx -> rdx:rcx, rsi -> rdi:rsi
>    1746   18     n 0       JNITest::nativeTest3([B[BIIIIII)V (0 bytes)   
> (static)
> 
> Thanks,
> Vladimir
> 
> 
> Tom Rodriguez wrote:
>> On Feb 24, 2012, at 4:21 PM, Tom Rodriguez wrote:
>>
>>> On Feb 24, 2012, at 12:14 PM, Vladimir Kozlov wrote:
>>>
>>>> Tom,
>>>>
>>>> I don't get T_ARRAY argument processing in ComputeMoveOrder(). 
>>>> T_ARRAY has two out arguments length and array pointer so two moves 
>>>> are involved. I don't see how code handles it.
>>> Yes it's a little odd.  I had convinced myself that because the 
>>> arguments locations and just rotated relative to each other that only 
>>> one argument could collide any testing indicated that it worked 
>>> without killing anything.  Now I'm not so sure.  I'm going to think 
>>> through it a bit more and maybe expand my test a bit.
>>
>> So the main part of the deals with partial overlap between the source 
>> register and destinations.  For single moves if the source and dest 
>> are the same then no edge is created but the array moves have to do 
>> something different.  Because of the argument ordering in almost all 
>> cases there is partial overlap between s and d1,d2.  In the partial 
>> overlap case picking the second register if the source and the first 
>> are the same will accurately capture the dependence ordering.  The 
>> only case where they are fully disjoint occurs with an array argument 
>> in rcx being moved into r8/r9 and in that case r8 and r9 are being 
>> moved to the stack so they are free.  I attached JVM output showing 
>> the argument shuffle order for all the signatures generated by the test.
>>
>> It's not a completely general algorithm but it's adequate for the 
>> usage.  The moves assembly code actually validates that a register 
>> isn't killed before being used it would have tripped assert is there 
>> were any issues.
>>
>> I could do something different to be more general but I can't see an 
>> easy way to represent this using the code I've written.
>>
>> tom
>>
>>
>>
>>
>> ------------------------------------------------------------------------
>>
>>
>>>> I think next code in break_cycle() could be simpler since it is 
>>>> cycle chain:
>>>>
>>>> 1416       MoveOperation* p = prev();
>>>> 1417       if (p->_next != NULL) {
>>>> 1418         p->_next->_prev = NULL;
>>>> 1419       }
>>>>
>>>> as this:
>>>>
>>>> 1416       MoveOperation* p = prev();
>>>> 1417       assert(p->_next == this, "");
>>>> 1418       _prev = NULL;
>>> You're right.  I had refactored the code so that the calling context 
>>> changed but didn't simplify it in  that way.   I'll fix it.
>>>
>>>> Why you don't want new test to be executed during regular testing? 
>>>> You used "manual" and double "##". Did you try to run it with jtreg.
>>> jtreg doesn't support compiling libraries as part of a test so it has 
>>> to be run by hand.  By default jtreg will run manual tests unless you 
>>> specify -automatic.  I kind of assumed that's how it's being run 
>>> during nightlies but maybe that's not true.  Maybe it's not 
>>> appropriate for a jtreg test?
>>>
>>> tom
>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> Tom Rodriguez wrote:
>>>>> http://cr.openjdk.java.net/~never/7145024
>>>>> 489 lines changed: 451 ins; 22 del; 16 mod; 3645 unchg
>>>>> 7145024: Crashes in ucrypto related to C2
>>>>> Reviewed-by:
>>>>> There are two issues here.  The first issue, that resulted in the
>>>>> asserts was incorrect incrementing of the slots when building the oop
>>>>> map.  Fixing that exposed the second issue which is that different
>>>>> signatures may require very different orders for the move to avoid
>>>>> clobbering other arguments.  There's no simple way to order them
>>>>> safely so I resurrected some old C1 code for computing a safe order
>>>>> for issuing stores and break any cycles in those stores.  The code
>>>>> itself is fairly general but it's not necessary on the other platforms
>>>>> so I kept it in the platform dependent code instead of moving it into
>>>>> a shared file.  Tested with a new manual test that exercises all
>>>>> permutations of 8 arguments that mix primtives and arrays on Solaris,
>>>>> Linux and Windows.  Also tested with failing
>>>>> javasoft.sqe.tests.api.java.security.Signature.updateTests test.
>>