8333268: Fixes for static build

[magicus]

This patch contains a set of changes to improve static builds. They will pave the way for implementing a full static-only java launcher. The changes here will:

1. Make sure non-exported symbols are made local in the static libraries. This means that the risk of symbol conflict is the same for static libraries as for dynamic libraries (i.e. in practice zero, as long as a consistent naming scheme is used for exported functions).

2. Remove the work-arounds to exclude duplicated symbols.

3. Fix some code in hotspot and the JDK libraries that did not work properly with a static java launcher.

The latter fixes are copied from or inspired by the work done by @jianglizhou and her team as part of the Project Leyden Hermetic Java.

---

Progress

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

Issue

• JDK-8333268: Fixes for static build (Bug - P3)

Reviewers

• Erik Joelsson (@erikj79 - Reviewer) 🔄 Re-review required (review applies to b88d813e)

Contributors

• Jiangli Zhou <[email protected]>
• Magnus Ihse Bursie <[email protected]>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19478

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back ihse! 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]

@magicus This change is no longer ready for integration - check the PR body for details.

[openjdk]

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

• build
• client
• core-libs
• hotspot

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.

[magicus]

Some open questions:

• Do os::lookup_function need to be implemented on Windows too, for symmetry, even if it is only used on Unix platforms?

• Many of the changes in Hotspot boils down to os::dll_load doing the wrong thing when running with a static build. Perhaps we should provide a better function that knows how to find and load a symbol for both static and dynamic builds, and use that instead of making a lot of tests for static/dynamic on each location we need to look up a symbol from some other JDK library.

• I managed to replace most of the #ifdef STATIC_BUILD with runtime checks. There are some places remaining though. Apart from the #ifdefs needed for JNI/JVMTI, which will need spec changes to address, there are code in java_md_macosx.m, jio.c and awt_Mlib.c that I did not manage to turn into runtime checks. They will need some more thorough work than just changing an #ifdef to an if () {.

• And of course, the code in the build system to share all .o files except the two linktype files is still under development...

[magicus]

I moved this away from Draft state, since I think it needs some visibility, especially since it touches several different parts of the code base, and such reviews tend to take time.

I think the code here is good and basically okay to integrate. This patch will not on it's own solve the entire problem of building a proper static launcher, but it takes several important steps along the way. I think the changes here are reasonable to integrate into mainline at this point.

[mlbridge]

Webrevs

• 03: Full - Incremental (b88d813e)
• 02: Full - Incremental (4ab70df3)
• 01: Full - Incremental (e1c46562)
• 00: Full (6b24a789)

[magicus]

The GHA tests fails when building gtest on Linux. This will require some investigation.

[magicus]

Do os::lookup_function need to be implemented on Windows too, for symmetry, even if it is only used on Unix platforms?

@AlanBateman suggested to add lookup_function to os_windows.cpp as well, but just let it contain ShouldNotReachHere. That sounds like a good solution to me.

[magicus]

@jianglizhou This code is based on changes in the Hermetic Java repo, but I do not fully understand neither the comment nor what the purpose is. If you could explain this a bit I'd be grateful.

[jianglizhou]

The specific related commit in the hermetic Java branch is openjdk/leyden@53aa8f0.

