8254702: jpackage app launcher crashes on CentOS #2320

alexeysemenyukoracle · 2021-01-29T21:33:40Z

Fix for https://bugs.openjdk.java.net/browse/JDK-8254702

The fix splits Linux app launcher in app launcher and launcher shared lib. App launcher is pure C and doesn't have C++ code. App launcher lib incorporates bulk of C++ code from app launcher.
At startup app launcher loads launcher shared lib and calls functions it provides to get data for launching JVM (path to jli lib and arguments for JLI_Launch function call).
App launcher unloads launcher shared lib before launching JVM to remove C++ runtime from the process memory.

Getting rid of C++ code from app launcher required to rewrite app installation location lookup code from C++ to C. LinuxPackage.c source is C alternative for https://github.com/openjdk/jdk/blob/master/src/jdk.jpackage/linux/native/applauncher/Package.cpp and https://github.com/openjdk/jdk/blob/master/src/jdk.jpackage/linux/native/applauncher/Executor.cpp.

Layout of jpackage's native code changed:

common: code shared between all jpackage binaries.
applauncher: launcher only code.
applauncherlib: launcher lib code on Linux and launcher code on other platforms.
applaunchercommon: code shared between launcher and launcher lib on Linux and launcher code on other platforms.

Progress

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

Issue

JDK-8254702: jpackage app launcher crashes on CentOS

Reviewers

Alexander Matveev (@sashamatveev - Committer) ⚠️ Review applies to b493bcf
Erik Joelsson (@erikj79 - Reviewer)
Andy Herrick (@andyherrick - Reviewer)

Download

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

bridgekeeper · 2021-01-29T21:34:01Z

👋 Welcome back asemenyuk! 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 · 2021-01-29T21:34:51Z

@alexeysemenyukoracle The following label will be automatically applied to this pull request:

build

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

alexeysemenyukoracle · 2021-01-29T21:40:42Z

make/modules/jdk.jpackage/applauncherlib.ld-version-script

