RFR: 8331865: Consolidate size and alignment checks in LayoutPath [v2]

[Paul Sandoz]

On Thu, 16 May 2024 14:37:15 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* off...
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix copyrights

Took me a few passes to work it all out. Looks good. All the bounds checking action now consistently passes through the call to `checkEnclosingLayout` in adaption of the raw (and unsafe) segment accessing VarHandle.

Separately, we might be missing a few long argument accepting guard methods for simpler cases as I suspect they are still focused more on int index types.

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

PR Review: https://git.openjdk.org/jdk/pull/19251#pullrequestreview-2064493620