Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle #5027

Closed
wants to merge 43 commits into from

Conversation

mlchung
Copy link
Member

@mlchung mlchung commented Aug 5, 2021

This reimplements core reflection with method handles.

For Constructor::newInstance and Method::invoke, the new implementation uses MethodHandle. For Field accessor, the new implementation uses VarHandle. For the first few invocations of one of these reflective methods on a specific reflective object we invoke the corresponding method handle directly. After that we spin a dynamic bytecode stub defined in a hidden class which loads the target MethodHandle or VarHandle from its class data as a dynamically computed constant. Loading the method handle from a constant allows JIT to inline the method-handle invocation in order to achieve good performance.

The VM's native reflection methods are needed during early startup, before the method-handle mechanism is initialized. That happens soon after System::initPhase1 and before System::initPhase2, after which we switch to using method handles exclusively.

The core reflection and method handle implementation are updated to handle chained caller-sensitive method calls [1] properly. A caller-sensitive method can define with a caller-sensitive adapter method that will take an additional caller class parameter and the adapter method will be annotated with @CallerSensitiveAdapter for better auditing. See the detailed description from [2].

Ran tier1-tier8 tests.

[1] https://bugs.openjdk.java.net/browse/JDK-8013527
[2] https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issues

  • JDK-8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle
  • JDK-8013527: calling MethodHandles.lookup on itself leads to errors

Reviewers

Contributors

  • Peter Levart <plevart@openjdk.org>
  • Claes Redestad <redestad@openjdk.org>
  • Mandy Chung <mchung@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5027/head:pull/5027
$ git checkout pull/5027

Update a local copy of the PR:
$ git checkout pull/5027
$ git pull https://git.openjdk.java.net/jdk pull/5027/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5027

View PR using the GUI difftool:
$ git pr show -t 5027

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5027.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 5, 2021

👋 Welcome back mchung! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot changed the title JDK-8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle Aug 5, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 5, 2021

@mlchung The following labels will be automatically applied to this pull request:

  • build
  • core-libs
  • hotspot-jfr
  • kulla
  • security
  • serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added security serviceability build core-libs kulla hotspot-jfr labels Aug 5, 2021
@mlchung
Copy link
Member Author

@mlchung mlchung commented Aug 6, 2021

/label remove kulla

@mlchung
Copy link
Member Author

@mlchung mlchung commented Aug 6, 2021

/label remove security

@openjdk openjdk bot removed the kulla label Aug 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 6, 2021

@mlchung
The kulla label was successfully removed.

@openjdk openjdk bot added rfr and removed security labels Aug 6, 2021
Copy link
Contributor

@AlanBateman AlanBateman left a comment

Good work.

useDirectMethodHandle = METHOD_MH_ACCESSOR;
} else if (val.equals("fields")) {
useDirectMethodHandle = FIELD_MH_ACCESSOR;
}
Copy link
Contributor

@AlanBateman AlanBateman Oct 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to see that property can be used to control both.

@mlchung
Copy link
Member Author

@mlchung mlchung commented Oct 28, 2021

Thanks all for the review. JEP 416 is now target to JDK 18. I'll integrate this PR today.

@mlchung
Copy link
Member Author

@mlchung mlchung commented Oct 28, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Oct 28, 2021

Going to push as commit c6339cb.
Since your change was applied there have been 16 commits pushed to the master branch:

  • 5a768f7: 8276054: JMH benchmarks for Fences
  • 6d8fa8f: 8255286: Implement ParametersTypeData::print_data_on fully
  • 63b9f8c: 8153490: Cannot setBytes() if incoming buffer's length is bigger than number of elements we want to insert.
  • cb989cf: 8275052: AArch64: Severe AES/GCM slowdown on MacOS for short blocks
  • c92f230: 8276110: Problemlist javax/swing/JMenu/4515762/bug4515762.java for macos12
  • 309acbf: 8275703: System.loadLibrary fails on Big Sur for libraries hidden from filesystem
  • abe52ae: 8275518: accessibility issue in Inet6Address docs
  • 85d8cd8: 8276100: Remove G1SegmentedArray constructor name parameter
  • a343fa8: 8275865: Print deoptimization statistics in product builds
  • bec977c: 8275917: Some locks shouldn't allow_vm_block
  • ... and 6 more: https://git.openjdk.java.net/jdk/compare/9a3e9542997860de79d07a4411b1007e9cd5c348...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 28, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 28, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 28, 2021

@mlchung Pushed as commit c6339cb.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@martin-g
Copy link

@martin-g martin-g commented Nov 13, 2021

Hi,

Apache Wicket tests started failing with JDK 18 b23 and I think it is caused by this PR.
I've sent an email to Rory O'Donnell and his team because we participate at https://wiki.openjdk.java.net/display/quality/Quality+Outreach
The email with the explanation of the problem could be seen at https://markmail.org/message/o3gw72bwsfrpaf2n

