Skip to content

[GR-60395] Allow combination of captureCallState and critical(true). #10301

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

Merged
merged 1 commit into from
Dec 13, 2024
Merged

[GR-60395] Allow combination of captureCallState and critical(true). #10301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
163 changes: 128 additions & 35 deletions substratevm/src/com.oracle.svm.core.foreign/src/com/oracle/svm/core/foreign/AbiUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,12 @@

import com.oracle.svm.core.SubstrateTargetDescription;
import com.oracle.svm.core.config.ConfigurationValues;
import com.oracle.svm.core.foreign.AbiUtils.Adapter.Adaptation;
import com.oracle.svm.core.graal.code.AssignedLocation;
import com.oracle.svm.core.headers.LibC;
import com.oracle.svm.core.headers.WindowsAPIs;
import com.oracle.svm.core.util.BasedOnJDKClass;
import com.oracle.svm.core.util.BasedOnJDKFile;
import com.oracle.svm.core.util.VMError;

import jdk.graal.compiler.api.replacements.Fold;
Expand Down Expand Up @@ -230,11 +232,15 @@ public static Adaptation check(Class<?> type) {
}

public static Adaptation extract(Extracted as, Class<?> type) {
return new Extract(Objects.requireNonNull(as), Objects.requireNonNull(type));
return new ExtractSingle(as, type);
}

public static Adaptation extractSegmentPair(Extracted as, Class<?> type) {
return new ExtractSegmentPair(as);
}

public static Adaptation drop() {
return Extract.DROP;
return Drop.SINGLETON;
}

public static Adaptation reinterpret(JavaKind to) {
Expand Down Expand Up @@ -342,41 +348,77 @@ public List<AssignedLocation> apply(AssignedLocation parameter) {
@Override
public List<ValueNode> apply(ValueNode parameter, Map<Extracted, ValueNode> extractedArguments, List<ValueNode> originalArguments, int originalArgumentIndex,
Consumer<Node> appendToGraph) {
return List.of(computeAbsolutePointerFromSegmentPair(parameter, originalArguments, originalArgumentIndex, appendToGraph));
}

static ValueNode computeAbsolutePointerFromSegmentPair(ValueNode parameter, List<ValueNode> originalArguments, int originalArgumentIndex, Consumer<Node> appendToGraph) {
var offsetArg = originalArguments.get(originalArgumentIndex + 1);

// I originally wanted to use an OffsetAddressNode here
// (followed by WordCastNode.addressToWord),
// but NativeMemorySegmentImpls return null for `unsafeGetBase`,
// which seems to break the graph somewhere later.
/*
* It would be most suitable to use OffsetAddressNode here (followed by
* WordCastNode.addressToWord) but NativeMemorySegmentImpls return null for
* `unsafeGetBase`,which seems to break the graph somewhere later.
*/
var basePointer = WordCastNode.objectToUntrackedPointer(parameter, ConfigurationValues.getWordKind());
appendToGraph.accept(basePointer);
var absolutePointer = AddNode.add(basePointer, offsetArg);
appendToGraph.accept(absolutePointer);

return List.of(absolutePointer);
return absolutePointer;
}
}

private static final class Extract extends Adaptation {
private static final Extract DROP = new Extract(null, null);
private final Extracted as;
private final Class<?> type;
/**
* Extract adaptations consume one or more stub parameters. In this case, "consuming" means
* that the adapted parameter(s) won't be passed to the stub's target but will be used by
* the stub itself. The result is usually one ValueNode that will be put into the
* 'extractedArguments' table.
*/
private abstract static class Extract extends Adaptation {
final Extracted as;

private Extract(Extracted as, Class<?> type) {
private Extract(Extracted as) {
this.as = as;
this.type = type;
}

@Override
public final List<AssignedLocation> apply(AssignedLocation parameter) {
return List.of();
}
}

private static final class Drop extends Extract {
private static final Drop SINGLETON = new Drop();

private Drop() {
super(null);
}

@Override
public List<Class<?>> apply(Class<?> parameter) {
if (type != null && parameter != type) {
throw new IllegalArgumentException("Expected type " + type + ", got " + parameter);
}
return List.of();
}

@Override
public List<AssignedLocation> apply(AssignedLocation parameter) {
public List<ValueNode> apply(ValueNode parameter, Map<Extracted, ValueNode> extractedArguments, List<ValueNode> originalArguments, int originalArgumentIndex,
Consumer<Node> appendToGraph) {
return List.of();
}
}

private final static class ExtractSingle extends Extract {
private final Class<?> type;

private ExtractSingle(Extracted as, Class<?> type) {
super(Objects.requireNonNull(as));
this.type = Objects.requireNonNull(type);
}

@Override
public List<Class<?>> apply(Class<?> parameter) {
if (type != null && parameter != type) {
throw new IllegalArgumentException("Expected type " + type + ", got " + parameter);
}
return List.of();
}

Expand All @@ -392,6 +434,37 @@ public List<ValueNode> apply(ValueNode parameter, Map<Extracted, ValueNode> extr
return List.of();
}
}

/**
* Similar to {@link ComputeAddressFromSegmentPair}, consumes two parameters, i.e., an
* Object + offset pair, and creates and AddNode that computes the absolute address.
*/
private static final class ExtractSegmentPair extends Extract {

private ExtractSegmentPair(Extracted as) {
super(Objects.requireNonNull(as));
}

@Override
public List<Class<?>> apply(Class<?> parameter) {
if (parameter != Object.class) {
throw new IllegalArgumentException("Expected type " + Object.class + ", got " + parameter);
}
return List.of();
}

@Override
public List<ValueNode> apply(ValueNode parameter, Map<Extracted, ValueNode> extractedArguments, List<ValueNode> originalArguments, int originalArgumentIndex,
Consumer<Node> appendToGraph) {
assert as != null;
if (extractedArguments.containsKey(as)) {
throw new IllegalStateException("%s was already extracted (%s).".formatted(as, extractedArguments.get(as)));
}
ValueNode extracted = ComputeAddressFromSegmentPair.computeAbsolutePointerFromSegmentPair(parameter, originalArguments, originalArgumentIndex, appendToGraph);
extractedArguments.put(as, extracted);
return List.of();
}
}
}

@Platforms(Platform.HOSTED_ONLY.class)
Expand Down Expand Up @@ -438,6 +511,7 @@ public final Adapter.Result.TypeAdaptation adapt(JavaEntryPointInfo jep) {
/**
* Generate additional argument adaptations which are not done by HotSpot.
*/
@BasedOnJDKFile("https://github.com/openjdk/jdk/blob/jdk-24+27/src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequenceBuilder.java#L103-L147")
@Platforms(Platform.HOSTED_ONLY.class)
protected List<Adapter.Adaptation> generateAdaptations(NativeEntryPointInfo nep) {
List<Adapter.Adaptation> adaptations = new ArrayList<>(Collections.nCopies(nep.methodType().parameterCount(), null));
Expand All @@ -446,36 +520,55 @@ protected List<Adapter.Adaptation> generateAdaptations(NativeEntryPointInfo nep)
adaptations.set(current++, Adapter.check(long.class));
}
adaptations.set(current++, Adapter.extract(Adapter.Extracted.CallTarget, long.class));
if (nep.capturesCallState()) {
adaptations.set(current++, Adapter.extract(Adapter.Extracted.CaptureBufferAddress, long.class));
}

// Special handling in case Linker.Option.critical(true) is passed.
// See the doc of Adapter.CastObjectToWord.apply.
// See the doc of class Adapter.ComputeAddressFromSegmentPair
var storages = nep.parametersAssignment();

/*
* It is possible to combine linker options 'captureCallState(...)' and 'critical(true)'.
* This means that one can pass a heap memory segment as capture buffer address.
*/
if (nep.capturesCallState()) {
if (nep.allowHeapAccess()) {
VMError.guarantee(storages[current] != null && storages[current + 1] == null);
// consumes two parameters (i.e. object + offset pair)
handleCriticalWithHeapAccess(nep, current + 1, adaptations, Adapter.extractSegmentPair(Adapter.Extracted.CaptureBufferAddress, long.class));
current += 2;
} else {
adaptations.set(current, Adapter.extract(Adapter.Extracted.CaptureBufferAddress, long.class));
current++;
}
}

for (int i = current; i < storages.length; ++i) {
var storage = storages[i];
if (storage == null) {
VMError.guarantee(nep.allowHeapAccess(), "A storage may only be null when the Linker.Option.critical(true) option is passed.");
VMError.guarantee(JavaKind.fromJavaClass(
nep.methodType().parameterArray()[i]) == JavaKind.Long &&
JavaKind.fromJavaClass(nep.methodType().parameterArray()[i - 1]) == JavaKind.Object,
"""
Storage is null, but the other parameters are inconsistent.
Storage may be null only if its kind is Long and previous kind is Object.
See jdk/internal/foreign/abi/x64/sysv/CallArranger.java:286""");
VMError.guarantee(
adaptations.get(i) == null && adaptations.get(i - 1) == null,
"This parameter already has an adaptation when it should not.");

adaptations.set(i, Adapter.drop());
adaptations.set(i - 1, Adapter.computeAddressFromSegmentPair());
handleCriticalWithHeapAccess(nep, i, adaptations, Adapter.computeAddressFromSegmentPair());
}
}

return adaptations;
}

@BasedOnJDKFile("https://github.com/openjdk/jdk/blob/jdk-24+27/src/java.base/share/classes/jdk/internal/foreign/abi/x64/sysv/CallArranger.java#L280-L290")
private static void handleCriticalWithHeapAccess(NativeEntryPointInfo nep, int i, List<Adaptation> adaptations, Adaptation adaptation) {
VMError.guarantee(nep.allowHeapAccess(), "A storage may only be null when the Linker.Option.critical(true) option is passed.");
VMError.guarantee(
JavaKind.fromJavaClass(nep.methodType().parameterArray()[i]) == JavaKind.Long &&
JavaKind.fromJavaClass(nep.methodType().parameterArray()[i - 1]) == JavaKind.Object,
"""
Storage is null, but the other parameters are inconsistent.
Storage may be null only if its kind is Long and previous kind is Object.
See jdk/internal/foreign/abi/x64/sysv/CallArranger.java:286""");
VMError.guarantee(
adaptations.get(i) == null && adaptations.get(i - 1) == null,
"This parameter already has an adaptation when it should not.");

adaptations.set(i, Adapter.drop());
adaptations.set(i - 1, adaptation);
}

@Platforms(Platform.HOSTED_ONLY.class)
protected List<Adapter.Adaptation> generateAdaptations(JavaEntryPointInfo jep) {
List<Adapter.Adaptation> adaptations = new ArrayList<>(Collections.nCopies(jep.handleType().parameterCount(), null));
Expand Down
5 changes: 3 additions & 2 deletions ...ign/src/com/oracle/svm/core/foreign/Target_jdk_internal_foreign_abi_NativeEntryPoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2023, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -58,7 +58,8 @@ public static Target_jdk_internal_foreign_abi_NativeEntryPoint make(ABIDescripto
MethodType methodType,
boolean needsReturnBuffer,
int capturedStateMask,
boolean needsTransition) {
boolean needsTransition,
boolean usingAddressPairs) {
Copy link
Collaborator

@zakkak zakkak Dec 13, 2024

Choose a reason for hiding this comment

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

@fangerer is the lack of a @BasedOnJDKFile annotation on this method intentional?

Copy link
Member

@fangerer fangerer Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, since the substitution class is annotated with @TargetClass, we track changes in the original class anyway.

Copy link
Collaborator

@zakkak zakkak Dec 13, 2024

Choose a reason for hiding this comment

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

Good, thank you for the explanation

/*
* A VMStorage may be null only when the Linker.Option.critical(allowHeapAccess=true) option
* is passed. (see
Expand Down