review (M) for 4926272: methodOopDesc::method_from_bcp is unsafe

[Vladimir Kozlov]

Looks good.

Vladimir

Tom Rodriguez wrote:
> On Jan 11, 2011, at 3:51 PM, John Rose wrote:
> 
>> On Jan 11, 2011, at 2:31 PM, Tom Rodriguez wrote:
>>
>>> http://cr.openjdk.java.net/~never/4926272
>>> 4926272: methodOopDesc::method_from_bcp is unsafe
>> This is a painful but long-needed cleanup.  There is lots of good code deletion.  Thank you.
> 
> I've updated the webrev wit the following changes.  It's now always method, bcp/bci.  I removed methodOop from a few minor places where it wasn't needed.  I changed bytecode to extract the Code in the constructor and made a few changes in Bytescodes to allow this.  This would allow it work with breakpoints in the CI.  I changed Bytecode_loadconstant and Bytecode_member_ref to inherit from Bytecode and removed some redundant/unneeded fields and methods.  I added constructors that take a ciBytecodeStream* to build the CI versions of the Bytecode and placed the definitions in ciStreams.hpp so they could be inlined.  Moved the must_rewrite logic into Bytecodes with the can_rewrite stuff.  Added java_code_at and code_at to methodOop.  Renamed Bytecode_invoke_at_check to Bytecode_invoke_check.
> 
> tom
> 
>> Bytecode_member_ref and Bytecode_loadconstant would look better as subclasses of Bytcode also.  Was there a mismatch somewhere?  Suggest pushing the Bytecode superclass down into those guys.
>>
>> That's an economical move passing methodOop(NULL) from the CI.
>>
>> The two binary overloadings of Bytecodes::code_at have swapped arg-order; this smells worse now.  Can something be done?  Maybe just put the (now required) methodOop first always or (better) second always?  Wait, I guess I see the convention:  bcp/oop vs. oop/bci.  (Seen in Bytecode constructors and various accessors in Bytecodes::.)  That is an uncomfortable way to deal with the bci/bcp duality, which might be worth fixing now.  Fixing it might double your (already considerable) work.  OTOH, fixing that might just touch use points you are already touching now anyway.  Suggest moving the methodOop after the bcp/bci in all touched API points and immediate neighbors.
>>
>> The name Bytecode_invoke_at_check doesn't read so well anymore; it is not part of a pattern.  Suggest Bytecode_invoke_check.
>>
>> Reviewed!
>>
>> -- John
>