review for 7047961: JSR 292 MethodHandleWalk swap args doesn't handle T_LONG and T_DOUBLE properly

[Tom Rodriguez]

Thanks!

tom

On May 25, 2011, at 2:33 PM, Vladimir Kozlov wrote:

> Yes, it looks good.
> 
> Thanks,
> Vladimir
> 
> Tom Rodriguez wrote:
>> On May 25, 2011, at 12:59 PM, Vladimir Kozlov wrote:
>>> Why you used DEBUG instead of ASSERT? For fastdebug build DEBUG is not defined.
>> I fixed that.
>>> Not all ctor initialize ArgToken._value field.
>> In those cases the value field isn't/shouldn't be used.  Same with handle.  The accessors for those values also check that they are the right token type which ensures that they are only accessed if they have been initialized.
>>> Could you add comment to next check because it looks strange since it narrow error cases? In other words, for example, why it is OK to have bad argument index with conv_op <= OP_RETYPE_RAW:
>>> 
>>> +       if ((arg_slot < 0 || arg_slot >= _outgoing.length()) &&
>>> +           conv_op > java_lang_invoke_AdapterMethodHandle::OP_RETYPE_RAW &&
>>> +           conv_op != java_lang_invoke_AdapterMethodHandle::OP_COLLECT_ARGS &&
>>> +           conv_op != java_lang_invoke_AdapterMethodHandle::OP_FOLD_ARGS) {
>>> +         lose(err_msg("bad argument index %d", arg_slot), CHECK_(empty));
>>>       }
>> Because it isn't bad in that case.  ;)  Is this ok:
>>     // Check that the arg_slot is valid.  In most cases it must be                                                                                       // within range of the current arguments but there are some                                                                                          // exceptions.  Those are sanity checked in their implemention                                                                                        // below.                                                                                                                                            if ((arg_slot < 0 || arg_slot >= _outgoing.length()) &&
>>          conv_op > java_lang_invoke_AdapterMethodHandle::OP_RETYPE_RAW &&
>>          conv_op != java_lang_invoke_AdapterMethodHandle::OP_COLLECT_ARGS &&
>>          conv_op != java_lang_invoke_AdapterMethodHandle::OP_FOLD_ARGS) {
>>        lose(err_msg("bad argument index %d", arg_slot), CHECK_(empty));
>>      There really should be more sharing between the sanity checks in methodHandles.cpp and methodHandleWalk but I think that's for another day.
>>> verify_args_and_signature() should be under "ifdef ASSERT".
>> ok.  The webrev is updated.
>> tom
>>> Vladimir
>>> 
>>> Tom Rodriguez wrote:
>>>> http://cr.openjdk.java.net/~never/7047961
>>>> 316 lines changed: 213 ins; 27 del; 76 mod; 5061 unchg
>>>> 7047961: JSR 292 MethodHandleWalk swap args doesn't handle T_LONG and T_DOUBLE properly
>>>> Reviewed-by:
>>>> Several more errors were found with handling op arguments in
>>>> MethodHandleWalk.  swap wasn't dealing with the double word arguments
>>>> properly.  collect and fold are allowed to have 0 length operations.
>>>> rotate was using the wrong type to determine the size of the
>>>> operation.  boxing operations are allowed on subword types.  nop
>>>> conversion operations should just do nothing.  I also removed the
>>>> SlotState wrapper since its only purpose seemed to be to keep track of
>>>> the arg types when symbolic was used and I modified tt_symbolic to
>>>> keep accurate track of the type instead of claiming to be a long.  I
>>>> also added more detail to most of the failure messages and fixed a
>>>> missing ResourceMark along with adding a lot more sanity checks.
>>>> Tested with vm/mlvm and java/lang/invoke regression tests.