On Wed, 15 May 2024 15:43:45 GMT, Maurizio Cimadamore <[email protected]>
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)...
Do we have any performance tests available to see if there are any potential
impacts?
src/java.base/share/classes/java/lang/invoke/VarHandleSegmentViewBase.java line
48:
> 46: final long alignmentMask;
> 47:
> 48: VarHandleSegmentViewBase(VarForm form, boolean be, long
> alignmentMask, boolean exact) {
Nit: Copyright year. This also applies to some other files in this PR.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/19251#issuecomment-2114955648
PR Review Comment: https://git.openjdk.org/jdk/pull/19251#discussion_r1603156019
