On Wed, 31 Jan 2024 00:10:26 GMT, Maurizio Cimadamore <[email protected]>
wrote:
>> The implementation of method `VectorSpecies::fromMemorySegment`, in
>> `AbstractSpecies::fromMemorySegment`, neglects to perform bounds checks on
>> the offset argument when the method is compiled by C2 (bounds checks are
>> performed when interpreted and by C1).
>>
>> This is an oversight and explicit bounds checks are required, as is already
>> case for the other load and store memory access methods (including storing
>> to memory memory segments).
>>
>> The workaround is to call the static method `{T}Vector::fromMemorySegment`.
>>
>> The fix is for the implementation(s) of `VectorSpecies::fromMemorySegment`
>> to do the same and call `{T}Vector::fromMemorySegment`, following the same
>> pattern for implementations of `VectorSpecies::fromArray`.
>>
>> The tests have been conservatively updated to call the species access method
>> where possible in the knowledge that it calls the vector access method (the
>> tests were intended to test out of bounds access when compiled by C2).
>>
>> Thinking ahead its tempting to remove the species access methods,
>> simplifying functionality that is duplicated.
>
> test/jdk/jdk/incubator/vector/templates/X-LoadStoreTest.java.template line
> 271:
>
>> 269: @DontInline
>> 270: static $abstractvectortype$ fromArray($type$[] a, int i) {
>> 271: return ($abstractvectortype$) SPECIES.fromArray(a, i);
>
> These new tests focus on the species method - but should we also test the
> statics factories in the record class, just in case a regression is
> introduced and the two implementation start diverging again?
My expectation is the risk is small, but of course non-zero. These tests can be
expensive to run so i was trying balance the risk without increasing test
execution times.
I could strengthen the comment from:
@ForceInline
@Override final
public ByteVector fromMemorySegment(MemorySegment ms, long offset,
ByteOrder bo) {
// User entry point: Be careful with inputs.
return ByteVector
.fromMemorySegment(this, ms, offset, bo);
}
to:
@ForceInline
@Override final
public ByteVector fromMemorySegment(MemorySegment ms, long offset,
ByteOrder bo) {
// User entry point
// Defer only to the equivalent method on the vector class, using
the same inputs
return ByteVector
.fromMemorySegment(this, ms, offset, bo);
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17621#discussion_r1472162760
