[hs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
David Holmes
david.holmes at oracle.com
Wed May 23 21:39:15 UTC 2018
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