review for 7145024: Crashes in ucrypto related to C2

[Vladimir Kozlov]

I looked on code in unpack_array_argument() and see that it should work for all 
cases since you are using tmp register to keep value. So it does not matter if 
it moves rsi -> rdi:rsi or rdi -> rdi:rsi for array argument.

Sorry for noise. Code looks good.

Thanks,
Vladimir

Vladimir Kozlov wrote:
> 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.
>>>