The change in os_linux.cpp here is to make sure that the libjvm.so related path manipulation is conditionally done only. The check at line 599 looks for "/libjvm.so" substring, so we only chop off (*pslash = \0at line 601) that part when necessary. In the static JDK case, there is nolibjvm.soand the path string is<jdk_path>/bin/javastatic`, which should not be affected. Otherwise, it could fail.

I found the code was not very easy to follow when running into problems and fixing for static support. So I added a bit more comments in the code here. The comment above about /{client|server|hotspot} was there originally. I think we no longer have those directories. We can cleanup that later, since it needs some more testing.

@magicus, thanks a lot for extracting/reworking/cleaning-up the static Java changes from the hermetic Java branch. That's a substantial amount of work!

I have one quick comment about the removal of STATIC_LIB_EXCLUDE_OBJS changes. Will post it as separate comment for the related code.

I'll also look closely of the vm & jdk changes and compare those with the changes in the hermetic Java branch this week.

[magicus]

The reason the gtest failed was that we build a static library libgtest.a, which is linked with the gtest libjvm.so. With the changes in this PR, libgtest.a was being built using the ld -r + objcopy --localize-hidden method. This caused some weird issues with gcc, related to C++ code and the COMDAT object info.

I failed to track down any proper solution, so instead I added a patch where the libraries that we explicitly declare as STATIC_LIBRARIES are linked as before, without the partial linking step. These libraries are only intended for internal consumption (that is, they are linked to and used by another, "external" library), and so the extra protection added by the partial linking is not really needed.

It's a bit sad that this did not work, but it is no big deal. It won't affect files released in the image, and it will not be a regression as compared to now.

[magicus]

/contributor add jiangli
/contributor add ihse

[openjdk]

@magicus
Contributor Jiangli Zhou <[email protected]> successfully added.

[openjdk]

@magicus
Contributor Magnus Ihse Bursie <[email protected]> successfully added.

[erikj79]

Build changes look ok.

/reviewers 2

[jianglizhou]

I've looked through all JDK and VM changes and left comments in various places. All the rest changes in PR look good. Thanks again for extracting these changes from the leyden/hermeticJava branch and integrating with mainline!

My other main question is why the javastatic linking work is not included in the PR together with these runtime changes.

IIUC from our meetings and mailing list discussions, the initial integration PR needs to include the part for statically linking the javastatic. That's a minimum requirement for testing/verifying the runtime changes when integrating into the mainline, which is also the reason why we haven't starting integrating any of the runtime changes so far. Has that been changed?

[bridgekeeper]

@magicus This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[openjdk]

@magicus this pull request can not be integrated into master 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 static-linking-progress
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[magicus]

I've reverted this PR back to Draft state. This was how I originally intended for this to be viewed at this stage, but I got struck by pre-vacation hubris and thought that maybe it was after all ready for integration, so I could get it done with before I was leaving for the summer. This was clearly not the case.

I'll look through all comments to this PR carefully and determine how to go forward with this. It might be that this PR still tries to do too many things at once (even though I have removed all the hermetic-java specific changes), so that it needs to be split into several different PRs.

[magicus]

IIUC from our meetings and mailing list discussions, the initial integration PR needs to include the part for statically linking the javastatic. That's a minimum requirement for testing/verifying the runtime changes when integrating into the mainline, which is also the reason why we haven't starting integrating any of the runtime changes so far. Has that been changed?

No, that has not been changed, and it was the "get this done before vacation" stress that got the better of me, and I forgot or ignored (I don't recall which anymore) that this is an absolute requirement for this code to be properly tested.

I'll get back to fixing the proper static launcher build, and get that properly working, before trying to push any other changes into mainline.

[magicus]

@AlanBateman

Having libjdwp link to libjvm was a surprise but I think okay.

This is since it needs to access JVM_IsStaticallyLinked. The alternative is to create yet another "is static" file that is compiled twice, just for libjdwp, but I think already having two is one too many (even if I realize that it is neccessary), so I'd like to avoid that.

As a rule of thumb, most JDK libraries link with either libjvm or libjava or both, so I don't think this needs to be of any concern.

[magicus]

I have broken out the parts that is just about replacing #ifdefs with a runtime function call to determine static/dynamic loading, into #20666. That should hopefully be trivial to integrate since it is a pure refactoring, but it will reduce the size and complexity of this PR.

[bridgekeeper]

@magicus This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@magicus This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@magicus This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@magicus This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[magicus]

This is superseded by #20837.

This PR did contain changes that would hide local symbols on more build targets/toolchains than linux/clang; these should be resurrected in a future follow-up on JDK-8339480.