review for 7145024: Crashes in ucrypto related to C2

[Tom Rodriguez]

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.

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