RFR 8139163: InstanceKlass::cast passes through NULL

[Coleen Phillimore]

On 10/23/15 4:30 PM, Kim Barrett wrote:
> 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.

Kim,

Thanks for reviewing this.

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

Yes, I can make this change.  This is better than a raw C cast.
>
> ------------------------------------------------------------------------------
> 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;
> }

True, I didn't get all of them.  I'll change this to your suggestion.  
It's much cleaner.  I was trying to be minimal here.
>
> ------------------------------------------------------------------------------
> 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.

Yes, good find.

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

Yes, I realized vtable is in both but was trying not to change as much, 
but I can clean this up too.
>
> ------------------------------------------------------------------------------
> src/share/vm/oops/klass.hpp
>
> Pre-existing: Why aren't Klass::vtable and Klass::vtable_length pure
> virtual?

I don't know!   yes, they should be and it feels a lot safer to have 
them pure virtual so nobody gets null/0 by mistake.
>
> ------------------------------------------------------------------------------
> 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; ...) ...

Yup.
>
> Pre-existing:
> 762   // Iterate on all superclasses, which should have instanceKlasses
>
> "should have" => "should be" ?  Also need a period.

ok.

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

Yes we were able to make method_holder() return InstanceKlass a while 
ago.   Hopefully Valhalla doesn't break it.
>
> ------------------------------------------------------------------------------
> 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?
>
> ------------------------------------------------------------------------------
>

There are a bunch of resource marks around in the probable callers. I'm 
not going to mess with this.

Do you want to see another webrev with the additional cleanups?

Thanks!
Coleen