<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type content="text/html; charset=iso-8859-1"><meta name=Generator content="Microsoft Word 12 (filtered medium)"><style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri","sans-serif";
color:black;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0cm;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";
color:black;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;
color:black;}
span.removed
{mso-style-name:removed;}
span.EmailStyle20
{mso-style-type:personal;
font-family:"Calibri","sans-serif";
color:windowtext;}
span.EmailStyle21
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:70.85pt 70.85pt 70.85pt 70.85pt;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]--></head><body bgcolor=white lang=SV link=blue vlink=purple><div class=WordSection1><p class=MsoNormal><span style='color:#1F497D'>Hi Yumin,<o:p></o:p></span></p><p class=MsoNormal><span style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'>Thanks for taking a look.<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'>For 1,<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'>Thanks for pointing out the default initialization step. I have updated the code to use it.<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'>For 2,<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'>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) *<b>before</b>* the seeked bci is executed. For these, the fr().interpreter_frame_expression_stack_size() should == length.<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'>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.<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'>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)<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'>I have updated the webrev to check this invariant in an assertion as well.<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'>Updated webrev: <a href="http://cr.openjdk.java.net/~mgronlun/8038624/webrev02/">http://cr.openjdk.java.net/~mgronlun/8038624/webrev02/</a><o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'>Thanks<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'>Markus<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'><o:p> </o:p></span></p><div><div style='border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0cm 0cm 0cm'><p class=MsoNormal><b><span lang=EN-US style='font-size:10.0pt;font-family:"Tahoma","sans-serif";color:windowtext'>From:</span></b><span lang=EN-US style='font-size:10.0pt;font-family:"Tahoma","sans-serif";color:windowtext'> Yumin Qi <br><b>Sent:</b> den 2 april 2014 19:33<br><b>To:</b> Markus Grönlund; serviceability-dev@openjdk.net; hotspot-runtime-dev<br><b>Subject:</b> Re: RFR(S): 8038624:interpretedVFrame::expressions() must respect InterpreterOopMap for liveness<o:p></o:p></span></p></div></div><p class=MsoNormal><o:p> </o:p></p><p class=MsoNormal>Markus,<br><br> Excellent finding and fix. I have question:<br><br><br><o:p></o:p></p><pre>324 StackValueCollection* interpretedVFrame::expressions() const {<o:p></o:p></pre><pre> 325 <o:p></o:p></pre><pre> 326 int length = 0;<o:p></o:p></pre><pre> 327 InterpreterOopMap oop_mask; <<<< ------------------ default initialize will set _expression_stack_size<o:p></o:p></pre><pre> to 0.<o:p></o:p></pre><pre> 328 <o:p></o:p></pre><pre> 329 if (!method()->is_native()) {<o:p></o:p></pre><pre> 330 // Get oopmap describing oops and int for current bci<o:p></o:p></pre><pre> 331 if (TraceDeoptimization && Verbose) {<o:p></o:p></pre><pre> 332 methodHandle m_h(method());<o:p></o:p></pre><pre> 333 OopMapCache::compute_one_oop_map(m_h, bci(), &oop_mask);<o:p></o:p></pre><pre> 334 } else {<o:p></o:p></pre><pre> 335 method()->mask_for(bci(), &oop_mask);<o:p></o:p></pre><pre> 336 }<o:p></o:p></pre><pre> 337 <o:p></o:p></pre><pre> 338 length = oop_mask.expression_stack_size(); <<<<<-------------------------- 1) <o:p></o:p></pre><pre> 339 }<o:p></o:p></pre><pre> 340 <o:p></o:p></pre><pre> 341 StackValueCollection* result = new StackValueCollection(length);<o:p></o:p></pre><pre> 342 <o:p></o:p></pre><pre> 343 if (0 == length) {<o:p></o:p></pre><pre> 344 return result;<o:p></o:p></pre><pre> 345 }<o:p></o:p></pre><pre> 346 <o:p></o:p></pre><pre> 347 int nof_locals = method()->max_locals();<o:p></o:p></pre><pre> 348 <o:p></o:p></pre><pre> 349 // handle expressions<o:p></o:p></pre><pre> 350 for(int i=0; i < length; i++) { <<<<------------------------------------------------------2)<o:p></o:p></pre><pre> 351 // Find stack location<o:p></o:p></pre><pre> 352 intptr_t *addr = fr().interpreter_frame_expression_stack_at(i); <<<<------------------- 2)<o:p></o:p></pre><pre> 353 <o:p></o:p></pre><pre> 354 // Depending on oop/int put it in the right package<o:p></o:p></pre><pre> 355 StackValue *sv;<o:p></o:p></pre><pre> 356 if (oop_mask.is_oop(i + nof_locals)) {<o:p></o:p></pre><pre> 357 // oop value<o:p></o:p></pre><pre> 358 Handle h(*(oop *)addr);<o:p></o:p></pre><pre> 359 sv = new StackValue(h);<o:p></o:p></pre><pre> 360 } else {<o:p></o:p></pre><pre> 361 // integer<o:p></o:p></pre><pre> 362 sv = new StackValue(*addr);<o:p></o:p></pre><pre> 363 }<o:p></o:p></pre><pre> 364 assert(sv != NULL, "sanity check");<o:p></o:p></pre><pre> 365 result->add(sv);<o:p></o:p></pre><pre> 366 }<o:p></o:p></pre><pre> 367 return result;<o:p></o:p></pre><pre> 368 }<o:p></o:p></pre><p class=MsoNormal><br>For 1) and 2), the length may not be consistent, this is from your analysis from JIRA, what if <o:p></o:p></p><pre><span class=removed><span style='color:brown'>interpreter_frame_expression_stack_size() is different from 'length'? Is there a possibility out of bound here?<o:p></o:p></span></span></pre><pre><span class=removed><span style='color:brown'><o:p> </o:p></span></span></pre><pre><span class=removed><span style='color:brown'>Thanks<o:p></o:p></span></span></pre><pre><span class=removed><span style='color:brown'>Yumin<o:p></o:p></span></span></pre><pre><span class=removed><span style='color:brown'><o:p> </o:p></span></span></pre><pre><span class=removed><span style='color:brown'><o:p> </o:p></span></span></pre><p class=MsoNormal> <br> <o:p></o:p></p><div><p class=MsoNormal>On 4/2/2014 7:29 AM, Markus Grönlund wrote:<o:p></o:p></p></div><blockquote style='margin-top:5.0pt;margin-bottom:5.0pt'><p class=MsoNormal>Greetings,<o:p></o:p></p><p class=MsoNormal> <o:p></o:p></p><p class=MsoNormal><span lang=EN-US>Kindly asking for reviews for the following change:</span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US> </span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US>Bug(s): <a href="http://bugs.openjdk.java.net/browse/JDK-8038624">http://bugs.openjdk.java.net/browse/JDK-8038624</a> </span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US><a href="https://bugs.openjdk.java.net/browse/JDK-8038344">https://bugs.openjdk.java.net/browse/JDK-8038344</a> </span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US> </span><o:p></o:p></p><p class=MsoNormal>Webrev: <a href="http://cr.openjdk.java.net/%7Emgronlun/8038624/webrev01/">http://cr.openjdk.java.net/~mgronlun/8038624/webrev01/</a> <o:p></o:p></p><p class=MsoNormal> <o:p></o:p></p><p class=MsoNormal><span lang=EN-US>Problem description:</span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US> </span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US>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).</span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US> </span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US>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.<br><br>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: <br><br>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) <br><br>2. Indexes out of bounds for the oop_map/bit_mask (see <a href="https://bugs.openjdk.java.net/browse/JDK-8038344">https://bugs.openjdk.java.net/browse/JDK-8038344</a> ), 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”</span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US> </span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US>Tested by running:</span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US> </span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US>nsk/jdi/*</span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US> </span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US> </span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US>Other info:</span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US> </span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US>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.</span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US> </span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US> </span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US>Thanks in advance</span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US>Markus</span><o:p></o:p></p></blockquote><p class=MsoNormal><span style='font-size:12.0pt;font-family:"Times New Roman","serif"'><o:p> </o:p></span></p></div></body></html>