RFR 8139163: InstanceKlass::cast passes through NULL

[Kim Barrett]

On Oct 22, 2015, at 8:10 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
> 
> 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.

Here’s the rest.  I’ll reply to your responses separately.

------------------------------------------------------------------------------
src/share/vm/oops/klass.cpp
563   if (oop_is_instance()) {
564     InstanceKlass* ik = InstanceKlass::cast(const_cast<Klass*>(this));

Instead of casting away const and using InstanceKlass::cast, how about
just

 const InstanceKlass* ik = static_cast<const InstanceKlass*>(this);

------------------------------------------------------------------------------
src/share/vm/oops/klass.cpp
691 bool Klass::verify_vtable_index(int i) {
692   if (oop_is_instance()) {
693     int limit = InstanceKlass::cast(this)->vtable_length()/vtableEntry::size();
694     assert(i >= 0 && i < limit, "index %d out of bounds %d", i, limit);
695   } else {
696     assert(oop_is_array(), "Must be");
697     int limit = ArrayKlass::cast(this)->vtable_length()/vtableEntry::size();
698     assert(i >= 0 && i < limit, "index %d out of bounds %d", i, limit);
699   }
700   return true;
701 }

Why are there any casts here at all?  vtable_length is a member of
Klass.  E.g. why not:

bool Klass::verify_vtable_index(int i) {
 int limit = vtable_length() / vtableEntry::size();
 assert(i >= 0 && i < limit, "index %d out of bounds %d, i, limit);
 return true;
}

------------------------------------------------------------------------------ 
src/share/vm/oops/klassVtable.cpp
 67   vtable_length = super == NULL ? 0 : InstanceKlass::cast(super)->vtable_length();

I don't think we need a cast here; vtable_length is a member of Klass.

------------------------------------------------------------------------------ 
src/share/vm/oops/klassVtable.cpp
130     // copy methods from superKlass
131     // can't inherit from array class, so must be InstanceKlass
132     // InstanceKlass::cast asserts that the Klass is an InstanceKlass
133     InstanceKlass* sk = InstanceKlass::cast(super());
134     klassVtable* superVtable = sk->vtable();
135     assert(superVtable->length() <= _length, "vtable too short");

New comment line 132 isn't very interesting, even if the cast is
retained.

But why bother casting at all?  Just

 klassVtable* superVtable = super->vtable();

should be sufficient.  And we don't actually care about whether it's
an ArrayKlass or InstanceKlass here.  If we did, then just assert that
the super is an InstanceKlass (as in the old code) would be better.

------------------------------------------------------------------------------
src/share/vm/oops/klass.hpp

Pre-existing: Why aren't Klass::vtable and Klass::vtable_length pure
virtual?

------------------------------------------------------------------------------
src/share/vm/oops/klassVtable.cpp
761   Klass* cursuper;
762   // Iterate on all superclasses, which should have instanceKlasses 
763   // Note that we explicitly look for overpasses at each level.
764   // Overpasses may or may not exist for supers for pass 1,
765   // they should have been created for pass 2 and later.
766 
767   for (cursuper = super; cursuper != NULL; cursuper = cursuper->super())
768   {
769      if (InstanceKlass::cast(cursuper)->find_local_method(name, signature,

I would put the declaration for cursuper in the for-loop
initialization, e.g.

 for (Klass* cursuper = super; ...) ...

Pre-existing:
762   // Iterate on all superclasses, which should have instanceKlasses 

"should have" => "should be" ?  Also need a period.

------------------------------------------------------------------------------ 
src/share/vm/oops/method.cpp
302   return method_holder()->name();

Thank you!

------------------------------------------------------------------------------ 
src/share/vm/services/threadService.cpp
888                    obj->klass()->external_name());
912                 waitingToLockBlocker->klass()->external_name());

Pre-existing: Shouldn't there be a ResourceMark somewhere nearby
associated with these calls to external_name?

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