Make GraalVM compatible with JDK 20+27 plus #5751
Conversation
oracle-contributor-agreement
bot
added
the
OCA Verified
|
@zakkak contemporary JDK 20 (Januray 13 e0081462f4561), build passes with this branch, although building Hello world fails at linker stage: It's the added static native void ensureMaterializedForStackWalk(Object o); Not sure what to do with it, mimicking what you did is not what helps... /me goona look at the JDK JIRA to figure out what it does... /edit Not sure if relevant, but it looks like the symbol has this |
Yes I noticed that too. My understanding is that we should not substitute this method, but instead find out how to allow GraalVM to invoke it. If you come to any better conclusion please share. |
|
@zakkak I can see While the missing one is ...and that is nowhere to be seen. Looking at some other symbols in I am wondering why |
|
My autogenerated contains the symbol called correctly though: |
|
If I get it right,
I suspect that the registration in [5] is the one that leads in the JNI export in the generated header file, despite it not being necessary (as there is no code referencing it, and there is no implementation of the method either), one could say it's actually missleading as one might thing they can invoke the method from their JNI code. Now on the GraalVM/Mandrel land. My guestimate is that GraalVM not being aware that As noted by @adinn, when doing ^^ we need to ensure that the optimization/substitution happens late enough in order to prevent Graal from figuring out that the invocation is actually a NOP. [1] openjdk/jdk@221e1a4#diff-018aa61d1a7aafcf70a535fcd40a318a4bd6511fd40ac39ce4be90cc52216749 |
zakkak
force-pushed
the
2023-01-12-jdk20-27-plus-compatibility
branch
from
f03c0d2 to
9668ae4
Compare
|
I prepared 44c1201 which treats |
zakkak
force-pushed
the
2023-01-12-jdk20-27-plus-compatibility
branch
2 times, most recently
from
0ac1139 to
367f63a
Compare
|
Thanks to @gergo- (for pointing me to |
|
There might be something wrong with my local setup, although the very latest JDK 20: Whereas: TBH, I was unaware that there is an ongoing GraalVM RISC-V work. Gonna check Mandrel packaging later... |
Hi @Karm, that's fixed by graalvm/mandrel-packaging#300, please pull mandrel-packaging's |
|
@zakkak There are some style issues the CI build complains about. Want to fix those? |
|
@zakkak I believe that translating the call to |
zakkak
force-pushed
the
2023-01-12-jdk20-27-plus-compatibility
branch
from
367f63a to
775187b
Compare
| @Inject @TargetElement(onlyWith = LoomJDK.class) // | ||
| @Inject @TargetElement(onlyWith = {HasSetExtentLocalCache.class, HasExtentLocalCache.class, LoomJDK.class}) // | ||
| @RecomputeFieldValue(kind = RecomputeFieldValue.Kind.Reset) // | ||
| Object[] extentLocalCache; |
There was a problem hiding this comment.
There is no need to inject differently-named fields based on the name in the JDK that we're building on. You can rename the field to scopedValueCache in either case and use it from extentLocalCache and setExtentLocalCache too.
| private static void registerThreadPlugin(InvocationPlugins plugins, Replacements replacements) { | ||
| InvocationPlugin threadPlugin = new InvocationPlugin("ensureMaterializedForStackWalk", Object.class) { | ||
| @Override | ||
| public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Receiver receiver, ValueNode object) { | ||
| b.add(new BlackholeNode(object)); | ||
| return true; | ||
| } | ||
| }; | ||
| Registration r = new Registration(plugins, "java.lang.Thread", replacements); | ||
| r.registerConditional(hasEnsureMaterializedForStackWalk(), threadPlugin); | ||
| } |
There was a problem hiding this comment.
@mur47x111 @dougxc this intrinsic would also be used on HotSpot, please review.
There was a problem hiding this comment.
Looks good. This is the same change @gilles-duboscq made on a similar PR.
| * A predicate that returns {@code true} iff {@code boolean java.lang.Thread.setExtentLocalCache()} | ||
| * exists. | ||
| */ | ||
| public class HasSetExtentLocalCache implements BooleanSupplier { |
There was a problem hiding this comment.
I don't see a need to have this class in addition to HasExtentLocalCache when both of them were changed in the same commit. The same goes for HasSetScopedValueCache.
| @Override | ||
| public boolean getAsBoolean() { | ||
| try { | ||
| Class.forName("java.lang.Thread").getDeclaredMethod("extentLocalCache"); |
There was a problem hiding this comment.
You can use ReflectionUtil.lookupMethod(false, Thread.class, "extentLocalCache") != null. (Not sure why you look up Thread by name here and elsewhere)
There was a problem hiding this comment.
My bad, copy pasted code without giving it much thought.
There was a problem hiding this comment.
Shouldn't I set optional to true in lookupMethod though?
| * A predicate that returns {@code true} iff {@code boolean java.lang.Thread.extentLocalCache()} | ||
| * exists. | ||
| */ | ||
| public class HasExtentLocalCache implements BooleanSupplier { |
There was a problem hiding this comment.
I think those classes should just go into Target_java_lang_Thread.java since it doesn't seem like they'll be needed anywhere else.
(You can also remove the Javadoc comment because it's quite self-explanatory)
|
@zakkak My local setup:
Still ends up in this when trying to link a hello world app: Any idea what should I do differently? |
...m/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/methodhandles/MethodHandleFeature.java
Outdated
Show resolved
Hide resolved
| InvocationPlugin threadPlugin = new InvocationPlugin("ensureMaterializedForStackWalk", Object.class) { | ||
| @Override | ||
| public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Receiver receiver, ValueNode object) { | ||
| b.add(new BlackholeNode(object)); |
There was a problem hiding this comment.
I think it would be helpful to add an extra String property to BlackholeNode so you can pass "Thread.ensureMaterializedForStackWalk" here and make it show up in IGV as a node labeled Blackhole Thread.ensureMaterializedForStackWalk. Otherwise people seeing a random blackhole in a graph might be very confused where it comes from, since this is a new use case for this node.
There was a problem hiding this comment.
Do I understand correctly that the following would suffice?
modified compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/debug/BlackholeNode.java
@@ -51,12 +51,18 @@ public final class BlackholeNode extends FixedWithNextNode implements LIRLowerab
public static final NodeClass<BlackholeNode> TYPE = NodeClass.create(BlackholeNode.class);
@Input ValueNode value;
+ @OptionalInput String comment;
public BlackholeNode(ValueNode value) {
super(TYPE, StampFactory.forVoid());
this.value = value;
}
+ public BlackholeNode(ValueNode value, String comment) {
+ this(value);
+ this.comment = comment;
+ }
+
public ValueNode getValue() {
return value;
}
modified compiler/src/org.graalvm.compiler.replacements/src/org/graalvm/compiler/replacements/StandardGraphBuilderPlugins.java
@@ -2297,7 +2297,7 @@ public class StandardGraphBuilderPlugins {
InvocationPlugin threadPlugin = new InvocationPlugin("ensureMaterializedForStackWalk", Object.class) {
@Override
public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Receiver receiver, ValueNode object) {
- b.add(new BlackholeNode(object));
+ b.add(new BlackholeNode(object, "ensureMaterializedForStackWalk"));
return true;
}
};There was a problem hiding this comment.
Yes, that's about it. You have to remove the @OptionalInput on the String, only nodes can (and must) be inputs. Other properties are not annotated.
You should also add something like nameTemplate = "Blackhole {p#reason}" to the class's @NodeInfo so that the reason is shown as part of the name of the node itself. This might require reason to be non-null, so probably it should have a default value of "".
The corresponding methods were introduced in https://bugs.openjdk.org/browse/JDK-8286666 by openjdk/jdk20u@221e1a4#diff-2a5b4b651fa41e70f7953f7ee06e964d8396fcd1fe5755a1609e81b3f84d9224
zakkak
force-pushed
the
2023-01-12-jdk20-27-plus-compatibility
branch
from
775187b to
798ad66
Compare
zakkak
force-pushed
the
2023-01-12-jdk20-27-plus-compatibility
branch
2 times, most recently
from
c0f61e2 to
cb727c4
Compare
@Karm the key difference I see is that you are building your own JDK 20 while I am using A mistake I made while working on this, was using an old build instead of the latest one generated by mandrel-packaging when testing... Although trivial, please make sure you are using the latest build. You can a successfull build here https://github.com/graalvm/mandrel/actions/runs/3958942405 |
zakkak
force-pushed
the
2023-01-12-jdk20-27-plus-compatibility
branch
from
9107b3e to
83d596a
Compare
I am sure that is not the case. I did code changes before (printf debug...), I know the build script correctly cleans up and uses the correct dir.
I can se that. Yet no luck. I got paranoid and did this: Graal: And I used downloaded Temurin instead of my own JDK build. It didn't help: OKAforementioned Ubuntu Github action: NOKMy CentOS 8 Stream workstation: Dunno. Could it be the older toolchain I am using? |
No, apparently some change after the feedback breaks the PR. |
I am glad I am not out of my mind :-D I did try on a clean room Ubuntu 20 VM to check how a different toolchain behaves and it's the same... |
Forces its argument to be materialized.
Rely on extentLocalCache and scopedLocalCache presence for substituting setExtentLocalCache and setScopedLocalCache, as these methods are tightly coupled.
zakkak
force-pushed
the
2023-01-12-jdk20-27-plus-compatibility
branch
from
83d596a to
f01989e
Compare
|
Sorry for that @Karm The issue was in modified compiler/src/org.graalvm.compiler.replacements/src/org/graalvm/compiler/replacements/StandardGraphBuilderPlugins.java
@@ -2286,7 +2286,7 @@ public class StandardGraphBuilderPlugins {
private static boolean hasEnsureMaterializedForStackWalk() {
try {
- Thread.class.getDeclaredMethod("ensureMaterializedForStackWalk");
+ Thread.class.getDeclaredMethod("ensureMaterializedForStackWalk", Object.class);
} catch (Exception e) {
return false;Should be fixed now if you want to give it another go. |
|
@zakkak It works for me now, JDK 20 (1c840506 from Jan 19th). THX 🙏 |
|
For issues or PRs like this one that relate to compatibility with upstream OpenJDK changes, i'd like to introduce a new label so that we can easily find them, filter notifications etc. |
|
Yes, I think |
|
Here is the integration PR(#5831) for the current changes related to JDK-20. |
gilles-duboscq
added
the
openjdk-updates
|
Superseded by #5831 |
https://bugs.openjdk.org/browse/JDK-8298177 and https://bugs.openjdk.org/browse/JDK-8286666 introduced some breaking changes.
Closes #5750