8307253: Make FunctionDescriptor::toMethodType an instance method of Linker #830

mcimadamore · 2023-05-02T14:07:40Z

This patch moves FunctionDescriptor::toMethodType as an instance method of Linker. This allows linker implementations to be more opinionated on how the function descriptor -> method type conversion should behave.

Progress

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

Issue

JDK-8307253: Make FunctionDescriptor::toMethodType an instance method of Linker

Reviewers

Jorn Vernee (@JornVernee - Committer) ⚠️ Review applies to c63efe57

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 830

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-05-02T14:08:35Z

👋 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.

mcimadamore · 2023-05-02T14:08:53Z

src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/CallArranger.java

-                    for (StorageCalculator.StructStorage(
-                            long offset, Class<?> ca, int byteWidth, VMStorage storage
-                    ) : structStorages) {
+                    for (StorageCalculator.StructStorage structStorage : structStorages) {


This change is not strictly required, but I was getting weird errors when building javadoc from IntelliJ w/o it. Since patterns in for-each are going away anyways, I thought perhaps we could just take care of this.

mlbridge · 2023-05-02T14:13:48Z

Webrevs

Fix javadoc and impl for FunctionDescriptor::toMethodType to allow sequence layouts.

mcimadamore · 2023-05-04T09:53:07Z

After some offline discussion, we realized that the current API is actually good enough: FunctionDescriptor::toMethodType defines the "lowest common denominator" for turning a function descriptor into a MethodType. In doing this conversion, we follow the carrier types indicated by value layouts. For structs/unions/sequences we assume the carrier is MemorySegment (it is possible that in the future we will also add a carrier() accessor on composite layouts that can be overridden). Finally, for padding layout we just throw (after all, a padding argument/return doesn't seem to make much sense). Capturing this conversion at the level of FunctionDescription still seems like the right thing to do.

However, there is a small detail which this patch fixes: FunctionDescriptor::toMethodType currently rejects sequence layouts. While it is certainly the case that the C ABI does not allow to pass arrays by value. And even if this is the most common choice, this is not the only possible choice (for instance, Pascal has special convention for passing arrays to functions where the size of the array is known). As such, I think we should not bias the conversion with what we know the native linker will and will not support.

(sidebar: I've personally always been of the opinion that, even if C ABI do not allow passing arrays by value, since we have a relatively straightforward way to turn a sequence layout into an equivalent struct layout, perhaps the native linker should also be not too biased about which layouts it accepts - after all struct Point { int x; int y; } and int[2] are not so far apart)

For this reason, I've tweaked the checks in the conversion, and then made some checks stricter in the linker - so that now sequence layouts are rejected at the level of the linker.

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

openjdk · 2023-05-04T13:01:54Z

@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:

8307253: Make FunctionDescriptor::toMethodType an instance method of Linker

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 no new commits pushed to the foreign-memaccess+abi branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

… construction)

JornVernee

Nice! Couple of minor comments.

JornVernee · 2023-05-08T13:00:14Z

src/java.base/share/classes/jdk/internal/foreign/FunctionDescriptorImpl.java

+        if (resLayout instanceof PaddingLayout) {
+            throw new IllegalArgumentException("Unsupported padding layout return in function descriptor: " + resLayout);
+        }
+        Optional<MemoryLayout> paddingLayout = argLayouts.stream().filter(l -> l instanceof PaddingLayout).findAny();


This could use Stream::anyMatch.

Uhm - I thought about that when writing the code, but then I can't print out the problematic layout.

test/jdk/java/foreign/TestFunctionDescriptor.java

mcimadamore · 2023-05-08T15:55:28Z

/integrate

openjdk · 2023-05-08T15:57:21Z

Going to push as commit 139a8d2.

openjdk · 2023-05-08T15:57:37Z

@mcimadamore Pushed as commit 139a8d2.

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

Initial push

083bf57

mcimadamore commented May 2, 2023

View reviewed changes

openjdk bot added the rfr Ready for review label May 2, 2023

Revert changes.

f5b4ef7

Fix javadoc and impl for FunctionDescriptor::toMethodType to allow sequence layouts.

JornVernee approved these changes May 4, 2023

View reviewed changes

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

openjdk bot added the ready Ready to be integrated label May 4, 2023

mcimadamore added 2 commits May 5, 2023 15:24

Merge branch 'foreign-memaccess+abi' into toMethodType

e8cbd9c

Address review comments (check for padding during function descriptor…

c63efe5

… construction)

JornVernee approved these changes May 8, 2023

View reviewed changes

Address review comment (tweak test to use non-void factory)

c5a733d

openjdk bot added the integrated Pull request has been integrated label May 8, 2023

openjdk bot closed this May 8, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review labels May 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8307253: Make FunctionDescriptor::toMethodType an instance method of Linker #830

8307253: Make FunctionDescriptor::toMethodType an instance method of Linker #830

Uh oh!

mcimadamore commented May 2, 2023 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented May 2, 2023

Uh oh!

mcimadamore May 2, 2023

Uh oh!

mlbridge bot commented May 2, 2023 •

edited

Loading

Uh oh!

mcimadamore commented May 4, 2023 •

edited

Loading

Uh oh!

Uh oh!

openjdk bot commented May 4, 2023

Uh oh!

JornVernee left a comment

Uh oh!

JornVernee May 8, 2023

Uh oh!

mcimadamore May 8, 2023

Uh oh!

JornVernee May 8, 2023

Uh oh!

Uh oh!

mcimadamore commented May 8, 2023

Uh oh!

openjdk bot commented May 8, 2023

Uh oh!

openjdk bot commented May 8, 2023

Uh oh!

Uh oh!

8307253: Make FunctionDescriptor::toMethodType an instance method of Linker #830

8307253: Make FunctionDescriptor::toMethodType an instance method of Linker #830

Uh oh!

Conversation

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

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented May 2, 2023

Uh oh!

mcimadamore May 2, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

mcimadamore commented May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

openjdk bot commented May 4, 2023

Uh oh!

JornVernee left a comment

Choose a reason for hiding this comment

Uh oh!

JornVernee May 8, 2023

Choose a reason for hiding this comment

Uh oh!

mcimadamore May 8, 2023

Choose a reason for hiding this comment

Uh oh!

JornVernee May 8, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcimadamore commented May 8, 2023

Uh oh!

openjdk bot commented May 8, 2023

Uh oh!

openjdk bot commented May 8, 2023

Uh oh!

Uh oh!

mcimadamore commented May 2, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 2, 2023 •

edited

Loading

mcimadamore commented May 4, 2023 •

edited

Loading