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

Kim Barrett kim.barrett at oracle.com
Fri Oct 23 00:10:18 UTC 2015

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?

More information about the hotspot-dev mailing list