RFR: 8299089: Instrument global jni handles with tag to make them distinguishable

[Axel Boldt-Christmas]

On Tue, 10 Jan 2023 08:26:50 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> All the tags are mutually exclusive.
>> _Note there is a minor difference in naming here. We just call them local, global or [j]weak. But in the jni.h and the API documentation jweak is also known as a Weak Global Ref or JNIWeakGlobalRefType, and global is a Global ref or JNIGlobalRefType. But they are still mutually exclusive and it is just the naming that both use the word global._
>> 
>> An alternative implementation would be to have the two bits mean global/local and strong/weak with the local weak combination being disallowed. I think the only benefit would be that it would fit more with the API naming that a global weak is global and weak tagged, a global is global tagged and local has no tags. 
>> But it would confuse the internal meaning of `global_handle`, does it include global weak or not? I think there is probably a better way of naming all these, but I also think the proposed change is the least disruptive to established nomenclature.
>> 
>> `is_global_tagged` is enough if you only look at the hotspot code. But we will get handles from the JNI external interface, and we want to check that the storage is correct in conjunction with checking that the tags are correct. (I think this is the idea at least. Similarly how before this change `is_weak_global_handle` both checked the tag and the storage, while just checking the tag would have been enough).
>> 
>> Looking at the code there is probably room for improvement here, as the `checked_jni_Delete*` which uses the `is_*_handle` functions also uses `jniCheck::validate_object` which only checks the tags (not the storage) before trying to look at the contents. So I am unsure what happens if the input handle has a bad tag/storage.
>
>> All the tags are mutually exclusive.
> 
> I see; could those tags be implemented using enum then? Sth like:
> 
> 
> enum Tag {
>   local = 0b00,
>   jweak = 0b01,
>   global = 0b10,
>   mask = 0b11,
> }
> 
> bool is_tagged_with(Handle h, Tag t) {
>   return (h & mask) == t.
> }
> 
> 
> This makes the "mutual exclusiveness" more explicit and can probaly remove some redundancy in `assert(is_jweak); assert(!is_global);`.
> 
> 
>> But we will get handles from the JNI external interface, and we want to check that the storage is correct in conjunction with checking that the tags are correct.
> 
> I don't know much about JNI; I thought all handles must be constructed using JNI APIs -- so, they must be correctly tagged.

That is probably clearer. Will take a look at it.

That is how it should be used and I think the effort is that if you do something bad you get a ReportJNIFatalError with some message instead of maybe crashing in the VM. But probably cleaner and more accurate if the code which checks the storage is move to `jniCheck::validate_handle` and have the handle checks just use the tag.

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

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