@cl4es
Copy link
Member

@cl4es cl4es commented Nov 14, 2021

@martin-g mutating static final fields with reflection setAccessible(true) is an ugly hack with partially undefined behavior that can lead to all manners of bugs.. but I think this is an unintentional behavior change.

@mlchung should verify, but it looks like the MethodHandles.unreflect API used internally in the new implementation is slightly stricter and ignores the setAccessible(true) for the trusted finality check. This added strictness is established behavior for the public MethodHandles.unreflect API point, which seem perfectly fine there (the MH API is strictly adhering to regular java access rules). For this new reflection implementation we should probably make an exception to that strictness to be perfectly backwards compatible.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 14, 2021

Mailing list message from Alan Bateman on core-libs-dev:

On 14/11/2021 00:59, Claes Redestad wrote:

:
@martin-g mutating static final fields with reflection `setAccessible(true)` is an ugly hack with partially undefined behavior that can lead to all manners of bugs.. but I think this is an unintentional behavior change.

@mlchung should verify, but it looks like the `MethodHandles.unreflect` API used internally in the new implementation is slightly stricter and ignores the `setAccessible(true)` for the trusted finality check. This added strictness is established behavior for the public `MethodHandles.unreflect` API point, which seem perfectly fine there (the MH API is strictly adhering to regular java access rules). For this new reflection implementation we should probably make an exception to that strictness to be perfectly backwards compatible.

If I read this correctly, a wicked Wicket test is making use of a
private method in java.lang.Class so it can hack jlr.Field and change
its internal "modifiers" field. I don't think the JDK should be expected
to keep crazy hacks like this working from release to release.

Are there many Wicket tests trying to modify static field fields? Have
you looked at using an agent to drop the final modifier from these
fields when loading the classes?

-Alan

@martin-g
Copy link

@martin-g martin-g commented Nov 14, 2021

Thank you for the comments!
There is no need to be harsh though! We just made use of old Reflection APIs. They were working for many years and according to StackOverflow Apache Wicket is not the only one doing this!
In our case it is done in a test case, so it is easy to rework it.

Are there many Wicket tests trying to modify static field fields?

No! We have just one failing test with JDK 18 b23.

In someone else's case it might be needed to "hack" a third party library.

As a good citizen I'm reporting it back to you!

martin-g added a commit to apache/wicket that referenced this issue Nov 14, 2021
Because it is not possible to modify it in JDK 18 b23.
The OpenJDK team is informed.
They will probably fix the regression. Or document it that it is not possible anymore to do such hacks.

openjdk/jdk#5027 (comment)
@cl4es
Copy link
Member

@cl4es cl4es commented Nov 14, 2021

@martin-g I wasn't making any judgement. To qualify why this is not something I think anyone should do: poking into reflection internals to make a static final field mutable opens you up to situations where some readers still see the old value. An optimizing compiler (JIT or AOT) may treat the value as constant and propagates it. Future JVM features might decide to be even more aggressive, and trust these fields even more completely. While it's unspecified what should happen after overwriting a static final field with reflection, it does work if you're lucky. The one-off use in Wicket for a test seem benign enough.

Alan: changing Field.modifiers still works, but dropping the final modifier is not enough for this to work in the new impl. It won't be hard to adapt to the new world. Users who relies on this today could for example opt-out of the new MH-based impl using -Djdk.reflect.useDirectMethodHandle=false and get the old behavior. I've checked using a minimal reproducer I extracted from the Wicket sources that this works.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 15, 2021

Mailing list message from Alan Bateman on core-libs-dev:

On 14/11/2021 22:56, Claes Redestad wrote:

:
Alan: changing `Field.modifiers` still works, but dropping the final modifier is not enough for this to work in the new impl. It won't be hard to adapt to the new world. Users who relies on this today could for example opt-out of the new MH-based impl using `-Djdk.reflect.useDirectMethodHandle=false` and get the old behavior. I've checked using a minimal reproducer I extracted from the Wicket sources that this works.

Sure, but I don't think that would be enough as Wicket would also need
to open java.lang and java.lang.reflect to allow it continue to access
private members of Class and Field. I assume the test started emitting
"Illegal reflective access ..." warnings in JDK 9 and it stopped working
in JDK 16, and somewhere along the line the maintainers must have added
--add-opens to get it to work. It's just not tenable, hopefully the
project will find a way to re-write that test.

-Alan

martin-g added a commit to apache/wicket that referenced this issue Nov 15, 2021
Since

