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

[Vladimir Ivanov]

Thanks for the feedback, John.

I see your point.

Actually, after looking at check_valid_for_instantiation more carefully, 
I found one more missing check in the intrinsic: Class instantiation is 
forbidden in InstanceKlass::check_valid_for_instantiation, but the 
intrinsic allows it.

So I agree it would be desirable to minimize duplication in the code.

Am I right that you are in favor of the following approach?

   http://cr.openjdk.java.net/~vlivanov/8153540/webrev.slow_path/

I'll experiment to see how does it shape out in both cases.

Best regards,
Vladimir Ivanov

On 4/12/16 11:09 PM, John Rose 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.
>
> — John