Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8261906: Improve jextract support for virtual functions #456

Closed
wants to merge 13 commits into from

Conversation

@mcimadamore
Copy link
Collaborator

@mcimadamore mcimadamore commented Feb 18, 2021

This patch add support for function pointer calls. Whenever jextract encounters a function pointer variable (be it a global variable, or a struct field), it will additionally emit code to call the function pointer - by means of a static wrapper around a virtual method handle.

The implementation is relatively straightforward, although there is some duplication in the code that is being emitted. I played with this a bit, to see if duplication could be removed, but I ended up with a single routine to generate wrappers which worked across the axis { virtual, non virtual } x { struct, global } - which in the end I found too complex for my taste (note that, e.g. when we're inside a struct, the logic to get the virtual address depends on what else is generated inside the struct, so there's a lot of ad-hocness).

For these reason I ended up with separate emit function for virtual wrappers (e.g. I did not reuse the code for native functions) - the code which emit virtual function wrappers is also overridden in StructBuilder. My sense is that if we move the code to use text blocks, the duplication will be much less problematic.

In OutputFactory, I cleaned up things a bit - since I realized that support for valist doesn't work - this is now removed. I will file a followup issue to add proper VaList support, both in downcalls and upcalls.


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Issue

  • JDK-8261906: Improve jextract support for virtual functions

Reviewers

Download

$ git fetch https://git.openjdk.java.net/panama-foreign pull/456/head:pull/456
$ git checkout pull/456

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 18, 2021

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

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Feb 18, 2021

@mcimadamore this pull request can not be integrated into foreign-jextract due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout virtual-calls
git fetch https://git.openjdk.java.net/panama-foreign foreign-jextract
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge foreign-jextract"
git push

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 18, 2021

Loading

Copy link
Member

@JornVernee JornVernee left a comment

LGTM. Minor comments inline

Loading

test/jdk/tools/jextract/funcPointerInvokers/func.h Outdated Show resolved Hide resolved
Loading
Loading
@openjdk
Copy link

@openjdk openjdk bot commented Feb 23, 2021

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

8261906: Improve jextract support for virtual functions

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-jextract 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-jextract branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added ready and removed merge-conflict labels Feb 23, 2021
@mcimadamore
Copy link
Collaborator Author

@mcimadamore mcimadamore commented Feb 23, 2021

I've addressed the various comments; I ended up simplifying the code a bit and remove the Constant.MethodHandle abstraction which was, I think, a bit too convoluted. Now virtual function invokers are generated as part of JavaSourceBuilder::addVar, which I think makes the most sense from the perspective of OutputFactory. To facilitate passing multiple arguments from OutputFactory to the various builders, I've added a couple of records (VarInfo and FunctionInfo) which store various info about variables and functions (e.g. carrier, method types, layouts, descriptors). This also allowed me to reuse most of the code in OutputFactory between visitVariable/visitFunction and generateFunctionalInteraface. Now all the three methods call a factory which returns an optional FunctionInfo (if missing, it means that the function/variable/func interface cannot be generated).

Loading

* add copyright headers in func.h/libFunc.h
@mcimadamore
Copy link
Collaborator Author

@mcimadamore mcimadamore commented Mar 3, 2021

Question: I realized there are two ways to support function pointers: one would be to do what this patch does, e.g. add an extra accessor to "call" the underlying function pointer. The other would be to have an accessor which, given a memory address encoding a function pointer, returns an instance of the functional interface that can be used to call it. In other words, we have a choice here:

typedef void (*func)(int,float);

func f;

One option is to generate a toplevel accessor, like this patch does:

void f(int x0, float x1) { ... }

another option is to piggy back on the functional interface that jextract emits for the typedef:

func f();

Both are valid approaches. The latter seem to depend on JDK-8259054

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 3, 2021

Mailing list message from Sebastian Stenzel on panama-dev:

Not sure if I understand this correctly, BUT I think the latter approach would simplify JDK-8259054 as well, wouldn't it?

Loading

@JornVernee
Copy link
Member

@JornVernee JornVernee commented Mar 3, 2021

I think both options look good. The latter is maybe a little more flexible, but the usage of the former looks more like what you would see in C:

foo->f(10, 21F)

In Java:

foo.f(segment, 10, 21F)

vs.

foo.f(segment).apply(10, 21F)

Maybe we can just do both? Otherwise, I think out of these 2 I prefer the first, since it's the more primitive variant of the 2 (i.e. no need to create an intermediate wrapper instance).

Loading

@mcimadamore
Copy link
Collaborator Author

@mcimadamore mcimadamore commented Mar 3, 2021

Maybe we can just do both? Otherwise, I think out of these 2 I prefer the first, since it's the more primitive variant of the 2 (i.e. no need to create an intermediate wrapper instance).

