On Wed, 15 May 2024 15:43:45 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* offset (e.g. an offset in which the index part has already been mixed > in)... This pull request has now been integrated. Changeset: c003c120 Author: Maurizio Cimadamore <mcimadam...@openjdk.org> URL: https://git.openjdk.org/jdk/commit/c003c1207fae07bcfe5a6f642a9c05e6c591e7a6 Stats: 205 lines in 12 files changed: 72 ins; 39 del; 94 mod 8331865: Consolidate size and alignment checks in LayoutPath Reviewed-by: psandoz, jvernee ------------- PR: https://git.openjdk.org/jdk/pull/19251