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

8266814: Improve library loading with SymbolLookup abstraction #531

Closed

Conversation

@mcimadamore
Copy link
Collaborator

@mcimadamore mcimadamore commented May 10, 2021

This patch implements the library loading abstraction described in:

https://mail.openjdk.java.net/pipermail/panama-dev/2021-May/013684.html

That is, a functional interface called SymbolLookup, and a couple of factories to get a lookup for a given classloader, and to get a system lookup, useful to lookup C symbols.

To implement the system lookup, we load msvcrt.dll on Windows, while we build and load an empty library (which depends on libc) on Mac/Linux. This approach is better than relying on RTLD_DEFAULT (which can sometimes leak symbols from libraries loaded independently). Also, doing this bypasses the problem of figuring out the location of libc, which, on Linux system is particularly gnarly, because of the multi-arch support.


Progress

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

Issue

  • JDK-8266814: Improve library loading with SymbolLookup abstraction

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/panama-foreign pull/531/head:pull/531
$ git checkout pull/531

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 531

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/panama-foreign/pull/531.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 10, 2021

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

* the JVM or, worse, silently result in memory corruption. Thus, clients should refrain from depending on
* restricted methods, and use safe and supported functionalities, where possible.
*
* @return a symbol lookup suitable to find symbols in libraries loaded by given classloader.
Copy link
Member

@sundararajana sundararajana May 10, 2021

Choose a reason for hiding this comment

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

"by given classloader" should be "by the caller's class loader"

@CallerSensitive
static SymbolLookup loaderLookup(ClassLoader loader) {
Reflection.ensureNativeAccess(Reflection.getCallerClass());
Objects.requireNonNull(loader);
Copy link
Member

@sundararajana sundararajana May 10, 2021

Choose a reason for hiding this comment

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

why do we filter null (bootstrap) loader?

Copy link
Collaborator Author

@mcimadamore mcimadamore May 10, 2021

Choose a reason for hiding this comment

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

Because boot loader is under control of the JDK, not the user. I don't think it's desirable to introduce coupling between user code and set of libraries which happen to be loaded by boot loader.

* The key abstractions introduced to support foreign function access is {@link jdk.incubator.foreign.CLinker}.
* CLinker provides linking capabilities which allow to model foreign functions as {@link java.lang.invoke.MethodHandle} instances,
* The key abstractions introduced to support foreign function access are {@link jdk.incubator.foreign.SymbolLookup} and {@link jdk.incubator.foreign.CLinker}.
* The former is used to load foreign libraries, as well as to lookup symbols inside said libraries; the latter
Copy link
Member

@sundararajana sundararajana May 10, 2021

Choose a reason for hiding this comment

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

"is used to load foreign libraries" should be removed?

@openjdk openjdk bot added the rfr label May 10, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 10, 2021

Webrevs

* the JVM or, worse, silently result in memory corruption. Thus, clients should refrain from depending on
* restricted methods, and use safe and supported functionalities, where possible.
*
* @return a symbol lookup suitable to find symbols in libraries loaded by the caller's classloader.
Copy link
Member

@JornVernee JornVernee May 10, 2021

Choose a reason for hiding this comment

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

Should this include an @throws for the ICE that can be thrown by the native access check?

LIBS := $(LIBCXX), \
))

else ifeq ($(call isTargetOs, windows), false)
Copy link
Member

@JornVernee JornVernee May 10, 2021

Choose a reason for hiding this comment

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

I feel like this code, and maybe also the associated code in SystemLookup that loads msvcrt.dll could do with a comment that explains why the same strategy is not used on Windows; namely that symbol lookup on Windows does not search a library's dependencies, as opposed to dlsym, so it's not as easy to re-export the symbols in msvcrt by creating a shim library.


final NativeLibrary syslookup = switch (CABI.current()) {
case SysV, AArch64 -> NativeLibraries.rawNativeLibraries(SystemLookup.class, false).loadLibrary("syslookup");
case Win64 -> NativeLibraries.rawNativeLibraries(SystemLookup.class, false)
Copy link
Member

@JornVernee JornVernee May 10, 2021

Choose a reason for hiding this comment

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

e.g. a short comment here that explains why Windows is different.

Copy link
Collaborator Author

@mcimadamore mcimadamore May 10, 2021

Choose a reason for hiding this comment

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

I'll add a comment here

final NativeLibrary syslookup = switch (CABI.current()) {
case SysV, AArch64 -> NativeLibraries.rawNativeLibraries(SystemLookup.class, false).loadLibrary("syslookup");
case Win64 -> NativeLibraries.rawNativeLibraries(SystemLookup.class, false)
.loadLibrary(Path.of(System.getenv("SystemRoot"), "System32", "msvcrt.dll").toString());
Copy link
Member

@JornVernee JornVernee May 10, 2021

Choose a reason for hiding this comment

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

Looks like this loadLibrary call does not except paths:

assert name.indexOf(File.separatorChar) < 0;

So this is causing an assertion error on Windows. I'll try to find a fix.

Copy link
Collaborator Author

@mcimadamore mcimadamore May 10, 2021

Choose a reason for hiding this comment

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

That's a bug - we should use System::load with a full path

Copy link
Member

@JornVernee JornVernee May 10, 2021

Choose a reason for hiding this comment

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

This patch fixes the problem:

Suggested change
.loadLibrary(Path.of(System.getenv("SystemRoot"), "System32", "msvcrt.dll").toString());
.loadLibrary(null, Path.of(System.getenv("SystemRoot"), "System32", "msvcrt.dll").toFile());

Copy link
Member

@sundararajana sundararajana May 10, 2021

Choose a reason for hiding this comment

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

does this work?! Path load has to use System::load as @mcimadamore mentioned...

Copy link
Member

@JornVernee JornVernee May 10, 2021

Choose a reason for hiding this comment

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

As far as I can tell there's no load on NativeLibraries, but this method loads from the supplied file.

Copy link
Collaborator Author

@mcimadamore mcimadamore May 10, 2021

Choose a reason for hiding this comment

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

As far as I can tell there's no load on NativeLibraries, but this method loads from the supplied file.

Right - note that this code was NOT using System::loadLibrary, but NativeLibraries::loadLibrary - hence the confusion.

mcimadamore and others added 2 commits May 10, 2021
…SymbolLookup.java

Co-authored-by: Jorn Vernee <JornVernee@users.noreply.github.com>
Copy link
Member

@sundararajana sundararajana left a comment

LGTM

@openjdk
Copy link

@openjdk openjdk bot commented May 11, 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:

8266814: Improve library loading with SymbolLookup abstraction

Reviewed-by: jvernee, sundar

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.

@openjdk openjdk bot added the ready label May 11, 2021
@mcimadamore
Copy link
Collaborator Author

@mcimadamore mcimadamore commented May 11, 2021

/integrate

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

@openjdk openjdk bot commented May 11, 2021

@mcimadamore Pushed as commit 9d22e43.

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

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