Skip to content

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.

Sign up for GitHub

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
wants to merge 13 commits into from
Closed

8315771: [JVMCI] Resolution of bootstrap methods with int[] static arguments #15588

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b0940de
Implement lookup of bootstrap methods with static arguments of type i…
Zeavee Sep 6, 2023
2cdc0b4
Rewrite doc for bootstrapArgumentIndexAt and add second argument to a…
Zeavee Sep 6, 2023
b33ffed
Add more details to the documentation and add a pseudo code example f…
Zeavee Sep 7, 2023
093d0ce
Add missing line of pseudo code and applied modification suggestion
Zeavee Sep 7, 2023
4ec0a7b
Add CachedBSMArgs to resolve arguments lazily instead of letting the …
Zeavee Sep 7, 2023
079969c
Fix BootstrapMethodInvocation.getStaticArguments documentation and ad…
Zeavee Sep 8, 2023
d6aa593
Use lookupConstant instead of lookupConstantInPool in CachedBSMArgs.get
Zeavee Sep 13, 2023
d75e092
Add undefined behavior documentation for bootstrapArgumentIndexAt and…
Zeavee Sep 13, 2023
f4377fc
Update getStaticArguments documentation example and update TestDynami…
Zeavee Sep 14, 2023
9e0627a
Add -XX:UseBootstrapCallInfo=3 to TestDynamicConstant
Zeavee Sep 19, 2023
ea4aa98
Add an extra run configuration for TestDynamicConstant instead of rep…
Zeavee Sep 19, 2023
7c27481
Update BootstrapMethodInvocation.getStaticArguments documentation
Zeavee Sep 20, 2023
7a8d9f2
Iterate over all static arguments in TestDynamicConstant
Zeavee Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 37 additions & 6 deletions src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotConstantPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -526,6 +529,37 @@ private int flags() {
return UNSAFE.getInt(getConstantPoolPointer() + config().constantPoolFlagsOffset);
}

static class CachedBSMArgs extends AbstractList<JavaConstant> {
Copy link
Member

Choose a reason for hiding this comment

The 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];
}

