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

8254024: Enhance native libs for AWT and Swing to work with GraalVM Native Image #562

Closed
wants to merge 1 commit into from

Conversation

arodionov
Copy link

@arodionov arodionov commented Oct 8, 2020

The following PR fixes https://bugs.openjdk.java.net/browse/JDK-8254024

Starting from version 11.0.9, all JDK libraries also build as static libraries (JEP 178: Statically-Linked JNI Libraries).
The purpose of using static libraries is to build GraalVM Native image statically linked with Java native libraries, to shipping single executable without runtime dependencies from JRE.

For some static libraries, it leads to an issue: if one static library is trying to load another static library in runtime using dlopen (and then uses dlsym to find symbols). With static libraries, this is not possible and all dynamic calls should be replaced.

This happens in libawt while it loads awt-xawt or awt_headless and in mlib_image.

Current PR fixes this issue for AWT libraries that allow building Swing/AWT applications as a Native image and have no runtime dependencies from JRE.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8254024: Enhance native libs for AWT and Swing to work with GraalVM Native Image

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/562/head:pull/562
$ git checkout pull/562

@bridgekeeper bridgekeeper bot added the oca label Oct 8, 2020
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 8, 2020

Hi @arodionov, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user arodionov" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@openjdk
Copy link

openjdk bot commented Oct 8, 2020

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

  • 2d
  • awt

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 2d awt labels Oct 8, 2020
@arodionov
Copy link
Author

arodionov commented Oct 8, 2020

/covered

@bridgekeeper bridgekeeper bot added the oca-verify label Oct 8, 2020
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 8, 2020

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

@robilad
Copy link

robilad commented Oct 9, 2020

Hi, please send the details whose OCA you are covered under to Dalibor.topic@oracle.com so that I can verify your account. Thanks!

@arodionov
Copy link
Author

arodionov commented Oct 9, 2020

@robilad Hi Dalibor! Have sent you email with details.
Thanks!

@arodionov
Copy link
Author

arodionov commented Oct 13, 2020

/signed

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 13, 2020

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

@bridgekeeper bridgekeeper bot removed oca oca-verify labels Oct 14, 2020
@openjdk openjdk bot added the rfr label Oct 14, 2020
@arodionov
Copy link
Author

arodionov commented Oct 14, 2020

@bobvandette Could you please join to the review?

@mlbridge
Copy link

mlbridge bot commented Oct 14, 2020

Webrevs

@prrace
Copy link
Contributor

prrace commented Oct 14, 2020

It is generally not expected for such a proposed change to pop up out of nowhere from someone who is a new contributor
without prior discussion and so forth.
I have no idea where this change is coming from, why we would want it, and how it is expected to be supported and you have provided zero
explanation of any of it in the PR. I think you need to start by explaining the big picture, and what you expect to do
with this and how you are guaranteeing to support it and ensure it breaks NOTHING else. At all.
And then we could also use some technical details on what you perceive is needed and why this resolives it.
Also is this "everything" ?

@prrace
Copy link
Contributor

prrace commented Oct 14, 2020

/reviewers 3

@openjdk
Copy link

openjdk bot commented Oct 14, 2020

@prrace
The number of required reviews for this PR is now set to 3 (with at least 1 of role reviewers).

@bobvandette
Copy link
Contributor

bobvandette commented Oct 14, 2020

It is generally not expected for such a proposed change to pop up out of nowhere from someone who is a new contributor
without prior discussion and so forth.
I have no idea where this change is coming from, why we would want it, and how it is expected to be supported and you have provided zero
explanation of any of it in the PR. I think you need to start by explaining the big picture, and what you expect to do
with this and how you are guaranteeing to support it and ensure it breaks NOTHING else. At all.
And then we could also use some technical details on what you perceive is needed and why this resolives it.
Also is this "everything" ?

I've been coaching @arodionov through this change. This fix extends the work I did to allow a Java program to be created using all static libraries. This PR fixes issues where there are multiply defined symbols in libawt, the headless and xawt toolkits when linked into a single executable. It also solves the problem where dlsym doesn't work for non shared libraries.

@prrace
Copy link
Contributor

prrace commented Oct 14, 2020

Sorry I am not familiar with that work. Please provide more info.

@bobvandette
Copy link
Contributor

bobvandette commented Oct 14, 2020

Sorry I am not familiar with that work. Please provide more info.

