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)...
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
