-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8315771: [JVMCI] Resolution of bootstrap methods with int[] static arguments #15588
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
Changes from all commits
b0940de
2cdc0b4
b33ffed
093d0ce
4ec0a7b
079969c
d6aa593
d75e092
f4377fc
9e0627a
ea4aa98
7c27481
7a8d9f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,8 @@ | |
| import static jdk.vm.ci.hotspot.HotSpotVMConfig.config; | ||
| import static jdk.vm.ci.hotspot.UnsafeAccess.UNSAFE; | ||
|
|
||
| import java.util.AbstractList; | ||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.List; | ||
| import java.util.stream.Collectors; | ||
|
|
@@ -38,6 +40,7 @@ | |
| import jdk.vm.ci.meta.JavaField; | ||
| import jdk.vm.ci.meta.JavaMethod; | ||
| import jdk.vm.ci.meta.JavaType; | ||
| import jdk.vm.ci.meta.PrimitiveConstant; | ||
| import jdk.vm.ci.meta.ResolvedJavaMethod; | ||
| import jdk.vm.ci.meta.ResolvedJavaType; | ||
| import jdk.vm.ci.meta.Signature; | ||
|
|
@@ -526,6 +529,60 @@ private int flags() { | |
| return UNSAFE.getInt(getConstantPoolPointer() + config().constantPoolFlagsOffset); | ||
| } | ||
|
|
||
| /** | ||
| * Represents a list of static arguments from a {@link BootstrapMethodInvocation} of the form | ||
| * {{@code arg_count}, {@code pool_index}}, meaning the arguments are not already resolved | ||
| * and that the JDK has to lookup the arguments when they are needed. The {@code bssIndex} | ||
| * corresponds to {@code pool_index} and the {@code size} corresponds to {@code arg_count}. | ||
| */ | ||
| static class CachedBSMArgs extends AbstractList<JavaConstant> { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This class needs documentation. |
||
| private final JavaConstant[] cache; | ||
| private final HotSpotConstantPool cp; | ||
| private final int bssIndex; | ||
|
|
||
| CachedBSMArgs(HotSpotConstantPool cp, int bssIndex, int size) { | ||
| this.cp = cp; | ||
| this.bssIndex = bssIndex; | ||
| this.cache = new JavaConstant[size]; | ||
| } | ||
|
|
||
| /** | ||
| * Lazily resolves and caches the argument at the given index and returns it. The method | ||
| * {@link CompilerToVM#bootstrapArgumentIndexAt} is used to obtain the constant pool | ||
| * index of the entry and the method {@link ConstantPool#lookupConstant} is used to | ||
| * resolve it. If the resolution failed, the index is returned as a | ||
| * {@link PrimitiveConstant}. | ||
| * | ||
| * @param index index of the element to return | ||
| * @return A {@link JavaConstant} corresponding to the static argument requested. A return | ||
| * value of type {@link PrimitiveConstant} represents an unresolved constant pool entry | ||
| */ | ||
| @Override | ||
| public JavaConstant get(int index) { | ||
| JavaConstant res = cache[index]; | ||
| if (res == null) { | ||
| int argCpi = compilerToVM().bootstrapArgumentIndexAt(cp, bssIndex, index); | ||
| Object object = cp.lookupConstant(argCpi, false); | ||
| if (object instanceof PrimitiveConstant primitiveConstant) { | ||
| res = runtime().getReflection().boxPrimitive(primitiveConstant); | ||
| } else if (object instanceof JavaConstant javaConstant) { | ||
| res = javaConstant; | ||
| } else if (object instanceof JavaType type) { | ||
| res = runtime().getReflection().forObject(type); | ||
| } else { | ||
| res = JavaConstant.forInt(argCpi); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on my understanding and my testing, this can only be reached for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's right based on the spec for |
||
| } | ||
| cache[index] = res; | ||
| } | ||
| return res; | ||
| } | ||
|
|
||
| @Override | ||
| public int size() { | ||
| return cache.length; | ||
| } | ||
| } | ||
|
|
||
| static class BootstrapMethodInvocationImpl implements BootstrapMethodInvocation { | ||
| private final boolean indy; | ||
| private final ResolvedJavaMethod method; | ||
|
|
@@ -604,8 +661,9 @@ public BootstrapMethodInvocation lookupBootstrapMethodInvocation(int index, int | |
| staticArgumentsList = List.of((JavaConstant[]) staticArguments); | ||
| } else { | ||
| int[] bsciArgs = (int[]) staticArguments; | ||
| String message = String.format("Resolving bootstrap static arguments for %s using BootstrapCallInfo %s not supported", method.format("%H.%n(%p)"), Arrays.toString(bsciArgs)); | ||
| throw new IllegalArgumentException(message); | ||
| int argCount = bsciArgs[0]; | ||
| int bss_index = bsciArgs[1]; | ||
| staticArgumentsList = new CachedBSMArgs(this, bss_index, argCount); | ||
| } | ||
| return new BootstrapMethodInvocationImpl(tag.name.equals("InvokeDynamic"), method, name, type, staticArgumentsList); | ||
| default: | ||
|
|
||
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.
What happens when
indexis out of bounds?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.
Currently, it returns another constant pool index or a random number. I can add a check in the
CachedBSMArgs.getmethod to prevent this.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.
Yes please. We should opt for exceptions over undefined behavior whenever possible.
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.
After double checking, there is in fact already an
assertfor checking if the index is out of bounds. Should I change it to an exception?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.
Is the assertion in code that can throw exceptions?
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.
The assertion is here. I don't think we can throw exception there. I could check the size in
jvmciCompilerToVM.bootstrapArgumentIndexAt, but it would require a bit of code duplication to obtain it.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.
Ok. You should just add a note in the javadoc that the result is undefined if
indexis out of bounds. This is all internal API so I assume we can guarantee the value is correct?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.
Yes, it is only used in
CachedBSMArgs.get, which will throw anArrayIndexOutOfBoundsExceptionbefore we arrive there.