dependencies bug

[Tom Rodriguez]

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.

tom