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

Coleen Phillimore coleen.phillimore at oracle.com
Fri Oct 23 01:57:20 UTC 2015

Hi Kim, Thank you for wading through this in detail! I really appreciate it.

On 10/22/15 8:10 PM, Kim Barrett 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. ------------------------------------------------------------------------------ src/share/vm/classfile/systemDictionary.cpp 1850 if ((*klassp) == NULL) { 1851 Klass* k; 1852 if (mustload) { 1853 k = resolveorfail(symbol, true, CHECK0); // load required class 1854 } else { 1855 k = resolveornull(symbol, CHECK0); // 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.

Yes, a null check is needed here.

 (*klassp) = (k == NULL) ? NULL : InstanceKlass::cast(k);

------------------------------------------------------------------------------ src/share/vm/gc/g1/g1CollectedHeap.cpp 4595 // this can be null so don't call InstanceKlass::cast 4596 return staticcast<InstanceKlass*>(klass); But what if it's a non-NULL non-InstanceKlass? Maybe add assert(klass == NULL || klass->isinstance(), ...).

But that's the exactly the line above it.

------------------------------------------------------------------------------ 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 (inimethodresolve && 316 result != NULL && 317 ik->isinterface() && 318 (result->isstatic() || !result->ispublic()) && 319 result->methodholder() == SystemDictionary::Objectklass()) { 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.

It is an InstanceKlass at this point and I use 'ik' in this 'if' statement so I need it here.

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 (!resolvedmethod->isabstract() && 383 (InstanceKlass::cast(klass())->defaultmethods() != NULL)) { 384 int index = InstanceKlass::findmethodindex(ik->defaultmethods(), 385 name, signature, Klass::findoverpass, 386 Klass::findstatic, Klass::findprivate); 387 if (index >= 0 ) { 388 vtableindex = ik->defaultvtableindices()->at(index); 389 } 390 } 391 if (vtableindex == Method::invalidvtableindex) { 392 // get vtableindex for miranda methods 393 ResourceMark rm; 394 klassVtable *vt = ik->vtable(); 395 vtableindex = vt->indexofmiranda(name, signature); 396 } Another case where moving the InstanceKlass::cast earlier might be an unintended behavioral change. For example, if resolvedmethod->isabstract() is true and the vtableindex 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.

I need 'ik' here too and it is an InstanceKlass (we bailed out already for array klass), but I missed a cast at line 383 that I don't need.

The InstanceKlass::cast() has no cost in product mode or side effects so it's not something that needs to be optimized if the resolve_method is abstract.

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

No idea.

------------------------------------------------------------------------------ src/share/vm/oops/instanceKlass.cpp 1543 bool InstanceKlass::hasredefinedthisorsuper() const { 1544 Klass* klass = constcast<InstanceKlass*>(this); 1545 while (klass != NULL) { 1546 if (InstanceKlass::cast(klass)->hasbeenredefined()) { 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*. Because I can't pass const Klass* into InstanceKlass::cast(Klass* k) and it's a bigger change that I can't deal with to make InstanceKlass::cast take and return const Klass. So I can rewrite it like this.

I decided it looked best to take out const for the function itself. It's not called in a context where it matters, although I like const this pointers.

------------------------------------------------------------------------------ src/share/vm/oops/instanceKlass.cpp 2326 bool InstanceKlass::issameclasspackage(Klass* class2) { 2348 bool InstanceKlass::issameclasspackage(oop classloader2, Symbol* classname2) { Thank you! ------------------------------------------------------------------------------ src/share/vm/oops/instanceKlass.cpp 2898 InstanceKlass* ik = constcast<InstanceKlass*>(this); Casting away const. Ick! Can we have an RFE to fix this?

No. We have enough other things to clean up.

Thanks! Coleen

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

More information about the hotspot-dev mailing list