@Override
public JavaConstant get(int index) {
JavaConstant res = cache[index];
if (res == null) {
int argCpi = compilerToVM().bootstrapArgumentIndexAt(cp, bssIndex, index);
res = compilerToVM().lookupConstantInPool(cp, argCpi, false);
if (res == null) {
res = JavaConstant.forInt(argCpi);
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 CONSTANT_Dynamic constant pool entries, as other types of entries should resolve even with resolve = false. Am I correct or am I missing some edge cases?

Copy link
Member

Choose a reason for hiding this comment

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

I think that's right based on the spec for ConstantPool.lookupConstant.

}
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;
Expand Down Expand Up @@ -583,11 +617,6 @@ private static String argumentAsString(JavaConstant arg) {
}
}

@Override
public int bootstrapArgumentIndexAt(int cpi, int index) {
return compilerToVM().bootstrapArgumentIndexAt(this, cpi, index);
}

@Override
public BootstrapMethodInvocation lookupBootstrapMethodInvocation(int index, int opcode) {
int cpi = opcode == -1 ? index : indyIndexConstantPoolIndex(index, opcode);
Expand All @@ -609,7 +638,9 @@ public BootstrapMethodInvocation lookupBootstrapMethodInvocation(int index, int
staticArgumentsList = List.of((JavaConstant[]) staticArguments);
} else {
int[] bsciArgs = (int[]) staticArguments;
staticArgumentsList = List.of(Arrays.stream(bsciArgs).mapToObj(i -> JavaConstant.forInt(i)).toArray(JavaConstant[]::new));
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:
Expand Down
51 changes: 17 additions & 34 deletions src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/ConstantPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,24 +133,6 @@ default JavaMethod lookupMethod(int cpi, int opcode) {
* The details for invoking a bootstrap method associated with a {@code CONSTANT_Dynamic_info}
* or {@code CONSTANT_InvokeDynamic_info} pool entry.
*
* The procedure to obtain and use a {@link BootstrapMethodInvocation} is the following:
*
* <pre>
* bsmInvocation = constantpool.lookupBootstrapMethodInvocation(index, opcode);
* staticArguments = bsmInvocation.getStaticArguments();
* if staticArguments are PrimitiveConstant {
* argCount = staticArguments.get(0).asInt();
* cpi = staticArguments.get(1).asInt();
* for (int i = 0; i < argCount; ++i) {
* argCpi = constantpool.bootstrapArgumentIndexAt(cpi, i);
* arguments[i] = constantpool.lookupConstant(argCpi, resolve);
* }
* call bootstrap method with newly resolved arguments
* } else {
* call bootstrap method with provided arguments
* }
* </pre>
*
* @jvms 4.4.10 The {@code CONSTANT_Dynamic_info} and {@code CONSTANT_InvokeDynamic_info}
* Structures
* @jvms 4.7.23 The {@code BootstrapMethods} Attribute
Expand Down Expand Up @@ -183,27 +165,28 @@ interface BootstrapMethodInvocation {
/**
* Gets the static arguments with which the bootstrap method will be invoked.
*
* An argument of type {@link PrimitiveConstant} represents a {@code CONSTANT_Dynamic_info}
Copy link
Member

Choose a reason for hiding this comment

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

Can it not be an index to any valid LDC constant? This is implied by this code in your Graal PR:

if (constant instanceof PrimitiveConstant primitiveConstant) {
    int argCpi = primitiveConstant.asInt();
    Object argConstant = lookupConstant(argCpi, opcode == Opcodes.INVOKEDYNAMIC ? Opcodes.LDC : opcode, allowBootstrapMethodInvocation);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, sorry, I will fix this documentation.

Copy link
Contributor Author

@Zeavee Zeavee Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

From the user point of view, a PrimitiveConstant can in fact only be a ConstantDynamic, because the other type of constant are automatically resolved in CachedBSMArgs.get. In the Graal PR, lookupConstant is a recursive call, allowing to call the associated bootstrap method, but the other type of entries will go in the else branch.

* entry. To resolve this entry, the corresponding bootstrap method has to be called first:
*
* <pre>
* resolveIndyOrCondy(int index, int opcode) {
* bsmInvocation = cp.lookupBootstrapMethodInvocation(index, opcode);
* staticArguments = bsmInvocation.getStaticArguments();
* for each argument in staticArguments {
* if argument is PrimitiveArgument {
* // argument is a condy, so opcode becomes -1
* resolveIndyOrCondy(argument.asInt(), -1);
* }
* }
* call original boostrap method with resolved arguments
* }
* </pre>
*
* @jvms 5.4.3.6
*/
List<JavaConstant> getStaticArguments();
}

/**
* Gets the constant pool index of a static argument of a {@code CONSTANT_Dynamic_info} or
* @{code CONSTANT_InvokeDynamic_info} entry. Used when the list of static arguments in the
* {@link BootstrapMethodInvocation} is a {@code List<PrimitiveConstant>} 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 cpi} corresponds to
* {@code pool_index} and the {@code index} has to be smaller than {@code arg_count}.
*
* @param cpi the index of a {@code CONSTANT_Dynamic_info} or @{code CONSTANT_InvokeDynamic_info} entry
* @param index the index of the static argument in the list of static arguments
* @return the constant pool index associated with the static argument
*/
default int bootstrapArgumentIndexAt(int cpi, int index) {
throw new UnsupportedOperationException();
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

This is very confusing documentation. It's not clear what index or entry you are referring to each time you say "index". Please provide a concrete example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have rewritten the documentation, I hope it is clearer. I also added a second argument to be able to access different different static arguments on the same bootstrap specifier.

Copy link
Member

Choose a reason for hiding this comment

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

It still needs an example showing how this API should be used. Please add a complete (pseudo-code) example to the javadoc of BootstrapMethodInvocation.

* Gets the details for invoking a bootstrap method associated with the
* {@code CONSTANT_Dynamic_info} or {@code CONSTANT_InvokeDynamic_info} pool entry
Expand Down