RFR 8139163: InstanceKlass::cast passes through NULL
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 (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.
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 static_cast<InstanceKlass*>(klass);
>
> But what if it's a non-NULL non-InstanceKlass?
> Maybe add assert(klass == NULL || klass->is_instance(), ...).
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 (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.
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 (!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.
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::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*.
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::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?
No. We have enough other things to clean up.
Thanks!
Coleen
>
> ------------------------------------------------------------------------------
>
More information about the hotspot-dev
mailing list