United StatesChange Country, Oracle Worldwide Web Sites Communities I am a... I want to...
Bug ID: JDK-7105305 assert check_method_context proper context
JDK-7105305 : assert check_method_context proper context

Details
Type:
Bug
Submit Date:
2011-10-26
Status:
Closed
Updated Date:
2012-10-01
Project Name:
JDK
Resolved Date:
2012-01-23
Component:
hotspot
OS:
generic,solaris_10
Sub-Component:
compiler
CPU:
x86
Priority:
P4
Resolution:
Fixed
Affected Versions:
hs16,hs23
Fixed Versions:
hs23 (b06)

Related Reports
Backport:
Backport:
Duplicate:
Relates:

Sub Tasks

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/hsx/hotspot-comp/hotspot/rev/c9a03402fe56
                                     
2011-11-09
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.
                                     
2011-11-09
EVALUATION

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

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

See main CR
                                     
2011-11-30
EVALUATION

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



Hardware and Software, Engineered to Work Together