RFR(S): 8038624:interpretedVFrame::expressions() must respect InterpreterOopMap for liveness
coleen.phillimore at oracle.com
Fri Apr 4 16:02:26 UTC 2014
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).
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
> 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.
> *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
> 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
> 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
> On 4/2/14 10:29 AM, Markus Grönlund wrote:
> Kindly asking for reviews for the following change:
> Bug(s): http://bugs.openjdk.java.net/browse/JDK-8038624
> Webrev: http://cr.openjdk.java.net/~mgronlun/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
> 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:
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the hotspot-runtime-dev