commit 191de98 (HEAD -> master, origin/master)
Author: Martin Grigorov <mgrigorov@apache.org>
Date:   Sun Nov 14 21:56:30 2021 +0200

    Remove 'final' modifier from a static field

    Because it is not possible to modify it in JDK 18 b23.
    The OpenJDK team is informed.
    They will probably fix the regression. Or document it that it is not possible anymore to do such hacks.

    openjdk/jdk#5027 (comment)

org.apache.wicket.util.string.Strings#SESSION_ID_PARAM is no more 'final' so there is no need to change the modifiers
@martin-g
Copy link

@martin-g martin-g commented Nov 15, 2021

I have already fixed our build with apache/wicket@191de98 and apache/wicket@128125f
Depending on your decision whether to make it possible again to drop final for static fields I will either revert these changes or not.
The main purpose of my report is to let you know about this "regression".

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 15, 2021

Mailing list message from David Holmes on core-libs-dev:

Hi Alan,

On 15/11/2021 5:11 pm, Alan Bateman wrote:

On 14/11/2021 22:56, Claes Redestad wrote:

:
Alan: changing `Field.modifiers` still works, but dropping the final
modifier is not enough for this to work in the new impl. It won't be
hard to adapt to the new world. Users who relies on this today could
for example opt-out of the new MH-based impl using
`-Djdk.reflect.useDirectMethodHandle=false` and get the old behavior.
I've checked using a minimal reproducer I extracted from the Wicket
sources that this works.

Sure, but I don't think that would be enough as Wicket would also need
to open java.lang and java.lang.reflect to allow it continue to access
private members of Class and Field.

I think there may be a misunderstanding here, AFAICS they are using
reflection to remove the final-ness of a field in their own classes, not
modifying anything in Class or Field.

Cheers,
David
-----

I assume the test started emitting
"Illegal reflective access ..." warnings in JDK 9 and it stopped working
in JDK 16, and somewhere along the line the maintainers must have added
--add-opens to get it to work. It's just not tenable, hopefully the
project will find a way to re-write that test.

-Alan

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 15, 2021

Mailing list message from Alan Bateman on core-libs-dev:

On 15/11/2021 09:48, David Holmes wrote:

I think there may be a misunderstanding here, AFAICS they are using
reflection to remove the final-ness of a field in their own classes,
not modifying anything in Class or Field.

That's the outcome. To get there, they call a private method
getDeclaredFields0 on j.l.Class and also change the value of the private
"modifiers" field in jlr.Field. It's just not tenable to hack into
private members like this. Martin seems to have done the right thing and
removed it.

-Alan.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 15, 2021

Mailing list message from David Holmes on core-libs-dev:

On 15/11/2021 8:14 pm, Alan Bateman wrote:

On 15/11/2021 09:48, David Holmes wrote:

I think there may be a misunderstanding here, AFAICS they are using
reflection to remove the final-ness of a field in their own classes,
not modifying anything in Class or Field.

That's the outcome. To get there, they call a private method
getDeclaredFields0 on j.l.Class and also change the value of the private
"modifiers" field in jlr.Field. It's just not tenable to hack into
private members like this. Martin seems to have done the right thing and
removed it.

Apologies - the misunderstanding was mine.

David

-Alan.

@mlchung
Copy link
Member Author

@mlchung mlchung commented Nov 15, 2021

@martin-g Thanks for reporting this. Appreciated.

JEP 416 makes java.lang.reflect classes trusted that reveals this another attempt to change the value of the private final Field::modifiers field via reflection. Since JDK 12 after https://bugs.openjdk.java.net/browse/JDK-8210496, all security-sensitive fields in Field and other java.lang.reflect classes are filtered from reflective access. In other words, since Java 12, Field::modifiers cannot be found through reflection and hence it can't be used to change the value of the modifiers of a field. The implementation of JEP 416 hardens the restriction further to use the private method Class::getDeclaredFields0 to obtain a Field instance of the filtered modifiers field. To drop final from the modifiers, one should look into using an instrumentation agent, as Alan suggests.

@mlchung
Copy link
Member Author

@mlchung mlchung commented Nov 16, 2021

I further looked at the specification and implementation of Field::set and adding java.lang.reflect to the trusted final field package list does not relate to this issue. I was confused with the isTrustedFinalField check which is supposed to check if the given Field object has read-only access. Below clarifies the real reason why this hack no longer works.

If the underlying field is final, a Field object has write access if and only if

  • setAccessible(true) has succeeded for this Field object;
  • the field is non-static; and
  • the field's declaring class is not a hidden class; and
  • the field's declaring class is not a record class.

The hack dropping final from a static final Field makes a Field object which has only read-only access to get write access. With JEP 416, this hack no longer works because the read-only access is set according to the original field modifiers but not the Field object.

@mlchung mlchung deleted the reimplement-method-invoke branch Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs integrated