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

Markus Grönlund markus.gronlund at oracle.com
Wed Apr 2 18:41:31 UTC 2014

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/







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



  Excellent finding and fix. I have question:

324 StackValueCollection* interpretedVFrame::expressions() const {
 326   int length = 0;
 327   InterpreterOopMap oop_mask;    <<<< ------------------ default initialize will set _expression_stack_size
 to 0.
 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     }
 338     length = oop_mask.expression_stack_size();        <<<<<-------------------------- 1) 
 339   }
 341   StackValueCollection* result = new StackValueCollection(length);
 343   if (0 == length) {
 344     return result;
 345   }
 347   int nof_locals = method()->max_locals();
 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)
 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?


On 4/2/2014 7: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: HYPERLINK "http://cr.openjdk.java.net/%7Emgronlun/8038624/webrev01/"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 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:





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...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20140402/eedb49c5/attachment.html>

More information about the hotspot-runtime-dev mailing list