On Tue, 21 May 2024 10:20:27 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org> wrote:
>> When creating a nested memory access var handle, we ensure that the segment >> is accessed at the correct alignment for the root layout being accessed. But >> we do not ensure that the segment has at least the size of the accessed root >> layout. Example: >> >> >> MemoryLayout LAYOUT = sequenceLayout(2, structLayout(JAVA_INT.withName("x"), >> JAVA_INT.withName("y"))); >> VarHandle X_HANDLE = LAYOUT.varHandle(PathElement.sequenceElement(), >> PathElement.groupElement("x")); >> >> >> If I have a memory segment whose size is 12, I can successfully call >> X_HANDLE on it with index 1, like so: >> >> >> MemorySegment segment = Arena.ofAuto().allocate(12); >> int x = (int)X_HANDLE.get(segment, 0, 1); >> >> >> This seems incorrect: after all, the provided segment doesn't have enough >> space to fit _two_ elements of the nested structs. >> >> This PR adds checks to detect this kind of scenario earlier, thus improving >> usability. To achieve this we could, in principle, just tweak >> `LayoutPath::checkEnclosingLayout` and add the required size check. >> >> But doing so will add yet another redundant check on top of the other >> already added by [pull/19124](https://git.openjdk.org/jdk/pull/19124). >> Instead, this PR rethinks how memory segment var handles are created, in >> order to eliminate redundant checks. >> >> The main observation is that size and alignment checks depend on an >> _enclosing_ layout. This layout *might* be the very same value layout being >> accessed (this is the case when e.g. using `JAVA_INT.varHandle()`). But, in >> the general case, the accessed value layout and the enclosing layout might >> differ (e.g. think about accessing a struct field). >> >> Furthermore, the enclosing layout check depends on the _base_ offset at >> which the segment is accessed, _prior_ to any index computation that occurs >> if the accessed layout path has any open elements. It is important to notice >> that this base offset is only available when looking at the var handle that >> is returned to the user. For instance, an indexed var handle with >> coordinates: >> >> >> (MemorySegment, long /* base */, long /* index 1 */, long /* index 2 */, >> long /* index 3 */) >> >> >> Is obtained through adaptation, by taking a more basic var handle of the >> form: >> >> >> (MemorySegment, long /* offset */) >> >> >> And then injecting the result of the index multiplication into `offset`. As >> such, we can't add an enclosing layout check inside the var handle returned >> by `VarHandles::memorySegmentViewHandle`, as doing so will end up seeing the >> *wrong* off... > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix typo in javadoc Marked as reviewed by jvernee (Reviewer). src/java.base/share/classes/java/lang/invoke/X-VarHandleSegmentView.java.template line 123: > 121: static $type$ get(VarHandle ob, Object obb, long base) { > 122: VarHandleSegmentViewBase handle = (VarHandleSegmentViewBase)ob; > 123: AbstractMemorySegmentImpl bb = checkReadOnly(obb, true); For getter methods, which pass a constant `true` here, `checkReadOnly` essentially just does a null check and cast on the segment. Not sure if it's worth simplifying... (I'm happy if you want to leave it like this as well) ------------- PR Review: https://git.openjdk.org/jdk/pull/19251#pullrequestreview-2072366007 PR Review Comment: https://git.openjdk.org/jdk/pull/19251#discussion_r1610647133