review for 7105305: assert check_method_context proper context

[Tom Rodriguez]

On Oct 26, 2011, at 3:45 PM, Tom Rodriguez wrote:

> 
> On Oct 26, 2011, at 3:20 PM, John Rose wrote:
> 
>> On Oct 26, 2011, at 2:26 PM, Tom Rodriguez wrote:
>> 
>>> This may causes some other CTW issues but I will file separate bugs for those, unless they have a simple fix I can include here.
>> 
>> Good.
> 
> Thanks for identifying the real fix.

I was reverifying the fix since I'd done my testing on something that I though was equivalent, but I realized this doesn't work.

      if (   !Dependencies::is_concrete_method(lm)
          && !Dependencies::is_concrete_method(m)
          && Klass::cast(lm->method_holder())->is_subtype_of(m->method_holder()))
        // Method m is overridden by lm, but both are non-concrete.                                                                                    
        return true;

In the failing case one method is static and the other isn't so changing is_concrete_method doesn't solve the problem.  I think it needs a separate guard, like so:

@@ -795,6 +795,10 @@
       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 methods don't override non-static so punt                                                                                            
+        return true;                                                                                                                                  
+      }                                                                                                                                                
       if (   !Dependencies::is_concrete_method(lm)                                                                                                    
&& !Dependencies::is_concrete_method(m)                                                                                                      
           && Klass::cast(lm->method_holder())->is_subtype_of(m->method_holder()))

That's what I'd originally tested which is why I was reverifying John's suggested fix.  It seemed trivially equivalent at first glance but the && means it doesn't work.  I don't think it can be converted to || without breaking the original intent of the test.

tom

> 
>> 
>> Besides uncovering new CTW bugs, I think there's a risk in initializing CTW classes eagerly:  It might cause CTW to give up on processing a class C if its referent D fails to initialize.  This could reduce coverage, since perhaps C used to load and get compiled, where now D's failed initialization will cause C to be skipped.
> 
> It doesn't work that way:
> 
>        constantPoolKlass::preload_and_initialize_all_classes(k->constants(), THREAD);
>        if (HAS_PENDING_EXCEPTION) {
>          // If something went wrong in preloading we just ignore it                                                                                    
>          clear_pending_exception_if_not_oom(CHECK);
>          tty->print_cr("Preloading failed for (%d) %s", _compile_the_world_counter, buffer);
>        }
> 
> Previously we would load the class and we might throw exceptions and that wouldn't stop compiles either.  We just have more opportunities to throw exceptions that we will just ignore.
> 
> tom
> 
>> 
>> I suggest adding a print statement to the path where D's initialization fails, so we can track whether coverage gets reduced.
>> 
>> -- John
>