RFR(S): 8038624:interpretedVFrame::expressions() must respect InterpreterOopMap for liveness
Yumin Qi
yumin.qi at oracle.com
Wed Apr 2 19:23:53 UTC 2014
Looks good to me. (not a reviewer:-) )
Thanks for the explain.
Yumin
On 4/2/2014 11:41 AM, Markus Grönlund wrote:
>
> Hi Yumin,
>
> Thanks for taking a look.
>
> For 1,
>
> Thanks for pointing out the default initialization step. I have
> updated the code to use it.
>
> For 2,
>
> From what I have seen so far, the bci liveness information in the
> bit_mask for the expression stack is, for all bytecodes (except
> invoke*), reflecting the current top-of-stack state (tos) **before**
> the seeked bci is executed. For these, the
> fr().interpreter_frame_expression_stack_size() should == length.
>
> For invoke* instructions, the bci liveness information in the bit_mask
> reflects the current top-of-stack state *after* the seeked bci is
> executed. Here, fr().interpreter_frame_expression_stack_size() will
> most likely be > length.
>
> This means indexing into via
> fr().interpreter_frame_expression_stack_at(i) should be safe ( index
> variable will index subset or equal to real expression stack len -1)
>
> I have updated the webrev to check this invariant in an assertion as well.
>
> Updated webrev: http://cr.openjdk.java.net/~mgronlun/8038624/webrev02/
> <http://cr.openjdk.java.net/%7Emgronlun/8038624/webrev02/>
>
> Thanks
>
> Markus
>
> *From:*Yumin Qi
> *Sent:* den 2 april 2014 19:33
> *To:* Markus Grönlund; serviceability-dev at openjdk.net; hotspot-runtime-dev
> *Subject:* Re: RFR(S): 8038624:interpretedVFrame::expressions() must
> respect InterpreterOopMap for liveness
>
> Markus,
>
> Excellent finding and fix. I have question:
>
>
> 324 StackValueCollection* interpretedVFrame::expressions() const {
> 325
> 326 int length = 0;
> 327 InterpreterOopMap oop_mask; <<<< ------------------ default initialize will set _expression_stack_size
> to 0.
> 328
> 329 if (!method()->is_native()) {
> 330 // Get oopmap describing oops and int for current bci
> 331 if (TraceDeoptimization && Verbose) {
> 332 methodHandle m_h(method());
> 333 OopMapCache::compute_one_oop_map(m_h, bci(), &oop_mask);
> 334 } else {
> 335 method()->mask_for(bci(), &oop_mask);
> 336 }
> 337
> 338 length = oop_mask.expression_stack_size(); <<<<<-------------------------- 1)
> 339 }
> 340
> 341 StackValueCollection* result = new StackValueCollection(length);
> 342
> 343 if (0 == length) {
> 344 return result;
> 345 }
> 346
> 347 int nof_locals = method()->max_locals();
> 348
> 349 // handle expressions
> 350 for(int i=0; i < length; i++) { <<<<------------------------------------------------------2)
> 351 // Find stack location
> 352 intptr_t *addr = fr().interpreter_frame_expression_stack_at(i); <<<<------------------- 2)
> 353
> 354 // Depending on oop/int put it in the right package
> 355 StackValue *sv;
> 356 if (oop_mask.is_oop(i + nof_locals)) {
> 357 // oop value
> 358 Handle h(*(oop *)addr);
> 359 sv = new StackValue(h);
> 360 } else {
> 361 // integer
> 362 sv = new StackValue(*addr);
> 363 }
> 364 assert(sv != NULL, "sanity check");
> 365 result->add(sv);
> 366 }
> 367 return result;
> 368 }
>
>
> For 1) and 2), the length may not be consistent, this is from your
> analysis from JIRA, what if
>
> interpreter_frame_expression_stack_size() is different from 'length'? Is there a possibility out of bound here?
>
> Thanks
> Yumin
>
>
>
>
> On 4/2/2014 7: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-runtime-dev/attachments/20140402/3e6fe066/attachment-0001.html>
More information about the hotspot-runtime-dev
mailing list