<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    Hi,<br>
    <br>
    <div class="moz-cite-prefix">On 2016-10-26 14:37, Vladimir Ivanov
      wrote:<br>
    </div>
    <blockquote
      cite="mid:44ce8160-d080-6e49-1b59-7c3cc3b810db@oracle.com"
      type="cite">
      <blockquote type="cite"><a class="moz-txt-link-freetext"
          href="http://cr.openjdk.java.net/%7Eneliasso/8160543/webrev.11">http://cr.openjdk.java.net/~neliasso/8160543/webrev.11</a>
        <br>
      </blockquote>
      <br>
      src/share/vm/c1/c1_GraphBuilder.cpp: <br>
      <br>
      +  assert(will_link == target->is_loaded(), "Check"); <br>
      <br>
      Please, add meaningful assert message or leave it empty. <br>
    </blockquote>
    <br>
    Removed comment.<br>
    <br>
    <blockquote
      cite="mid:44ce8160-d080-6e49-1b59-7c3cc3b810db@oracle.com"
      type="cite"> <br>
      <br>
      src/share/vm/c1/c1_LIRGenerator.cpp: <br>
      <br>
      +  bool optimized = x->target_is_final(); // implies that it is
      loaded too. <br>
    </blockquote>
    <br>
    What do you suggest here?<br>
    <br>
    <blockquote
      cite="mid:44ce8160-d080-6e49-1b59-7c3cc3b810db@oracle.com"
      type="cite"> <br>
      src/share/vm/code/compiledIC.cpp: <br>
      <br>
      -    if (static_bound || is_optimized) { <br>
      +    if (is_optimized) { <br>
      <br>
      It seems static_bound == true not only for final methods: <br>
      methodHandle SharedRuntime::resolve_sub_helper(JavaThread *thread,
      <br>
      ... <br>
          bool static_bound =
      call_info.resolved_method()->can_be_statically_bound(); <br>
      <br>
      bool Method::can_be_statically_bound(AccessFlags
      class_access_flags) const { <br>
        if (is_final_method(class_access_flags))  return true; <br>
        return vtable_index() == nonvirtual_vtable_index; <br>
      } <br>
      <br>
      Maybe mark other cases (but when a method is loaded) w/ optimized
      flag as well? <br>
    </blockquote>
    <br>
    I tested this and it is not enough either. When optimized is true -
    the callee is final and it was loaded, and then we know we can use
    the verified entry. Otherwise we can't.<br>
    <br>
    There is a comment a few lines down I didn't see before, it explains
    well why static_bound was used. I will change the comment.<br>
    <br>
        // Note: the following problem exists with Compiler1:<br>
        //   - at compile time we may or may not know if the destination
    is final<br>
        //   - if we know that the destination is final, we will emit an
    optimized<br>
        //     virtual call (no inline cache), and need a Method* to
    make a call<br>
        //     to the interpreter<br>
    >>  //   - if we do not know if the destination is final, we
    emit a standard<br>
        //     virtual call, and use CompiledICHolder to call
    interpreted code<br>
        //     (no static call stub has been generated)<br>
        //     However in that case we will now notice it is
    static_bound<br>
        //     and convert the call into what looks to be an optimized<br>
        //     virtual call. This causes problems in verifying the IC
    because<br>
        //     it look vanilla but is optimized. Code in
    is_call_to_interpreted<br>
        //     is aware of this and weakens its asserts.<br>
    <br>
    This static_bound case is the one at the arrows. It is wrong because
    if the call was final and the target was loaded - optimized would be
    set. If it was not loaded, then we couldn't emit a null check, and
    we must use unverified entry even if we later learn it was final.
    The same goes for the alternative vtable check.<br>
    <br>
    I use the CallableStatementTests (listed in the bug report) to wrap
    my head around this. The target method is not loaded at compile
    time, but we will find it final in compute_monomorphic.<br>
    <br>
    <blockquote
      cite="mid:44ce8160-d080-6e49-1b59-7c3cc3b810db@oracle.com"
      type="cite"> Also, please, add a comment in
      CompiledIC::compute_monomorphic_entry why static_bound isn't
      relevant anymore. <br>
      <br>
    </blockquote>
    <br>
    Will do. I also must look at the other used of static_bound and see
    if they are still relevant.<br>
    <br>
    <blockquote
      cite="mid:44ce8160-d080-6e49-1b59-7c3cc3b810db@oracle.com"
      type="cite">Otherwise, looks good. <br>
      <br>
      Best regards, <br>
      Vladimir Ivanov <br>
    </blockquote>
    <br>
    Thanks for having a look at this!<br>
    <br>
    I will post a new webrev.<br>
    <br>
    //Nils<br>
    <br>
    <blockquote
      cite="mid:44ce8160-d080-6e49-1b59-7c3cc3b810db@oracle.com"
      type="cite"> <br>
      <blockquote type="cite">On 2016-10-17 11:06, Lindenmaier, Goetz
        wrote: <br>
        <blockquote type="cite">Hi Nils, <br>
          <br>
          the webrev still looks good. <br>
          <br>
          <blockquote type="cite">I removed the assert because it will
            always hold. <br>
          </blockquote>
          I would probably keep the assert like this: <br>
          assert(!target->is_loaded() || klass->is_loaded(),
          "loaded target must <br>
          imply loaded klass"); <br>
          <br>
          <blockquote type="cite">I may remove some more <br>
            klass->is_loaded() calls but I need to test it thoroughly
            first. <br>
          </blockquote>
          Yes, I think the two below could be removed safely. Or do you
          expect <br>
          concurrent changes of this information?  See below. <br>
          <br>
          Best regards, <br>
             Goetz. <br>
          <br>
          <br>
          diff -r 30e7565e278d src/share/vm/c1/c1_GraphBuilder.cpp <br>
          --- a/src/share/vm/c1/c1_GraphBuilder.cpp       Mon Oct 17
          10:50:54 <br>
          2016 +0200 <br>
          +++ b/src/share/vm/c1/c1_GraphBuilder.cpp       Mon Oct 17
          11:03:40 <br>
          2016 +0200 <br>
          @@ -1814,6 +1814,7 @@ <br>
              const Bytecodes::Code bc_raw = stream()->cur_bc_raw();
          <br>
              assert(declared_signature != NULL, "cannot be null"); <br>
              assert(will_link == target->is_loaded(), "Check"); <br>
          +  assert(!target->is_loaded() || klass->is_loaded(),
          "Loaded target <br>
          must imply loaded klass."); <br>
          <br>
              ciInstanceKlass* klass = target->holder(); <br>
          <br>
          @@ -1863,7 +1864,7 @@ <br>
              ciMethod* cha_monomorphic_target = NULL; <br>
              ciMethod* exact_target = NULL; <br>
              Value better_receiver = NULL; <br>
          -  if (UseCHA && DeoptC1 &&
          klass->is_loaded() && target->is_loaded()
          && <br>
          +  if (UseCHA && DeoptC1 &&
          klass->is_loaded() && <br>
                  !(// %%% FIXME: Are both of these relevant? <br>
                    target->is_method_handle_intrinsic() || <br>
                    target->is_compiled_lambda_form()) && <br>
          @@ -1983,8 +1984,7 @@ <br>
              } <br>
          <br>
              // check if we could do inlining <br>
          -  if (!PatchALot && Inline && <br>
          -      klass->is_loaded() && target->is_loaded()
          && <br>
          +  if (!PatchALot && Inline &&
          klass->is_loaded() && <br>
                  (klass->is_initialized() ||
          klass->is_interface() && <br>
          target->holder()->is_initialized()) <br>
                  && !patch_for_appendix) { <br>
                // callee is known => check if we have static binding
          <br>
          <br>
          <br>
          <br>
          <blockquote type="cite">-----Original Message----- <br>
            From: Nils Eliasson [<a class="moz-txt-link-freetext"
              href="mailto:nils.eliasson@oracle.com">mailto:nils.eliasson@oracle.com</a>]
            <br>
            Sent: Donnerstag, 13. Oktober 2016 15:56 <br>
            To: Lindenmaier, Goetz <a class="moz-txt-link-rfc2396E"
              href="mailto:goetz.lindenmaier@sap.com"><goetz.lindenmaier@sap.com></a>;
            hotspot-compiler- <br>
            <a class="moz-txt-link-abbreviated"
              href="mailto:dev@openjdk.java.net">dev@openjdk.java.net</a>
            compiler <a class="moz-txt-link-rfc2396E"
              href="mailto:hotspot-compiler-dev@openjdk.java.net"><hotspot-compiler-dev@openjdk.java.net></a>
            <br>
            Subject: Re: [9] RFR (M): 8160543: C1: Crash in <br>
            java.lang.String.indexOf in <br>
            some java.sql tests <br>
            <br>
            Hi Goetz, <br>
            <br>
            Thanks for the feedback! <br>
            <br>
            I have been busy with a performance regression that this
            code triggered <br>
            - JDK-8167656. <br>
            <br>
            I removed the assert because it will always hold. I may
            remove some more <br>
            klass->is_loaded() calls but I need to test it thoroughly
            first. <br>
            <br>
            The indentation looks crappy in the webrev but ok in my
            editor, <br>
            something with the diff I guess. <br>
            <br>
            New webrev: <a class="moz-txt-link-freetext"
              href="http://cr.openjdk.java.net/%7Eneliasso/8160543/webrev.10/">http://cr.openjdk.java.net/~neliasso/8160543/webrev.10/</a>
            <br>
            <br>
            Regards, <br>
            Nils <br>
            <br>
            On 2016-09-07 12:07, Lindenmaier, Goetz wrote: <br>
            <blockquote type="cite">Hi Nils, <br>
              <br>
              I encountered this issue in our nightly jck test runs with
              -Xcomp. <br>
              I applied your fix to the VM tested, and I can no more
              observe <br>
              the error. <br>
              <br>
              Also, I had a look at the code. It looks good. <br>
              The if around assert(klass->is_loaded(), "sanity");
              could be merged <br>
              into the <br>
            </blockquote>
            assertion. <br>
            <blockquote type="cite">Also, if this holds, a row of calls
              to klass->is_loaded() can be <br>
              removed. <br>
              <br>
              Please fix the indentation in c1_GraphBuilder.cpp 2056ff.
              <br>
              <br>
              Thanks for fixing this, <br>
                  Goetz. <br>
              <br>
              <blockquote type="cite">-----Original Message----- <br>
                From: hotspot-compiler-dev [<a
                  class="moz-txt-link-freetext"
                  href="mailto:hotspot-compiler-dev">mailto:hotspot-compiler-dev</a>-
                <br>
                <a class="moz-txt-link-abbreviated"
                  href="mailto:bounces@openjdk.java.net">bounces@openjdk.java.net</a>]
                On Behalf Of Nils Eliasson <br>
                Sent: Mittwoch, 31. August 2016 16:31 <br>
                To: <a class="moz-txt-link-abbreviated"
                  href="mailto:hotspot-compiler-dev@openjdk.java.net">hotspot-compiler-dev@openjdk.java.net</a>
                compiler <hotspot- <br>
              </blockquote>
            </blockquote>
            compiler- <br>
            <blockquote type="cite">
              <blockquote type="cite"><a
                  class="moz-txt-link-abbreviated"
                  href="mailto:dev@openjdk.java.net">dev@openjdk.java.net</a>>
                <br>
                Subject: [9] RFR (M): 8160543: C1: Crash in <br>
                java.lang.String.indexOf in <br>
              </blockquote>
            </blockquote>
            some <br>
            <blockquote type="cite">
              <blockquote type="cite">java.sql tests <br>
                <br>
                Hi, <br>
                <br>
                This is fixes for bug [1] JDK-8160543 "C1: Crash in <br>
                java.lang.String.indexOf <br>
              </blockquote>
            </blockquote>
            in <br>
            <blockquote type="cite">
              <blockquote type="cite">some java.sql tests" and [2]
                JDK-8154172 "NPE is thrown instead of <br>
                linkage <br>
                error when invoking nonexistent method " <br>
                <br>
                * Description: <br>
                <br>
                Problem in bug #2: A method that is not loaded must not
                have null <br>
                check <br>
              </blockquote>
            </blockquote>
            at <br>
            <blockquote type="cite">
              <blockquote type="cite">the call. The unloaded method may
                not exist and then we may throw a <br>
              </blockquote>
            </blockquote>
            NPE <br>
            <blockquote type="cite">
              <blockquote type="cite">on a null receiver before
                LinkageError and violate the VM spec. <br>
                <br>
                Problem in bug #1: A final method that is not loaded at
                compile <br>
                time (the <br>
                final-property is unknown), but is actually loaded from
                another <br>
              </blockquote>
            </blockquote>
            classloader <br>
            <blockquote type="cite">
              <blockquote type="cite">(and may already be compiled) must
                null check its receiver. The <br>
                null check <br>
                can not be at the call site since it would violate #2.
                Instead the <br>
                call will <br>
              </blockquote>
            </blockquote>
            have to <br>
            <blockquote type="cite">
              <blockquote type="cite">use the target methods unverified
                entry point. <br>
                <br>
                An additional problem i encountered is that profiling
                requires a <br>
                null check, <br>
                but if the method isn't loaded we can't add one. So we
                can't profile <br>
              </blockquote>
            </blockquote>
            unloaded <br>
            <blockquote type="cite">
              <blockquote type="cite">methods. <br>
                <br>
                The solution to these problems shouldn't introduce any
                regression <br>
                in the <br>
                normal use case. Unloaded methods is only common in the
                compiler <br>
              </blockquote>
            </blockquote>
            when <br>
            <blockquote type="cite">
              <blockquote type="cite">using -Xcomp when the interpreter
                hasn't made sure everything is <br>
              </blockquote>
            </blockquote>
            loaded. I <br>
            <blockquote type="cite">
              <blockquote type="cite">have made the trade-off that it is
                acceptable to have an performance <br>
                regression in the -Xcomp case in order to meet the VM
                specification. <br>
                <br>
                * Summary of changes: <br>
                <br>
                hotspot/src/share/vm/code/compiledIC.cpp <br>
                <br>
                -    if (static_bound || is_optimized) { <br>
                +    if (is_optimized) { <br>
                <br>
                static_bound is true if the method at resolve time is
                declared <br>
                final. This is <br>
                uninteresting, we need to know if the call was known
                final at compile <br>
              </blockquote>
            </blockquote>
            time. <br>
            <blockquote type="cite">
              <blockquote type="cite">is_optimized however is only true
                if the targets was loaded and was <br>
                final <br>
              </blockquote>
            </blockquote>
            at <br>
            <blockquote type="cite">
              <blockquote type="cite">compile time. This change makes
                sure that we get a call to the <br>
                unverified <br>
                entry point if there was no null check emitted. <br>
                <br>
                hotspot/src/share/vm/c1/c1_GraphBuilder.cpp <br>
                Contains both changed and some simplifications. The
                is_resolved method <br>
                has been exploded, and redundant check was removed. The
                major <br>
              </blockquote>
            </blockquote>
            change is <br>
            <blockquote type="cite">
              <blockquote type="cite">where we decide if a null check
                should be emitted and when <br>
                profiling can <br>
              </blockquote>
            </blockquote>
            be <br>
            <blockquote type="cite">
              <blockquote type="cite">added. <br>
                <br>
                * Testing <br>
                <br>
                These are some useful test I have run with and without
                -Xcomp and with <br>
              </blockquote>
            </blockquote>
            and <br>
            <blockquote type="cite">
              <blockquote type="cite">without -XX:TieredStopAtLevel=1: <br>
                -
                jdk/test/java/sql/testng/test/sql/CallableStatementTests.java
                <br>
                (for bug <br>
              </blockquote>
            </blockquote>
            #1) <br>
            <blockquote type="cite">
              <blockquote type="cite">- JCK/BINC (binc02908m01 for bug
                #2 and all binc0500) <br>
                - hotspot/test/compiler/linkage/LinkageErrors.java <br>
                <br>
                I will await complete runs of hs-comp tier 0 - 5 before
                checkin. <br>
                <br>
                Please review, <br>
                <br>
                Regards, <br>
                Nils Eliasson <br>
                <br>
                <br>
                <br>
                Webrev: <a class="moz-txt-link-freetext"
                  href="http://cr.openjdk.java.net/%7Eneliasso/8160543/webrev.09/">http://cr.openjdk.java.net/~neliasso/8160543/webrev.09/</a>
                <br>
                Bug [1]: <a class="moz-txt-link-freetext"
                  href="https://bugs.openjdk.java.net/browse/JDK-8160543">https://bugs.openjdk.java.net/browse/JDK-8160543</a>
                <br>
                Bug [2]: <a class="moz-txt-link-freetext"
                  href="https://bugs.openjdk.java.net/browse/JDK-8160383">https://bugs.openjdk.java.net/browse/JDK-8160383</a>
                <br>
              </blockquote>
            </blockquote>
          </blockquote>
        </blockquote>
        <br>
      </blockquote>
    </blockquote>
    <br>
  </body>
</html>