On Thu, 16 May 2024 10:54:15 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* off...
>
> 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.
Note: we don't even check for overflows, e.g. `offset < 0`. The reasoning here
is that all layouts check for overflow on construction, so it's never possible
to construct a layout whose size is greater than `Long.MAX_VALUE`. This
constraint also affects computation for indexed var handles, since all the
`long` indices corresponding to open path elements are checked against their
bounds (and their bounds must be small enough so that the enclosing layout has
a size smaller than `Long.MAX_VALUE`).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19251#discussion_r1603142096
