[hs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control

David Holmes david.holmes at oracle.com
Wed May 30 09:42:37 UTC 2018


Good to hear! Thanks Boris.

David

On 30/05/2018 7:04 PM, Boris Ulasevich wrote:
> Hi David,
> 
>    With v2,v3,v4 patches applied I have runtime/Nestmates passed on ARM32.
> 
> regards,
> Boris
> 
> On 29.05.2018 23:52, David Holmes wrote:
>> 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