Request for review (S) 4926272: methodOopDesc::method_from_bcp is unsafe

Coleen Phillimore coleen.phillimore at
Mon Jan 3 17:15:29 PST 2011

So now I remember why this bug starts with 49*.  I added methodOop as a 
parameter to many of the code_at() calls a long time ago, which made 
this stop crashing all the time but found that the fan out from changes 
from adding methodOop to code_at, length_at, special_length_at, 
java_code_at, just in bytecodes is huge.   And that's before rewriting 
this Bytecode class to be a resource object with a methodHandle saved in 
it.  That also didn't take into account that we'd have to transition 
into the VM when we have a ciMethod for some of the calls, although I 
think you're saying we don't need to do that.

Anyways, the whole change to add methodOop seems like a lot of overhead 
and changes for a rare case.

The fix I have is a "hack" but it's pretty easy to understand and is 
localized to the few cases we call method_from_bcp().  Actually I missed 
one that we could have passed methodOop which would have made the bug 
really hard to reproduce.


On 1/3/2011 6:27 PM, Tom Rodriguez wrote:
> On Jan 3, 2011, at 1:38 PM, Coleen Phillimore wrote:
>> Ramki and Keith pointed out that there is a non-zero probability that constMethodKlass's address could look like a set of bytecodes so we could fail to search up to the top of the constMethodOop type.  A new cleaner webrev which solves the problem for when there is no permgen is available here.   This was tested with 32 and 64 bit with the failing testcases and -XX:+UseConcMarkSweepGC.  Please review.
>> open webrev at
> That seems like a hack to support an existing hack.  Couldn't we remove the underlying assumption that requires converting from bcp to method?  If we were writing this from scratch and it was proposed to do it this way, I think everyone would rightfully gag.  All uses of a bcp should have the methodOop itself nearby, so it's only a matter of passing it in as needed.  All the calls in Bytecodes can trivially have a methodOop argument added.  The harder/uglier piece is Bytecode flyweight objects in bytecode.hpp.  These would have to be converted into ResourceObj or StackObj and keep around a pointer to the methodOop. There's also the oddity that when the CI uses these routines the bcp doesn't actually point into the underlying methodOop but in those cases the conversion back to methodOop will never be required because we don't have any breakpoint bytecodes in the CI copy.
> tom
>> Thanks,
>> Coleen
>> On 12/29/2010 6:18 PM, Coleen Phillimore wrote:
>>> Summary: Instead of using GC to find the block start, scan backward in memory to known contents in the constMethodOop
>>> open webrev at
>>> bug link at
>>> Tested on 32 and 64 bit platforms with the test cases that reliably failed.
>>> Thanks,
>>> Coleen

More information about the hotspot-runtime-dev mailing list