Skip to content

Commit

Permalink
8318072: DowncallLinker does not acquire/release segments in interpreter
Browse files Browse the repository at this point in the history
Reviewed-by: mcimadamore
  • Loading branch information
JornVernee committed Oct 14, 2023
1 parent 56aa1e8 commit 1d54e73
Show file tree
Hide file tree
Showing 6 changed files with 57 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ public void verify(Deque<Class<?>> stack) {
@Override
public void interpret(Deque<Object> stack, StoreFunc storeFunc,
LoadFunc loadFunc, SegmentAllocator allocator) {
storeFunc.store(storage(), type(), stack.pop());
storeFunc.store(storage(), stack.pop());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ static Object box(List<Binding> bindings, LoadFunc loadFunc, SegmentAllocator al

@FunctionalInterface
public interface StoreFunc {
void store(VMStorage storage, Class<?> type, Object o);
void store(VMStorage storage, Object o);
}

@FunctionalInterface
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,22 @@
import jdk.internal.access.SharedSecrets;
import sun.security.action.GetPropertyAction;

import java.lang.foreign.AddressLayout;
import java.lang.foreign.Arena;
import java.lang.foreign.MemorySegment;
import java.lang.foreign.SegmentAllocator;
import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.stream.Stream;

import jdk.internal.foreign.AbstractMemorySegmentImpl;
import jdk.internal.foreign.MemorySessionImpl;

import static java.lang.invoke.MethodHandles.collectArguments;
import static java.lang.invoke.MethodHandles.foldArguments;
import static java.lang.invoke.MethodHandles.identity;
Expand Down Expand Up @@ -93,9 +99,8 @@ public MethodHandle getBoundMethodHandle() {
handle = BindingSpecializer.specializeDowncall(handle, callingSequence, abi);
} else {
Map<VMStorage, Integer> argIndexMap = SharedUtils.indexMap(argMoves);
Map<VMStorage, Integer> retIndexMap = SharedUtils.indexMap(retMoves);

InvocationData invData = new InvocationData(handle, argIndexMap, retIndexMap);
InvocationData invData = new InvocationData(handle, callingSequence, argIndexMap);
handle = insertArguments(MH_INVOKE_INTERP_BINDINGS.bindTo(this), 2, invData);
MethodType interpType = callingSequence.callerMethodType();
if (callingSequence.needsReturnBuffer()) {
Expand Down Expand Up @@ -146,17 +151,17 @@ private VMStorage[] toStorageArray(Binding.Move[] moves) {
return Arrays.stream(moves).map(Binding.Move::storage).toArray(VMStorage[]::new);
}

private record InvocationData(MethodHandle leaf, Map<VMStorage, Integer> argIndexMap, Map<VMStorage, Integer> retIndexMap) {}
private record InvocationData(MethodHandle leaf, CallingSequence callingSequence, Map<VMStorage, Integer> argIndexMap) {}

Object invokeInterpBindings(SegmentAllocator allocator, Object[] args, InvocationData invData) throws Throwable {
Arena unboxArena = callingSequence.allocationSize() != 0
? SharedUtils.newBoundedArena(callingSequence.allocationSize())
: SharedUtils.DUMMY_ARENA;
List<MemorySessionImpl> acquiredScopes = new ArrayList<>();
try (unboxArena) {
MemorySegment returnBuffer = null;

// do argument processing, get Object[] as result
Object[] leafArgs = new Object[invData.leaf.type().parameterCount()];
if (callingSequence.needsReturnBuffer()) {
// we supply the return buffer (argument array does not contain it)
Object[] prefixedArgs = new Object[args.length + 1];
Expand All @@ -165,10 +170,21 @@ Object invokeInterpBindings(SegmentAllocator allocator, Object[] args, Invocatio
System.arraycopy(args, 0, prefixedArgs, 1, args.length);
args = prefixedArgs;
}

Object[] leafArgs = new Object[invData.leaf.type().parameterCount()];
for (int i = 0; i < args.length; i++) {
Object arg = args[i];
if (callingSequence.functionDesc().argumentLayouts().get(i) instanceof AddressLayout) {
MemorySessionImpl sessionImpl = ((AbstractMemorySegmentImpl) arg).sessionImpl();
if (!(callingSequence.needsReturnBuffer() && i == 0)) { // don't acquire unboxArena's scope
sessionImpl.acquire0();
// add this scope _after_ we acquire, so we only release scopes we actually acquired
// in case an exception occurs
acquiredScopes.add(sessionImpl);
}
}
BindingInterpreter.unbox(arg, callingSequence.argumentBindings(i),
(storage, type, value) -> leafArgs[invData.argIndexMap.get(storage)] = value, unboxArena);
(storage, value) -> leafArgs[invData.argIndexMap.get(storage)] = value, unboxArena);
}

// call leaf
Expand All @@ -194,6 +210,10 @@ public Object load(VMStorage storage, Class<?> type) {
return BindingInterpreter.box(callingSequence.returnBindings(), (storage, type) -> o,
allocator);
}
} finally {
for (MemorySessionImpl sessionImpl : acquiredScopes) {
sessionImpl.release0();
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ private static Object invokeInterpBindings(MethodHandle leaf, Object[] lowLevelA
Object[] returnValues = new Object[invData.retIndexMap.size()];
if (leaf.type().returnType() != void.class) {
BindingInterpreter.unbox(o, invData.callingSequence.returnBindings(),
(storage, type, value) -> returnValues[invData.retIndexMap.get(storage)] = value, null);
(storage, value) -> returnValues[invData.retIndexMap.get(storage)] = value, null);
}

if (returnValues.length == 0) {
Expand All @@ -187,15 +187,10 @@ private static Object invokeInterpBindings(MethodHandle leaf, Object[] lowLevelA
} else {
assert invData.callingSequence.needsReturnBuffer();

Binding.VMStore[] retMoves = invData.callingSequence.returnBindings().stream()
.filter(Binding.VMStore.class::isInstance)
.map(Binding.VMStore.class::cast)
.toArray(Binding.VMStore[]::new);

assert returnValues.length == retMoves.length;
assert returnValues.length == invData.retMoves().length;
int retBufWriteOffset = 0;
for (int i = 0; i < retMoves.length; i++) {
Binding.VMStore store = retMoves[i];
for (int i = 0; i < invData.retMoves().length; i++) {
Binding.VMStore store = invData.retMoves()[i];
Object value = returnValues[i];
SharedUtils.writeOverSized(returnBuffer.asSlice(retBufWriteOffset), store.type(), value);
retBufWriteOffset += invData.abi.arch.typeSize(store.storage().type());
Expand Down
15 changes: 13 additions & 2 deletions test/jdk/java/foreign/LibraryLookupTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,19 @@
import static org.testng.Assert.*;

/*
* @test
* @run testng/othervm --enable-native-access=ALL-UNNAMED LibraryLookupTest
* @test id=specialized
* @run testng/othervm
* -Djdk.internal.foreign.DowncallLinker.USE_SPEC=true
* --enable-native-access=ALL-UNNAMED
* LibraryLookupTest
*/

/*
* @test id=interpreted
* @run testng/othervm
* -Djdk.internal.foreign.DowncallLinker.USE_SPEC=false
* --enable-native-access=ALL-UNNAMED
* LibraryLookupTest
*/
public class LibraryLookupTest {

Expand Down
15 changes: 13 additions & 2 deletions test/jdk/java/foreign/SafeFunctionAccessTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,19 @@
*/

/*
* @test
* @run testng/othervm --enable-native-access=ALL-UNNAMED SafeFunctionAccessTest
* @test id=specialized
* @run testng/othervm
* -Djdk.internal.foreign.DowncallLinker.USE_SPEC=true
* --enable-native-access=ALL-UNNAMED
* SafeFunctionAccessTest
*/

/*
* @test id=interpreted
* @run testng/othervm
* -Djdk.internal.foreign.DowncallLinker.USE_SPEC=false
* --enable-native-access=ALL-UNNAMED
* SafeFunctionAccessTest
*/

import java.lang.foreign.Arena;
Expand Down

1 comment on commit 1d54e73

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

Please sign in to comment.