RFR 8139163: InstanceKlass::cast passes through NULL (original) (raw)

Coleen Phillimore coleen.phillimore at oracle.com
Fri Oct 23 21:34:10 UTC 2015

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 (oopisinstance()) { 564 InstanceKlass* ik = InstanceKlass::cast(constcast<Klass*>(this)); Instead of casting away const and using InstanceKlass::cast, how about just const InstanceKlass* ik = staticcast<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::verifyvtableindex(int i) { 692 if (oopisinstance()) { 693 int limit = InstanceKlass::cast(this)->vtablelength()/vtableEntry::size(); 694 assert(i >= 0 && i < limit, "index %d out of bounds %d", i, limit);_ _695 } else {_ _696 assert(oopisarray(), "Must be");_ _697 int limit = ArrayKlass::cast(this)->vtablelength()/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? vtablelength is a member of_ _Klass. E.g. why not:_ _bool Klass::verifyvtableindex(int i) {_ _int limit = vtablelength() / 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 vtablelength = super == NULL ? 0 : InstanceKlass::cast(super)->vtablelength(); I don't think we need a cast here; vtablelength 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::vtablelength 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)->findlocalmethod(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 methodholder()->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()->externalname()); 912 waitingToLockBlocker->klass()->externalname()); Pre-existing: Shouldn't there be a ResourceMark somewhere nearby associated with these calls to externalname? ------------------------------------------------------------------------------

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

More information about the hotspot-dev mailing list