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

8244959: Jextract's VarargsInvoker fails to link functions when passing integer types other than long #164

Closed
wants to merge 3 commits into from

Conversation

@slowhog
Copy link
Collaborator

@slowhog slowhog commented May 13, 2020

This PR makes sure VarargsInvoker use exact size match layout for carrier type for varargs parameters.

Use sprintf as the test case, tested on Mac OS X, need verification on Windows to verify the needed share library is loaded by JVM.


Progress

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

Issue

  • JDK-8244959: Jextract's VarargsInvoker fails to link functions when passing integer types other than long

Reviewers

  • Athijegannathan Sundararajan (sundar - Committer) ⚠️ Review applies to 36c21c2
  • Maurizio Cimadamore (mcimadamore - Committer)
  • Jorn Vernee (jvernee - Committer) ⚠️ Review applies to 36c21c2

Download

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

…ng integer types other than long
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 13, 2020

👋 Welcome back henryjen! A progress list of the required criteria for merging this PR into foreign-jextract will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label May 13, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 13, 2020

Webrevs

@openjdk
Copy link

@openjdk openjdk bot commented May 14, 2020

@slowhog This change now passes all automated pre-integration checks, type /integrate in a new comment to proceed. After integration, the commit message will be:

8244959: Jextract's VarargsInvoker fails to link functions when passing integer types other than long

Reviewed-by: sundar, mcimadamore, jvernee
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 3 commits pushed to the foreign-jextract branch:

  • 111e5da: Automatic merge of foreign-abi into foreign-jextract
  • 32d1f6c: 8244938: Crash in foreign ABI CallArranger class when a test native function returns a nested struct
  • fa9ef4b: 8245003: jextract does not generate accessor for MemorySegement typed values

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge foreign-jextract into your branch, and then specify the current head hash when integrating, like this: /integrate 111e5da5aadaf843f7ff61221816fa4c6f473872.

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

@openjdk openjdk bot added the ready label May 14, 2020
Copy link
Collaborator

@mcimadamore mcimadamore left a comment

I guess this works, although, from a puristic perspective, in C when you call a variadic function, the trailing arguments are promoted to either int, or double or left alone (for numerics bigger than unsigned int). In other words, when you pass a char to a printf call, C will never really pass a char there - it will pass an int. It would be interesting to to do some experiments, where printf is invoked and some negative short or byte number is passed in, to see if the C code displays the number correctly, or if the lack of adequate sign extension is creating issues.

…ng integer types other than long
@slowhog
Copy link
Collaborator Author

@slowhog slowhog commented May 14, 2020

Implement argument promote instead, as we use invoke(), the conversion is done by MethodHandle.

Also added test for float values.

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Good work. I suspect that if we ported the StdLibTest to work on jextracted functions, we could probably stress test the varargs support quite a bit (as well as other things). Perhaps might be worth investing some time on that separately.

@slowhog
Copy link
Collaborator Author

@slowhog slowhog commented May 14, 2020

/integrate

@openjdk openjdk bot closed this May 14, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels May 14, 2020
@openjdk
Copy link

@openjdk openjdk bot commented May 14, 2020

@slowhog The following commits have been pushed to foreign-jextract since your change was applied:

  • 111e5da: Automatic merge of foreign-abi into foreign-jextract
  • 32d1f6c: 8244938: Crash in foreign ABI CallArranger class when a test native function returns a nested struct
  • fa9ef4b: 8245003: jextract does not generate accessor for MemorySegement typed values

Your commit was automatically rebased without conflicts.

Pushed as commit f1c6b46.

@slowhog slowhog deleted the slowhog:8244959 branch May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.