RFR 8139163: InstanceKlass::cast passes through NULL

[Kim Barrett]

On Oct 22, 2015, at 4:05 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
> 
> Summary: Reduce raw (InstanceKlass*) casts and InstanceKlass::cast, which no long allows null
> 
> Somewhat of a tedious thing to code review (sorry).   Tested with internal remote-build-and-test.
> 
> open webrev at http://cr.openjdk.java.net/~coleenp/8139163.01/
> bug link https://bugs.openjdk.java.net/browse/JDK-8139163.01
> 
> Thanks,
> Coleen

Only a partial review; I didn't get through all the files, and have
run out of time today.  Here's what I have so far.  I'll continue
looking tomorrow.

------------------------------------------------------------------------------ 
src/share/vm/classfile/systemDictionary.cpp
1850   if ((*klassp) == NULL) {
1851     Klass* k;
1852     if (must_load) {
1853       k = resolve_or_fail(symbol, true, CHECK_0); // load required class
1854     } else {
1855       k = resolve_or_null(symbol,       CHECK_0); // load optional klass
1856     }
1857     (*klassp) = InstanceKlass::cast(k);
1858   }
1859   return ((*klassp) != NULL);

The code would seem to imply that k passed to InstanceKlass::cast here
could be NULL.  If not, then the final check for *klassp != NULL seems
unnecessary.

------------------------------------------------------------------------------ 
src/share/vm/gc/g1/g1CollectedHeap.cpp
4595     // this can be null so don't call InstanceKlass::cast
4596     return static_cast<InstanceKlass*>(klass);

But what if it's a non-NULL non-InstanceKlass?
Maybe add assert(klass == NULL || klass->is_instance(), ...).

------------------------------------------------------------------------------ 
src/share/vm/interpreter/linkResolver.cpp
 310   InstanceKlass* ik = InstanceKlass::cast(klass());
 311 
 312   // JDK 8, JVMS 5.4.3.4: Interface method resolution should
 313   // ignore static and non-public methods of java.lang.Object,
 314   // like clone, finalize, registerNatives.
 315   if (in_imethod_resolve &&
 316       result != NULL &&
 317       ik->is_interface() &&
 318       (result->is_static() || !result->is_public()) &&
 319       result->method_holder() == SystemDictionary::Object_klass()) {
 320     result = NULL;
 321   }

In the old code, that block starting with the "JDK 8" comment doesn't
seem to require klass() to be an InstanceKlass (at least not
obviously).  So moving the InstanceKlass::cast earlier might be an
unintended change.

There is later code that does require an InstanceKlass, so moving the
cast to after this block might be ok.

------------------------------------------------------------------------------ 
src/share/vm/interpreter/linkResolver.cpp
 379   InstanceKlass* ik = InstanceKlass::cast(klass());
 380 
 381   // First check in default method array
 382   if (!resolved_method->is_abstract() &&
 383     (InstanceKlass::cast(klass())->default_methods() != NULL)) {
 384     int index = InstanceKlass::find_method_index(ik->default_methods(),
 385                                                  name, signature, Klass::find_overpass,
 386                                                  Klass::find_static, Klass::find_private);
 387     if (index >= 0 ) {
 388       vtable_index = ik->default_vtable_indices()->at(index);
 389     }
 390   }
 391   if (vtable_index == Method::invalid_vtable_index) {
 392     // get vtable_index for miranda methods
 393     ResourceMark rm;
 394     klassVtable *vt = ik->vtable();
 395     vtable_index = vt->index_of_miranda(name, signature);
 396   }

Another case where moving the InstanceKlass::cast earlier might be an
unintended behavioral change.  For example, if
resolved_method->is_abstract() is true and the vtable_index doesn't
match then there is no other code that would require the result of the
cast, and so there wouldn't be a requirement for it to succeed.

------------------------------------------------------------------------------ 
src/share/vm/memory/heapInspection.cpp
 351     const InstanceKlass* k = (InstanceKlass*)cie->klass();
 352     Klass* super = cie->klass()->super();

Christian already pointed out the apparently no longer used k.  Why
wasn't there an unused variable warning?

------------------------------------------------------------------------------ 
src/share/vm/oops/instanceKlass.cpp
1543 bool InstanceKlass::has_redefined_this_or_super() const {
1544   Klass* klass = const_cast<InstanceKlass*>(this);
1545   while (klass != NULL) {
1546     if (InstanceKlass::cast(klass)->has_been_redefined()) {
1547       return true;
1548     }
1549     klass = klass->super();
1550   }
1551   return false;
1552 }

I don't understand the initial const-removing cast here.  Why not just
declare klass to be Klass const*?  It used to be InstanceKlass const*.

------------------------------------------------------------------------------ 
src/share/vm/oops/instanceKlass.cpp
2326 bool InstanceKlass::is_same_class_package(Klass* class2) {
2348 bool InstanceKlass::is_same_class_package(oop classloader2, Symbol* classname2) {

Thank you!

------------------------------------------------------------------------------ 
src/share/vm/oops/instanceKlass.cpp
2898   InstanceKlass* ik = const_cast<InstanceKlass*>(this);

Casting away const.  Ick!  Can we have an RFE to fix this?

------------------------------------------------------------------------------