<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Since:<div><br></div><div><div style="margin: 0px; font-size: 11px; font-family: Monaco;">  <span style="color: #931a68">int</span>               <span style="color: #0326cc">_interp_only_mode</span>;</div><div><br></div><div>is an int field I would prefer to actually read the value as an int instead of just a byte on x86:</div><div><pre><span class="new" style="color: blue;">+    __ cmpb(Address(r15_thread, JavaThread::interp_only_mode_offset()), 0);</span>
</pre></div><div>Otherwise this looks good.</div><div><div><br></div><div>On May 13, 2014, at 11:30 AM, Staffan Larsen <<a href="mailto:staffan.larsen@oracle.com">staffan.larsen@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On 13 maj 2014, at 18:53, Daniel D. Daugherty <<a href="mailto:daniel.daugherty@oracle.com">daniel.daugherty@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">
  
    <meta content="text/html; charset=windows-1252" http-equiv="Content-Type">
  
  <div bgcolor="#FFFFFF" text="#000000">
    <tt>> new webrev is here:
      <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~sla/8041934/webrev.02/">http://cr.openjdk.java.net/~sla/8041934/webrev.02/</a><br>
      <br>
    </tt><tt>src/share/vm/runtime/sharedRuntime.hpp<br>
      </tt><tt>    No comments.<br>
      <br>
    </tt><tt><tt><tt>src/share/vm/runtime/sharedRuntime.cpp<br>
              No comments.<br>
          <br>
        </tt></tt>src/cpu/sparc/vm/sharedRuntime_sparc.cpp<br>
          No comments.<br>
      <br>
      src/cpu/x86/vm/sharedRuntime_x86_32.cpp<br>
          No comments.<br>
      <br>
      src/cpu/x86/vm/sharedRuntime_x86_64.cpp<br>
          No comments.<br>
      <br>
      On the switch from call_VM_leaf() -> call_VM(), I looked
      through all<br>
      the mentions of the string call_VM in the three src/cpu files.
      Your<br>
      change adds the first call_VM() use in all three files and the
      other<br>
      places that mention the string "call_VM" seem to have gone through<br>
      a great deal of trouble not to use call_VM() directly. I have no<br>
      specific thing I think is wrong, but I find this data worrisome…</tt></div></blockquote><div><br></div><div>Yes, it would be great if someone from the compiler team could review this, too.</div><div><br></div><div>Thanks,</div><div>/Staffan</div><br><blockquote type="cite"><div bgcolor="#FFFFFF" text="#000000"><tt>
      <br>
      Thumbs up!<br>
      <br>
      Dan<br>
      <br>
      <br>
    </tt>
    <div class="moz-cite-prefix">On 5/13/14 3:20 AM, Staffan Larsen
      wrote:<br>
    </div>
    <blockquote cite="mid:C6B702AE-DCB5-4E11-8879-D8540E3E969A@oracle.com" type="cite">
      <meta http-equiv="Content-Type" content="text/html;
        charset=windows-1252">
      <br>
      <div>
        <div>On 9 maj 2014, at 20:18, <a moz-do-not-send="true" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
          wrote:</div>
        <br class="Apple-interchange-newline">
        <blockquote type="cite">
          <meta content="text/html; charset=windows-1252" http-equiv="Content-Type">
          <div text="#000000" bgcolor="#FFFFFF">
            <div class="moz-cite-prefix">Staffan,<br>
              <br>
              This is important discovery, thanks!<br>
              The fix looks good to me.<br>
              One question below.<br>
              <br>
              Thanks,<br>
              Serguei<br>
              <br>
              <br>
              On 5/9/14 3:47 AM, Staffan Larsen wrote:<br>
            </div>
            <blockquote cite="mid:2E48F010-CE96-4676-8763-E801CBCF5D00@oracle.com" type="cite">
              <pre wrap="">On 8 maj 2014, at 19:05, Daniel D. Daugherty <a moz-do-not-send="true" class="moz-txt-link-rfc2396E" href="mailto:daniel.daugherty@oracle.com"><daniel.daugherty@oracle.com></a> wrote:

</pre>
              <blockquote type="cite">
                <blockquote type="cite">
                  <pre wrap="">webrev: <a moz-do-not-send="true" class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Esla/8041934/webrev.00/">http://cr.openjdk.java.net/~sla/8041934/webrev.00/</a>
</pre>
                </blockquote>
                <pre wrap="">src/share/vm/runtime/sharedRuntime.hpp
   No comments.

src/share/vm/runtime/sharedRuntime.cpp
   line 994: JRT_LEAF(int, SharedRuntime::jvmti_method_exit(
       I'm not sure that JRT_LEAF is right. I would think that
       JvmtiExport::post_method_exit() would have to grab one or
       more locks with all the state checks it has to make...

       For reference, InterpreterRuntime::post_method_exit()
       is a wrapper around JvmtiExport::post_method_exit()
       and it is IRT_ENTRY instead of IRT_LEAF.
</pre>
              </blockquote>
              <pre wrap="">Good catch!</pre>
            </blockquote>
            <br>
            Q: Is correct to use <span class="new">call_VM_leaf</span>
            <meta http-equiv="content-type" content="text/html;
              charset=windows-1252">
            (vs <span class="new">call_VM</span>
            <meta http-equiv="content-type" content="text/html;
              charset=windows-1252">
            ) in the  sharedRuntime_<arch>.cpp ?<br>
            <br>
            <meta http-equiv="content-type" content="text/html;
              charset=windows-1252">
            <pre><span class="new">+    __ call_VM_leaf(</span>
<span class="new">+         CAST_FROM_FN_PTR(address, SharedRuntime::jvmti_method_exit),</span>
<span class="new">+         thread, rax);</span></pre>
          </div>
        </blockquote>
        <div><br>
        </div>
        <div>That is another good catch! It should probably be call_VM
          as you note. The reason for all these leaf references is
          because we used the dtrace probe as a template - obviously
          without fully understanding what we did :-/</div>
        <div><br>
        </div>
        <div>I have changed to code to use call_VM instead. This also
          required a change from jccb to jcc for the jump (which is now
          longer than an 8-bit offset). </div>
        <div><br>
        </div>
        <div>new webrev is here: <a moz-do-not-send="true" href="http://cr.openjdk.java.net/%7Esla/8041934/webrev.02/">http://cr.openjdk.java.net/~sla/8041934/webrev.02/</a></div>
        <div><br>
        </div>
        <div>
          <div>Thanks,</div>
          <div>/Staffan</div>
          <div><br>
          </div>
        </div>
        <div><br>
        </div>
        <blockquote type="cite">
          <div text="#000000" bgcolor="#FFFFFF"><br>
            <blockquote cite="mid:2E48F010-CE96-4676-8763-E801CBCF5D00@oracle.com" type="cite">
              <blockquote type="cite">
                <pre wrap="">src/cpu/sparc/vm/sharedRuntime_sparc.cpp
   No comments.

src/cpu/x86/vm/sharedRuntime_x86_32.cpp
   No comments.

src/cpu/x86/vm/sharedRuntime_x86_64.cpp
   No comments.

I'm guessing that PPC has the same issue, but I'm presuming
that someone else (Vladimir?) will handle that…
</pre>
              </blockquote>
              <pre wrap="">Yes, I was hoping that I could file a follow-up bug for the platforms I didn’t know how to fix.

Updated review: <a moz-do-not-send="true" class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Esla/8041934/webrev.01/">http://cr.openjdk.java.net/~sla/8041934/webrev.01/</a>

Thanks,
/Staffan

</pre>
              <blockquote type="cite">
                <pre wrap="">Dan


On 5/8/14 12:06 AM, Staffan Larsen wrote:
</pre>
                <blockquote type="cite">
                  <pre wrap="">All,

This is a fix for an assert in JVMTI that verifies that JVMTI’s internal notion of the number of frames on the stack is correct.

When running in -Xcomp mode and enable single-stepping (or method_entry/exit) we will revert all frames on the stack to be run by the interpreter. Only the interpreter can send single-step and method_entry/exit. However, if one of the frames is a native wrapper, that frame will not be reverted (presumably because we don't know how to do that). This will cause us to miss a method_exit event when that native frame is popped. This in turn will mess up the logic in JVMTI that keeps track of the number of frames currently on the stack (see JvmtiThreadState::_cur_stack_depth) and will trigger the assert.

The proposed solution is to include a method_exit event in the native wrapper frame if interpreted mode has been enabled. This needs updates to SharedRuntime::generate_native_wrapper() for all platforms.

Kudos to Rickard for helping me write the code.

webrev: <a moz-do-not-send="true" class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Esla/8041934/webrev.00/">http://cr.openjdk.java.net/~sla/8041934/webrev.00/</a>
bug: <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8041934">https://bugs.openjdk.java.net/browse/JDK-8041934</a>

The fix has been verified by running the failing test in JPRT with -Xcomp.

Thanks,
/Staffan
</pre>
                </blockquote>
              </blockquote>
            </blockquote>
            <br>
          </div>
        </blockquote>
      </div>
      <br>
    </blockquote>
    <br>
  </div>

</blockquote></div><br></div></blockquote></div><br></div></body></html>