Here's the JEP for the work done in JDK8 to allow libraries to be built and used statically.

https://bugs.openjdk.java.net/browse/JDK-8046168

Here's the changes done to allow a Mac build to be done fully statically.

http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/55573c377d64

@mrserb
Copy link
Member

mrserb commented Oct 15, 2020

/reviewers 2

@openjdk
Copy link

openjdk bot commented Oct 15, 2020

@mrserb
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@mrserb
Copy link
Member

mrserb commented Oct 15, 2020

Here's the JEP for the work done in JDK8 to allow libraries to be built and used statically.

Does it mean that in the case of static-libs both variants of AWT libraries(xawt and headless) will be loaded?
How it is possible to test the current changes?

@prrace
Copy link
Contributor

prrace commented Oct 15, 2020

I would have to suppose that isn't possible but it is not clear whether the authors
of the fix are OK with this or not because the PR is very light on background and explanation. Which is a blocker for me.

I don't get the motivation from the JEP as to how it applies here.
There are two motivations listed

  1. For environments that don't support dynamic linking, and that doesn't apply to 64 bit Linux server so we can rule that out
  2. For applications that "want" to statically link. I want a lot of things but wanting it isn't a very well expressed motivation for something like this.

@arodionov
Copy link
Author

arodionov commented Oct 15, 2020

Does it mean that in the case of static-libs both variants of AWT libraries(xawt and headless) will be loaded?

No, they shouldn't. If this happens it causes duplicate symbol error during linkage phase.
For GraalVM native image we select what library should be linked based on -Djava.awt.headless property.

How it is possible to test the current changes?

For GraalVM native image, we run JCK tests api/java_awt and api/javax_swing. Those tests built as a native image.

@mrserb
Copy link
Member

mrserb commented Nov 14, 2020

The changes in awt_LoadLibrary.c and XToolkit.c is mostly noop except excluding dladdr/dlopen. Since XsessionWMcommand/AWTIsHeadless are not used at least in the mainline(jdk16).
The changes in the mlib look fine.

The testing is in progress.

@mrserb
Copy link
Member

mrserb commented Nov 14, 2020

please merge the master to this branch, it is quite outdated, and update the last copyright date to 2020.

@arodionov
Copy link
Author

arodionov commented Nov 30, 2020

@mrserb I have rebased PR on the master, but XsessionWMcommand and AWTIsHeadless functions are still at XToolkit.c in the mainline. So, to prevent the conflicting symbols error during static libraries linkage (libawt.a and libawt_xawt.a), we need to rename or to hide conflicting functions.

mrserb
mrserb approved these changes Dec 3, 2020
@arodionov
Copy link
Author

arodionov commented Dec 4, 2020

@dougxc could you please review AWT static-libs enhancement for GraalVM Native Image?

@dougxc
Copy link
Member

dougxc commented Dec 4, 2020

@dougxc could you please review AWT static-libs enhancement for GraalVM Native Image?

Sorry, this needs to be reviewed by people who understand this area of the code. Maybe @mrserb can suggest someone.

@arodionov
Copy link
Author

arodionov commented Dec 8, 2020

@prrace I have added more information to PR description, sorry for the inconvenience.

Regarding the use case of using AWT static libraries: with GraalVM Native image we can take a jar with some AWT/Swing application and convert it (using gcc and static libraries) to binary executable (currently only for Linux 64) and then run it without JRE.

@prrace
Copy link
Contributor

prrace commented Dec 8, 2020

This happens in libawt while it loads awt-xawt or awt_headless and in mlib_image.

So I am supposing this means you are producing two different static images ? - either all the time or based on a build flag ?
One is linking in the X11 lib, the other the headless stub lib ??

Can you confirm that understanding ?
This of course doesn't scale very well - meaning you are lucky we don't do this more often as you may need more combinations.

@arodionov
Copy link
Author

arodionov commented Dec 9, 2020

No, we produce only one image. The headless/non-headless mode is defined by the system property -Djava.awt.headless=....
Here is a command example for native image generation:
$ ~/graalvm-ce-java11-21.0.0-dev/bin/native-image -H:ConfigurationFileDirectories=./configs -Djava.awt.headless=true AWTFixExample

With -Djava.awt.headless=false to the gcc linker we pass libawt.a and libawt_headless.a
With -Djava.awt.headless=true to the gcc linker we pass libawt.a and libawt-xawt.a
If the generated image demands libmlib_image.a it will be also linked.

@prrace
Copy link
Contributor

prrace commented Dec 9, 2020

No, we produce only one image. The headless/non-headless mode is defined by the system property -Djava.awt.headless=....
Here is a command example for native image generation:
$ ~/graalvm-ce-java11-21.0.0-dev/bin/native-image -H:ConfigurationFileDirectories=./configs -Djava.awt.headless=true AWTFixExample

With -Djava.awt.headless=false to the gcc linker we pass libawt.a and libawt_headless.a
With -Djava.awt.headless=true to the gcc linker we pass libawt.a and libawt-xawt.a
If the generated image demands libmlib_image.a it will be also linked.

-Djava.awt.headless is a runtime property not a build time property. So you are doing domething very weird.
How does that image with "false" run a UI app ? It can't. And yet the one with "true" still can run without an xserver, except it will not start unless you have X11 libs installed - and I am reasonably sure you aren't statically linking all the system provided libraries ... I hope ...

So how does a "user" use all of this ? Seems they must create their own app by native compile + linking.
Can they create an image that can run any java app, or must the app like AWTFixExample be linked in to the image ?

@arodionov
Copy link
Author

arodionov commented Dec 9, 2020

At the current stage, we made the -Djava.awt.headless option be used during the build time, at the same time it does no influence at the runtime. The user should know the environment where it is planned to run a native image in advance. It is his responsibility to pass correct options to the native image compiler.

