[hs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
Boris Ulasevich
boris.ulasevich at bell-sw.com
Tue May 29 15:46:53 UTC 2018
Hi David,
ARM32 part of the change looks good for me.
Do you want me to apply latest (v4-incr?) patch and run tests?
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