8289002: Minimal x86_64 VM build fails with GCC 11: 'this' pointer is null

[shipilev]

These build failures are specific to Minimal VM, because it disables "management" feature, and paths like these start to return NULL:

  static InstanceKlass* com_sun_management_internal_DiagnosticCommandImpl_klass(TRAPS)
      NOT_MANAGEMENT_RETURN_(NULL);

I propose we handle the NULL-s properly and throwing early when those are detected. We return/throw what similar code in the affected methods do. I went back and forth with the patch, and settled on throwing from MemoryManager::get_memory_manager_instance, and handling the exception at its uses.

Additional testing:

• Linux x86_64 fastdebug minimal build with GCC 11.3.0
• Linux x86_64 fastdebug server with GCC 9.3.0, tier1
• Linux x86_64 fastdebug server with GCC 9.3.0, tier2
• Linux x86_64 fastdebug server with GCC 9.3.0, hotspot_serviceability

---

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-8289002: Minimal x86_64 VM build fails with GCC 11: 'this' pointer is null

Reviewers

• David Holmes (@dholmes-ora - Reviewer)
• Andrew Haley (@theRealAph - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9718

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

Using diff file

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

[bridgekeeper]

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

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

• hotspot

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.

[mlbridge]

Webrevs

• 01: Full - Incremental (5e0ed005)
• 00: Full (aceebea1)

[dholmes-ora]

I can't help but feel that without INCLUDE_MANAGEMENT we should not be getting to the code concerned, so something seems amiss to me. But I don't have cycles at the moment to try and trace through this.

[dholmes-ora]

Though it is also possible code changes have crept in that have not taken INCLUDE_MANAGEMENT into account.

[shipilev]

Though it is also possible code changes have crept in that have not taken INCLUDE_MANAGEMENT into account.

My initial patch was going in that direction: protecting some paths with INCLUDE_MANAGEMENT, but it quickly gets to look intrusive, questionable and ugly for me. Returning/throwing on those paths is much cleaner.

[dholmes-ora]

But the higher-level API's must be aware that management support may not exist and they should then be specified to behave a certain way when that is true. That specified behaviour may not be throwing your new exceptions. If the APIs do not account for the management interface not being available, then that is a significant bug that needs fixing.

That said I have to refresh my memory as to whether the Minimal VM is only supported by certain compact profiles (which would exclude the high-level API's we are discussing). It may simply be an invalid configuration to have a Minimal VM but a full set of Java API's.

[AlanBateman]

The minimal VM build dates from the compact profiles defined in Java 8. The compact1 and compact2 profiles didn't have the j.l.management APIs so a minimal VM build of libjvm without the "management" code would have been okay. For Java 9+ then a minimal VM build without "management" only make sense when using jlink to create a run-time image that doesn't have the java.management module.

[dholmes-ora]

Thanks Alan (@AlanBateman )! That is what I was reaching for. I guess we lost the actual constraint when we moved to 9 that the Minimal VM cannot be used when the management module is present, as we presumably have no specification for how that module behaves in that case.

Now that doesn't help in the current situation where GCC complains about a NULL this ptr as it cannot know about the fact this code should be unreachable in practice. I'm tempted to say we should use guarantees in the VM code and abort if we encounter the NULL - would that satisfy GCC? It seems better to fail fast and completely with such a misconfigured runtime environment.

[shipilev]

Now that doesn't help in the current situation where GCC complains about a NULL this ptr as it cannot know about the fact this code should be unreachable in practice. I'm tempted to say we should use guarantees in the VM code and abort if we encounter the NULL - would that satisfy GCC?

I tried to put guarantee()-s or conditional fatal() on those paths, and this still does not please GCC. I think it is the recurrent problem with those methods not marked as noreturn . So it works if we put the return-s as well. Like this:

diff --git a/src/hotspot/share/services/diagnosticFramework.cpp b/src/hotspot/share/services/diagnosticFramework.cpp
index 31ef9d0061b..944e161c321 100644
--- a/src/hotspot/share/services/diagnosticFramework.cpp
+++ b/src/hotspot/share/services/diagnosticFramework.cpp
@@ -462,6 +462,10 @@ void DCmdFactory::send_notification_internal(TRAPS) {
   if (notif) {
 
     Klass* k = Management::com_sun_management_internal_DiagnosticCommandImpl_klass(CHECK);
+    if (k == nullptr) {
+      fatal("Should have the class");
+      return;
+    }
     InstanceKlass* dcmd_mbean_klass = InstanceKlass::cast(k);
 
     JavaValue result(T_OBJECT);
diff --git a/src/hotspot/share/services/memoryManager.cpp b/src/hotspot/share/services/memoryManager.cpp
index 38e308a8a15..d9527810e31 100644
--- a/src/hotspot/share/services/memoryManager.cpp
+++ b/src/hotspot/share/services/memoryManager.cpp
@@ -100,6 +100,11 @@ instanceOop MemoryManager::get_memory_manager_instance(TRAPS) {
       signature = vmSymbols::createMemoryManager_signature();
     }
 
+    if (k == nullptr) {
+      fatal("Should have the class");
+      return nullptr;
+    }
+
     InstanceKlass* ik = InstanceKlass::cast(k);
 
     JavaCalls::call_static(&result,

That would be okay with you, @dholmes-ora?

[AlanBateman]

Thanks Alan (@AlanBateman )! That is what I was reaching for. I guess we lost the actual constraint when we moved to 9 that the Minimal VM cannot be used when the management module is present, as we presumably have no specification for how that module behaves in that case.

There wasn't any real constraint in JDK 8 either, it was just that the minimal VM build was for compact1 and compact2. It's a topic that I discussed a few times with Bob Vandette but it never rose to the level of something that the module system should support. A jlink plugin (or an update to ExcludeVMPlugin) could detect attempts to create a run-time image that contains both the minimal VM and the java.management module but may not be worth it.

In any case, a run-time image with the minimal VM will fail with exceptions like this:

Exception in thread "main" java.lang.InternalError: Unsupported Management version
	at java.base/jdk.internal.loader.NativeLibraries.load(Native Method)
	at java.base/jdk.internal.loader.NativeLibraries$NativeLibraryImpl.open(NativeLibraries.java:331)
	at java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:197)
	at java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:139)
	at java.base/jdk.internal.loader.NativeLibraries.findFromPaths(NativeLibraries.java:259)
	at java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:249)
	at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:2413)
	at java.base/java.lang.Runtime.loadLibrary0(Runtime.java:849)
	at java.base/java.lang.System.loadLibrary(System.java:2045)
	at java.management/java.lang.management.ManagementFactory.lambda$loadNativeLib$8(ManagementFactory.java:1025)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:319)
	at java.management/java.lang.management.ManagementFactory.loadNativeLib(ManagementFactory.java:1024)
	at java.management/java.lang.management.ManagementFactory.<clinit>(ManagementFactory.java:1019)

