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

[John Rose]

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?

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 agree with your other points.)

-- John