review for 7045514: SPARC assembly code for JSR 292 ricochet frames

[Tom Rodriguez]

On Jun 1, 2011, at 7:19 PM, John Rose wrote:

> On Jun 1, 2011, at 6:15 PM, Vladimir Kozlov wrote:
> 
>> Christian is hero to write all this :)
> 
> I agree.  :-)
> 
>> MethodHandles::verify_stack_move() has commented code. Should UNREASONABLE_STACK_MOVE depends on LP64?
> 
> It could.  Tom writes this: // extra-large, XXX large enough?

I had disabled because it was triggering but it didn't seem like there was a real problem.  This was in 32 bit mode I think.  I meant to go back and figure out why it was wrong.  I'll see if I can correct it.

> 
> The key idea is that the most a MH can move the SP is by up to 255 argument positions, assuming an extreme spread or collect operation.
> 
> Since an argument is 4 or 8 bytes, we round up and get the constant.  Could also use something like (MAX_ARITY+FENCE_POST_ERROR)*stackElementSize, but that seems like overkill to me.  The idea is to defend (partially) against garbage values that move the SP by large leaps.
> 
>> Could you move following verification code under first condition?:
>> 
>> +       if (collect_count_constant >= 0) {
>> +         collect_count = collect_count_constant;
>> +       } else {
>> +         __ load_method_handle_vmslots(O1_collect_count, G3_method_handle, O2_scratch);
>> +         collect_count = O1_collect_count;
>> +       }
>> + #ifdef ASSERT
>> +       if (VerifyMethodHandles && collect_count_constant >= 0) {
>> +         BLOCK_COMMENT("verify collect_count_constant {");
>> +         __ load_method_handle_vmslots(O3_scratch, G3_method_handle, O2_scratch);
>> 
>> The same for dest_slot_constant verification.
> 
> Remember that it's also helpful, when reading the code, to see both cases together (constant/nonconstant).  Either way is fine with me.

I kind of prefer it separate given the ordering but maybe if they were swapped then the verify code wouldn't be in the middle of the main logic.  At root I'm happy either way.  Which would you prefer?

tom

> 
> (I agree with your other points.)
> 
> -- John