JDK-7105305 : assert check_method_context proper context
  • Type: Bug
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: hs16,hs23
  • Priority: P4
  • Status: Closed
  • Resolution: Fixed
  • OS: generic,solaris_10
  • CPU: x86
  • Submitted: 2011-10-26
  • Updated: 2012-10-01
  • Resolved: 2012-01-23
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 7 JDK 8 Other
7u4Fixed 8Fixed hs23Fixed
Related Reports
Duplicate :  
Relates :  
Description
While testing the metadata compiler changes in CTW I hit an assertion failure in dependencies, so this is probably a question for John.  We have the following classes:

public interface javasoft.sqe.tests.vm.invokeinterface.invokeinterface019.invokeinterface01901.invokeinterface01901i{
   public abstract int f1(int, float);
}

public class javasoft.sqe.tests.vm.invokeinterface.invokeinterface019.invokeinterface01901.invokeinterface019c extends java.lang.Object implements javasoft.sqe.tests.vm.invokeinterface.invokeinterface019.invokeinterface01901.invokeinterface01901i{
   public int f1(int, float);
   public javasoft.sqe.tests.vm.invokeinterface.invokeinterface019.invokeinterface01901.invokeinterface019c();
}

public class javasoft.sqe.tests.vm.invokeinterface.invokeinterface019.invokeinterface01901.invokeinterface019d extends javasoft.sqe.tests.vm.invokeinterface.invokeinterface019.invokeinterface01901.invokeinterface019c{
   public static int f1(int, float);
   public javasoft.sqe.tests.vm.invokeinterface.invokeinterface019.invokeinterface01901.invokeinterface019d();
}

Note that the subclass has a static method with the same name and signature as the super.  javac would never allow this but it's a legal class file.  We've got a piece of code roughly like this:

invokeinterface019c  i = new invokeinterface019d();
i.f1(0, 0);

We head down into CHA to bind the method f1 and call check_method_context, where it looks up the method in invokeinterface019d using just the name and signature, which returns the static method.  We then assert because the methods aren't the same.  check_method_context appears to be slightly sloppy about it's checking so I'm assuming it should have a guard like this:

diff -r 8d6869d5ef1a src/share/vm/code/dependencies.cpp                                                     
--- a/src/share/vm/code/dependencies.cpp                                                                    
+++ b/src/share/vm/code/dependencies.cpp                                                                    
@@ -795,6 +795,9 @@
      if (!(lm->is_public() || lm->is_protected()))                                                         
        // Method is [package-]private, so the override story is complex.                                   
        return true;  // Must punt the assertion to true.                                                   
+      if (lm->is_static())                                                                                 
+        // Static doesn't override non-static                                                              
+        return true;  // Punt                                                                              
      if (   !Dependencies::is_concrete_method(lm)                                                          
&& !Dependencies::is_concrete_method(m)                                                                     
          && Klass::cast(lm->method_holder())->is_subtype_of(m->method_holder()))

The other interesting bit is that the reason this showed up in the metadata repo is because separation of oops and metadata exposed this mistake, which was fixed:

diff -r 42783d1414b2 src/share/vm/oops/constantPoolKlass.cpp                                                
--- a/src/share/vm/oops/constantPoolKlass.cpp                                                               
+++ b/src/share/vm/oops/constantPoolKlass.cpp                                                               
@@ -532,7 +532,7 @@
    if (cp->tag_at(i).is_unresolved_klass()) {                                                              
      // This will force loading of the class                                                               
      klassOop klass = cp->klass_at(i, CHECK);                                                              
-      if (klass->is_instance()) {                                                                          
+      if (klass->klass_part()->oop_is_instance()) {                                                        
        // Force initialization of class                                                                    
instanceKlass::cast(klass)->initialize(CHECK);                                                              
      }

The previous code would never succeed because a klass is never an instance, so we weren't actually preloading any classes in the constant pool.

Comments
EVALUATION http://hg.openjdk.java.net/lambda/lambda/hotspot/rev/c9a03402fe56
22-03-2012

EVALUATION See main CR
30-11-2011

EVALUATION http://hg.openjdk.java.net/hsx/hotspot-emb/hotspot/rev/c9a03402fe56
29-11-2011

EVALUATION http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/c9a03402fe56
15-11-2011

EVALUATION 7105305: assert check_method_context proper context Reviewed-by: jrose, kvn preload_and_initialize_all_classes intends to initialize all instanceKlasses but is using the wrong check. Fixing that exposes a bug in the dependencies where the is_concrete_method isn't checking for is_static. The fix is to make it consistent with the ciMethod variant. Tested with full CTW.
09-11-2011

EVALUATION http://hg.openjdk.java.net/hsx/hotspot-comp/hotspot/rev/c9a03402fe56
09-11-2011