<html>
  <head>
    <meta content="text/html; charset=ISO-8859-1"
      http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    Hi Vladimir,<br>
    <br>
    I have a question concerning the following lines:<br>
    <br>
    <pre><b><font color="blue">+</font><font color="blue">   // Verify IC only when nmethod installation is finished.</font></b>
<b><font color="blue">+</font><font color="blue">   bool is_installed = !(_state == alive &amp;&amp; method()-&gt;code() != this);</font></b>
<b><font color="blue">+</font><font color="blue">   if (is_installed) 


</font></b></pre>
    Wouldn't it be also sufficient to check if a method is installed
    with the following lines?<br>
    <br>
    bool is_installed = method()-&gt;code() == this;<br>
    <br>
    <br>
    Best,<br>
    Albert<br>
    <br>
    <br>
    <div class="moz-cite-prefix">On 11/07/2013 03:50 PM, Vladimir Ivanov
      wrote:<br>
    </div>
    <blockquote cite="mid:527BA899.2040708@oracle.com" type="cite">Vladimir,
      thank you for the feedback.
      <br>
      <br>
      What do you think about the following version?
      <br>
      <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~vlivanov/8023037/webrev.03/">http://cr.openjdk.java.net/~vlivanov/8023037/webrev.03/</a>
      <br>
      <br>
      Best regards,
      <br>
      Vladimir Ivanov
      <br>
      <br>
      On 11/7/13 2:52 AM, Vladimir Kozlov wrote:
      <br>
      <blockquote type="cite">Vladimir,
        <br>
        <br>
        Thank you for finding the real cause.
        <br>
        <br>
        I think you need the comment before nm-&gt;verify() in
        new_nmethod()
        <br>
        explaining why we should avoid safepoint as you explained here.
        <br>
        <br>
        I am not comfortable about passing new allow_safepoints
        parameter
        <br>
        through all verify calls because it is the only one case. We
        still have
        <br>
        not installed nmethod at this point. There should be some check
        <br>
        (nmethod's flags/state) which we can use to skip
        CompiledIC_lock.
        <br>
        <br>
        I thought about the need to verify scopes in
        verify_interrupt_point() in
        <br>
        all cases. But the verification code in ScopeDesc::verify() is
        <br>
        commented. What? In general it is good to call it anyway. And
        you don't
        <br>
        need IC to get the end_of_call() - I think we can inline what
        <br>
        CompiledIC_at() does:
        nativeCall_at(call_site)-&gt;end_of_call().
        <br>
        <br>
        This code calls CompiledIC_at() because it does IC verification
        which we
        <br>
        can skip in our case.
        <br>
        <br>
        Thanks,
        <br>
        Vladimir
        <br>
        <br>
        On 11/6/13 6:57 AM, Vladimir Ivanov wrote:
        <br>
        <blockquote type="cite">Very good point, Vladimir!
          <br>
          <br>
          I should have looked for the bug in a different place :-)
          <br>
          <br>
          The problem is triggered by class redefinition and it can be
          performed
          <br>
          under Compile_lock or during safepoint. So, in
          ciEnv::register_method
          <br>
          it's not enough to grab Compile_lock. In addition, we need to
          ensure
          <br>
          that there are no safepoints possible during method
          installation to
          <br>
          guarantee no dependency is invalidated.
          <br>
          <br>
          There's one place in nmethod verification code (see
          MutexLocker
          <br>
          ml_verify (CompiledIC_lock) in
          nmethod::verify_interrupt_point) where
          <br>
          safepoint is possible and it causes the failure.
          <br>
          <br>
          The bug is present only in non-product builds since nmethod
          verification
          <br>
          is absent in product bits.
          <br>
          <br>
          Previous fix also works, but I decided to go another route and
          forbid
          <br>
          safepoints at all while holding Compile_lock.
          <br>
          <br>
          Updated webrev (will appear once cr.ojn is up):
          <br>
          <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~vlivanov/8023037/webrev.02/">http://cr.openjdk.java.net/~vlivanov/8023037/webrev.02/</a>
          <br>
          <br>
          Best regards,
          <br>
          Vladimir Ivanov
          <br>
          <br>
          On 11/6/13 2:36 AM, Vladimir Kozlov wrote:
          <br>
          <blockquote type="cite">You need to be careful to avoid
            deadlock since there is call:
            <br>
            <br>
            1035&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; old-&gt;make_not_entrant();
            <br>
            <br>
            Any way there is comment in register_method():
            <br>
            <br>
            &nbsp; 936&nbsp;&nbsp;&nbsp;&nbsp; // Prevent SystemDictionary::add_to_hierarchy from
            running
            <br>
            &nbsp; 937&nbsp;&nbsp;&nbsp;&nbsp; // and invalidating our dependencies until we
            install this
            <br>
            method.
            <br>
            &nbsp; 938&nbsp;&nbsp;&nbsp;&nbsp; MutexLocker ml(Compile_lock);
            <br>
            <br>
            So how it happens that the method is invalidated by
            dependencies?
            <br>
            <br>
            Thanks,
            <br>
            Vladimir
            <br>
            <br>
            On 11/5/13 1:52 PM, Vladimir Ivanov wrote:
            <br>
            <blockquote type="cite">Ouch, I was misled by it's name. So,
              I just lowered the likelihood of
              <br>
              the failure then.
              <br>
              <br>
              make_not_entrant_or_zombie holds Patching_lock when it
              patches &amp;
              <br>
              unlinks
              <br>
              a nmethod.
              <br>
              <br>
              Do you see any problems with using it to guard method
              installation
              <br>
              (method-&gt;set_code(method, nm)) in register_method() as
              well?
              <br>
              <br>
              Best regards,
              <br>
              Vladimir Ivanov
              <br>
              <br>
              On 11/6/13 1:11 AM, Vladimir Kozlov wrote:
              <br>
              <blockquote type="cite">Note nmethodLocker is not lock:
                <br>
                <br>
                void nmethodLocker::lock_nmethod(nmethod* nm, bool
                zombie_ok) {
                <br>
                &nbsp;&nbsp; if (nm == NULL)&nbsp; return;
                <br>
                &nbsp;&nbsp; Atomic::inc(&amp;nm-&gt;_lock_count);
                <br>
                &nbsp;&nbsp; guarantee(zombie_ok || !nm-&gt;is_zombie(), "cannot
                lock a zombie
                <br>
                method");
                <br>
                }
                <br>
                <br>
                The guard is nm-&gt;is_locked_by_vm() but neither
                <br>
                make_not_entrant_or_zombie () or register_method() use
                it.
                <br>
                <br>
                So how nmethodLocker helps here?
                <br>
                <br>
                Vladimir
                <br>
                <br>
                On 11/5/13 12:42 PM, Vladimir Ivanov wrote:
                <br>
                <blockquote type="cite">Thanks! Missed that one. Fixed.
                  <br>
                  <br>
                  <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~vlivanov/8023037/webrev.01">http://cr.openjdk.java.net/~vlivanov/8023037/webrev.01</a>
                  <br>
                  <br>
                  Best regards,
                  <br>
                  Vladimir Ivanov
                  <br>
                  <br>
                  On 11/5/13 11:25 PM, Vladimir Kozlov wrote:
                  <br>
                  <blockquote type="cite">Should be
                    <br>
                    <br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~vlivanov/8023037/webrev.01/">http://cr.openjdk.java.net/~vlivanov/8023037/webrev.01/</a>
                    <br>
                    <br>
                    What about this place:
                    <br>
                    <br>
                    src/cpu/sparc/vm/sharedRuntime_sparc.cpp:&nbsp; if
                    (StressNonEntrant) {
                    <br>
                    <br>
                    Vladimir
                    <br>
                    <br>
                    On 11/5/13 11:10 AM, Vladimir Ivanov wrote:
                    <br>
                    <blockquote type="cite">Updated fix:
                      <br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~vlivanov/8023037/webrev.00/">http://cr.openjdk.java.net/~vlivanov/8023037/webrev.00/</a>
                      <br>
                      <br>
                      Removed broken StressNonEntrant code.
                      <br>
                      Updated comments.
                      <br>
                      <br>
                      Best regards,
                      <br>
                      Vladimir Ivanov
                      <br>
                      <br>
                      On 11/5/13 3:39 PM, Vladimir Ivanov wrote:
                      <br>
                      <blockquote type="cite"><a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~vlivanov/8023037/webrev.00/">http://cr.openjdk.java.net/~vlivanov/8023037/webrev.00/</a>
                        <br>
                        <br>
                        There's a race between compiler thread during
                        method registration
                        <br>
                        and
                        <br>
                        sweeper: sweeper can invalidate a nmethod which
                        hasn't been
                        <br>
                        installed
                        <br>
                        yet.
                        <br>
                        <br>
                        Consider the following scenario:
                        <br>
                        &nbsp;&nbsp; ciEnv::register_method:
                        <br>
                        &nbsp;&nbsp;&nbsp;&nbsp; - new nmethod(...)
                        <br>
                        <br>
                        &nbsp;&nbsp; sweeper:
                        <br>
                        &nbsp;&nbsp;&nbsp;&nbsp; - invalidates newly allocated nmethod and
                        patches VEP to call
                        <br>
                        handle_wrong_method
                        <br>
                        &nbsp;&nbsp;&nbsp;&nbsp; - tries to unlink it, but
                        method()-&gt;from_compiled_entry() !=
                        <br>
                        verified_entry_point() since nmethod hasn't been
                        installed yet
                        <br>
                        <br>
                        &nbsp;&nbsp; ciEnv::register_method:
                        <br>
                        &nbsp;&nbsp;&nbsp;&nbsp; - installs already invalidated nmethod
                        <br>
                        <br>
                        Calling corresponding Java method will lead to
                        infinite loop:
                        <br>
                        VEP of
                        <br>
                        the
                        <br>
                        compiled method calls handle_wrong_method and
                        call site resolution
                        <br>
                        returns the very same compiled method.
                        <br>
                        <br>
                        The fix is to grab a lock after nmethod is
                        allocated in the code
                        <br>
                        cache
                        <br>
                        and check that it hasn't been already
                        invalidated by the sweeper
                        <br>
                        before
                        <br>
                        proceeding. Sweeper already synchronizes on a
                        nmethod before
                        <br>
                        invalidating it.
                        <br>
                        <br>
                        Testing: failing test w/ diagnostic output.
                        <br>
                        <br>
                        Thanks!
                        <br>
                        <br>
                        Best regards,
                        <br>
                        Vladimir Ivanov
                        <br>
                      </blockquote>
                    </blockquote>
                  </blockquote>
                </blockquote>
              </blockquote>
            </blockquote>
          </blockquote>
        </blockquote>
      </blockquote>
    </blockquote>
    <br>
  </body>
</html>