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

[Yumin Qi]

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/ac926bbf/attachment-0001.html>