Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8081800

AbstractMethodError when evaluating a private method in an interface via debugger

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: P3
    • Resolution: Fixed
    • Affects Version/s: 9
    • Fix Version/s: 9
    • Component/s: hotspot
    • Labels:
      None
    • Subcomponent:
    • Introduced In Version:
      9
    • Resolved In Build:
      b143

      Description

      This is a method invocation issue on debugger backend in JDK 9.
      Method invocation of a private method declared in an interface gives AbstractMethodError.
      The issue can be reproduced via the attached test case. Unzip the project and run it via:
      /opt/java/jdk1.9.0/bin/java -classpath .../AbstractMethodErrorTest/build/classes abstractmethoderrortest.AbstractMethodErrorTest
      After
      Breakpoint hit at abstractmethoderrortest.App:31
      it throws:
      com.sun.jdi.InvocationException: Exception occurred in target VM
      at com.sun.tools.jdi.ObjectReferenceImpl.invokeMethod(ObjectReferenceImpl.java:436)
      at abstractmethoderrortest.EventThread.testMethodInvoke(EventThread.java:362)
      at abstractmethoderrortest.EventThread.access$200(EventThread.java:57)
      at abstractmethoderrortest.EventThread$ThreadTrace.breakpointEvent(EventThread.java:321)
      at abstractmethoderrortest.EventThread.breakpointEvent(EventThread.java:474)
      at abstractmethoderrortest.EventThread.handleEvent(EventThread.java:414)
      at abstractmethoderrortest.EventThread.run(EventThread.java:121)
      Exception: instance of java.lang.AbstractMethodError(id=157)

      The method invocation is performed in EventThread.testMethodInvoke().

        Issue Links

          Activity

          Hide
          dholmes David Holmes added a comment -
          We do in fact initially set the vtable index to nonvirtual_vtable_index, but later logic then overwrites that with pending_vtable_index, which later leads to the throwing of AbstractMethodError. So I think the most appropriate, and conservative, fix here is to tweak update_inherited_vtable by changing this:

            } else if (klass->is_interface()) {
              allocate_new = false; // see note below in needs_new_vtable_entry
              // An interface never allocates new vtable slots, only inherits old ones.
              // This method will either be assigned its own itable index later,
              // or be assigned an inherited vtable index in the loop below.
              // default methods inherited by classes store their vtable indices
              // in the inheritor's default_vtable_indices.
              // default methods inherited by interfaces may already have a
              // valid itable index, if so, don't change it.
              // Overpass methods in an interface will be assigned an itable index later
              // by an inheriting class
              if (!is_default || !target_method()->has_itable_index()) {
                target_method()->set_vtable_index(Method::pending_itable_index);
              }

          to add a private check:

            } else if (klass->is_interface()) {
              allocate_new = false; // see note below in needs_new_vtable_entry
              // An interface never allocates new vtable slots, only inherits old ones.
              // This method will either be assigned its own itable index later,
              // or be assigned an inherited vtable index in the loop below.
              // default methods inherited by classes store their vtable indices
              // in the inheritor's default_vtable_indices.
              // default methods inherited by interfaces may already have a
              // valid itable index, if so, don't change it.
              // Overpass methods in an interface will be assigned an itable index later
              // by an inheriting class
          + // Private interface methods have no itable index and are always invoked nonvirtually
          ! if ((!is_default || !target_method()->has_itable_index()) && !target_method()->is_private()) {
                target_method()->set_vtable_index(Method::pending_itable_index);
              }

          which combined with not adding an itable index fixes the main issue - we will always do a nonvirtual invocation.

          As a side issue I also have to tweak an assertion in Method::can_be_statically_bound to recognize that private interface methods are also non-virtual.

          And I chose to fix Method::is_default_method to not claim private interface methods are default methods! Luckily we don't seem to use that method to make any major decisions! But there is one case where it disables CHA in ciMethod.cpp.

            // Disable CHA for default methods for now
            if (root_m->get_Method()->is_default_method()) {
              return NULL;
            }

          so that change might expose other issues if CHA can't actually deal with private interface methods.

          Then on the JDB side I have to make it aware of private interface methods so that a correct attempt at a nonvirtual invocation doesn't get rejected.
          Show
          dholmes David Holmes added a comment - We do in fact initially set the vtable index to nonvirtual_vtable_index, but later logic then overwrites that with pending_vtable_index, which later leads to the throwing of AbstractMethodError. So I think the most appropriate, and conservative, fix here is to tweak update_inherited_vtable by changing this:   } else if (klass->is_interface()) {     allocate_new = false; // see note below in needs_new_vtable_entry     // An interface never allocates new vtable slots, only inherits old ones.     // This method will either be assigned its own itable index later,     // or be assigned an inherited vtable index in the loop below.     // default methods inherited by classes store their vtable indices     // in the inheritor's default_vtable_indices.     // default methods inherited by interfaces may already have a     // valid itable index, if so, don't change it.     // Overpass methods in an interface will be assigned an itable index later     // by an inheriting class     if (!is_default || !target_method()->has_itable_index()) {       target_method()->set_vtable_index(Method::pending_itable_index);     } to add a private check:   } else if (klass->is_interface()) {     allocate_new = false; // see note below in needs_new_vtable_entry     // An interface never allocates new vtable slots, only inherits old ones.     // This method will either be assigned its own itable index later,     // or be assigned an inherited vtable index in the loop below.     // default methods inherited by classes store their vtable indices     // in the inheritor's default_vtable_indices.     // default methods inherited by interfaces may already have a     // valid itable index, if so, don't change it.     // Overpass methods in an interface will be assigned an itable index later     // by an inheriting class + // Private interface methods have no itable index and are always invoked nonvirtually ! if ((!is_default || !target_method()->has_itable_index()) && !target_method()->is_private()) {       target_method()->set_vtable_index(Method::pending_itable_index);     } which combined with not adding an itable index fixes the main issue - we will always do a nonvirtual invocation. As a side issue I also have to tweak an assertion in Method::can_be_statically_bound to recognize that private interface methods are also non-virtual. And I chose to fix Method::is_default_method to not claim private interface methods are default methods! Luckily we don't seem to use that method to make any major decisions! But there is one case where it disables CHA in ciMethod.cpp.   // Disable CHA for default methods for now   if (root_m->get_Method()->is_default_method()) {     return NULL;   } so that change might expose other issues if CHA can't actually deal with private interface methods. Then on the JDB side I have to make it aware of private interface methods so that a correct attempt at a nonvirtual invocation doesn't get rejected.
          Hide
          dholmes David Holmes added a comment - - edited
          Method::is_default_method is used in the following places, so we have to verify that excluding private interface methods does not affect the correctness of the code:

          ./share/vm/classfile/defaultMethods.cpp: if (entry.first->is_default_method()) {

          This code already works on a set of methods that has excluded private interface methods.

          ---

          ./share/vm/oops/klassVtable.cpp:

          - assert(super_method->is_default_method() || super_method->is_overpass()

          super_method can not be a private interface method, so operation of is_default_method() is unchanged.

          - if (old_method->is_default_method()) {

          This is only used in class redefinition to update the vtable/itable as needed, which doesn't apply to private interface methods, so use of is_default_method is correct. A new test verifies this.

          - if (m->is_default_method()) {

          This prints "default" for default methods so the change fixes a bug here.

          - if (m->is_default_method()) {

          This prints "default" for default methods so the change fixes a bug here.

          ---

          ./share/vm/oops/method.cpp: if (is_overpass() || is_default_method()) return false;

          This is in the context of:

          bool Method::is_final_method(AccessFlags class_access_flags) const {
            if (is_overpass() || is_default_method()) return false;
            return is_final() || class_access_flags.is_final();
          }

          This would potentially treat final private interface methods differently as we do not return false immediately. However you can not (at the language level) declare a private final interface method, nor a final interface. As future work we should reexamine this logic as all private methods are implicitly final and should treated as so.

          ---

          ./share/vm/oops/method.cpp: if (is_default_method()) {

          This prints "default" for default methods so the change fixes a bug here.

          ---

          ./share/vm/oops/instanceKlass.cpp: ((defaults_mode != skip_defaults) || !m->is_default_method())) {

          The full expression already skips private methods: if (m != NULL && m->is_public() && !m->is_static() && ((defaults_mode != skip_defaults) || !m->is_default_method())) {

          ---

          ./share/vm/prims/methodHandles.cpp: if (m->is_default_method()) {

          This prints "default" for default methods so the change fixes a bug here.

          ---

          ./share/vm/ci/ciMethod.cpp: if (root_m->get_Method()->is_default_method()) { return NULL; }

          This is in the context of "// Disable CHA for default methods for now". However there is earlier code that is intended to catch private or final methods:

          // Is it private or final?
           if (root_m->can_be_statically_bound()) {
          __ return root_m;
           }

          and that is the path we take.

          ---

          ./share/vm/code/dependencies.cpp: assert(fm->is_default_method(), "sanity");

          This occurs in code called from Dependencies::find_unique_concrete_method which in turn is called from two places:
          1. In ciMethod.cpp after we have already excluded private interface methods (as they are statically bound ciMethods)
          2. In jvmciCompilerToVM.cpp in code that can not be called for interface methods

          ---

          So all usages check out okay.
          Show
          dholmes David Holmes added a comment - - edited Method::is_default_method is used in the following places, so we have to verify that excluding private interface methods does not affect the correctness of the code: ./share/vm/classfile/defaultMethods.cpp: if (entry.first->is_default_method()) { This code already works on a set of methods that has excluded private interface methods. --- ./share/vm/oops/klassVtable.cpp: - assert(super_method->is_default_method() || super_method->is_overpass() super_method can not be a private interface method, so operation of is_default_method() is unchanged. - if (old_method->is_default_method()) { This is only used in class redefinition to update the vtable/itable as needed, which doesn't apply to private interface methods, so use of is_default_method is correct. A new test verifies this. - if (m->is_default_method()) { This prints "default" for default methods so the change fixes a bug here. - if (m->is_default_method()) { This prints "default" for default methods so the change fixes a bug here. --- ./share/vm/oops/method.cpp: if (is_overpass() || is_default_method()) return false; This is in the context of: bool Method::is_final_method(AccessFlags class_access_flags) const {   if (is_overpass() || is_default_method()) return false;   return is_final() || class_access_flags.is_final(); } This would potentially treat final private interface methods differently as we do not return false immediately. However you can not (at the language level) declare a private final interface method, nor a final interface. As future work we should reexamine this logic as all private methods are implicitly final and should treated as so. --- ./share/vm/oops/method.cpp: if (is_default_method()) { This prints "default" for default methods so the change fixes a bug here. --- ./share/vm/oops/instanceKlass.cpp: ((defaults_mode != skip_defaults) || !m->is_default_method())) { The full expression already skips private methods: if (m != NULL && m->is_public() && !m->is_static() && ((defaults_mode != skip_defaults) || !m->is_default_method())) { --- ./share/vm/prims/methodHandles.cpp: if (m->is_default_method()) { This prints "default" for default methods so the change fixes a bug here. --- ./share/vm/ci/ciMethod.cpp: if (root_m->get_Method()->is_default_method()) { return NULL; } This is in the context of "// Disable CHA for default methods for now". However there is earlier code that is intended to catch private or final methods: // Is it private or final?  if (root_m->can_be_statically_bound()) { __ return root_m;  } and that is the path we take. --- ./share/vm/code/dependencies.cpp: assert(fm->is_default_method(), "sanity"); This occurs in code called from Dependencies::find_unique_concrete_method which in turn is called from two places: 1. In ciMethod.cpp after we have already excluded private interface methods (as they are statically bound ciMethods) 2. In jvmciCompilerToVM.cpp in code that can not be called for interface methods --- So all usages check out okay.
          Hide
          dholmes David Holmes added a comment - - edited
          With these changes Method::can_be_statically_bound now reports true due to vtable_index() == nonvirtual_vtable_index. This is what we want as private interface methods are statically bound (and if all private methods were treated as final the same would be true). But we have to check all uses to ensure code will not be surprised if it encounters a private interface method.

          ./share/vm/ci/ciMethod.cpp:
           - _can_be_statically_bound = h_m()->can_be_statically_bound();

          Establishes ciMethod property based on Method property. However we later already have:

           if (!_can_be_statically_bound && h_m()->is_private())
              _can_be_statically_bound = true;

          So ciMethod reports can_be_ statically_bound for private interface methods both before and after this change.

          ./share/vm/opto/doCall.cpp
          ./share/vm/shark/sharkTopLevelBlock.cpp
          ./share/vm/ci/bcEscapeAnalyzer.cpp
          ./share/vm/c1/c1_GraphBuilder.cpp:

          All relate to ciMethod so ok.

          ---

          ./share/vm/runtime/sharedRuntime.cpp:

          bool static_bound = call_info.resolved_method()->can_be_statically_bound();

          Only called for invokevirtual, so should never encounter a private interface method.

          ---

          ./share/vm/runtime/sharedRuntime.cpp:

          This is in SharedRuntime::handle_ic_miss_helper:

            // Compiler1 can produce virtual call sites that can actually be statically bound
            // If we fell thru to below we would think that the site was going megamorphic
            // when in fact the site can never miss. Worse because we'd think it was megamorphic
            // we'd try and do a vtable dispatch however methods that can be statically bound
            // don't have vtable entries (vtable_index < 0) and we'd blow up. So we force a
            // reresolution of the call site (as if we did a handle_wrong_method and not an
            // plain ic_miss) and the site will be converted to an optimized virtual call site
            // never to miss again. I don't believe C2 will produce code like this but if it
            // did this would still be the correct thing to do for it too, hence no ifdef.
            //
            if (call_info.resolved_method()->can_be_statically_bound()) {
              methodHandle callee_method = SharedRuntime::reresolve_call_site(thread, CHECK_(methodHandle()));

          It is unclear if we could ever reach here for a private interface method, but if we did then this seem the right thing to do.

          ---

          ./share/vm/oops/cpCache.cpp: assert(method->can_be_statically_bound(), "");

          case Bytecodes::_invokevirtual:
           {
             if (!is_vtable_call) {
                assert(method->can_be_statically_bound(), "");
          ...
               } else {
                    assert(!method->can_be_statically_bound(), "");

          Private interface methods can't be called via invokevirtual, so we should not reach here.

          ---

          ./share/vm/oops/method.cpp: assert(m->can_be_statically_bound(), "");

          Relates to creation of MethodHandle intrinsic.

          ---

          ./share/vm/interpreter/linkResolver.cpp:

          - CallKind kind = (vtable_index >= 0 && !resolved_method->can_be_statically_bound() ? CallInfo::vtable_call : CallInfo::direct_call);

          vtable_index will not be >= 0 for private interface methods so the clause is not reachable in this case - and a direct call is the correct choice.

          - if (resolved_method->can_be_statically_bound()) {
             kind = CallInfo::direct_call;
            }

          This is the correct selection.

          assert(resolved_method->can_be_statically_bound(), "cannot override this method");

          Code should never be reachable for a private interface method.
          Show
          dholmes David Holmes added a comment - - edited With these changes Method::can_be_statically_bound now reports true due to vtable_index() == nonvirtual_vtable_index. This is what we want as private interface methods are statically bound (and if all private methods were treated as final the same would be true). But we have to check all uses to ensure code will not be surprised if it encounters a private interface method. ./share/vm/ci/ciMethod.cpp:  - _can_be_statically_bound = h_m()->can_be_statically_bound(); Establishes ciMethod property based on Method property. However we later already have:  if (!_can_be_statically_bound && h_m()->is_private())     _can_be_statically_bound = true; So ciMethod reports can_be_ statically_bound for private interface methods both before and after this change. ./share/vm/opto/doCall.cpp ./share/vm/shark/sharkTopLevelBlock.cpp ./share/vm/ci/bcEscapeAnalyzer.cpp ./share/vm/c1/c1_GraphBuilder.cpp: All relate to ciMethod so ok. --- ./share/vm/runtime/sharedRuntime.cpp: bool static_bound = call_info.resolved_method()->can_be_statically_bound(); Only called for invokevirtual, so should never encounter a private interface method. --- ./share/vm/runtime/sharedRuntime.cpp: This is in SharedRuntime::handle_ic_miss_helper:   // Compiler1 can produce virtual call sites that can actually be statically bound   // If we fell thru to below we would think that the site was going megamorphic   // when in fact the site can never miss. Worse because we'd think it was megamorphic   // we'd try and do a vtable dispatch however methods that can be statically bound   // don't have vtable entries (vtable_index < 0) and we'd blow up. So we force a   // reresolution of the call site (as if we did a handle_wrong_method and not an   // plain ic_miss) and the site will be converted to an optimized virtual call site   // never to miss again. I don't believe C2 will produce code like this but if it   // did this would still be the correct thing to do for it too, hence no ifdef.   //   if (call_info.resolved_method()->can_be_statically_bound()) {     methodHandle callee_method = SharedRuntime::reresolve_call_site(thread, CHECK_(methodHandle())); It is unclear if we could ever reach here for a private interface method, but if we did then this seem the right thing to do. --- ./share/vm/oops/cpCache.cpp: assert(method->can_be_statically_bound(), ""); case Bytecodes::_invokevirtual:  {    if (!is_vtable_call) {       assert(method->can_be_statically_bound(), ""); ...      } else {           assert(!method->can_be_statically_bound(), ""); Private interface methods can't be called via invokevirtual, so we should not reach here. --- ./share/vm/oops/method.cpp: assert(m->can_be_statically_bound(), ""); Relates to creation of MethodHandle intrinsic. --- ./share/vm/interpreter/linkResolver.cpp: - CallKind kind = (vtable_index >= 0 && !resolved_method->can_be_statically_bound() ? CallInfo::vtable_call : CallInfo::direct_call); vtable_index will not be >= 0 for private interface methods so the clause is not reachable in this case - and a direct call is the correct choice. - if (resolved_method->can_be_statically_bound()) {    kind = CallInfo::direct_call;   } This is the correct selection. assert(resolved_method->can_be_statically_bound(), "cannot override this method"); Code should never be reachable for a private interface method.
          Hide
          hgupdate HG Updates added a comment -
          URL: http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/4962f9f46728
          User: dholmes
          Date: 2016-10-04 03:00:10 +0000
          Show
          hgupdate HG Updates added a comment - URL: http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/4962f9f46728 User: dholmes Date: 2016-10-04 03:00:10 +0000
          Hide
          hgupdate HG Updates added a comment -
          URL: http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/4962f9f46728
          User: lana
          Date: 2016-11-03 02:17:58 +0000
          Show
          hgupdate HG Updates added a comment - URL: http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/4962f9f46728 User: lana Date: 2016-11-03 02:17:58 +0000

            People

            • Assignee:
              dholmes David Holmes
              Reporter:
              mentlich Martin Entlicher
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Due:
                Created:
                Updated:
                Resolved: