Skip to content

Enable LTW in JDK 16+ without --add-opens #117

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

Closed
kriegaex opened this issue Jan 18, 2022 · 7 comments · Fixed by #275
Closed

Enable LTW in JDK 16+ without --add-opens #117

kriegaex opened this issue Jan 18, 2022 · 7 comments · Fixed by #275
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@kriegaex
Copy link
Contributor

Since 1.9.7, the AspectJ release notes say:

Use LTW on Java 16+

Please note that if you want to use load-time weaving on Java 16+, the weaving agent collides with JEP 396 (Strongly Encapsulate JDK Internals by Default). Therefore, you need to set the JVM parameter --add-opens java.base/java.lang=ALL-UNNAMED in order to enable aspect weaving. This is due to the fact that the weaver uses internal APIs for which we have not found an adequate replacement yet when defining classes in different classloaders.

@aclement, do you have any ideas how to solve this?

See also:

Maybe taking a look at how other tools like Byte Buddy or Javassist try to define classes in other class-loaders can be helpful, e.g. https://github.com/jboss-javassist/javassist/blob/master/src/main/javassist/util/proxy/DefineClassHelper.java.

@aclement
Copy link
Contributor

@aclement, do you have any ideas how to solve this?

nope.

@KillerJmc
Copy link

This is a big issue. I don't want to write this to my vm option.

@kriegaex
Copy link
Contributor Author

kriegaex commented Feb 18, 2023

I don't want to write this to my vm option.

@KillerJmc, nobody does. But it is necessary. Feel free to either complain to the OpenJDK team, to suggest a technically viable strategy to solve the problem or provide a pull request. Alternatively, use compile-time instead of load-time weaving.

Hi @mlchung, is JDK-8200559 still scheduled to be fixed in JDK 21? If so, can you please elaborate on how we could in the future use the corresponding new JDK feature? You were talking about this issue with AspectJ project lead @aclement before in Bugzilla 546305. This is still a major issue for byte code engineering tools such as Byte Buddy or AspectJ.

kriegaex added a commit that referenced this issue Jan 28, 2024
### TODO: Add better description and force-push, if tests are OK. ###

Fixes #117.

Signed-off-by: Alexander Kriegisch <[email protected]>
kriegaex added a commit that referenced this issue Jan 28, 2024
Overhaul ClassLoaderWeavingAdaptor to use statically initialised Unsafe
instances and method handles pointing to their 'defineClass' methods.
Those now work universally on JDKs 8-21. In older JDKs, the method used
to be in sun.misc.Unsafe, in more recent ones on jdk.internal.misc.Unsafe.
It is challenging to fetch instances, especially as reflection
protection and module boundaries have been increased in the JDK
progressively. But finally, a solution was adapted from Byte Buddy (BB).
Kudos to BB author Rafael Winterhalter. The previous solution to use
ClassLoader::defineClass and require '--add-opens' is no longer
necessary for the first time since it became necessary in AspectJ 1.9.7
with Java 16 support.

Add org.ow2.asm:asm-common as a dependency everywhere org.ow2.asm:asm
was used before. Maybe that is too many places, but no worse than before.

Add missing dependency to loadtime to aspectjweaver. This kept a build
like "mvn install -am -pl aspectjweaver" from picking up changed
loadtime classes.

Fixes #117.

Signed-off-by: Alexander Kriegisch <[email protected]>
kriegaex added a commit that referenced this issue Jan 29, 2024
Overhaul ClassLoaderWeavingAdaptor to use statically initialised Unsafe
instances and method handles pointing to their 'defineClass' methods.
Those now work universally on JDKs 8-21. In older JDKs, the method used
to be in sun.misc.Unsafe, in more recent ones on jdk.internal.misc.Unsafe.
It is challenging to fetch instances, especially as reflection
protection and module boundaries have been increased in the JDK
progressively. But finally, a solution was adapted from Byte Buddy (BB).
Kudos to BB author Rafael Winterhalter. The previous solution to use
ClassLoader::defineClass and require '--add-opens' is no longer
necessary for the first time since it became necessary in AspectJ 1.9.7
with Java 16 support.

