dependencies bug

[Tom Rodriguez]

On Oct 25, 2011, at 1:01 AM, John Rose wrote:

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

That makes sense.  I'll do that.  Thanks!

tom

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