RFR(S): 8038624:interpretedVFrame::expressions() must respect InterpreterOopMap for liveness

[Coleen Phillimore]

Hi Markus,
I double checked this myself and I agree with you.   Your fix to this 
code looks really good.
Before you check this in, can you add a short comment before line 338 
why this doesn't equal fr().interpreter_frame_expression_stack_size();
in case someone wanders into this complicated code again?  The comment 
below in your RFR is fine (I don't need to read it again).
Thanks!
Coleen

On 4/4/14 11:56 AM, Markus Grönlund wrote:
>
> Hi Coleen,
>
> Deoptimization does not use this code for anything critical -- 
> deoptimization is based on compiledVFrame::expressions(), not 
> InterpretedVFrame::expressions().
>
> After deopting the compiled frame to an interpreter frame, there is an 
> debug section in vframeArrayElement::unpack_on_stack (#ifndef PRODUCT 
> which will call InterpretedVFrame::expressions() if you have set the 
> TraceDeoptimization && Verbose), which is used for outputting debug 
> information only.
>
> Thanks
>
> Markus
>
> *From:*Coleen Phillimore
> *Sent:* den 2 april 2014 22:57
> *To:* hotspot-runtime-dev at openjdk.java.net; 
> hotspot-compiler-dev at openjdk.java.net
> *Subject:* Re: RFR(S): 8038624:interpretedVFrame::expressions() must 
> respect InterpreterOopMap for liveness
>
>
> Markus,
>
> This is really disturbing because deoptimization uses this code to 
> layout the interpreter frame (unpack_on_stack).  Bugs in 
> deoptimization can be very subtle but we would see some bugs if this 
> was wrong, I think.
> Is this a recent failure and there was a change in some other area 
> that made this crash?
> I don't know why interpreter_frame_stack_size wouldn't be the same as 
> oopmap.expression_stack_size() unless it doesn't include the pushed 
> arguments?
>
> There's some odd code I've never understood for deoptimization in 
> interpreter/interpreter.cpp.   I think you should have someone from 
> the compiler group (added) who understands deoptimization review your 
> change.
>
> Coleen
>
> On 4/2/14 10:29 AM, Markus Grönlund wrote:
>
>     Greetings,
>
>     Kindly asking for reviews for the following change:
>
>     Bug(s): http://bugs.openjdk.java.net/browse/JDK-8038624
>
>     https://bugs.openjdk.java.net/browse/JDK-8038344
>
>     Webrev: http://cr.openjdk.java.net/~mgronlun/8038624/webrev01/
>     <http://cr.openjdk.java.net/%7Emgronlun/8038624/webrev01/>
>
>     Problem description:
>
>     An InterpreterOopMap for a particular bci position does not
>     include expression/operand stack liveness info in the
>     oop_mask/bit_mask if the bci is a call instruction, i.e. for the
>     invoke* instructions (invokevirtual, invokespecial, invokestatic,
>     invokedynamic, invokeinterface).
>
>     This leads to a discrepancy between what is actually on the
>     expression/operand stack (given via
>     fr().interpreter_frame_expression_stack_size()) and what is given
>     in the liveness oop_mask/bit_mask (given via InterpreterOopMap) at
>     a particular bci.
>
>     The code in interpretedVFrame::expressions() is currently based on
>     information given from
>     fr().interpreter_frame_expression_stack_size(), and will index
>     into the retrieved oop_mask/bit_mask based on this information
>     (expression slot nr + _max_locals). These indexes either:
>
>     1. Fetches a 0 (since no live info at that position in the mask)
>     if the index is low enough to still be inside the bit_mask word
>     boundary. It will then proceed to treat the expression slot (which
>     might be a real reference) as a T_INT (0 is a value, 1 is a
>     reference)
>
>     2. Indexes out of bounds for the oop_map/bit_mask (see
>     https://bugs.openjdk.java.net/browse/JDK-8038344 ), and picks up
>     information outside that is not related to a liveness bit mask. If
>     that position happens to yield a 1, but the real expression slot
>     is a value ("v"), the system can assert "(obj->is_oop()) failed:
>     not an oop: 0x00000001"
>
>     Tested by running:
>
>     nsk/jdi/*
>
>     Other info:
>
>     I dislike having to create a new StackValueCollection even though
>     I know the length is 0 and it will not be actively used. However,
>     this pattern of always creating and returning empty objects is
>     prevalent in this piece of code and is not easily detangled.
>
>     Thanks in advance
>
>     Markus
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20140404/17ecabd3/attachment-0001.html>