RFR: 8263589: Introduce JavaValue::get_oop/set_oop

[Coleen Phillimore]

On Mon, 15 Mar 2021 12:35:47 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

> JavaValue is a small wrapper class that wraps values used to pass arguments and results between native and Java.
> 
> When JavaCalls::call returns an object, the value stored in the JavaValue is not a handliezed jobject. Instead it's a raw oop. So, most of the code handling the `result`, fetches the result as a jobject, and then immediately casts it to an oop. For example:
>   oop res = (oop)result.get_jobject();
> 
> I'd like to change this code to be:
>   oop res = result.get_oop();
> 
> The motivations for this patch is:
> 
> 1) Minimize the places where we pass around oops in jobject variables. Maybe at some point we'll have converted the JVM to only use the jobject type when passing around JNI handle. We need to be stricter with the types when we continue develop our GCs and their barriers.
> 
> 2) Limit the number of places in the code where we perform raw oop casts. We have a helper cast function for that, cast_to_oop, but not all code use it. I have future patches where the compiler will completely forbid raw cast to oops (in fastdebug builds). With that in place, I can then add more stricter oop verification code when oops are created. This helps catching bugs earlier.
> 
> ---
> 
> When reviewing this patch, take an extra look at the change  to oopsHierarchy.hpp. This was done to support jvmciEnv.cpp code:
>   JVMCIObject wrap(oop obj)...
>   JVMCIObjectArray wrap(objArrayOop obj)...
>   JVMCIPrimitiveArray wrap(typeArrayOop obj) ...
> Previously, `wrap((oop)result.get_jobject())` called the first function. When the code was changed to `wrap(result.get_oop())`, where `get_oop()` returns a `oopDesc*`, the compiler didn't know what conversion in oopsHierarchy.hpp to use. Therefore, I replaced the overly permissive `void*` constructor with a constructor that only takes the corresponding `type##OopDesc*`.
> 
> An alternative would be to let get_oop() return an oop, but then that would add an unwanted a dependency between globalDefinitions.hpp and  oopsHierarchy.hpp. An earlier version of this patch did return an oop instead of oopDesc*, but it also moved entire JavaValue class out of globalDefinitions.hpp into a new javaValue.hpp file, and had a corresponding javaValue.inline.hpp file.
> 
> Even if we end up using the proposed `oopDesc* get_oop()` version, maybe moving the class to javaValues.hpp would still makes sense?

This change looks really good to me.  I have no objection to oopDesc* in JavaCallValue.  We use oopDesc* in all places where the class oop would interfere with values passed between Java and the vm.

src/hotspot/share/utilities/globalDefinitions.hpp line 809:

> 807:     jint     i;
> 808:     jlong    l;
> 809:     jobject  h;

Do we still need jobject after this change?

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

Marked as reviewed by coleenp (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3013