Skip to content

8309880: Add support for linking libffi on Windows and Mac #14446

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

Closed
wants to merge 5 commits into from

Conversation

JornVernee
Copy link
Member

@JornVernee JornVernee commented Jun 13, 2023

Update the make/autoconf/lib-ffi.m4 script to support using libffi on Windows and Mac.

For Windows I had to tweak fallbackLinker.c to be able to build: there was an import of stdint.h missing, and since it was using WSAGetLastError it needed to link against ws2_32.lib.

This PR also contains a fix originally made by @shipilev as part of: #13827 (with a minor tweak), in order to be able to build the fallback linker on mac correctly. I also disabled 3 tests that were failing when using the (libffi-based) fallback linker on that platform.


I've updated the createLibffiBundle.sh script for Mac, since I got it to work with a devkit, but I didn't manage to do the same for Windows. The steps I took to make my Windows libffi bundle were as follows:

  1. run 'x64 Native Tools Command Prompt for VS 2022'. cl.exe and link.exe should be on path
  2. run ucrt64 (this is one of the shell environments that comes with MSYS2). This should carry over the environment set up by the VS dev prompt.
  3. then, in the libffi repo root folder:
    3.a run autogen.sh
    3.b run:
bash configure \
  CC="/path/to/libffi/msvcc.sh -m64" \
  CXX="/path/to/libffi/msvcc.sh -m64" \
  CPPFLAGS="-DFFI_BUILDING_DLL" \
  --disable-docs \
  --prefix=<install dest>

(<install dest> can be whatever you like. That's what you point --with-libffi to).

  1. run make install. This should create the <intstall dest> directory with the files: include/ffi.h, include/ffitarget.h, lib/libffi.dll. It also creates a lib/libffi.lib file, but it is of the wrong file type, DLL rather than LIBRARY.
  2. Manually create a working .lib file:
    5.a use dumpbin /exports libffi.dll to get a list of exported symbols
    5.b put them in a libffi.def file: EXPORTS on the first line, then a symbol on each line following
    5.c run lib /def:libffi.def /machine:x64 /out:libffi.lib to create the right .lib file (lib is a visual studio tool)

Testing:

  • manual testing on Windows/x64 and Mac/AArch64, by running the jdk_foreign test suite with -Djdk.internal.foreign.CABI=FALLBACK (i.e. using the fallback linker).
  • Linux/x64 Zero test run of the jdk_foreign suite
  • Linux/AArch64 Zero build

Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8309880: Add support for linking libffi on Windows and Mac (Enhancement - P4)

Reviewers

Contributors

  • Aleksey Shipilev <shade@openjdk.org>
  • Jorn Vernee <jvernee@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14446/head:pull/14446
$ git checkout pull/14446

Update a local copy of the PR:
$ git checkout pull/14446
$ git pull https://git.openjdk.org/jdk.git pull/14446/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14446

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14446.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 13, 2023

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

@openjdk
Copy link

openjdk bot commented Jun 13, 2023

@JornVernee The following labels will be automatically applied to this pull request:

  • build
  • core-libs
  • hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added build build-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Jun 13, 2023
@JornVernee
Copy link
Member Author

/contributor add @shipilev
/contributor add @JornVernee

@openjdk
Copy link

openjdk bot commented Jun 13, 2023

@JornVernee
Contributor Aleksey Shipilev <shade@openjdk.org> successfully added.

@JornVernee
Copy link
Member Author

/label remove hotspot-compiler

@openjdk
Copy link

openjdk bot commented Jun 13, 2023

@JornVernee
Contributor Jorn Vernee <jvernee@openjdk.org> successfully added.

@openjdk openjdk bot removed the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jun 13, 2023
@openjdk
Copy link

openjdk bot commented Jun 13, 2023

@JornVernee
The hotspot-compiler label was successfully removed.

@JornVernee JornVernee marked this pull request as ready for review June 13, 2023 13:46
@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 13, 2023
@mlbridge
Copy link

mlbridge bot commented Jun 13, 2023

Webrevs

Copy link
Member

@erikj79 erikj79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add the instructions for building libffi for Windows in a comment in createLibffiBundle.sh? I think it would be good to keep those around now that you have figured out how to do it.

- Tab to spaces in java.base/Lib.gmk
- Add comment with Windows build instructions to createLibffiBundle.sh
@JornVernee
Copy link
Member Author

I've fixed the tab, and added the instructions for Windows to createLibffiBundle.sh.

@openjdk
Copy link

openjdk bot commented Jun 13, 2023

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

8309880: Add support for linking libffi on Windows and Mac

Co-authored-by: Aleksey Shipilev <shade@openjdk.org>
Co-authored-by: Jorn Vernee <jvernee@openjdk.org>
Reviewed-by: erikj

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 22 new commits pushed to the master branch:

  • b5b5b7c: 8309884: missing @SInCE tags in com.sun.source.*
  • 75dcc4e: 8307508: IndirectVarHandle.isAccessModeSupported throws NPE
  • bed9161: 8308474: DSA does not reset SecureRandom when initSign is called again
  • 3eec179: 8309854: ciReplay TestServerVM test fails with Graal
  • 1401087: 8309753: Include array classes in the output of -XX:+PrintSharedArchiveAndExit
  • e138685: 8309882: LinkedHashMap adds an errant serializable field
  • c0aa6bf: 8309390: [JVMCI] improve copying system properties into libgraal
  • 63843b1: 8309907: Remove unused _print_gc_overhead_limit_would_be_exceeded
  • 6d05360: 8304403: Remove unused methods in RangeCheckElimination::Bound
  • 9b0baa1: 8306281: function isWsl() returns false on WSL2
  • ... and 12 more: https://git.openjdk.org/jdk/compare/3ce1240ca1b4139980444c171e317f4bfeff9314...master

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

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 13, 2023
@JornVernee
Copy link
Member Author

I've also filed: https://bugs.openjdk.org/browse/JDK-8309954 as documentation of why the tests are disabled.

@JornVernee
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Jun 14, 2023

Going to push as commit 4c18b9e.
Since your change was applied there have been 32 commits pushed to the master branch:

  • 1d1ed0d: 8309957: Rename JDK-8309595 test to conform
  • e3d6fc8: 8309847: FrameForm and RegisterForm constructors should initialize all members
  • bd79db3: 8309613: [Windows] hs_err files sometimes miss information about the code containing the error
  • 63fe413: 8309890: TestStringDeduplicationInterned.java waits for the wrong condition
  • ba837b4: 8309910: Introduce jdk.internal.net.http.HttpConnection.getSNIServerNames() method
  • 5d19319: 8309878: Reduce inclusion of resolvedIndyEntry.hpp
  • 8aad881: 8309934: Update GitHub Actions to use JDK 17 for building jtreg
  • 9bfe415: 8305104: Remove the old core reflection implementation
  • bfef3c3: 8309955: Matcher uses @SInCE {@inheritdoc}
  • d7251c1: 8309757: com/sun/jdi/ReferrersTest.java fails with virtual test thread factory
  • ... and 22 more: https://git.openjdk.org/jdk/compare/3ce1240ca1b4139980444c171e317f4bfeff9314...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jun 14, 2023
@openjdk openjdk bot closed this Jun 14, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 14, 2023
@openjdk
Copy link

openjdk bot commented Jun 14, 2023

@JornVernee Pushed as commit 4c18b9e.

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

@JornVernee JornVernee deleted the LibFFI_Mac_Win branch June 14, 2023 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

2 participants