RFR (S): 8153540: C2 intrinsic for Unsafe.allocateInstance doesn't properly filter out array classes
christian.thalinger at oracle.com
Tue Apr 12 20:40:14 UTC 2016
> On Apr 12, 2016, at 10:09 AM, John Rose <john.r.rose at oracle.com> wrote:
> On Apr 12, 2016, at 9:55 AM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com <mailto:vladimir.x.ivanov at oracle.com>> wrote:
>> On 4/12/16 7:33 PM, Vladimir Kozlov wrote:
>>> You did not fix comment:
>>> + // public native Object Unsafe.allocateInstance(Class<?> cls);
>>> should be:
>>> + // private native Object allocateInstance0(Class<?> cls) throws
>> Ok, finally found where it is :-)
>> Incorporated (will update the webrev shortly).
>>> An other question: does it really throw InstantiationException?
>> Yes, it does throw IE from runtime call on slow path for abstract classes & interfaces (they have slow bit set in layout_helper).
>> I didn't move the check into Java, because I didn't want to add yet another guard on fast path.
> A fix is necessary, but I'm not comfortable with the shape of the checking logic.
> The C-coded JNI function (not the intrinsic) just surfaces the function JNIEnv::AllocObject.
> This function calls some complicated C++ logic in Klass::check_valid_for_instantiation
> to check for various things, including arrays and abstracts. (There's also a primitive check.)
> So the problem is that the JIT intrinsic doesn't mimic all these checks.
> And a good tactic is to lift such checks into Java code, since Java is maintainable.
> But, there is still a maintenance problem: The checks in the proposed chance
> overlap with, but do not cover, the checks performed by JNIEnv::AllocObject.
> Thus, it is difficult to prove that they are correct. Some additional checks are
> performed (in an ad hoc manner) by the JIT intrinsic.
> Thus, the checking for a valid class is now in three places: 1. JNIEnv::AllocObject
> (when the intrinsic is not used), 2. the new Java code (whether the intrinsic is used
> or not), and 3. the partial checks in the intrinsic code (library_call.cpp).
> The unit test will prevent regressions, but the code is still messy and hard to work with.
> Can we make it better at this point? Maybe not; maybe this is the least-bad point fix.
> But it seems to me that a less-bad fix would put the required logic in two places
> rather than three. Two ways to do that are 1. push the prim and array checks from
> Java down into the JIT intrinsic, next to the pre-existing checks, or 2. pull the
> pre-existing JIT intrinsic tests up into Java. Option 2 seems to require a new
> intrinsic to capture the pre-existing intrinsic tests.
> On the whole, since this Unsafe API point simply exposes JNIEnv::AllocObject,
> I suggest doing the necessary work in library_call.cpp to make the intrinsic
> accurately reflect that JNI function. That will make the checks easier to verify
> and maintain. I don't think (AM I REALLY SAYING THIS?) the Java-based checks
> help much in this particular case.
-1 (for obvious reasons)
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the hotspot-compiler-dev