Add org.ow2.asm:asm-common as a dependency everywhere org.ow2.asm:asm
was used before. Maybe that is too many places, but no worse than before.

Add missing dependency on loadtime to aspectjweaver. This kept a build
like "mvn install -am -pl aspectjweaver" from picking up changed
loadtime classes.

Fixes #117.

Signed-off-by: Alexander Kriegisch <[email protected]>
kriegaex added a commit that referenced this issue Jan 29, 2024
Overhaul ClassLoaderWeavingAdaptor to use statically initialised Unsafe
instances and method handles pointing to their 'defineClass' methods.
Those now work universally on JDKs 8-21. In older JDKs, the method used
to be in sun.misc.Unsafe, in more recent ones on jdk.internal.misc.Unsafe.
It is challenging to fetch instances, especially as reflection
protection and module boundaries have been increased in the JDK
progressively. But finally, a solution was adapted from Byte Buddy (BB).
Kudos to BB author Rafael Winterhalter. The previous solution to use
ClassLoader::defineClass and require '--add-opens' is no longer
necessary for the first time since it became necessary in AspectJ 1.9.7
with Java 16 support.

Add org.ow2.asm:asm-common as a dependency everywhere org.ow2.asm:asm
was used before. Maybe that is too many places, but no worse than before.

Add missing dependency on loadtime to aspectjweaver. This kept a build
like "mvn install -am -pl aspectjweaver" from picking up changed
loadtime classes.

Fixes #117.

Signed-off-by: Alexander Kriegisch <[email protected]>
@kriegaex kriegaex self-assigned this Jan 29, 2024
@kriegaex kriegaex added this to the 1.9.21.1 milestone Jan 29, 2024
@kriegaex
Copy link
Contributor Author

We finally have a solution for this, and I am actually a little bit proud of the achievement, even though part of the credit goes to Byte Buddy, where I found something to adapt into AspectJ, but using ASM directly instead (which is also used under the hood in BB, even though not so visible in the code there). The relevant code sections in ClassLoaderWeavingAdaptor were overhauled and streamlined.

What I am not proud of is that we still need Unsafe. But JDK-8200559 is still open and unsolved since 2018, going on 6 years now. I kicked off some new discussion on GitHub in a related PR, which also led to new mailing list discussion triggered by Rafael Winterhalter (author of Byte Buddy). The solution he originally devised in his PR would have been simple to adapt by AspectJ. The one he suggests now is much more complicated to satisfy module security concerns by the JDK team. I think, it would be quite a nightmare for us to adapt, but on top of that the present solution would still stay in AspectJ for a long time, because the existing JDKs do not have that hypothetical new API.

@kriegaex
Copy link
Contributor Author

kriegaex commented Jan 29, 2024

@aclement, what I have no idea about is whether LTW used to work on GraalVM (never tried using it) before and, if so, it would still work after this change. Maybe you or one of your team mates could verify this. It might have an impact on Spring Native too, I have no idea.

@KillerJmc
Copy link

KillerJmc commented Feb 9, 2024

Wow! Awesome! Please notify me when the new version is released.

@kriegaex
Copy link
Contributor Author

kriegaex commented Feb 9, 2024

You should not expect me to personally notify you of a new release. I already send notifications to the AspectJ users and announcements mailing lists, and you can easily subscribe to GitHub releases using the "Watch" button and then customise the notifications of your choice.

image

image

Please also refrain from needless full quotes. I just edited your post. Thank you.

kriegaex added a commit that referenced this issue Apr 7, 2024
Relates to #117. In commit f986c3d, the asm-commons dependency became
necessary to pull off the new trick to define classes in arbitrary class
loaders during LTW. The dependency was added to aspectjweaver, but not
to aspectjtools due to an oversight. As aspectjtools is meant to be a
super set of aspectjweaver, add the dependency to the assembly
descriptor.

Signed-off-by: Alexander Kriegisch <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants