Re: RFR: 8312522: Implementation of Foreign Function & Memory API [v10]
On Wed, 6 Sep 2023 16:03:40 GMT, Paul Sandoz wrote: >> [This SO question](https://stackoverflow.com/a/40348010) points to a gitlab >> repo that seems to have the latest version: >> https://gitlab.com/x86-psABIs/x86-64-ABI But, I'm not sure how stable that >> is, or if that's an authoritative source. >> >> Alternatively, we could refer to the name only: "System V Application Binary >> Interface - AMD64 Architecture Processor Supplement" (or "x86-64 psABI") >> Then people can google for themselves and find it. > > Yeah, its hard to find the official and latest version. Referring to the full > title will help. I've added the name now: https://github.com/openjdk/jdk/pull/15103/commits/a48a77bcdadda65a81ad30abf00e6da46a56b933 - PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1318472039
Re: RFR: 8312522: Implementation of Foreign Function & Memory API [v10]
On Wed, 6 Sep 2023 10:50:59 GMT, Jorn Vernee wrote: >> src/java.base/share/classes/java/lang/foreign/Linker.java line 152: >> >>> 150: * >>> 151: * The following table shows some examples of how C types are modelled >>> in Linux/x64 (all the examples provided >>> 152: * here will assume these platform-dependent mappings): >> >> Up to you, but it might be useful to link to the ABI specifications if the >> links are considered stable. > > [This SO question](https://stackoverflow.com/a/40348010) points to a gitlab > repo that seems to have the latest version: > https://gitlab.com/x86-psABIs/x86-64-ABI But, I'm not sure how stable that > is, or if that's an authoritative source. > > Alternatively, we could refer to the name only: "System V Application Binary > Interface - AMD64 Architecture Processor Supplement" (or "x86-64 psABI") Then > people can google for themselves and find it. Yeah, its hard to find the official and latest version. Referring to the full title will help. - PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1317517319
Re: RFR: 8312522: Implementation of Foreign Function & Memory API [v10]
On Tue, 5 Sep 2023 21:01:00 GMT, Paul Sandoz wrote: >> Jorn Vernee has updated the pull request incrementally with five additional >> commits since the last revision: >> >> - 8315096: Allowed access modes in memory segment should depend on layout >> alignment >> >>Reviewed-by: psandoz >> - Add missing @implSpec to AddressLayout >> >>Reviewed-by: pminborg >> - Fix misc typos in FFM API javadoc >> >>Reviewed-by: jvernee >> - Clarify javadoc w.r.t. exceptions thrown by a memory access var handle >> (part two) >> >>Reviewed-by: pminborg >> - Clarify javadoc w.r.t. exceptions thrown by a memory access var handle >> >>Reviewed-by: jvernee > > src/java.base/share/classes/java/lang/foreign/Linker.java line 152: > >> 150: * >> 151: * The following table shows some examples of how C types are modelled >> in Linux/x64 (all the examples provided >> 152: * here will assume these platform-dependent mappings): > > Up to you, but it might be useful to link to the ABI specifications if the > links are considered stable. There is this: https://refspecs.linuxbase.org/elf/x86_64-abi-0.99.pdf I'm not sure how stable this is. I don't think that website is an authoritative source. (at least, not to the degree it is for e.g. AArch64: https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#595homogeneous-aggregates). Note also that is a draft. I think the final version is paywalled. Alternatively, we could refer to the name only: "System V Application Binary Interface - AMD64 Architecture Processor Supplement" (or "x86-64 psABI") Then people can google for themselves and find it. For instance, [this SO question](https://stackoverflow.com/a/40348010) points to a gitlab repo with a more up to date version: https://gitlab.com/x86-psABIs/x86-64-ABI/-/jobs/artifacts/master/raw/x86-64-ABI/abi.pdf?job=build - PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1317100703
Re: RFR: 8312522: Implementation of Foreign Function & Memory API [v10]
On Tue, 5 Sep 2023 21:10:47 GMT, Paul Sandoz wrote:
>> Jorn Vernee has updated the pull request incrementally with five additional
>> commits since the last revision:
>>
>> - 8315096: Allowed access modes in memory segment should depend on layout
>> alignment
>>
>>Reviewed-by: psandoz
>> - Add missing @implSpec to AddressLayout
>>
>>Reviewed-by: pminborg
>> - Fix misc typos in FFM API javadoc
>>
>>Reviewed-by: jvernee
>> - Clarify javadoc w.r.t. exceptions thrown by a memory access var handle
>> (part two)
>>
>>Reviewed-by: pminborg
>> - Clarify javadoc w.r.t. exceptions thrown by a memory access var handle
>>
>>Reviewed-by: jvernee
>
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 443:
>
>> 441: *
>> 442: * {@snippet lang = "java":
>> 443: * offset = this.offsetHandle(P).invokeExact(B, I1, I2, ... In);
>
> Suggestion:
>
> * O = this.offsetHandle(P).invokeExact(B, I1, I2, ... In);
>
> To align with the use of `O` later on.
Good catch. The `O` later on was added separately, hence they are out of sync.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1317041205
Re: RFR: 8312522: Implementation of Foreign Function & Memory API [v10]
On Mon, 4 Sep 2023 15:54:41 GMT, Jorn Vernee wrote:
>> This patch contains the implementation of the foreign linker & memory API
>> JEP for Java 22. The initial patch is composed of commits brought over
>> directly from the [panama-foreign
>> repo](https://github.com/openjdk/panama-foreign). The main changes found in
>> this patch come from the following PRs:
>>
>> 1. https://github.com/openjdk/panama-foreign/pull/836 Where previous
>> iterations supported converting Java strings to and from native strings in
>> the UTF-8 encoding, we've extended the supported encoding to all the
>> encodings found in the `java.nio.charset.StandardCharsets` class.
>> 2. https://github.com/openjdk/panama-foreign/pull/838 We dropped the
>> `MemoryLayout::sequenceLayout` factory method which inferred the size of the
>> sequence to be `Long.MAX_VALUE`, as this led to confusion among clients. A
>> client is now required to explicitly specify the sequence size.
>> 3. https://github.com/openjdk/panama-foreign/pull/839 A new API was added:
>> `Linker::canonicalLayouts`, which exposes a map containing the
>> platform-specific mappings of common C type names to memory layouts.
>> 4. https://github.com/openjdk/panama-foreign/pull/840 Memory access
>> varhandles, as well as byte offset and slice handles derived from memory
>> layouts, now feature an additional 'base offset' coordinate that is added to
>> the offset computed by the handle. This allows composing these handles with
>> other offset computation strategies that may not be based on the same memory
>> layout. This addresses use-cases where clients are working with 'dynamic'
>> layouts, whose size might not be known statically, such as variable length
>> arrays, or variable size matrices.
>> 5. https://github.com/openjdk/panama-foreign/pull/841 Remove this now
>> redundant API. Clients can simply use the difference between the base
>> address of two memory segments.
>> 6. https://github.com/openjdk/panama-foreign/pull/845 Disambiguate uses of
>> `SegmentAllocator::allocateArray`, by renaming methods that both allocate +
>> initialize memory segments to `allocateFrom`. (see the original PR for the
>> problematic case)
>> 7. https://github.com/openjdk/panama-foreign/pull/846 Improve the
>> documentation for variadic functions.
>> 8. https://github.com/openjdk/panama-foreign/pull/849 Several test fixes to
>> make sure the `jdk_foreign` tests can pass on 32-bit machines, taking
>> linux-x86 as a test bed.
>> 9. https://github.com/openjdk/panama-foreign/pull/850 Make the linker API
>> required. The `Linker::nativeLinker` method is not longer allowed to throw
>> an `UnsupportedO...
>
> Jorn Vernee has updated the pull request incrementally with five additional
> commits since the last revision:
>
> - 8315096: Allowed access modes in memory segment should depend on layout
> alignment
>
>Reviewed-by: psandoz
> - Add missing @implSpec to AddressLayout
>
>Reviewed-by: pminborg
> - Fix misc typos in FFM API javadoc
>
>Reviewed-by: jvernee
> - Clarify javadoc w.r.t. exceptions thrown by a memory access var handle
> (part two)
>
>Reviewed-by: pminborg
> - Clarify javadoc w.r.t. exceptions thrown by a memory access var handle
>
>Reviewed-by: jvernee
Recommend you do a sweep through the code for unused imports and fields and any
rogue `@since` in the internal classes.
src/java.base/share/classes/java/lang/foreign/AddressLayout.java line 53:
> 51: *
> 52: * @implSpec
> 53: * This class is immutable, thread-safe and href="{@docRoot}/java.base/java/lang/doc-files/ValueBased.html">value-based.
Strictly speaking it's the implementations, as stated in the `Linker`:
* Implementations of this interface are immutable, thread-safe and value-based.
src/java.base/share/classes/java/lang/foreign/Linker.java line 152:
> 150: *
> 151: * The following table shows some examples of how C types are modelled
> in Linux/x64 (all the examples provided
> 152: * here will assume these platform-dependent mappings):
Up to you, but it might be useful to link to the ABI specifications if the
links are considered stable.
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 439:
> 437: *
> 438: *
> 439: * If the provided layout path {@code P} contains no dereference
> elements, then the offset of the access operation is
Suggestion:
* If the provided layout path {@code P} contains no dereference elements,
then the offset {@code O} of the access operation is
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 443:
> 441: *
> 442: * {@snippet lang = "java":
> 443: * offset = this.offsetHandle(P).invokeExact(B, I1, I2, ... In);
Suggestion:
* O = this.offsetHandle(P).invokeExact(B, I1, I2, ... In);
To align with the use of `O` later on.
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 536:
> 534: *
> 535: *
>
Re: RFR: 8312522: Implementation of Foreign Function & Memory API [v10]
> This patch contains the implementation of the foreign linker & memory API JEP > for Java 22. The initial patch is composed of commits brought over directly > from the [panama-foreign repo](https://github.com/openjdk/panama-foreign). > The main changes found in this patch come from the following PRs: > > 1. https://github.com/openjdk/panama-foreign/pull/836 Where previous > iterations supported converting Java strings to and from native strings in > the UTF-8 encoding, we've extended the supported encoding to all the > encodings found in the `java.nio.charset.StandardCharsets` class. > 2. https://github.com/openjdk/panama-foreign/pull/838 We dropped the > `MemoryLayout::sequenceLayout` factory method which inferred the size of the > sequence to be `Long.MAX_VALUE`, as this led to confusion among clients. A > client is now required to explicitly specify the sequence size. > 3. https://github.com/openjdk/panama-foreign/pull/839 A new API was added: > `Linker::canonicalLayouts`, which exposes a map containing the > platform-specific mappings of common C type names to memory layouts. > 4. https://github.com/openjdk/panama-foreign/pull/840 Memory access > varhandles, as well as byte offset and slice handles derived from memory > layouts, now feature an additional 'base offset' coordinate that is added to > the offset computed by the handle. This allows composing these handles with > other offset computation strategies that may not be based on the same memory > layout. This addresses use-cases where clients are working with 'dynamic' > layouts, whose size might not be known statically, such as variable length > arrays, or variable size matrices. > 5. https://github.com/openjdk/panama-foreign/pull/841 Remove this now > redundant API. Clients can simply use the difference between the base address > of two memory segments. > 6. https://github.com/openjdk/panama-foreign/pull/845 Disambiguate uses of > `SegmentAllocator::allocateArray`, by renaming methods that both allocate + > initialize memory segments to `allocateFrom`. (see the original PR for the > problematic case) > 7. https://github.com/openjdk/panama-foreign/pull/846 Improve the > documentation for variadic functions. > 8. https://github.com/openjdk/panama-foreign/pull/849 Several test fixes to > make sure the `jdk_foreign` tests can pass on 32-bit machines, taking > linux-x86 as a test bed. > 9. https://github.com/openjdk/panama-foreign/pull/850 Make the linker API > required. The `Linker::nativeLinker` method is not longer allowed to throw an > `UnsupportedOperationException` on ... Jorn Vernee has updated the pull request incrementally with five additional commits since the last revision: - 8315096: Allowed access modes in memory segment should depend on layout alignment Reviewed-by: psandoz - Add missing @implSpec to AddressLayout Reviewed-by: pminborg - Fix misc typos in FFM API javadoc Reviewed-by: jvernee - Clarify javadoc w.r.t. exceptions thrown by a memory access var handle (part two) Reviewed-by: pminborg - Clarify javadoc w.r.t. exceptions thrown by a memory access var handle Reviewed-by: jvernee - Changes: - all: https://git.openjdk.org/jdk/pull/15103/files - new: https://git.openjdk.org/jdk/pull/15103/files/ea3daaed..de3e7479 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15103=09 - incr: https://webrevs.openjdk.org/?repo=jdk=15103=08-09 Stats: 306 lines in 6 files changed: 218 ins; 12 del; 76 mod Patch: https://git.openjdk.org/jdk/pull/15103.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15103/head:pull/15103 PR: https://git.openjdk.org/jdk/pull/15103
