RFR: 8293207: Add assert to JVM_ReferenceRefersTo to clarify its API

Axel Boldt-Christmas aboldtch at openjdk.org
Fri Sep 2 07:12:42 UTC 2022


On Fri, 2 Sep 2022 00:22:27 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> JVM_ReferenceRefersTo should only be used for soft/weak references and not phantom references, which has its own implementation. This is not clear from the name. `java_lang_ref_Reference::weak_referent_no_keepalive(ref_oop)` is only a valid call if `!java_lang_ref_Reference::is_phantom(ref_oop)`.
>> 
>> Adds an assert and comment to make this clear.
>> 
>> Testing: Oracle platforms tier 1-3
>
> src/hotspot/share/prims/jvm.cpp line 3286:
> 
>> 3284:   oop ref_oop = JNIHandles::resolve_non_null(ref);
>> 3285:   // PhantomReference has it's own implementation of refersTo().
>> 3286:   assert(!java_lang_ref_Reference::is_phantom(ref_oop), "precondition");
> 
> Maybe instead assertions should be added to the java_lang_ref_Reference functions for accessing the referent?  (weak_referent_no_keepalive, weak_referent, phantom_referent_no_keepalive)

I fell like that is some what orthogonal to what this PR is trying to address. This wants to make the precondition clear for the JVM entry API. As a PhantomReference is a Reference it was not clear from JVM_ReferenceRefersTo entry that it is not the entry for PhantomReference. This assert and comment aims to make this clear.

But I do agree that all the accessors in `java_lang_ref_Reference` should assert the type. I will make another issue with the following change.

oop java_lang_ref_Reference::weak_referent_no_keepalive(oop ref) {
+ assert(!java_lang_ref_Reference::is_phantom(ref_oop), "must be Weak or Soft Reference");
  return ref->obj_field_access<ON_WEAK_OOP_REF | AS_NO_KEEPALIVE>(_referent_offset);
}

oop java_lang_ref_Reference::weak_referent(oop ref) {
+ assert(!java_lang_ref_Reference::is_phantom(ref_oop), "must be Weak or Soft Reference");
  return ref->obj_field_access<ON_WEAK_OOP_REF>(_referent_offset);
}

oop java_lang_ref_Reference::phantom_referent_no_keepalive(oop ref) {
+ assert(java_lang_ref_Reference::is_phantom(ref_oop), "must be Phantom Reference");
  return ref->obj_field_access<ON_PHANTOM_OOP_REF | AS_NO_KEEPALIVE>(_referent_offset);
}

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

PR: https://git.openjdk.org/jdk/pull/10117


More information about the hotspot-dev mailing list