[hs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
David Holmes
david.holmes at oracle.com
Mon Jun 4 11:03:57 UTC 2018
Thanks for the review Serguei.
David
On 4/06/2018 7:10 PM, serguei.spitsyn at oracle.com wrote:
> Hi David,
>
> It looks good.
> Nice approach to fix these tests.
>
> Thanks,
> Serguei
>
>
> On 6/4/18 00:15, David Holmes wrote:
>> This update fixes some tests that were being excluded temporarily, but
>> which can now run under nestmates.
>>
>> Incremental hotspot webrev:
>> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.hotspot.v5-incr/
>>
>>
>> Full hotspot webrev:
>> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.hotspot.v5/
>>
>> Change summary:
>>
>> - test/hotspot/jtreg/vmTestbase/nsk/stress/except/except004.java (see:
>> 8203046):
>>
>> The test expected an IllegalAccessException using reflection to access
>> a private field of a nested class. That's actually a reflection bug
>> that nestmates fixes. So relocated the Abra.PRIVATE_FIELD to a
>> top-level package-access class Ext
>>
>> - all other tests involve class redefinition (see: 8199450):
>>
>> These tests were failing because the RedefineClassHelper, if passed a
>> string containing "class A$B { ...}" doesn't define a nested class but
>> a top-level class called A$B (which is perfectly legal). The
>> redefinition itself would fail as the old class called A$B was a
>> nested class and you're not allowed to change the nest attributes in
>> class redefinition or transformation.
>>
>> The fix is simply to factor out the A$B class being redefined to being
>> a top-level package access class in the same source file, called A_B,
>> and with all references to "B" suitable adjusted.
>>
>> [The alternate fix considered would be to update the
>> RedefineClassHelper and its use of the InMemoryJavaCompiler so that
>> the tests would pass in a string like "class A { class B { ... } }"
>> and then read back the bytes for A$B with nest attributes intact. But
>> that is a non-trivial task and it isn't really significant that the
>> classes used in these tests were in fact nested.]
>>
>>
>> Thanks,
>> David
>>
>>
>> On 28/05/2018 9:20 PM, David Holmes wrote:
>>> I've added some missing JNI tests for the basic access checks. Given
>>> JNI ignores access it should go without saying that JNI access to
>>> nestmates will work fine, but it doesn't hurt to verify that.
>>>
>>> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.hotspot.v4-incr/
>>>
>>>
>>> Thanks,
>>> David
>>>
>>> On 24/05/2018 7:48 PM, David Holmes wrote:
>>>> Here are the further updates based on review comments and rebasing
>>>> to get the vmTestbase updates for which some closed test changes now
>>>> have to be applied to the open versions.
>>>>
>>>> Incremental hotspot webrev:
>>>> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.hotspot.v3-incr/
>>>>
>>>>
>>>> Full hotspot webrev:
>>>> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.hotspot.v3/
>>>>
>>>> Change summary:
>>>>
>>>> test/hotspot/jtreg/ProblemList.txt
>>>> - Exclude vmTestbase/nsk/stress/except/except004.java under 8203046
>>>>
>>>> test/hotspot/jtreg/vmTestbase/vm/runtime/defmeth/BasicTest.java
>>>> test/hotspot/jtreg/vmTestbase/vm/runtime/defmeth/PrivateMethodsTest.java
>>>>
>>>> - updated to work with new invokeinterface rules and nestmate changes
>>>> - misc cleanups
>>>>
>>>> src/hotspot/share/runtime/reflection.?pp
>>>> - rename verify_field_access to verify_member_access (it's always
>>>> been mis-named and I nearly forgot to do this cleanup!) and rename
>>>> field_class to member_class
>>>> - add TRAPS to verify_member_access to allow use with CHECK macros
>>>>
>>>> src/hotspot/share/ci/ciField.cpp
>>>> src/hotspot/share/classfile/classFileParser.cpp
>>>> src/hotspot/share/interpreter/linkResolver.cpp
>>>> - updated to use THREAD/CHECK with verify_member_access
>>>> - for ciField rename thread to THREAD so it can be used with
>>>> HAS_PENDING_EXCEPTION
>>>>
>>>> src/hotspot/share/oops/instanceKlass.cpp
>>>> - use CHECK_false when calling nest_host()
>>>> - fix indent near nestmate code
>>>>
>>>> src/hotspot/share/oops/instanceKlass.hpp
>>>> - make has_nest_member private
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>> On 23/05/2018 4:57 PM, David Holmes wrote:
>>>>> Here are the updates so far in response to all the review comments.
>>>>>
>>>>> Incremental webrev:
>>>>> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.hotspot.v2-incr/
>>>>>
>>>>>
>>>>> Full webrev:
>>>>> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.hotspot.v2/
>>>>>
>>>>> Change summary:
>>>>>
>>>>> test/runtime/Nestmates/reflectionAPI/*
>>>>> - moved to java/lang/reflect/Nestmates
>>>>>
>>>>> src/hotspot/cpu/arm/templateTable_arm.cpp
>>>>> - Fixed ARM invocation logic as provided by Boris.
>>>>>
>>>>> src/hotspot/share/interpreter/linkResolver.cpp
>>>>> - expanded comment regarding exceptions
>>>>> - Removed leftover debugging code
>>>>>
>>>>> src/hotspot/share/oops/instanceKlass.cpp
>>>>> - Removed FIXME comments
>>>>> - corrected incorrect comment
>>>>> - Fixed if/else formatting
>>>>>
>>>>> src/hotspot/share/oops/instanceKlass.hpp
>>>>> - removed unused debug method
>>>>>
>>>>> src/hotspot/share/oops/klassVtable.cpp
>>>>> - added comment by request of Karen
>>>>>
>>>>> src/hotspot/share/runtime/reflection.cpp
>>>>> - Removed FIXME comments
>>>>> - expanded comments in places
>>>>> - used CHECK_false
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>> On 15/05/2018 10:52 AM, 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