Request for review (S) 4926272: methodOopDesc::method_from_bcp is unsafe
coleen.phillimore at oracle.com
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 http://cr.openjdk.java.net/~coleenp/4926272_2/
> 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.
>> 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 http://cr.openjdk.java.net/~coleenp/4926272/
>>> bug link at http://bugs.sun.com/view_bug.do?bug_id=4926272
>>> Tested on 32 and 64 bit platforms with the test cases that reliably failed.
More information about the hotspot-runtime-dev