[foreign-memaccess+abi] RFR: 8301360: Add dereference layout paths to memory layout API

[Gavin Ray]

Awesome stuff here, read the test cases and this seems incredibly useful,
thank you!

On Mon, Jan 30, 2023 at 2:02 PM Jorn Vernee <jvernee at openjdk.org> wrote:

> On Mon, 30 Jan 2023 15:25:54 GMT, Maurizio Cimadamore <
> mcimadamore at openjdk.org> wrote:
>
> > Now that we have a more general way to attach target layouts to address
> layouts (see https://git.openjdk.org/panama-foreign/pull/775), it might
> be useful to add support for layout paths which perform dereference of a
> given address value. Consider this case:
> >
> >
> > struct b {
> >    int x;
> > };
> >
> > struct a {
> >     struct b *b;
> > };
> >
> >
> > Here, our goal is to construct a layout path for "a->b.x".
> >
> > To do this, we use a *dereference path element* (see
> `PathElement#dereferenceElement`). These comes in two flavors:
> >
> > * no argument factory - this assumes that the address layout being
> dereferenced has some target layout set;
> > * layout argument factory - this assumes that the address layout being
> dereferenced does *not* have a target layout set (e.g. it is not always
> possible to associate a target layout, especially in cyclic cases like a ->
> b -> a).
> >
> > When a dereference element is applied to an existing path, a var handle
> that fetches the address pointed to by the current path is pushed on the
> stack, and a new layout path is started. This way, dereference path can be
> used to "chain" together multiple layout paths. All the intermediate memory
> read operations occur using a *plain* get. For more complex access
> operation (e.g. if atomic access is required for the whole dereference
> chain), multiple var handles have to be obtained.
> >
> > It is an error to pass a dereference path to other layout operations
> such as `select`, `byte/bitOffset[Handle]`.
>
> Implementation looks good. I think there's some issue with resizing the
> segment for the layout accepting overload though (see inline comment).
>
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 638:
>
> > 636:          * The path element returned by this method does not alter
> the number of free dimensions of any path
> > 637:          * that is combined with such element. Using this path
> layout to dereference an address layout
> > 638:          * that has no target layout results in an {@link
> IllegalArgumentException} (e.g. when
>
> This IAE should be specified on `varHandle` as well.
>
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 652:
>
> > 650:          * The path element returned by this method does not alter
> the number of free dimensions of any path
> > 651:          * that is combined with such element. Using this path
> layout to dereference an address layout
> > 652:          * that already has a target layout results in an {@link
> IllegalArgumentException} (e.g. when
>
> Same here
>
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 658:
>
> > 656:          * @return a path element which dereferences an address
> layout.
> > 657:          */
> > 658:         static PathElement dereferenceElement(MemoryLayout layout) {
>
> This method needs to be restricted, I believe. Since it allows resizing a
> MemorySegment. The other overload of `dereferenceElement` is already
> covered because attaching a pointee layout to an address layout is
> restricted already.
>
> Consider the following:
>
>
> StructLayout hasAddr = MemoryLayout.structLayout(ADDRESS.withName("addr"));
>
> MemorySegment rawSeg = MemorySegment.allocateNative(0,
> SegmentScope.auto()); // inaccessible segment
> // rawSeg.get(JAVA_INT, 0); // not allowed
> MemorySegment addrHolder = MemorySegment.allocateNative(hasAddr,
> SegmentScope.auto());
> addrHolder.set(ADDRESS, 0, rawSeg); // fine
> VarHandle derefHandle = hasAddr.varHandle(groupElement("addr"),
> dereferenceElement(JAVA_INT));
> int x = derefHandle.get(addrHolder); // oops!
>
>
> Assuming I eye-balled this code correctly, I think this is problematic?
>
> src/java.base/share/classes/jdk/internal/foreign/LayoutPath.java line 278:
>
> > 276:     private static LayoutPath derefPath(MemoryLayout layout,
> MethodHandle handle, LayoutPath encl) {
> > 277:         MethodHandle[] handles = new
> MethodHandle[encl.derefAdapters.length + 1];
> > 278:         System.arraycopy(encl.derefAdapters, 0, handles, 0,
> encl.derefAdapters.length);
>
> This could be simplified using Arrays.copyOf:
>
> Suggestion:
>
>         MethodHandle[] handles = Arrays.copyOf(encl.derefAdapters,
> encl.derefAdapter.length + 1);
>
>
> (no need for separate call to System.arraycopy)
>
> -------------
>
> PR: https://git.openjdk.org/panama-foreign/pull/776
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/panama-dev/attachments/20230130/3a5c7467/attachment-0001.htm>