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

[Christian Thalinger]

> 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
>>> 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.

-1 (for obvious reasons)

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20160412/8aabe708/attachment-0001.html>