dependencies bug

[John Rose]

On Oct 24, 2011, at 4:48 PM, Tom Rodriguez wrote:

> 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()))

Yes, that's a reasonable detuning of the assert.  But I think it would be better if you were to make the two overloadings of Dependencies::is_concrete_method be consistent.  (I think that would fix the same assertion bug.)  The first occurrence of is_abstract is_concrete_method(methodOop m) was probably a typo for is_static.

> 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.

Wow.  That's turn-of-the-century dead code.  Ancient zombies from the crypt.

-- John