@@ -0,0 +1,9 @@
+{


Linker script is "just in case" to guarantee that only controlled set of functions is exported from launcher lib. Current set of g++ OpenJDK command line options disables export of functions with external linkage by default. However in my local builds this was not the case and the whole C++ runtime statically linked in launcher lib got exported. This prevented it from unloading from launcher's process memory when it was dlclose()-ed. This linker script used in linkage of launcher lib will guarantee this will not happen.

alexeysemenyukoracle · 2021-01-29T21:44:16Z

src/jdk.jpackage/share/native/common/tstrings.cpp

@@ -52,13 +52,13 @@ tstring unsafe_format(tstring::const_pointer format, ...) {
 #ifdef _MSC_VER
        ret = _vsntprintf_s(&*fmtout.begin(), fmtout.size(), _TRUNCATE, format, args);
 #else
-#ifdef __GNUC__


Local build with older g++compiler bugfix.

mlbridge · 2021-01-29T21:51:16Z

Webrevs

05: Full - Incremental (3e73e07)
04: Full - Incremental (b241357)
03: Full - Incremental (b6c62a2)
02: Full - Incremental (9174425)
01: Full - Incremental (b493bcf)
00: Full (14d277a27b2e31a140e678659d4951a3168c790a)

prrace · 2021-01-29T21:57:17Z

/label add core-libs

openjdk · 2021-01-29T21:57:44Z

@prrace
The core-libs label was successfully added.

prrace · 2021-01-29T22:00:32Z

So after this change if you bundle and run an app on Linux and then do "ps" .. what is shown to be running ? Java or the app-name you expected ?

alexeysemenyukoracle · 2021-01-29T22:17:33Z

"ps" will show app-name. It was never Java for jpackage apps before this patch and this patch doesn't change it.

magicus

This whole change seems very messy to me. :-(

I'm having a hard time even untangling the PR to understand what's going on. Are you creating two new directories, "applauncherlib" and "applauncherlibcommon"? First of all, for shared libraries, the norm is to have a "lib-" prefix, not a "-lib" suffix. Secondly, there is already a "common" directory, is that not enough?

src/jdk.jpackage/share/native/common/app.cpp

make/modules/jdk.jpackage/Lib.gmk

magicus · 2021-02-01T12:18:15Z

make/modules/jdk.jpackage/Lib.gmk

 ))

-$(BUILD_JPACKAGE_APPLAUNCHEREXE): $(call FindLib, java.base, java)


Why did you remove this dependency?

I moved it to the bottom of the file making all artifacts produced by make/modules/jdk.jpackage/Lib.gmk depend on java.bas and java. There is $(JPACKAGE_TARGETS): $(call FindLib, java.base, java) at the bottom of the file.

make/modules/jdk.jpackage/Lib.gmk

alexeysemenyukoracle · 2021-02-01T16:17:35Z

"common" was perfectly enough until this change. Unfortunately we cant just drop new C sources in "common" dir because we don't want them to be compiled with g++. That is why need new common directory (applauncherlibcommon) for C sources.

I'll rename applauncherlibcommon in libapplaunchercommon and applauncherlib in libapplauncher in the next commit.

erikj79 · 2021-02-01T18:24:23Z

"common" was perfectly enough until this change. Unfortunately we cant just drop new C sources in "common" dir because we don't want them to be compiled with g++. That is why need new common directory (applauncherlibcommon) for C sources.

We pick compiler based on file suffix, not directory, so it shouldn't matter where you put a .c file, it should always be compiled with gcc and .cpp files with g++. Which compiler is used to launch the linker can however differ. That's configured for each SetupNativeCompilation call with the different TOOLCHAIN settings.

alexeysemenyukoracle · 2021-02-01T18:35:54Z

Erik, thank you for explanation.

The launcher on Linux should not be linked with c++ runtime, that is why TOOLCHAIN_DEFAULT is used as a value for TOOLCHAIN property in BUILD_JPACKAGE_APPLAUNCHEREXE target on Linux.

Will SetupNativeCompilation work if TOOLCHAIN=TOOLCHAIN_DEFAULT and there are .cpp sources are in directories passed in SRC property of SetupNativeCompilation? Will it try to compile these sources? If it will ignore them and pick only .c files, that would be perfect.

mlbridge · 2021-02-02T00:01:48Z

Mailing list message from erik.joelsson at oracle.com on build-dev:

On 2021-02-01 10:38, Alexey Semenyuk wrote:

On Mon, 1 Feb 2021 18:24:23 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

"common" was perfectly enough until this change. Unfortunately we cant just drop new C sources in "common" dir because we don't want them to be compiled with g++. That is why need new common directory (applauncherlibcommon) for C sources.

I'll rename applauncherlibcommon in libapplaunchercommon and applauncherlib in libapplauncher in the next commit.
"common" was perfectly enough until this change. Unfortunately we cant just drop new C sources in "common" dir because we don't want them to be compiled with g++. That is why need new common directory (applauncherlibcommon) for C sources.
We pick compiler based on file suffix, not directory, so it shouldn't matter where you put a .c file, it should always be compiled with gcc and .cpp files with g++. Which compiler is used to launch the linker can however differ. That's configured for each SetupNativeCompilation call with the different TOOLCHAIN settings.
Erik, thank you for explanation.

The launcher on Linux should not be linked with c++ runtime, that is why TOOLCHAIN_DEFAULT is used at a value for TOOLCHAIN property in BUILD_JPACKAGE_APPLAUNCHEREXE target on Linux.

Will SetupNativeCompilation work if `TOOLCHAIN=TOOLCHAIN_DEFAULT` and there are .cpp sources are in directories passed in `SRC` property of SetupNativeCompilation? Will it try to compile these sources? If it will ignore them and pick only .c files, that would be perfect.

SetupNativeCompilation will by default include all src files found in
any directory given to SRC, recursively. You can use EXCLUDES,
EXCLUDE_FILES and EXCLUDE_PATTERN to exclude files or directories from
SRC. You can also use EXTRA_FILES to pick specific files outside of any
directory in SRC. Sorting files in separate directories or using
EXCLUDE*/EXTRA_FILES are both possible and picking the right solution is
mostly down to taste.

/Erik

alexeysemenyukoracle · 2021-02-05T17:44:34Z

Reworked the fix to avoid creation of extra source directories and file renames.

erikj79

I like this much better, thanks for taking the time! Build changes look ok.

openjdk · 2021-02-05T17:53:45Z

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

8254702: jpackage app launcher crashes on CentOS

Reviewed-by: almatvee, erikj, herrick

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

0218917: 8258732: runtime/cds/appcds/dynamicArchive/DynamicArchiveRelocationTest.java fails
f9df366: 8242300: SystemDictionary::resolve_super_or_fail() should look for the super class first
43ae0cf: 8261167: print_process_memory_info add a close call after fopen
48f5220: 8260369: [PPC64] Add support for JDK-8200555
224c166: 8261213: [BACKOUT] MutableSpace's end should be atomic
3495feb: 8260296: SA's dumpreplaydata fails
0791899: 8261154: Memory leak in Java_java_lang_ClassLoader_defineClass0 with long class names
78b0d32: 8234534: Simplify CardTable code after CMS removal
ee2f205: 8260926: Trace resource exhausted events unconditionally
1e0a101: 8259862: MutableSpace's end should be atomic
... and 114 more: https://git.openjdk.java.net/jdk/compare/20e7df506f06bb34de3d473baa180de1d22ba3d0...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.

andyherrick

looks ok if build team approves

alexeysemenyukoracle · 2021-02-05T20:44:05Z

/integrate

openjdk · 2021-02-05T20:45:00Z

@alexeysemenyukoracle Since your change was applied there have been 127 commits pushed to the master branch:

7a6c176: 8260736: Shenandoah: Cleanup includes in ShenandoahGC and families
4a89733: 8261198: [macOS] Incorrect JNI parameters in number conversion in A11Y code
4a1814c: 8261179: Norwegian Bokmål Locale fallback issue
0218917: 8258732: runtime/cds/appcds/dynamicArchive/DynamicArchiveRelocationTest.java fails
f9df366: 8242300: SystemDictionary::resolve_super_or_fail() should look for the super class first
43ae0cf: 8261167: print_process_memory_info add a close call after fopen
48f5220: 8260369: [PPC64] Add support for JDK-8200555
224c166: 8261213: [BACKOUT] MutableSpace's end should be atomic
3495feb: 8260296: SA's dumpreplaydata fails
0791899: 8261154: Memory leak in Java_java_lang_ClassLoader_defineClass0 with long class names
... and 117 more: https://git.openjdk.java.net/jdk/compare/20e7df506f06bb34de3d473baa180de1d22ba3d0...master

Your commit was automatically rebased without conflicts.

Pushed as commit fac3c2d.

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

openjdk bot added the build build-dev@openjdk.org label Jan 29, 2021

alexeysemenyukoracle commented Jan 29, 2021

View reviewed changes

alexeysemenyukoracle marked this pull request as ready for review January 29, 2021 21:45

openjdk bot added the rfr Pull request is ready for review label Jan 29, 2021

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jan 29, 2021

8254702: jpackage app launcher crashes on CentOS

b493bcf

alexeysemenyukoracle force-pushed the JDK-8254702 branch from 14d277a to b493bcf Compare January 29, 2021 22:58

sashamatveev approved these changes Jan 30, 2021

View reviewed changes

magicus reviewed Feb 1, 2021

View reviewed changes

src/jdk.jpackage/share/native/common/app.cpp Show resolved Hide resolved

magicus reviewed Feb 1, 2021

View reviewed changes

make/modules/jdk.jpackage/Lib.gmk Show resolved Hide resolved

magicus reviewed Feb 1, 2021

View reviewed changes

magicus suggested changes Feb 1, 2021

View reviewed changes

make/modules/jdk.jpackage/Lib.gmk Outdated Show resolved Hide resolved

8254702: jpackage app launcher crashes on CentOS

9174425

alexeysemenyukoracle added 4 commits February 5, 2021 08:17

8254702: jpackage app launcher crashes on CentOS

d098cd4

8254702: jpackage app launcher crashes on CentOS

b6c62a2

8254702: jpackage app launcher crashes on CentOS

b241357

8254702: jpackage app launcher crashes on CentOS

3e73e07

erikj79 approved these changes Feb 5, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 5, 2021

andyherrick approved these changes Feb 5, 2021

View reviewed changes

openjdk bot closed this Feb 5, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 5, 2021

alexeysemenyukoracle deleted the JDK-8254702 branch January 18, 2022 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8254702: jpackage app launcher crashes on CentOS #2320

8254702: jpackage app launcher crashes on CentOS #2320

alexeysemenyukoracle commented Jan 29, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 29, 2021

openjdk bot commented Jan 29, 2021

alexeysemenyukoracle Jan 29, 2021

alexeysemenyukoracle Jan 29, 2021

mlbridge bot commented Jan 29, 2021 •

edited

Loading

prrace commented Jan 29, 2021

openjdk bot commented Jan 29, 2021

prrace commented Jan 29, 2021

alexeysemenyukoracle commented Jan 29, 2021

magicus left a comment

magicus Feb 1, 2021

alexeysemenyukoracle Feb 1, 2021

alexeysemenyukoracle commented Feb 1, 2021

erikj79 commented Feb 1, 2021

alexeysemenyukoracle commented Feb 1, 2021 •

edited

Loading

mlbridge bot commented Feb 2, 2021

alexeysemenyukoracle commented Feb 5, 2021

erikj79 left a comment

openjdk bot commented Feb 5, 2021 •

edited

Loading

andyherrick left a comment

alexeysemenyukoracle commented Feb 5, 2021

openjdk bot commented Feb 5, 2021

		))

		$(BUILD_JPACKAGE_APPLAUNCHEREXE): $(call FindLib, java.base, java)

