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

[Tom Rodriguez]

On Jan 11, 2011, at 4:21 PM, 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.
>> 
>> 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.
> 
> The mismatch is that Bytecode works with bcp but member_ref and loadconstant use bci and I hadn't originally wanted to touch those.  Additionally they use methodHandle instead of methodOop which makes them safer but causes creation of Handles that didn't previously occur.  The use of method/bci also means they can't work with the CI because the bcp is unrelated to the method.  I don't see a really clean way to shoehorn these together without making lots of changes of dubious value.  For the most part these trivial classes and the inheritance relation is primarily implementation inheritance so the benefits of sharing are somewhat minimal.  Anyway, this part feels like a thread I don't want to pull right now.

Actually I might be able to paper over that by the doing early conversion of _breakpoint bytecodes.  Then the methodOop value wouldn't have to be kept in the Bytecode instance and the same Bytecode class could be made to work for both methodOop and ciMethod by just having different constructors.  I'm looking at it now.

tom

> 
>> 
>> That's an economical move passing methodOop(NULL) from the CI.
> 
> ;)  Nice choice of words.  I wanted it to be explicit and I also included asserts to make sure it was done that way.  I'd considered moving the construction of these into the various bytecode streams to hide the details of how they were created.  That doesn't work for some of the uses but it might be an effective way to do it for 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.
> 
> I wasn't happy with this myself.  I'll swap it around to method, bcp.
> 
>> 
>> The name Bytecode_invoke_at_check doesn't read so well anymore; it is not part of a pattern.  Suggest Bytecode_invoke_check.
> 
> Ok.  I didn't like this much but it wasn't easy to clean up the use sites so I left it mostly as is.
> 
> I'll send out mail when I've updated it.  Thanks!
> 
> tom
> 
>> 
>> Reviewed!
>> 
>> -- John
>