RFR: 8331865: Consolidate size and alignment checks in LayoutPath

[Maurizio Cimadamore]

On Wed, 15 May 2024 15:43:45 GMT, Maurizio Cimadamore <mcimadamore at 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)...

src/java.base/share/classes/java/lang/invoke/X-VarHandleSegmentView.java.template line 100:

> 98: 
> 99:     @ForceInline
> 100:     static AbstractMemorySegmentImpl checkReadOnly(Object obb, boolean ro) {

This and the following methods are the bulk of the changes in this template. That is, we no longer check size and alignment of the accessed segment. Every other change in this template is needed to get rid of fields and parameters that are no longer used.

src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 831:

> 829:     @Override
> 830:     public void set(AddressLayout layout, long offset, MemorySegment value) {
> 831:         Objects.requireNonNull(value);

This has been added, otherwise it would be possible to pass a `null` as the value of `value`, and not get an NPE, in case e.g. the alignment of the segment is incorrect (because we now check that before we even try to perform the memory access).

test/jdk/java/foreign/TestAccessModes.java line 62:

> 60:         } catch (IllegalArgumentException ex) {
> 61:             // access is unaligned
> 62:             assertTrue(segment.maxByteAlignment() < layout.byteAlignment());

Note: this change is required because, before this PR, we used to issue UOE for a bad access mode regardless of the alignment with which we accessed the segment (well, only for toplevel var handles). Now we uniformly check alignment _before_ access mode, for both toplevel and nested var handles, so this assertion needed to be tweaked.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/19251#discussion_r1603127240
PR Review Comment: https://git.openjdk.org/jdk/pull/19251#discussion_r1603120915
PR Review Comment: https://git.openjdk.org/jdk/pull/19251#discussion_r1603124981