[9] RFR (S): 8153540: C2 intrinsic for Unsafe.allocateInstance doesn't properly filter out array classes

[John Rose]

On Apr 12, 2016, at 9:55 AM, Vladimir Ivanov <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
>> InstantiationException;
> 
> 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.

— John
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20160412/661aa70a/attachment.html>