[dholmes-ora]

I'm okay with this approach. Thanks.

[openjdk]

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

8289002: Minimal x86_64 VM build fails with GCC 11: 'this' pointer is null

Reviewed-by: dholmes, aph

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

• 83dc2e3: 8292062: misc javax/swing tests failing
• 4913380: 8292007: Do not include vmSymbol.hpp in method.hpp
• 6397d56: 8151430: (fs) BasicFileAttributeView.setTimes should support setting file create time on OS X
• 57e0da1: 8292132: ProblemList jdk/internal/misc/TerminatingThreadLocal/TestTerminatingThreadLocal.java
• 17c77b5: 8292119: ProblemList java/awt/image/multiresolution/MultiresolutionIconTest.java on linux-x64 and windows-all
• d889264: 8290836: Improve test coverage for XPath functions: String Functions
• ae1a9a0: 8292060: Make ClassFileVersionTest.java version adapting
• 4040927: 8290047: (fs) FileSystem.getPathMatcher does not check for ":" at last index
• 8d0d9ea: 8291238: JDK can't be built without G1
• aff7689: 8292075: Remove deleted test compiler/rangechecks/TestRangeCheckHoistingScaledIV.java from ProblemList
• ... and 82 more: https://git.openjdk.org/jdk/compare/a2cff2634cdaa0fa7ba2a1acd951d6f521f59f6d...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.

[shipilev]

I'm okay with this approach. Thanks.

Thanks!

Any other reviews?

[shipilev]

Ping? I would like to push it soon. Thanks!

[shipilev]

Thank you!

/integrate

[openjdk]

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

• ecfa38f: 8281966: Absolute path of symlink is null in JFileChooser
• 83dc2e3: 8292062: misc javax/swing tests failing
• 4913380: 8292007: Do not include vmSymbol.hpp in method.hpp
• 6397d56: 8151430: (fs) BasicFileAttributeView.setTimes should support setting file create time on OS X
• 57e0da1: 8292132: ProblemList jdk/internal/misc/TerminatingThreadLocal/TestTerminatingThreadLocal.java
• 17c77b5: 8292119: ProblemList java/awt/image/multiresolution/MultiresolutionIconTest.java on linux-x64 and windows-all
• d889264: 8290836: Improve test coverage for XPath functions: String Functions
• ae1a9a0: 8292060: Make ClassFileVersionTest.java version adapting
• 4040927: 8290047: (fs) FileSystem.getPathMatcher does not check for ":" at last index
• 8d0d9ea: 8291238: JDK can't be built without G1
• ... and 83 more: https://git.openjdk.org/jdk/compare/a2cff2634cdaa0fa7ba2a1acd951d6f521f59f6d...master

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev Pushed as commit 37d3146.

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