[hs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
David Holmes
david.holmes at oracle.com
Tue May 29 20:52:56 UTC 2018
Hi Boris,
On 30/05/2018 1:46 AM, Boris Ulasevich wrote:
> Hi David,
>
> ARM32 part of the change looks good for me.
> Do you want me to apply latest (v4-incr?) patch and run tests?
Please do.
Thanks,
David
> Boris
>
> On 24.05.2018 00:39, David Holmes wrote:
>> Hi Boris,
>>
>> Thanks for verifying my changes. In case you didn't see you can apply
>> the updated patch to my v1 changes from:
>>
>> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.hotspot.v2/
>>
>> One follow up below.
>>
>> On 23/05/2018 11:40 PM, Boris Ulasevich wrote:
>>> Hi David,
>>>
>>> some minor comments below..
>>>
>>> On 23.05.2018 09:57, David Holmes wrote:
>>>> Hi Boris,
>>>>
>>>> On 17/05/2018 7:23 PM, Boris Ulasevich wrote:
>>>>> Hi David,
>>>>>
>>>>> You are right! My bad, R2_tmp parameter conflicts with Rklass on
>>>>> check_klass_subtype(..) call. See correct patch in attach. Now all
>>>>> runtime/Nestmates tests passed! :)
>>>>
>>>> I went to aplpy your patch and found it's not a diff against my
>>>> patch but against the original non-nestmate code,
>>>
>>> Yes, sorry. For me it was natural to apply patch from your review to
>>> local repo, rework it and just send updated diff from my machine :)
>>>
>>>> so I want to be clear on the changes. AFAICS the differences are:
>>>>
>>>> - const Register Rklass = R3_tmp;
>>>> + const Register Rklass = R2_tmp; // Note! Same register with Rrecv
>>>
>>> Yes.
>>>
>>>> This initially concerned me as we stomp on Rrecv when we do the
>>>> load_klass, but then you moved the:
>>>>
>>>> __ load_klass(Rklass, Rrecv);
>>>>
>>>> to after the object case, which used Rrecv. I had assumed Rrecv was
>>>> somehow used when we actually do the call, but I'm assuming
>>>> jump_from_interpreted is done in such as way that the receiver is
>>>> still available on the interpreter stack and is used from there.
>>>
>>> Ok. I'm not sure I understand you well. When I moved load_klass call
>>> down my point was to skip unnecessary load for the notObjectMethod
>>> case (Rklass is not required in this case) and to make possible to
>>> reuse same R2 register by both Rklass and Rrecv register.
>>
>> Right. My concern was an assumption that as Rrecv was the receiver
>> object that we had to leave it intact ready for the actual method
>> invocation. But that isn't the case. The real method invocation
>> happens elsewhere and the receiver is obtained by other means.
>>
>> Thanks,
>> David
>>
>>>> - __ check_klass_subtype(Rklass, Rinterf, R1_tmp, R0_tmp, subtype);
>>>> + __ check_klass_subtype(Rklass, Rinterf, R1_tmp, R3_tmp, noreg,
>>>> subtype);
>>>>
>>>> Okay - reworking of tmp regs.
>>>
>>> Yes, ARM32 requires one more tmp reg, and we can't spoil R0.
>>>
>>>> - __ jump_from_interpreted(Rindex);
>>>> + __ jump_from_interpreted(Rmethod);
>>>>
>>>> I'm going to trust this is okay :) It's not at all clear to me how
>>>> the f2 Method* gets passed through on different platforms -
>>>> sometimes its in the "index" and sometimes the "method" registers.
>>>
>>> I see. On ARM/AARCH64 we have preallocated register Rmethod/rmethod
>>> to hold current method pointer and prepare_invoke call to setup
>>> registers properly.
>>>
>>>> (I _really_ wish there was consistency in terminology across the
>>>> different platforms - this code is awful for trying to compare the
>>>> different platforms to figure out what to do on a new one.)
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> regards,
>>>>> Boris
>>>>>
>>>>> On 17.05.2018 11:24, David Holmes wrote:
>>>>>> On 17/05/2018 6:13 PM, Boris Ulasevich wrote:
>>>>>>> Hi David,
>>>>>>>
>>>>>>> I see three tests failing:
>>>>>>> > NullPointerException at
>>>>>>> TestInterfaceMethodSelection.doInvoke(TestInterfaceMethodSelection.java:191)
>>>>>>>
>>>>>>> > NullPointerException at
>>>>>>> TestInvoke.access_priv(TestInvoke.java:54)
>>>>>>> > InvocationTargetException at
>>>>>>> TestReflection.access_priv(TestReflection.java:61)
>>>>>>>
>>>>>>> I will send you test details in a separate mail.
>>>>>>
>>>>>> Ok. This indicates a bug in the assembly code. The NPE's will
>>>>>> likely be SEGVs caused by a zero register.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>
>>>>>>> Boris
>>>>>>>
>>>>>>> On 17.05.2018 00:23, David Holmes wrote:
>>>>>>>> Hi Boris,
>>>>>>>>
>>>>>>>> Many thanks for looking at this and working through the ARM code.
>>>>>>>>
>>>>>>>> I will study your patch in detail but am concerned by the
>>>>>>>> "passes most of runtime/Nestmates tests Ok."! What tests are not
>>>>>>>> passing?
>>>>>>>>
>>>>>>>> David
>>>>>>>>
>>>>>>>> On 17/05/2018 1:05 AM, Boris Ulasevich wrote:
>>>>>>>>> Hi David,
>>>>>>>>>
>>>>>>>>> Let us look to the change in templateTable_arm.cpp. I have
>>>>>>>>> several notes.
>>>>>>>>>
>>>>>>>>> 1. We have compilation error because check_klass_subtype call
>>>>>>>>> needs one more temporary register parameter. I think correct
>>>>>>>>> change is:
>>>>>>>>> check_klass_subtype(Rklass, Rinterf, R1_tmp, R0_tmp, subtype);
>>>>>>>>> ->
>>>>>>>>> check_klass_subtype(Rklass, Rinterf, R1_tmp, R0_tmp, noreg,
>>>>>>>>> subtype);
>>>>>>>>>
>>>>>>>>> 2. R0_tmp holds mdp used for profilation several lines below,
>>>>>>>>> so we can't spoil it. I think correct change is:
>>>>>>>>> check_klass_subtype(Rklass, Rinterf, R1_tmp, R0_tmp, noreg,
>>>>>>>>> subtype);
>>>>>>>>> ->
>>>>>>>>> check_klass_subtype(Rklass, Rinterf, R1_tmp, R2_tmp, noreg,
>>>>>>>>> subtype);
>>>>>>>>>
>>>>>>>>> 3. We can't jump to Rindex. I believe it was supposed to jump
>>>>>>>>> to Rmethod:
>>>>>>>>> jump_from_interpreted(Rindex);
>>>>>>>>> ->
>>>>>>>>> jump_from_interpreted(Rmethod);
>>>>>>>>>
>>>>>>>>> 4. Please note than Rklass and Rflags reuses same register.
>>>>>>>>> Before the change their life ranges had no intersection. I
>>>>>>>>> would propose to use R2 for Rklass (same with Rrecv) and move
>>>>>>>>> load_klass call after invokevirtual_helper call to avoid life
>>>>>>>>> range intersecton.
>>>>>>>>>
>>>>>>>>> See my patch against original templateTable_arm.cpp version in
>>>>>>>>> attach - with this change ARM build compiles and passes most of
>>>>>>>>> runtime/Nestmates tests Ok.
>>>>>>>>>
>>>>>>>>> regards,
>>>>>>>>> Boris
>>>>>>>>>
>>>>>>>>> On 15.05.2018 03:52, David Holmes wrote:
>>>>>>>>>> This review is being spread across four groups: langtools,
>>>>>>>>>> core-libs, hotspot and serviceability. This is the specific
>>>>>>>>>> review thread for hotspot - webrev:
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.hotspot.v1/
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> See below for full details - including annotated full webrev
>>>>>>>>>> guiding the review.
>>>>>>>>>>
>>>>>>>>>> The intent is to have JEP-181 targeted and integrated by the
>>>>>>>>>> end of this month.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> David
>>>>>>>>>> -----
>>>>>>>>>>
>>>>>>>>>> The nestmates project (JEP-181) introduces new classfile
>>>>>>>>>> attributes to identify classes and interfaces in the same
>>>>>>>>>> nest, so that the VM can perform access control based on those
>>>>>>>>>> attributes and so allow direct private access between
>>>>>>>>>> nestmates without requiring javac to generate synthetic
>>>>>>>>>> accessor methods. These access control changes also extend to
>>>>>>>>>> core reflection and the MethodHandle.Lookup contexts.
>>>>>>>>>>
>>>>>>>>>> Direct private calls between nestmates requires a more general
>>>>>>>>>> calling context than is permitted by invokespecial, and so the
>>>>>>>>>> JVMS is updated to allow, and javac updated to use,
>>>>>>>>>> invokevirtual and invokeinterface for private class and
>>>>>>>>>> interface method calls respectively. These changed semantics
>>>>>>>>>> also extend to MethodHandle findXXX operations.
>>>>>>>>>>
>>>>>>>>>> At this time we are only concerned with static nest
>>>>>>>>>> definitions, which map to a top-level class/interface as the
>>>>>>>>>> nest-host and all its nested types as nest-members.
>>>>>>>>>>
>>>>>>>>>> Please see the JEP for further details.
>>>>>>>>>>
>>>>>>>>>> JEP: https://bugs.openjdk.java.net/browse/JDK-8046171
>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8010319
>>>>>>>>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8197445
>>>>>>>>>>
>>>>>>>>>> All of the specification changes have been previously been
>>>>>>>>>> worked out by the Valhalla Project Expert Group, and the
>>>>>>>>>> implementation reviewed by the various contributors and
>>>>>>>>>> discussed on the valhalla-dev mailing list.
>>>>>>>>>>
>>>>>>>>>> Acknowledgments and contributions: Alex Buckley, Maurizio
>>>>>>>>>> Cimadamore, Mandy Chung, Tobias Hartmann, Vladimir Ivanov,
>>>>>>>>>> Karen Kinnear, Vladimir Kozlov, John Rose, Dan Smith, Serguei
>>>>>>>>>> Spitsyn, Kumar Srinivasan
>>>>>>>>>>
>>>>>>>>>> Master webrev of all changes:
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Annotated master webrev index:
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/jep181-webrev.html
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Performance: this is expected to be performance neutral in a
>>>>>>>>>> general sense. Benchmarking and performance runs are about to
>>>>>>>>>> start.
>>>>>>>>>>
>>>>>>>>>> Testing Discussion:
>>>>>>>>>> ------------------
>>>>>>>>>>
>>>>>>>>>> The testing for nestmates can be broken into four main groups:
>>>>>>>>>>
>>>>>>>>>> - New tests specifically related to nestmates and currently
>>>>>>>>>> in the runtime/Nestmates directory
>>>>>>>>>>
>>>>>>>>>> - New tests to complement existing tests by adding in
>>>>>>>>>> testcases not previously expressible.
>>>>>>>>>> - For example java/lang/invoke/SpecialInterfaceCall.java
>>>>>>>>>> tests use of invokespecial for private interface methods and
>>>>>>>>>> performing receiver typechecks, so we add
>>>>>>>>>> java/lang/invoke/PrivateInterfaceCall.java to do similar tests
>>>>>>>>>> for invokeinterface.
>>>>>>>>>>
>>>>>>>>>> - New JVM TI tests to verify the spec changes related to nest
>>>>>>>>>> attributes.
>>>>>>>>>>
>>>>>>>>>> - Existing tests significantly affected by the nestmates
>>>>>>>>>> changes, primarily:
>>>>>>>>>> - runtime/SelectionResolution
>>>>>>>>>>
>>>>>>>>>> In most cases the nestmate changes makes certain
>>>>>>>>>> invocations that were illegal, legal (e.g. not requiring
>>>>>>>>>> invokespecial to invoke private interface methods; allowing
>>>>>>>>>> access to private members via reflection/Methodhandles that
>>>>>>>>>> were previously not allowed).
>>>>>>>>>>
>>>>>>>>>> - Existing tests incidentally affected by the nestmate changes
>>>>>>>>>>
>>>>>>>>>> This includes tests of things utilising class
>>>>>>>>>> redefinition/retransformation to alter nested types but which
>>>>>>>>>> unintentionally alter nest relationships (which is not
>>>>>>>>>> permitted).
>>>>>>>>>>
>>>>>>>>>> There are still a number of tests problem-listed with issues
>>>>>>>>>> filed against them to have them adapted to work with
>>>>>>>>>> nestmates. Some of these are intended to be addressed in the
>>>>>>>>>> short-term, while some (such as the
>>>>>>>>>> runtime/SelectionResolution test changes) may not eventuate.
>>>>>>>>>>
>>>>>>>>>> - https://bugs.openjdk.java.net/browse/JDK-8203033
>>>>>>>>>> - https://bugs.openjdk.java.net/browse/JDK-8199450
>>>>>>>>>> - https://bugs.openjdk.java.net/browse/JDK-8196855
>>>>>>>>>> - https://bugs.openjdk.java.net/browse/JDK-8194857
>>>>>>>>>> - https://bugs.openjdk.java.net/browse/JDK-8187655
>>>>>>>>>>
>>>>>>>>>> There is also further test work still to be completed (the JNI
>>>>>>>>>> and JDI invocation tests):
>>>>>>>>>> - https://bugs.openjdk.java.net/browse/JDK-8191117
>>>>>>>>>> which will continue in parallel with the main RFR.
>>>>>>>>>>
>>>>>>>>>> Pre-integration Testing:
>>>>>>>>>> - General:
>>>>>>>>>> - Mach5: hs/jdk tier1,2
>>>>>>>>>> - Mach5: hs-nightly (tiers 1 -3)
>>>>>>>>>> - Targetted
>>>>>>>>>> - nashorn (for asm changes)
>>>>>>>>>> - hotspot: runtime/*
>>>>>>>>>> serviceability/*
>>>>>>>>>> compiler/*
>>>>>>>>>> vmTestbase/*
>>>>>>>>>> - jdk: java/lang/invoke/*
>>>>>>>>>> java/lang/reflect/*
>>>>>>>>>> java/lang/instrument/*
>>>>>>>>>> java/lang/Class/*
>>>>>>>>>> java/lang/management/*
>>>>>>>>>> - langtools: tools/javac
>>>>>>>>>> tools/javap
>>>>>>>>>>
More information about the hotspot-dev
mailing list