8346394: Bundled freetype library needs to have JNI_OnLoad for static builds

[magicus]

All JDK native libraries needs to have a proper JNI_OnLoad, set either by DEF_STATIC_JNI_OnLoad or DEF_JNI_OnLoad. This is so that in a static build, the JVM can recognize that the library is already loaded, and does not try to load a (non-existent) external library.

Our bundled version of freetype is missing such a marker.

This turns out to be a problem just on Windows, since the System.loadLibrary("freetype") code in FontManagerNativeLibrary is only executed on Windows. However, the cost of including this in the bundled versions on all platforms is very small, and it is a safeguard for any future changes. In practice, freetype bundling is only enabled on Windows anyways.

---

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-8346394: Bundled freetype library needs to have JNI_OnLoad for static builds (Bug - P3)

Reviewers

• Erik Joelsson (@erikj79 - Reviewer)
• Phil Race (@prrace - Reviewer) Review applies to 13c93e63

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22791

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

Using diff file

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

Using 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 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:

8346394: Bundled freetype library needs to have JNI_OnLoad for static builds

Reviewed-by: erikj, prr

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

• 4f44cf6: 8341481: [perf] vframeStreamCommon constructor may be optimized
• 390b205: 8346048: test/lib/containers/docker/DockerRunOptions.java uses addJavaOpts() from ctor
• 03821d9: 8346195: Fix static initialization problem in GDIHashtable
• a5503fb: 8346432: java.lang.foreign.Linker comment typo
• fbd76ca: 8337016: serviceability/jvmti/RedefineClasses/RedefineLeakThrowable.java gets Metaspace OOM
• baeb3d9: 8346304: SA doesn't need a copy of getModifierFlags
• 99af595: 8345942: Separate source output from class output when building microbenchmarks
• 8a64595: 8346282: [JVMCI] Add failure reason support to UnresolvedJava/Type/Method/Field

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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]

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

• build
• client

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.

[mlbridge]

Webrevs

• 02: Full - Incremental (0fada047)
• 01: Full - Incremental (13c93e63)
• 00: Full (bf0ad054)

[prrace]

If I understand correctly, this is a "new" requirement because of the need to support static builds ?

Seems odd if the library doesn't actually use JNI. And what if the JDK is trying to load a library that is not part of the JDK ?

freetype bundling is only enabled on Windows anyways.

We bundle it on mac too ..

[prrace]

Do not integrate this until I have approved.

[magicus]

If I understand correctly, this is a "new" requirement because of the need to support static builds ?

Yes.

Seems odd if the library doesn't actually use JNI.

DEF_STATIC_JNI_OnLoad is kind of a misnomer. It really means "mark this library as a JDK library, which is included in the statically linked binary, so if a request comes to load a dynamic library by this name, just ignore it, since it is already loaded". (I am thinking about renaming the macro to be more clear on this, as part of the general cleanup on how static builds are being done..)

So what this means is that the code executing System.loadLibrary("freetype") will not actually try to load a freetype.dll and fail.

And what if the JDK is trying to load a library that is not part of the JDK ?

Then it will not find the special marker method, and assume it will have to load it normally, as it then is a user-supplied or 3rd party dll.

freetype bundling is only enabled on Windows anyways.

We bundle it on mac too ..

Oh, that's right. But the macOS bundling is done purely at link time, so we do not have this problem, as far as I can tell.

[prrace]

We bundle it on mac too ..

Oh, that's right. But the macOS bundling is done purely at link time, so we do not have this problem, as far as I can tell.

I see (I think)

[magicus]

/integrate

[openjdk]

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

• 414eb6b: 8338714: vmTestbase/nsk/jdb/kill/kill001/kill001.java fails with JTREG_TEST_THREAD_FACTORY=Virtual
• dc71e8c: 8342995: Enhance Attach API to support arbitrary length arguments - Linux
• 4f44cf6: 8341481: [perf] vframeStreamCommon constructor may be optimized
• 390b205: 8346048: test/lib/containers/docker/DockerRunOptions.java uses addJavaOpts() from ctor
• 03821d9: 8346195: Fix static initialization problem in GDIHashtable
• a5503fb: 8346432: java.lang.foreign.Linker comment typo
• fbd76ca: 8337016: serviceability/jvmti/RedefineClasses/RedefineLeakThrowable.java gets Metaspace OOM
• baeb3d9: 8346304: SA doesn't need a copy of getModifierFlags
• 99af595: 8345942: Separate source output from class output when building microbenchmarks
• 8a64595: 8346282: [JVMCI] Add failure reason support to UnresolvedJava/Type/Method/Field

Your commit was automatically rebased without conflicts.

[openjdk]

@magicus Pushed as commit f3e2f88.

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

[dholmes-ora]

A bit late but why do we presume that just because this library is bundled that it has to be part of the static image?

This static build stuff continues to leave me with a "bad taste in my mouth".

[magicus]

A bit late but why do we presume that just because this library is bundled that it has to be part of the static image?

No? The library can be bundled as a dynamic library as well, that has always been and still is the case.

This static build stuff continues to leave me with a "bad taste in my mouth".

I agree that the static build stuff is still not up to normal JDK standards. It was originally "tacked on" from the side, with a lot of kludges to get it to work. I am trying to work through these problematic cases and clean up the code.

This current kludge is that there is a lot of code doing System.loadLibrary(someInternalLibrary), and for static builds these calls should really be a no-op. But how do you know? The solution chosen by the original implementors of static builds was to look for a symbol JNI_OnLoad_someInternalLibrary, and if it was found, skip loading the dynamic library. It can certainly be discussed if this was a good way of doing things, but that is how it currently works. (It does have the advantage that users can add static libraries of their own in this way.)

[dholmes-ora]

The solution chosen by the original implementors of static builds was to look for a symbol JNI_OnLoad_someInternalLibrary, and if it was found, skip loading the dynamic library.

Is that necessary? Presumably if you try to dynamically load a statically linked library it will fail. Does the failure indicate the reason? If so then just ignore it.

It does have the advantage that users can add static libraries of their own in this way.

Is this intended to be a supported mechanism? That needs to be specified and documented somewhere if so.

[magicus]

The solution chosen by the original implementors of static builds was to look for a symbol JNI_OnLoad_someInternalLibrary, and if it was found, skip loading the dynamic library.

Is that necessary? Presumably if you try to dynamically load a statically linked library it will fail. Does the failure indicate the reason? If so then just ignore it.

It is perhaps not necessary, but this is the way it is implemented, and had been for years. Just as you like to be conservative and don't change things that's been working for years, so am I a bit reluctant to change this behavior. Doing so would be a massive undertaking, on a completely different scale than just fixing a few issues to get the Windows static launcher to work properly.

It does have the advantage that users can add static libraries of their own in this way.

Is this intended to be a supported mechanism? That needs to be specified and documented somewhere if so.

Yes it is. It is documented in the JNI specification: https://docs.oracle.com/en/java/javase/23/docs/specs/jni/invocation.html#jni_onload_l

[dholmes-ora]

Yes it is. It is documented in the JNI specification: https://docs.oracle.com/en/java/javase/23/docs/specs/jni/invocation.html#jni_onload_l

Thanks for the memory jolt, I had forgotten about that.