8254702: jpackage app launcher crashes on CentOS #2320

8254702: jpackage app launcher crashes on CentOS #2320

Conversation

alexeysemenyukoracle commented Jan 29, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Jan 29, 2021

openjdk bot commented Jan 29, 2021

alexeysemenyukoracle Jan 29, 2021

Choose a reason for hiding this comment

alexeysemenyukoracle Jan 29, 2021

Choose a reason for hiding this comment

mlbridge bot commented Jan 29, 2021 • edited Loading

Webrevs

prrace commented Jan 29, 2021

openjdk bot commented Jan 29, 2021

prrace commented Jan 29, 2021

alexeysemenyukoracle commented Jan 29, 2021

magicus left a comment

Choose a reason for hiding this comment

magicus Feb 1, 2021

Choose a reason for hiding this comment

alexeysemenyukoracle Feb 1, 2021

Choose a reason for hiding this comment

alexeysemenyukoracle commented Feb 1, 2021

erikj79 commented Feb 1, 2021

alexeysemenyukoracle commented Feb 1, 2021 • edited Loading

mlbridge bot commented Feb 2, 2021

alexeysemenyukoracle commented Feb 5, 2021

erikj79 left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 5, 2021 • edited Loading

andyherrick left a comment

Choose a reason for hiding this comment

alexeysemenyukoracle commented Feb 5, 2021

openjdk bot commented Feb 5, 2021

alexeysemenyukoracle commented Jan 29, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 29, 2021 •

edited

Loading

alexeysemenyukoracle commented Feb 1, 2021 •

edited

Loading

openjdk bot commented Feb 5, 2021 •

edited

Loading