For example, for the code snippet AWTFixExample (mentioned above #562 (comment)), that is headless, the following set of static libraries are used for image build:

# Static libraries:
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/svm/clibraries/linux-amd64/liblibchelper.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/static/linux-amd64/glibc/liblcms.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/static/linux-amd64/glibc/libawt_headless.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/static/linux-amd64/glibc/libawt.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/static/linux-amd64/glibc/libmlib_image.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/static/linux-amd64/glibc/libnet.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/static/linux-amd64/glibc/libnio.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/static/linux-amd64/glibc/libjava.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/static/linux-amd64/glibc/libfdlibm.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/static/linux-amd64/glibc/libzip.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/svm/clibraries/linux-amd64/libjvm.a
# Other libraries: stdc++,m,pthread,dl,z,rt

For simple AWT UI Java app, the set of libraries are the following:

# Static libraries:
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/static/linux-amd64/glibc/libnet.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/static/linux-amd64/glibc/libjavajpeg.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/static/linux-amd64/glibc/libnio.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/svm/clibraries/linux-amd64/liblibchelper.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/static/linux-amd64/glibc/libjava.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/static/linux-amd64/glibc/liblcms.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/static/linux-amd64/glibc/libfontmanager.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/static/linux-amd64/glibc/libawt_xawt.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/static/linux-amd64/glibc/libawt.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/static/linux-amd64/glibc/libfdlibm.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/static/linux-amd64/glibc/libzip.a
#   ../../vms/graalvm-ce-java11-21.0.0-dev/lib/svm/clibraries/linux-amd64/libjvm.a
# Other libraries: X11,Xrender,Xext,Xi,stdc++,m,freetype,pthread,dl,z,rt

So how does a "user" use all of this ? Seems they must create their own app by native compile + linking.
Can they create an image that can run any java app, or must the app like AWTFixExample be linked in to the image ?

The global idea is that any existing Java app (even without source code) can be converted into a native image, that's why we have added AWT/Swing support to GraalVM.
To make a native image from the existing Java app, the user needs to have GraalVM, gcc with the set of libraries (platform dependent).
After running native-image command (from GraalVM), with a set of options and additional configuration files, and specifying the target jar or class-file, an executable binary for the current platform will be built.
There are limitations and the list of unsupported features (https://www.graalvm.org/reference-manual/native-image/Limitations/) for Java apps, so it may demand re-writing some code or providing so-called substitutions to make it compatible for the native-image compiler.

@arodionov
Copy link
Author

arodionov commented Dec 14, 2020

Copy link
Member

@magicus magicus left a comment

This makes sense from a build PoV, but you'll need approval from the client team as well.

@openjdk
Copy link

openjdk bot commented Dec 15, 2020

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

8254024: Enhance native libs for AWT and Swing to work with GraalVM Native Image

Reviewed-by: serb, ihse, bobv

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

  • c37eabe: 8252148: vmError::controlled_crash should be #ifdef ASSERT and move tests to gtest
  • 2273f95: 8234930: Use MAP_JIT when allocating pages for code cache on macOS
  • da2415f: 8257457: Update --release 16 symbol information for JDK 16 build 28
  • 36e2097: 8255917: runtime/cds/SharedBaseAddress.java failed "assert(reserved_rgn != 0LL) failed: No reserved region"
  • d53ee62: 8255899: Allow uninstallation of jpackage exe bundles
  • 65756ab: 8257802: LogCompilation throws couldn't find bytecode on JDK 8 log
  • a372be4: 8258244: Shenandoah: Not expecting forwarded object in roots during mark after JDK-8240868
  • 568dc29: 8185734: [Windows] Structured Exception Catcher missing around gtest execution
  • 3ab1dfe: 8257828: SafeFetch may crash if invoked in non-JavaThreads
  • 381021a: Merge
  • ... and 260 more: https://git.openjdk.java.net/jdk/compare/e3abe51a31a4dc544f26c9423c67d6f7b916e8a9...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mrserb, @magicus, @bobvandette) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Dec 15, 2020
@prrace
Copy link
Contributor

prrace commented Dec 15, 2020

@prrace It may be also useful to look at how we define a set of static libraries:
https://github.com/oracle/graal/blob/9390324021ab7b6ed391f2c8f62be490a7ab13db/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/jdk/JNIRegistrationAwt.java#L104

If by "help" you mean scare the life out of me ..
I am sure I don't understand all of it but enough to know I've never seen so many references to JDK internals and behaviours
by some code that is not part of the JDK itself.
It seems very fragile to the point of not being supportable.

If someone else wants to approve this change they can, but my conscience isn't letting me ..

Copy link
Contributor

@bobvandette bobvandette left a comment

I'm ok with these changes. They are no worse than the other STATIC_BUILD changes that already exist in the JDK sources.

@arodionov
Copy link
Author

arodionov commented Dec 15, 2020

/integrate

@openjdk openjdk bot added the sponsor label Dec 15, 2020
@openjdk
Copy link

openjdk bot commented Dec 15, 2020

@arodionov
Your change (at version 47252ec) is now ready to be sponsored by a Committer.

@bobvandette
Copy link
Contributor

bobvandette commented Dec 15, 2020

/sponsor

@openjdk openjdk bot closed this Dec 15, 2020
@openjdk openjdk bot added integrated and removed sponsor ready rfr labels Dec 15, 2020
@openjdk
Copy link

openjdk bot commented Dec 15, 2020

@bobvandette @arodionov Since your change was applied there have been 270 commits pushed to the master branch:

  • c37eabe: 8252148: vmError::controlled_crash should be #ifdef ASSERT and move tests to gtest
  • 2273f95: 8234930: Use MAP_JIT when allocating pages for code cache on macOS
  • da2415f: 8257457: Update --release 16 symbol information for JDK 16 build 28
  • 36e2097: 8255917: runtime/cds/SharedBaseAddress.java failed "assert(reserved_rgn != 0LL) failed: No reserved region"
  • d53ee62: 8255899: Allow uninstallation of jpackage exe bundles
  • 65756ab: 8257802: LogCompilation throws couldn't find bytecode on JDK 8 log
  • a372be4: 8258244: Shenandoah: Not expecting forwarded object in roots during mark after JDK-8240868
  • 568dc29: 8185734: [Windows] Structured Exception Catcher missing around gtest execution
  • 3ab1dfe: 8257828: SafeFetch may crash if invoked in non-JavaThreads
  • 381021a: Merge
  • ... and 260 more: https://git.openjdk.java.net/jdk/compare/e3abe51a31a4dc544f26c9423c67d6f7b916e8a9...master

Your commit was automatically rebased without conflicts.

Pushed as commit 7977e38.

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

@vjovanov
Copy link
Contributor

vjovanov commented Dec 16, 2020

@prrace I agree with you that our intrusion is a large maintenance overhead and that it can lead to potential errors. It is worth mentioning two things that could make you feel less scared:

  1. Much of this is code temporary: the code is here until we find a proper way to integrate this information into the JDK itself, without incurring a noticeable maintenance overhead for the JDK maintainers.
  2. We test all our code intensively with all the tests provided by the JDK assuring that on every release we pass the same tests that the JDK is passing.

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