RFR: 8321786: SegmentAllocator:allocateFrom(ValueLayout, MemorySegment, ValueLayout, long, long) spec mismatch in exception scenario [v4]

[Maurizio Cimadamore]

On Tue, 12 Dec 2023 13:23:27 GMT, Per Minborg <pminborg at openjdk.org> wrote:

>> This PR proposes to change the specification for some methods that take `long` offsets so that they will throw an `IllegalArgumentException` rather than an `IndexOutOfBoundsException` for negative values.
>> 
>> The PR also proposes to fix a bug where the allocation size would overflow and the specified exception was not thrown.
>
> Per Minborg has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Revert change in allocateNoInit

src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 396:

> 394:      * @throws IllegalArgumentException if {@code elementCount * sourceElementLayout.byteSize()} overflows
> 395:      * @throws IndexOutOfBoundsException if {@code sourceOffset > source.byteSize() - (elementCount * sourceElementLayout.byteSize())}
> 396:      * @throws IndexOutOfBoundsException if either {@code sourceOffset} or {@code elementCount} are {@code < 0}

I think that if elementCount < 0, we should throw IAE, not IOOBE. Note that we explain this method as:

 MemorySegment dest = this.allocate(elementLayout, elementCount);
 MemorySegment.copy(source, sourceElementLayout, sourceOffset, dest, elementLayout, 0, elementCount);


So, the set of exceptions thrown by these two methods should be preserved. This means that `allocate(MemoryLayout, long)` gets a first pass at checking - this means IAE for overflows and also for negative element count.

After that, the exceptions we can have are those specified in MemorySegment.copy.

src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 407:

> 405:         Objects.requireNonNull(sourceElementLayout);
> 406:         Objects.requireNonNull(elementLayout);
> 407:         Utils.checkNonNegativeIndex(elementCount, "elementCount");

I think this check should be omitted (see comment above)

src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 595:

> 593:                             long elementCount) {
> 594: 
> 595:         Utils.checkNonNegativeIndex(srcOffset, "srcOffset");

These new checks don't change the behavior, right? E.g. they will end up issuing an IIOBE, as before? So, why the changes? (note that these change might lead to double checks)

src/java.base/share/classes/jdk/internal/foreign/Utils.java line 216:

> 214:     }
> 215: 
> 216:     public static void checkNonNegativeArgument(long value, String name) {

Add `@ForceInline` to these

test/jdk/java/foreign/TestSegmentAllocators.java line 230:

> 228:             // IllegalStateException if the {@linkplain MemorySegment#scope() scope} associated
> 229:             // with {@code source} is not {@linkplain MemorySegment.Scope#isAlive() alive}
> 230:             Arena closedArena = Arena.ofConfined();

the scope/thread assertions are already checked in TestScopedOperations - I don't think we need this, but you might want to make sure a test case for this new allocator method is added there

test/jdk/java/foreign/TestSegmentAllocators.java line 248:

> 246:             }
> 247: 
> 248:             // ArithmeticException if {@code elementCount * sourceElementLayout.byteSize()} overflows

comment is wrong

test/jdk/java/foreign/TestSegments.java line 194:

> 192: 
> 193:     @Test(expectedExceptions = IndexOutOfBoundsException.class)
> 194:     public void testNegativeGetOffset() {

We have a test specifically for instance segment accessors (TestMemoryAccessInstance) which covers _all_ accessors - maybe better to move these new tests there?

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17079#discussion_r1424033026
PR Review Comment: https://git.openjdk.org/jdk/pull/17079#discussion_r1424035187
PR Review Comment: https://git.openjdk.org/jdk/pull/17079#discussion_r1424037078
PR Review Comment: https://git.openjdk.org/jdk/pull/17079#discussion_r1424037878
PR Review Comment: https://git.openjdk.org/jdk/pull/17079#discussion_r1424041046
PR Review Comment: https://git.openjdk.org/jdk/pull/17079#discussion_r1424041984
PR Review Comment: https://git.openjdk.org/jdk/pull/17079#discussion_r1424043834