8308293: A linker should expose the layouts it supports by mcimadamore · Pull Request #839 · openjdk/panama-foreign

mcimadamore · 2023-06-15T10:27:53Z

This is another stab at openjdk/jdk#14037

I believe, after some offline discussion, that we have found a more satisfying solution to the problem of JAVA_CHAR being exposed. Jorn suggested that linkers should also provide mappings for JNI types such as jint, jshort and such (which are aliases for our layout constants anyway). I think that's a great way to bring JAVA_CHAR back into the fold.

For now, I decided not to specify support for JNI canonical layouts (but I could do so, if that's preferred). I think the highest priority is to provide some stable mappings for C builtin types (+ size_t) as that's what 99% of developers will be struggling with.

API-wise, we just expose a map. In the preovious PR there were questions as to whether the map should be split into two methods. In general I see the following options:

Just expose a map (that's the primitive, other things can derived from it)
Expose a map, plus a method to get a canonical layout from a type name (that's the Charset approach, which has both availableCharsets and forName)
Expose a method to get canonical layout from name, plus a method that returns the set of supported canonical layout names

My (not so strong) preference would be for either (1) or (2).

Progress

Change must not contain extraneous whitespace
Change must be properly reviewed (1 review required, with at least 1 Committer)

Issues

JDK-8308293: A linker should expose the layouts it supports (Bug - P3)
JDK-8308304: A linker should expose the layouts it supports (CSR) (Withdrawn)

Reviewers

Jorn Vernee (@JornVernee - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/panama-foreign.git pull/839/head:pull/839
$ git checkout pull/839

Update a local copy of the PR:
$ git checkout pull/839
$ git pull https://git.openjdk.org/panama-foreign.git pull/839/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 839

View PR using the GUI difftool:
$ git pr show -t 839

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/panama-foreign/pull/839.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-06-15T10:29:39Z

👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into foreign-memaccess+abi will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2023-06-15T10:34:25Z

Webrevs

src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java

minborg · 2023-06-15T10:51:55Z

Maybe we could add a test that ensures basic mapping is in place?

mcimadamore · 2023-06-15T13:18:43Z

Maybe we could add a test that ensures basic mapping is in place?

Good idea.

src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java

src/java.base/share/classes/java/lang/foreign/Linker.java

Add test Add fallback linker support

JornVernee · 2023-06-16T12:17:50Z

test/jdk/java/foreign/TestLinker.java

 /*
 * @test
 * @enablePreview
- * @requires jdk.foreign.linker != "UNSUPPORTED"


This shouldn't be removed I think? (Note that the fallback linker is FALLBACK, so this test was already enabled for it)

Apologies - I was fighting with jtreg to get the fallback linker enabled (so I could test). This line got in between :-)

openjdk · 2023-06-16T13:20:01Z

@mcimadamore This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8308293: A linker should expose the layouts it supports

Reviewed-by: jvernee

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 33 new commits pushed to the foreign-memaccess+abi branch:

e8d2173: Automatic merge of master into foreign-memaccess+abi
387657c: Automatic merge of jdk:master into master
fb024fd: 8248149: G1: change _cleaning_claimed from int to bool
c2b043e: 8310183: Update GitHub Actions to use boot JDK for building jtreg
238c51e: 8293069: Make -XX:+Verbose less verbose
8c9b85a: 8303916: ThreadLists.java inconsistent results
227656f: 8309408: Thread.sleep cleanup
32243ef: 8310128: Switch with unnamed patterns erroneously non-exhaustive
cfae6ef: 8309778: java/nio/file/Files/CopyAndMove.java fails when using second test directory
db133db: 8310106: sun.security.ssl.SSLHandshake.getHandshakeProducer() incorrectly checks handshakeConsumers
... and 23 more: https://git.openjdk.org/panama-foreign/compare/b008428dd1fe92c0c955c396cc2f55207d1a3fd2...foreign-memaccess+abi

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the foreign-memaccess+abi branch, type /integrate in a new comment.

mcimadamore · 2023-06-16T14:11:20Z

Following some offline discussion, I've also decided to add support for wchar_t which is a type with platform-dependent size that can be useful when working with wide strings (esp. given the new, more general string methods).

mcimadamore · 2023-06-16T14:12:44Z

src/java.base/share/classes/jdk/internal/foreign/abi/fallback/LibFallback.java

        static final MemorySegment FLOAT_TYPE = MemorySegment.ofAddress(ffi_type_float());
        static final MemorySegment DOUBLE_TYPE = MemorySegment.ofAddress(ffi_type_double());
        static final MemorySegment POINTER_TYPE = MemorySegment.ofAddress(ffi_type_pointer());
+        static final int SIZEOF_SHORT = ffi_sizeof_short();


Decided to change this a bit - e.g. instead of returning FFI type (which we don't use), I return sizeof instead, which seems more directly usable. This leads to some code simplifications (see definition of SIZE_T in FFIType).

This looks good. ffi_type also has an alignment field, which we could also theoretically look at, but I think it's fine to assume that we always have naturally aligned types here.

mcimadamore · 2023-06-16T15:48:50Z

So, now that we got the implementation nailed, any further opinion on the API bikeshed? E.g. is a Map enough? Do we want to add other methods? Given this is a rather niche functionality (e.g. mostly for clients that want to dynamically discover layouts), my general feeling is to keep it simple and just expose a map. But I don't have a super strong opinion.

JornVernee · 2023-06-16T15:51:23Z

I'm in favor of using a map as well. I don't think there are any issue with doing that?

mcimadamore · 2023-06-19T11:24:41Z

/integrate

openjdk · 2023-06-19T11:25:56Z

Going to push as commit 7a6fc80.
Since your change was applied there have been 33 commits pushed to the foreign-memaccess+abi branch:

e8d2173: Automatic merge of master into foreign-memaccess+abi
387657c: Automatic merge of jdk:master into master
fb024fd: 8248149: G1: change _cleaning_claimed from int to bool
c2b043e: 8310183: Update GitHub Actions to use boot JDK for building jtreg
238c51e: 8293069: Make -XX:+Verbose less verbose
8c9b85a: 8303916: ThreadLists.java inconsistent results
227656f: 8309408: Thread.sleep cleanup
32243ef: 8310128: Switch with unnamed patterns erroneously non-exhaustive
cfae6ef: 8309778: java/nio/file/Files/CopyAndMove.java fails when using second test directory
db133db: 8310106: sun.security.ssl.SSLHandshake.getHandshakeProducer() incorrectly checks handshakeConsumers
... and 23 more: https://git.openjdk.org/panama-foreign/compare/b008428dd1fe92c0c955c396cc2f55207d1a3fd2...foreign-memaccess+abi

Your commit was automatically rebased without conflicts.

openjdk · 2023-06-19T11:26:03Z

@mcimadamore Pushed as commit 7a6fc80.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

ExE-Boss · 2023-08-01T11:16:01Z

src/java.base/share/classes/jdk/internal/foreign/abi/fallback/FallbackLinker.java

+
+    @Override
+    public Map<String, MemoryLayout> canonicalLayouts() {
+        return CANONICAL_LAYOUTS;


CANONICAL_LAYOUTS is set to a HashMap, which is not unmodifiable.

Suggested change

return CANONICAL_LAYOUTS;

return Collections.unmodifiableMap(CANONICAL_LAYOUTS);

Initial push

df5950f

openjdk bot added the rfr Ready for review label Jun 15, 2023

minborg reviewed Jun 15, 2023

View reviewed changes

src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java Outdated Show resolved Hide resolved

JornVernee reviewed Jun 15, 2023

View reviewed changes

mcimadamore added 2 commits June 16, 2023 12:25

Add review comments

b0ad1ba

Add test Add fallback linker support

Add trailing newline

2018c41

JornVernee reviewed Jun 16, 2023

View reviewed changes

Re-add accidentally removed line in test

1d1b103

JornVernee approved these changes Jun 16, 2023

View reviewed changes

openjdk bot added the ready Ready to be integrated label Jun 16, 2023

Add support for wchar_t

ac9fbf6

mcimadamore commented Jun 16, 2023

View reviewed changes

JornVernee approved these changes Jun 16, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jun 19, 2023

openjdk bot closed this Jun 19, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Jun 19, 2023

JornVernee mentioned this pull request Aug 1, 2023

8312522: Implementation of Foreign Function & Memory API openjdk/jdk#15103

Closed

5 tasks

ExE-Boss suggested changes Aug 1, 2023

View reviewed changes

	return CANONICAL_LAYOUTS;
	return Collections.unmodifiableMap(CANONICAL_LAYOUTS);

Conversation

mcimadamore commented Jun 15, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jun 15, 2023

Uh oh!

mlbridge bot commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

minborg commented Jun 15, 2023

Uh oh!

mcimadamore commented Jun 15, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JornVernee Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcimadamore Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jun 16, 2023

Uh oh!

mcimadamore commented Jun 16, 2023

Uh oh!

mcimadamore Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

JornVernee Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcimadamore commented Jun 16, 2023

Uh oh!

JornVernee commented Jun 16, 2023

Uh oh!

mcimadamore commented Jun 19, 2023

Uh oh!

openjdk bot commented Jun 19, 2023

Uh oh!

openjdk bot commented Jun 19, 2023

Uh oh!

ExE-Boss Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

mcimadamore commented Jun 15, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 15, 2023 •

edited

Loading

JornVernee Jun 16, 2023 •

edited

Loading

JornVernee Jun 16, 2023 •

edited

Loading