I have no strong preference, but the currently implemented option does have the advantage of not requiring an intermediate allocation for the wrapper (although not 100% how that matters in practice: I'd expect the object to be escape-analyzed if used only once).

Having two is problematic in terms of adding the required overloads (e.g. we'd need to make up a new name for both options, which might be hard to decipher).

It also occurs to me, that, with the currently implemented option, one could still do this:

func f = header::f;

E.g. if a functional interface is expected, it should be possible to instantiate it using a method reference.

Loading

@JornVernee
Copy link
Member

@JornVernee JornVernee commented Mar 3, 2021

Having two is problematic in terms of adding the required overloads (e.g. we'd need to make up a new name for both options, which might be hard to decipher).

Ah right. Maybe we could call the direct accessor f$call instead ?

It also occurs to me, that, with the currently implemented option, one could still do this:

func f = header::f;

E.g. if a functional interface is expected, it should be possible to instantiate it using a method reference.

That's an interesting idea, though I think it only works for global variables? (struct fields require a MemorySegment as well).

I think in general, when JDK-8259054 is implemented it should be easy enough to get a function interface instance:

func x = func.ofAddress(foo.f$get(segment));

So, maybe that's another reason to pick the simpler accessor...

Loading

@mcimadamore
Copy link
Collaborator Author

@mcimadamore mcimadamore commented Mar 3, 2021

Having two is problematic in terms of adding the required overloads (e.g. we'd need to make up a new name for both options, which might be hard to decipher).

Ah right. Maybe we could call the direct accessor f$call instead ?

It also occurs to me, that, with the currently implemented option, one could still do this:

func f = header::f;

E.g. if a functional interface is expected, it should be possible to instantiate it using a method reference.

That's an interesting idea, though I think it only works for global variables? (struct fields require a MemorySegment as well).

I think in general, when JDK-8259054 is implemented it should be easy enough to get a function interface instance:

func x = func.ofAddress(foo.f$get(segment));

So, maybe that's another reason to pick the simpler accessor...

I don't know. I think the argument cuts both ways. On the one hand, the simpler accessor seems more direct, but requires also more ad-hoc logic in code generation. On the other hand, if we fix JDK-8259054 and then we generate a function pointer accessor which does what you wrote above, we reuse more of the bits we already generate.

For instance, I like the fact that, by returning a functional interface, we stick to a principle where each bit of generate code does one thing: the accessor creates the functional interface; the apply method on the functional interface does the call (as opposed to have a single method which does both).

In other words, I find this:

foo.f(segment).apply(10, 21F)

more readable (not less) than (and less magic than):

foo.f(segment, 10, 21F)

Loading

@JornVernee
Copy link
Member

@JornVernee JornVernee commented Mar 3, 2021

I don't know. I think the argument cuts both ways. On the one hand, the simpler accessor seems more direct, but requires also more ad-hoc logic in code generation. On the other hand, if we fix JDK-8259054 and then we generate a function pointer accessor which does what you wrote above, we reuse more of the bits we already generate.

For instance, I like the fact that, by returning a functional interface, we stick to a principle where each bit of generate code does one thing: the accessor creates the functional interface; the apply method on the functional interface does the call (as opposed to have a single method which does both).

In other words, I find this:

foo.f(segment).apply(10, 21F)

more readable (not less) than (and less magic than):

foo.f(segment, 10, 21F)

Yeah, that's a good point. It also gets rid of the somewhat awkward segment prefix argument. Though I'm not sure I'd say that I find the former example less clunky-looking in comparison, it seems clearer that way, which I think is more important. So, you've convinced me ;) Let's go for the functional interface instance accessor.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 3, 2021

Mailing list message from Sebastian Stenzel on panama-dev:

On 3. Mar 2021, at 15:18, Maurizio Cimadamore <mcimadamore at openjdk.java.net> wrote:

In other words, I find this:

foo.f(segment).apply(10, 21F)

*more* readable (not less) than (and less magic than):

foo.f(segment, 10, 21F)

Furthermore, the `apply` function can be used from places that don't need to know about segments. This might be helpful for building libs with higher levels of abstraction.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 3, 2021

Mailing list message from Jorn Vernee on panama-dev:

Yep, that makes things a little bit easier as well. The alternative
would be to close over the segment manually:

??? MemorySegment segment = ...
??? func x = (int a, float b) -> foo.f(segment, a, b);

Jorn

On 03/03/2021 15:39, Sebastian Stenzel wrote:

On 3. Mar 2021, at 15:18, Maurizio Cimadamore <mcimadamore at openjdk.java.net> wrote:

In other words, I find this:

foo.f(segment).apply(10, 21F)

*more* readable (not less) than (and less magic than):

foo.f(segment, 10, 21F)
Furthermore, the `apply` function can be used from places that don't need to know about segments. This might be helpful for building libs with higher levels of abstraction.

Loading

@mcimadamore
Copy link
Collaborator Author

@mcimadamore mcimadamore commented Mar 4, 2021

I've uploaded a new iteration which emits a functional interface getter instead of the direct invoker we were generating before. The code got a bit simpler as the functional interface getter is small to generate compared to the mh invoker wrapper. There's a bit of dance in OutputFactory to get the name of the typedef which corresponds to the functional interface we have generated; the logic is not super robust, in the sense that we're just going to assume that if we see a function pointer type def, there's gonna be a functional interface with same name.

Loading

Copy link
Member

@JornVernee JornVernee left a comment

Changes look good! Though there is a merge failure it looks like.

Loading

@openjdk openjdk bot added merge-conflict and removed ready labels Mar 4, 2021
Copy link
Member

@JornVernee JornVernee left a comment

Looks good!

Loading

@mcimadamore
Copy link
Collaborator Author

@mcimadamore mcimadamore commented Mar 5, 2021

/integrate

Loading

@openjdk openjdk bot added ready and removed merge-conflict labels Mar 6, 2021
@openjdk openjdk bot closed this Mar 6, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 6, 2021

@mcimadamore Pushed as commit 7532916.

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants