On Mon, 15 Mar 2021 21:27:54 GMT, Coleen Phillimore <cole...@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. Thanks @coleenp @sspitsyn for reviewing! ------------- PR: https://git.openjdk.java.net/jdk/pull/3013
