RFR 8139163: InstanceKlass::cast passes through NULL

[Kim Barrett]

On Oct 22, 2015, at 9:57 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
> 
> On 10/22/15 8:10 PM, Kim Barrett wrote:
>> 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.

Right you are.  Sorry for the noise.

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

ik doesn’t need to be used in that “if” statement; the old code used klass and was changed
in a way that isn’t strictly necessary.  And there is nothing about the original “if” statement
that requires it to be an InstanceKlass.  That said, there is the earlier klass->oop_is_array()
and since array and instance fully partition Klass, then you are right that at the new earlier
location of the cast we do know the cast is ok.  So no further change needed here.

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

I overlooked the earlier array check here too.  OK.

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

Because we don’t turn on unused-variable warnings.  Sigh.