On Wed, 31 Jan 2024 00:10:26 GMT, Maurizio Cimadamore <mcimadam...@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