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

[Tom Rodriguez]

On Jan 3, 2011, at 5:15 PM, Coleen Phillimore wrote:

> 
> 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.

The existing code would crash and die if we ever exercised it for compiler code.  If we ever start supporting breakpoints in compiled code then we'd need to address that.

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

It's a lot of change but it's all straightforward change.

> 
> 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().

Well there's a permanent space overhead from your hack and I'm unclear why 0xffffffff is never a valid value in the bytecode stream.  u4s can appear in the bytecodes and 0xffffffff seems like a valid u4 value or even a pair of u2s.  It seems like a bad assumption to me and it's certainly not checked anywhere.

http://javaweb.sfbay.sun.com/~never/webrev/bcp is a version that always passes around the methodOop and converts Bytecode into a ResourceObj.  It might be nicer as a StackObj I think but that requires more changes at the uses.  Many of the other Bytecode objects are ResourceObj as well so in some ways it's more consistent.  I passed NULL for the CI uses since passing the real methodOop is problematic.  I had to add a couple ResourceMarks in InterpreterRuntime but it seems to run fine otherwise.

tom

> Actually I missed one that we could have passed methodOop which would have made the bug really hard to reproduce.
> 
> Thanks,
> Coleen
> 
> 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.
>> 
>> 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 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.
>>>> 
>>>> Thanks,
>>>> Coleen
>>>> 
>