<div dir="ltr">Awesome stuff here, read the test cases and this seems incredibly useful, thank you!</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jan 30, 2023 at 2:02 PM Jorn Vernee <<a href="mailto:jvernee@openjdk.org">jvernee@openjdk.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, 30 Jan 2023 15:25:54 GMT, Maurizio Cimadamore <<a href="mailto:mcimadamore@openjdk.org" target="_blank">mcimadamore@openjdk.org</a>> wrote:<br>
<br>
> Now that we have a more general way to attach target layouts to address layouts (see <a href="https://git.openjdk.org/panama-foreign/pull/775" rel="noreferrer" target="_blank">https://git.openjdk.org/panama-foreign/pull/775</a>), it might be useful to add support for layout paths which perform dereference of a given address value. Consider this case:<br>
> <br>
> <br>
> struct b {<br>
>    int x;<br>
> };<br>
> <br>
> struct a {<br>
>     struct b *b;<br>
> };<br>
> <br>
> <br>
> Here, our goal is to construct a layout path for "a->b.x".<br>
> <br>
> To do this, we use a *dereference path element* (see `PathElement#dereferenceElement`). These comes in two flavors:<br>
> <br>
> * no argument factory - this assumes that the address layout being dereferenced has some target layout set;<br>
> * 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).<br>
> <br>
> 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.<br>
> <br>
> It is an error to pass a dereference path to other layout operations such as `select`, `byte/bitOffset[Handle]`.<br>
<br>
Implementation looks good. I think there's some issue with resizing the segment for the layout accepting overload though (see inline comment).<br>
<br>
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 638:<br>
<br>
> 636:          * The path element returned by this method does not alter the number of free dimensions of any path<br>
> 637:          * that is combined with such element. Using this path layout to dereference an address layout<br>
> 638:          * that has no target layout results in an {@link IllegalArgumentException} (e.g. when<br>
<br>
This IAE should be specified on `varHandle` as well.<br>
<br>
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 652:<br>
<br>
> 650:          * The path element returned by this method does not alter the number of free dimensions of any path<br>
> 651:          * that is combined with such element. Using this path layout to dereference an address layout<br>
> 652:          * that already has a target layout results in an {@link IllegalArgumentException} (e.g. when<br>
<br>
Same here<br>
<br>
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 658:<br>
<br>
> 656:          * @return a path element which dereferences an address layout.<br>
> 657:          */<br>
> 658:         static PathElement dereferenceElement(MemoryLayout layout) {<br>
<br>
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.<br>
<br>
Consider the following:<br>
<br>
<br>
StructLayout hasAddr = MemoryLayout.structLayout(ADDRESS.withName("addr"));<br>
<br>
MemorySegment rawSeg = MemorySegment.allocateNative(0, SegmentScope.auto()); // inaccessible segment<br>
// rawSeg.get(JAVA_INT, 0); // not allowed<br>
MemorySegment addrHolder = MemorySegment.allocateNative(hasAddr, SegmentScope.auto());<br>
addrHolder.set(ADDRESS, 0, rawSeg); // fine<br>
VarHandle derefHandle = hasAddr.varHandle(groupElement("addr"), dereferenceElement(JAVA_INT));<br>
int x = derefHandle.get(addrHolder); // oops!<br>
<br>
<br>
Assuming I eye-balled this code correctly, I think this is problematic?<br>
<br>
src/java.base/share/classes/jdk/internal/foreign/LayoutPath.java line 278:<br>
<br>
> 276:     private static LayoutPath derefPath(MemoryLayout layout, MethodHandle handle, LayoutPath encl) {<br>
> 277:         MethodHandle[] handles = new MethodHandle[encl.derefAdapters.length + 1];<br>
> 278:         System.arraycopy(encl.derefAdapters, 0, handles, 0, encl.derefAdapters.length);<br>
<br>
This could be simplified using Arrays.copyOf:<br>
<br>
Suggestion:<br>
<br>
        MethodHandle[] handles = Arrays.copyOf(encl.derefAdapters, encl.derefAdapter.length + 1);<br>
<br>
<br>
(no need for separate call to System.arraycopy)<br>
<br>
-------------<br>
<br>
PR: <a href="https://git.openjdk.org/panama-foreign/pull/776" rel="noreferrer" target="_blank">https://git.openjdk.org/panama-foreign/pull/776</a><br>
</blockquote></div>