review for 7145024: Crashes in ucrypto related to C2

[Tom Rodriguez]

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


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: argorder.txt
Url: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20120227/e14fbb71/argorder-0001.txt 
-------------- next part --------------

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