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

8262011: [JVMCI] allow printing to tty from unattached libgraal thread #2640

Closed
wants to merge 1 commit into from

Conversation

@dougxc
Copy link
Member

@dougxc dougxc commented Feb 19, 2021

Currently, HotSpotJVMCIRuntime.writeDebugOutput does nothing if the current thread is not attached to HotSpot (i.e., Thread::current_or_null() == NULL). This means crucial debug info can be lost. For reference, an unattached libgraal thread is a thread started from within libgraal that has not yet attached itself to the VM (e.g., before this line) or has already detached itself (e.g., after this line).

The reason for the current behavior is that HotSpotJVMCIRuntime.writeDebugOutput passes a Java byte array to C++ code and the C++ code calls back into Java to decode the byte array into a native buffer. These call backs require the current thread to be attached to the VM.

This PR moves the Java-to-native-buffer decoding into Java and thus avoids the requirement for the current thread to be attached to the VM.

Tested in libgraal by patching Graal as follows:

diff --git a/compiler/src/org.graalvm.compiler.core/src/org/graalvm/compiler/core/GraalServiceThread.java b/compiler/src/org.graalvm.compiler.core/src/org/graalvm/compiler/core/GraalServiceThread.java
index 36064767c95..352395dd59b 100644
--- a/compiler/src/org.graalvm.compiler.core/src/org/graalvm/compiler/core/GraalServiceThread.java
+++ b/compiler/src/org.graalvm.compiler.core/src/org/graalvm/compiler/core/GraalServiceThread.java
@@ -43,7 +43,14 @@ public class GraalServiceThread extends Thread {
         try {
             runnable.run();
         } finally {
+            String debug = System.getenv("GraalServiceThread.debug");
             afterRun();
+            if ("true".equals(debug)) {
+                throw new InternalError("THROWN AFTER DETACHING");
+            }
         }
     }

Running without the changes in this PR:

> env GraalServiceThread.debug=true java -jar dacapo.jar avrora
===== DaCapo 9.12 avrora starting =====
===== DaCapo 9.12 avrora PASSED in 4270 msec =====

Running with the changes in this PR:

> env GraalServiceThread.debug=true java -jar dacapo.jar avrora
===== DaCapo 9.12 avrora starting =====
Exception in thread "LibGraalHotSpotGraalManagement-init" java.lang.InternalError: THROWN AFTER DETACHING
	at org.graalvm.compiler.core.GraalServiceThread.run(GraalServiceThread.java:52)
	at com.oracle.svm.core.thread.JavaThreads.threadStartRoutine(JavaThreads.java:519)
	at com.oracle.svm.core.posix.thread.PosixJavaThreads.pthreadStartRoutine(PosixJavaThreads.java:192)
===== DaCapo 9.12 avrora PASSED in 4688 msec =====

Progress

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

Issue

  • JDK-8262011: [JVMCI] allow printing to tty from unattached libgraal thread

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 19, 2021

👋 Welcome back dnsimon! 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
Copy link

@openjdk openjdk bot commented Feb 19, 2021

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

  • hotspot-compiler

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.

@@ -216,7 +216,9 @@ void JVMCI::vlog(int level, const char* format, va_list ap) {
StringEventLog* events = level == 1 ? _events : _verbose_events;
guarantee(events != NULL, "JVMCI event log not yet initialized");
Thread* thread = Thread::current_or_null_safe();
events->logv(thread, format, ap);
if (thread != NULL) {
events->logv(thread, format, ap);
Copy link
Member Author

@dougxc dougxc Feb 19, 2021

This fixes a bug found while testing this PR. The StringEventLog::logv method requires the current thread to not be NULL.

@dougxc dougxc marked this pull request as ready for review Feb 19, 2021
@openjdk openjdk bot added the rfr label Feb 19, 2021
@@ -1,163 +0,0 @@
/*
Copy link
Member Author

@dougxc dougxc Feb 19, 2021

DebugOutputTest has been removed since it's redundant with TestHotSpotJVMCIRuntime.writeDebugOutputTest.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 19, 2021

Webrevs

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Seems fine.

@openjdk
Copy link

@openjdk openjdk bot commented Feb 19, 2021

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

8262011: [JVMCI] allow printing to tty from unattached libgraal thread

Reviewed-by: kvn, never

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

  • 0257caa: 8261397: Try Catch Method Failing to Work When Dividing An Integer By 0
  • 8a2f589: 8260637: Shenandoah: assert(_base == Tuple) failure during C2 compilation
  • 67762de: 8262197: JDK-8242032 uses wrong contains_reference() in assertion code
  • 9d9bedd: 8262094: Handshake timeout scaled wrong
  • 29c7263: 8252709: Enable JVMCI when building linux-aarch64 at Oracle
  • 12f6ba0: 8262087: Use atomic boolean type in G1FullGCAdjustTask
  • a5c4b9a: 8260223: Handling of unnamed package in javadoc pages
  • 8cfea7c: 8261921: ClassListParser::current should be used only by main thread
  • 991f7c1: 8210373: Deadlock in libj2gss.so when loading "j2gss" and "net" libraries in parallel.
  • 0217d69: 8261975: Missing "classpath exception" in VectorSupport.java
  • ... and 40 more: https://git.openjdk.java.net/jdk/compare/61820b74dd804899f44aa21813d94c9bd2661d6a...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.

@openjdk openjdk bot added the ready label Feb 19, 2021
Copy link
Contributor

@tkrodriguez tkrodriguez left a comment

Looks good.

@dougxc
Copy link
Member Author

@dougxc dougxc commented Feb 23, 2021

/integrate

@openjdk openjdk bot closed this Feb 23, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Feb 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 23, 2021

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

  • 0257caa: 8261397: Try Catch Method Failing to Work When Dividing An Integer By 0
  • 8a2f589: 8260637: Shenandoah: assert(_base == Tuple) failure during C2 compilation
  • 67762de: 8262197: JDK-8242032 uses wrong contains_reference() in assertion code
  • 9d9bedd: 8262094: Handshake timeout scaled wrong
  • 29c7263: 8252709: Enable JVMCI when building linux-aarch64 at Oracle
  • 12f6ba0: 8262087: Use atomic boolean type in G1FullGCAdjustTask
  • a5c4b9a: 8260223: Handling of unnamed package in javadoc pages
  • 8cfea7c: 8261921: ClassListParser::current should be used only by main thread
  • 991f7c1: 8210373: Deadlock in libj2gss.so when loading "j2gss" and "net" libraries in parallel.
  • 0217d69: 8261975: Missing "classpath exception" in VectorSupport.java
  • ... and 40 more: https://git.openjdk.java.net/jdk/compare/61820b74dd804899f44aa21813d94c9bd2661d6a...master

Your commit was automatically rebased without conflicts.

Pushed as commit d2b9c22.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants