Skip to content
This repository was archived by the owner on Mar 19, 2024. It is now read-only.
/ jdk22 Public archive

8321786: SegmentAllocator:allocateFrom(ValueLayout, MemorySegment,ValueLayout,long,long) spec mismatch in exception scenario #42

Closed
minborg wants to merge 1 commit into from
Closed

8321786: SegmentAllocator:allocateFrom(ValueLayout, MemorySegment,ValueLayout,long,long) spec mismatch in exception scenario #42

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
2 changes: 1 addition & 1 deletion src/java.base/share/classes/java/lang/foreign/MemoryLayout.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1016,7 +1016,7 @@ static PaddingLayout paddingLayout(long byteSize) {
* @throws IllegalArgumentException if {@code elementLayout.byteSize() % elementLayout.byteAlignment() != 0}
*/
static SequenceLayout sequenceLayout(long elementCount, MemoryLayout elementLayout) {
MemoryLayoutUtil.requireNonNegative(elementCount);
Utils.checkNonNegativeArgument(elementCount, "elementCount");
Objects.requireNonNull(elementLayout);
Utils.checkElementAlignment(elementLayout,
"Element layout size is not multiple of alignment");
Expand Down
42 changes: 38 additions & 4 deletions src/java.base/share/classes/java/lang/foreign/MemorySegment.java
View file
Open in desktop

Large diffs are not rendered by default.

6 changes: 4 additions & 2 deletions src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import jdk.internal.foreign.ArenaImpl;
import jdk.internal.foreign.SlicingAllocator;
import jdk.internal.foreign.StringSupport;
import jdk.internal.foreign.Utils;
import jdk.internal.vm.annotation.ForceInline;

/**
Expand Down Expand Up @@ -390,9 +391,10 @@ default MemorySegment allocateFrom(AddressLayout layout, MemorySegment value) {
* with {@code source} is not {@linkplain MemorySegment.Scope#isAlive() alive}
* @throws WrongThreadException if this method is called from a thread {@code T},
* such that {@code source.isAccessibleBy(T) == false}
* @throws IndexOutOfBoundsException if {@code elementCount * sourceElementLayout.byteSize()} overflows
* @throws IllegalArgumentException if {@code elementCount * sourceElementLayout.byteSize()} overflows
* @throws IllegalArgumentException if {@code elementCount < 0}
* @throws IndexOutOfBoundsException if {@code sourceOffset > source.byteSize() - (elementCount * sourceElementLayout.byteSize())}
* @throws IndexOutOfBoundsException if either {@code sourceOffset} or {@code elementCount} are {@code < 0}
* @throws IndexOutOfBoundsException if {@code sourceOffset < 0}
*/
@ForceInline
default MemorySegment allocateFrom(ValueLayout elementLayout,
Expand Down
8 changes: 3 additions & 5 deletions src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,7 @@ public final MemorySegment reinterpret(Arena arena, Consumer<MemorySegment> clea

public MemorySegment reinterpretInternal(Class<?> callerClass, long newSize, Scope scope, Consumer<MemorySegment> cleanup) {
Reflection.ensureNativeAccess(callerClass, MemorySegment.class, "reinterpret");
if (newSize < 0) {
throw new IllegalArgumentException("newSize < 0");
}
Utils.checkNonNegativeArgument(newSize, "newSize");
if (!isNative()) throw new UnsupportedOperationException("Not a native segment");
Runnable action = cleanup != null ?
() -> cleanup.accept(SegmentFactories.makeNativeSegmentUnchecked(address(), newSize)) :
Expand Down Expand Up @@ -594,6 +592,7 @@ public static void copy(MemorySegment srcSegment, ValueLayout srcElementLayout,
MemorySegment dstSegment, ValueLayout dstElementLayout, long dstOffset,
long elementCount) {

Utils.checkNonNegativeIndex(elementCount, "elementCount");
AbstractMemorySegmentImpl srcImpl = (AbstractMemorySegmentImpl)srcSegment;
AbstractMemorySegmentImpl dstImpl = (AbstractMemorySegmentImpl)dstSegment;
if (srcElementLayout.byteSize() != dstElementLayout.byteSize()) {
Expand Down Expand Up @@ -625,7 +624,7 @@ public static void copy(MemorySegment srcSegment, ValueLayout srcElementLayout,
public static void copy(MemorySegment srcSegment, ValueLayout srcLayout, long srcOffset,
Object dstArray, int dstIndex,
int elementCount) {

Utils.checkNonNegativeIndex(elementCount, "elementCount");
var dstInfo = Utils.BaseAndScale.of(dstArray);
if (dstArray.getClass().componentType() != srcLayout.carrier()) {
throw new IllegalArgumentException("Incompatible value layout: " + srcLayout);
Expand All @@ -652,7 +651,6 @@ public static void copy(MemorySegment srcSegment, ValueLayout srcLayout, long sr
public static void copy(Object srcArray, int srcIndex,
MemorySegment dstSegment, ValueLayout dstLayout, long dstOffset,
int elementCount) {

var srcInfo = Utils.BaseAndScale.of(srcArray);
if (srcArray.getClass().componentType() != dstLayout.carrier()) {
throw new IllegalArgumentException("Incompatible value layout: " + dstLayout);
Expand Down
21 changes: 16 additions & 5 deletions src/java.base/share/classes/jdk/internal/foreign/Utils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,11 +200,8 @@ public static long pointeeByteAlign(AddressLayout addressLayout) {
}

public static void checkAllocationSizeAndAlign(long byteSize, long byteAlignment) {
// size should be >= 0
if (byteSize < 0) {
throw new IllegalArgumentException("Invalid allocation size : " + byteSize);
}

// byteSize should be >= 0
Utils.checkNonNegativeArgument(byteSize, "allocation size");
checkAlign(byteAlignment);
}

Expand All @@ -216,6 +213,20 @@ public static void checkAlign(long byteAlignment) {
}
}

@ForceInline
public static void checkNonNegativeArgument(long value, String name) {
if (value < 0) {
throw new IllegalArgumentException("The provided " + name + " is negative: " + value);
}
}

@ForceInline
public static void checkNonNegativeIndex(long value, String name) {
if (value < 0) {
throw new IndexOutOfBoundsException("The provided " + name + " is negative: " + value);
}
}

private static long computePadding(long offset, long align) {
boolean isAligned = offset == 0 || offset % align == 0;
if (isAligned) {
Expand Down
9 changes: 2 additions & 7 deletions src/java.base/share/classes/jdk/internal/foreign/layout/AbstractLayout.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,13 +151,8 @@ private static long requirePowerOfTwoAndGreaterOrEqualToOne(long value) {
}

public long scale(long offset, long index) {
if (offset < 0) {
throw new IllegalArgumentException("Negative offset: " + offset);
}
if (index < 0) {
throw new IllegalArgumentException("Negative index: " + index);
}

Utils.checkNonNegativeArgument(offset, "offset");
Utils.checkNonNegativeArgument(index, "index");
return Math.addExact(offset, Math.multiplyExact(byteSize(), index));
}

Expand Down
7 changes: 0 additions & 7 deletions src/java.base/share/classes/jdk/internal/foreign/layout/MemoryLayoutUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,6 @@ public final class MemoryLayoutUtil {
private MemoryLayoutUtil() {
}

public static long requireNonNegative(long value) {
if (value < 0) {
throw new IllegalArgumentException("The provided value was negative: " + value);
}
return value;
}

public static long requireByteSizeValid(long byteSize, boolean allowZero) {
if ((byteSize == 0 && !allowZero) || byteSize < 0) {
throw new IllegalArgumentException("Invalid byte size: " + byteSize);
Expand Down
4 changes: 2 additions & 2 deletions test/jdk/java/foreign/TestLayouts.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,13 +371,13 @@ public void testVarHandleCaching() {
}

@Test(expectedExceptions=IllegalArgumentException.class,
expectedExceptionsMessageRegExp=".*Negative offset.*")
expectedExceptionsMessageRegExp=".*offset is negative.*")
public void testScaleNegativeOffset() {
JAVA_INT.scale(-1, 0);
}

@Test(expectedExceptions=IllegalArgumentException.class,
expectedExceptionsMessageRegExp=".*Negative index.*")
expectedExceptionsMessageRegExp=".*index is negative.*")
public void testScaleNegativeIndex() {
JAVA_INT.scale(0, -1);
}
Expand Down
7 changes: 7 additions & 0 deletions test/jdk/java/foreign/TestMemoryAccessInstance.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,13 @@ public <X, L extends ValueLayout> void badAccessOverflowInIndexedAccess(String t
}
}

@Test(dataProvider = "segmentAccessors")
public <X, L extends ValueLayout> void negativeOffset(String testName, Accessor<X, L> accessor) {
MemorySegment segment = MemorySegment.ofArray(new byte[100]);
assertThrows(IndexOutOfBoundsException.class, () -> accessor.get(segment, -ValueLayout.JAVA_LONG.byteSize()));
assertThrows(IndexOutOfBoundsException.class, () -> accessor.set(segment, -ValueLayout.JAVA_LONG.byteSize(), accessor.value));
}

static final ByteOrder NE = ByteOrder.nativeOrder();

@DataProvider(name = "segmentAccessors")
Expand Down
3 changes: 3 additions & 0 deletions test/jdk/java/foreign/TestScopedOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
*/

import java.lang.foreign.Arena;
import java.lang.foreign.MemoryLayout;
import java.lang.foreign.MemorySegment;
import java.lang.foreign.ValueLayout;

Expand Down Expand Up @@ -135,6 +136,8 @@ public <Z> void testOpOutsideConfinement(String name, ScopedOperation<Z> scopedO
ScopedOperation.ofScope(a -> a.allocateFrom(ValueLayout.JAVA_FLOAT, new float[]{0}), "Arena::allocateFrom/float");
ScopedOperation.ofScope(a -> a.allocateFrom(ValueLayout.JAVA_LONG, new long[]{0}), "Arena::allocateFrom/long");
ScopedOperation.ofScope(a -> a.allocateFrom(ValueLayout.JAVA_DOUBLE, new double[]{0}), "Arena::allocateFrom/double");
var source = MemorySegment.ofArray(new byte[]{});
ScopedOperation.ofScope(a -> a.allocateFrom(ValueLayout.JAVA_INT, source, JAVA_BYTE, 0, 1), "Arena::allocateFrom/5arg");
};

@DataProvider(name = "scopedOperations")
Expand Down
77 changes: 77 additions & 0 deletions test/jdk/java/foreign/TestSegmentAllocators.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@
import java.nio.ShortBuffer;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.BiFunction;
import java.util.function.Function;
Expand Down Expand Up @@ -189,6 +191,81 @@ public void testAllocatorAllocateFromHeapSegment() {
}
}

// Invariant checking tests for the SegmentAllocator method:
// MemorySegment allocateFrom(ValueLayout elementLayout,
// MemorySegment source,
// ValueLayout sourceElementLayout,
// long sourceOffset,
// long elementCount) {
@Test
public void testAllocatorAllocateFromArguments() {
try (Arena arena = Arena.ofConfined()) {
var sourceElements = 2;
var source = arena.allocate(ValueLayout.JAVA_LONG, sourceElements);
var elementLayout = ValueLayout.JAVA_INT;
var sourceElementLayout = ValueLayout.JAVA_INT;

// IllegalArgumentException if {@code elementLayout.byteSize() != sourceElementLayout.byteSize()}
assertThrows(IllegalArgumentException.class, () ->
arena.allocateFrom(elementLayout, source, ValueLayout.JAVA_BYTE, 0, 1)
);

// IllegalArgumentException if source segment/offset
// are <a href="MemorySegment.html#segment-alignment">incompatible with the alignment constraint</a>
// in the source element layout
assertThrows(IllegalArgumentException.class, () ->
arena.allocateFrom(elementLayout, source.asSlice(1), sourceElementLayout, 0, 1)
);
assertThrows(IllegalArgumentException.class, () ->
arena.allocateFrom(elementLayout, source, sourceElementLayout, 1, 1)
);

// IllegalArgumentException if {@code elementLayout.byteAlignment() > elementLayout.byteSize()}
assertThrows(IllegalArgumentException.class, () ->
arena.allocateFrom(elementLayout.withByteAlignment(elementLayout.byteAlignment() * 2), source, sourceElementLayout, 1, 1)
);

// IllegalStateException if the {@linkplain MemorySegment#scope() scope} associated
// with {@code source} is not {@linkplain MemorySegment.Scope#isAlive() alive}
// This is tested in TestScopedOperations

// WrongThreadException if this method is called from a thread {@code T},
// such that {@code source.isAccessibleBy(T) == false}
CompletableFuture<Arena> future = CompletableFuture.supplyAsync(Arena::ofConfined);
Copy link

@mcimadamore mcimadamore Jan 10, 2024

Choose a reason for hiding this comment

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

I believe TestScopedOperations should check for WTE as well?

Copy link

@mcimadamore mcimadamore Jan 10, 2024

Choose a reason for hiding this comment

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

That test has two methods: testOpAfterClose and testOpOutsideConfinement, which are executed on all the test cases.

Copy link
Contributor Author

@minborg minborg Jan 12, 2024

Choose a reason for hiding this comment

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

Good idea. We could do that in a separate PR.

try {
Arena otherThreadArena = future.get();
assertThrows(WrongThreadException.class, () ->
otherThreadArena.allocateFrom(elementLayout, source, sourceElementLayout, 0, 1)
);
} catch (ExecutionException | InterruptedException e) {
fail("Unable to create arena", e);
}

// IllegalArgumentException if {@code elementCount * sourceElementLayout.byteSize()} overflows
assertThrows(IllegalArgumentException.class, () ->
arena.allocateFrom(elementLayout, source, sourceElementLayout, 0, Long.MAX_VALUE)
);

// IndexOutOfBoundsException if {@code sourceOffset > source.byteSize() - (elementCount * sourceElementLayout.byteSize())}
assertThrows(IndexOutOfBoundsException.class, () ->
arena.allocateFrom(elementLayout, source, sourceElementLayout, source.byteSize() - (1 * sourceElementLayout.byteAlignment()) + elementLayout.byteSize(), 1)
);

// IndexOutOfBoundsException if {@code sourceOffset < 0}
assertThrows(IndexOutOfBoundsException.class, () ->
arena.allocateFrom(elementLayout, source, sourceElementLayout, -elementLayout.byteSize(), 1)
);

// IllegalArgumentException if {@code elementCount < 0}
assertThrows(IllegalArgumentException.class, () ->
arena.allocateFrom(elementLayout, source, sourceElementLayout, 0, -1)
);


}
}


@Test
public void testArrayAllocateDelegation() {
AtomicInteger calls = new AtomicInteger();
Expand Down
60 changes: 60 additions & 0 deletions test/jdk/java/foreign/TestSegmentCopy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,66 @@ public void testHyperAlignedDst() {
MemorySegment.copy(segment, JAVA_BYTE.withByteAlignment(2), 0, segment, 0, 4);
}

@Test
public void testCopy5ArgWithNegativeValues() {
MemorySegment src = MemorySegment.ofArray(new byte[] {1, 2, 3, 4});
MemorySegment dst = MemorySegment.ofArray(new byte[] {1, 2, 3, 4});
assertThrows(IndexOutOfBoundsException.class, () ->
MemorySegment.copy(src, -1, dst, 0, 4)
);
assertThrows(IndexOutOfBoundsException.class, () ->
MemorySegment.copy(src, 0, dst, -1, 4)
);
assertThrows(IndexOutOfBoundsException.class, () ->
MemorySegment.copy(src, 0, dst, 0, -1)
);
}

@Test
public void testCopy7ArgWithNegativeValues() {
MemorySegment src = MemorySegment.ofArray(new byte[] {1, 2, 3, 4});
MemorySegment dst = MemorySegment.ofArray(new byte[] {1, 2, 3, 4});
assertThrows(IndexOutOfBoundsException.class, () ->
MemorySegment.copy(src, JAVA_BYTE, -1, dst, JAVA_BYTE, 0, 4)
);
assertThrows(IndexOutOfBoundsException.class, () ->
MemorySegment.copy(src, JAVA_BYTE, 0, dst, JAVA_BYTE, -1, 4)
);
assertThrows(IndexOutOfBoundsException.class, () ->
MemorySegment.copy(src, JAVA_BYTE, 0, dst, JAVA_BYTE, 0, -1)
);
}

@Test
public void testCopyFromArrayWithNegativeValues() {
MemorySegment src = MemorySegment.ofArray(new byte[] {1, 2, 3, 4});
byte[] dst = new byte[] {1, 2, 3, 4};
assertThrows(IndexOutOfBoundsException.class, () ->
MemorySegment.copy(src, JAVA_BYTE, -1, dst, 0, 4)
);
assertThrows(IndexOutOfBoundsException.class, () ->
MemorySegment.copy(src, JAVA_BYTE, 0, dst, -1, 4)
);
assertThrows(IndexOutOfBoundsException.class, () ->
MemorySegment.copy(src, JAVA_BYTE, 0, dst, 0, -1)
);
}

@Test
public void testCopyToArrayWithNegativeValues() {
byte[] src = new byte[] {1, 2, 3, 4};
MemorySegment dst = MemorySegment.ofArray(new byte[] {1, 2, 3, 4});
assertThrows(IndexOutOfBoundsException.class, () ->
MemorySegment.copy(src, -1, dst, JAVA_BYTE, 0, 4)
);
assertThrows(IndexOutOfBoundsException.class, () ->
MemorySegment.copy(src, 0, dst, JAVA_BYTE, -1, 4)
);
assertThrows(IndexOutOfBoundsException.class, () ->
MemorySegment.copy(src, 0, dst, JAVA_BYTE, 0, -1)
);
}

enum Type {
// Byte
BYTE(byte.class, JAVA_BYTE, i -> (byte)i),
Expand Down
11 changes: 5 additions & 6 deletions test/jdk/java/foreign/TestSegments.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import java.util.function.Supplier;

import static java.lang.foreign.ValueLayout.JAVA_INT;
import static java.lang.foreign.ValueLayout.JAVA_LONG;
import static org.testng.Assert.*;

public class TestSegments {
Expand All @@ -55,14 +56,13 @@ public void testBadAllocateAlign(long size, long align) {
@Test
public void testZeroLengthNativeSegment() {
try (Arena arena = Arena.ofConfined()) {
Arena session = arena;
var segment = session.allocate(0, 1);
var segment = arena.allocate(0, 1);
assertEquals(segment.byteSize(), 0);
MemoryLayout seq = MemoryLayout.sequenceLayout(0, JAVA_INT);
segment = session.allocate(seq);
segment = arena.allocate(seq);
assertEquals(segment.byteSize(), 0);
assertEquals(segment.address() % seq.byteAlignment(), 0);
segment = session.allocate(0, 4);
segment = arena.allocate(0, 4);
assertEquals(segment.byteSize(), 0);
assertEquals(segment.address() % 4, 0);
MemorySegment rawAddress = MemorySegment.ofAddress(segment.address());
Expand Down Expand Up @@ -133,8 +133,7 @@ public void testDerivedScopes(Supplier<MemorySegment> segmentSupplier) {
@Test
public void testEqualsOffHeap() {
try (Arena arena = Arena.ofConfined()) {
Arena scope1 = arena;
MemorySegment segment = scope1.allocate(100, 1);
MemorySegment segment = arena.allocate(100, 1);
assertEquals(segment, segment.asReadOnly());
assertEquals(segment, segment.asSlice(0, 100));
assertNotEquals(segment, segment.asSlice(10, 90));
Expand Down