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

Boris Ulasevich boris.ulasevich at bell-sw.com
Wed May 30 09:04:25 UTC 2018


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