-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Fix substratevm after 'JDK-8347287: JFR: Remove use of Security Manager' #10529
Fix substratevm after 'JDK-8347287: JFR: Remove use of Security Manager' #10529
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. This PR looks good to me. I left a few minor notes. Yes, mx native-unittest
will run all the JFR tests in com.oracle.svm.test.jfr
. Those are the only ones.
} | ||
@Alias | ||
@TargetElement(name = TargetElement.CONSTRUCTOR_NAME) | ||
native void originalConstructor(String p); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need originalConstructor
instead of just
@Alias
Target_jdk_jfr_internal_SecuritySupport_SafePath(String p) {
}
|
||
import jdk.jfr.internal.SecuritySupport.SafePath; | ||
import jdk.jfr.internal.SecuritySupport; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a few places like here where import jdk.jfr.internal.SecuritySupport;
is no longer needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think that SubstrateJVM.getDumpPath()
now needs to accommodate the case where SecuritySupportSafePathPresent
is false. In that case Target_jdk_jfr_internal_SecuritySupport.getPathInProperty
won't be available. In the latest version of Hotspot JFR code, it looks like the default path is the current working directory.
throw VMError.shouldNotReachHere("Paths from the image build must not be embedded into the Native Image."); | ||
} | ||
|
||
@Alias | ||
static native SafePath getPathInProperty(String prop, String subPath); | ||
@Alias @TargetElement(onlyWith = SecuritySupportSafePathPresent.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all these @TargetElement(onlyWith = SecuritySupportSafePathPresent.class)
can be applied to the @TargetClass
instead to make things a bit simpler
Thanks for the PR but we already have an internal PR with similar changes. We do update Native Image to the latest OpenJDK tag approximately once per week. So, I am closing this PR. |
Thanks for you contribution @simonis! I have not yet looked at your PR, but some general comments:
The Graal (native image and compiler) master branch is always only compatible to exactly one JDK version, namely the version that is specified in common.json. Supporting more than one version in one codebase is not maintainable (nor testable) and also defies our goal of aligning development more closely to https://github.com/openjdk/jdk. (Currently, there is still JDK 21 support, but that is hopefully going away soon, so lets ignore this.) We adopt every new promoted JDK build, so the compatible JDK version of Graal is bumped once a week. So a new JDK change that requires Graal adoption will be dealt with in the PR that updates the JDK. JDK-8347287 went in with JDK 25+6 and there is already a PR (#10513) that adopts this change, at least to the extent that all gates pass. So bottom line: for JDK adoptions always look out for our adoption PRs. Of course, our adoptions might not cover every single detail. In that case, we are grateful for follow-up PRs from the community after the JDK update has been merged. |
@roberttoyonaga, thanks a lot for your helpful review. Unfortunately, Oracle has closed this PR in favor of their own fix in #10513. @christianhaeubl, @zapster, thanks for the additional information. I'm fine if you fix this with your own changes in #10513. Nevertheless I want to mention, that before I started to work on this fix, I looked at the open GitHub issues and pull requests and couldn't find anything related to this issue. It is very unfortunate that the Graal project still doesn't have a public issue tracker. This makes it impossible for the community to follow-up on what you are working internally (in this case [GR-61425]) which can lead to double work and frustration. Thanks for your efforts, |
I understand the frustration in general. However for JDK adoptions, public issues would not really help a lot. GR-61425 literally says "Merge in latest tag jdk-25+6 from upstream repositories" and is generated automatically every week for every promoted JDK build. When it comes to JDK adoptions, my advice to avoid double work and frustration is the following:
|
JDK-8347287: JFR: Remove use of Security Manage which has removed the SecurityManager from the JFR files has screwed up the substratevm and the native image build.
This pull requests fixes the problem in such a way that substratevm/native image work again with both versions of the JDK, before and after JDK-8347287.
mx --primary-suite=substratevm --components=ni,nju,lg gate -t 'native unittests'
which were the only JFR tests I could find succeed after this PR with both versions of the JDK before and after JDK-8347287. Please let me know if there are other tests which I can run.