RFR: 8324858: [vectorapi] Bounds checking issues when accessing memory segments

[Paul Sandoz]

On Wed, 31 Jan 2024 00:10:26 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> 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