Hi Alan Thanks for getting back to me on this. :)
I've changed the hg_diff as described below, see the attached. > On 27/02/2018 15:04, Adam Farley8 wrote: > > Resending. Bump. :) > > On 14/02/2018 14:13, Adam Farley8 wrote: >>> Hi All, >>> >>> -- Short version -- >>> >>> Could a committer please take the fix for JDK-8190187 (full code included >>> in the bug) and: >>> >>> 1) Complete the CSR process for the new JNI Return code. >>> 2) Commit the changes that contain (a) the new return code, and (b) the >>> non-Hotspot code that handles the new code. >> The patches attached to the JIRA issue are missing the changes to the >> JVM TI spec (jvmti.xml). > >> I'm not seeing the JNI return codes in that file. Are you after one of those >> dated updates near the bottom? >> e.g. >> ``` >> <change date="14 February 2018" version="11.0.0"> >> Added JNI_SILENT_EXIT return code for early exits without errors. >> java.c's ParseArguments function now sets pret value to 2 for a NULL pwhat. >> This allows us to clearly identify when no class was set, but no other error has occurred. >> This is undone in java.c's ContinueInNewThread method, so the surface behaviour does not change. >> </change> >> ``` > The "Agent Start-Up" section is the section to look at. The important part is: > > "The return value from Agent_OnLoad or Agent_OnLoad_<agent-lib-name> is used to indicate an error. Any value other than zero indicates an error and causes termination of the VM." > > If there is special return value to mean "VM terminates without error" then this part of the spec will need to be adjusted. Ah, that makes sense. I altered that bit and regenerated the hg_diff. > An additional point is that you can start several agents from the command line, does the VM terminate after it has started all agents or does it terminate when the first agent returns asks the VM to terminate quietly? If I'm reading the code correctly, the loop that initialises the different agents (which I believe to be the loop containing "// Invoke the Agent_OnLoad function") is not interrupted by the return of a silent exit code, however it only takes one agent returning this code to cause the VM to be destroyed once startup is complete. > > > > > >>> There is also text to be written for the JNI spec if this proposal goes >>> ahead. > >> I assume you mean the "RETURNS" section of the JNI_CreateJavaVM >> bit on the invocation.html web page. Something like this? > >> ``` >> RETURNS: >> Returns JNI_OK on success; returns a suitable JNI error code (a negative number) on failure. >> >> The sole exception is a silent exit, which returns JNI error code JNI_SILENT_EXIT. >> This indicates that the VM cannot be used, but that this is the intended behaviour for the >> arguments used. E.g. -Xlog:help (which prints help output and then destroys the VM) >> ``` > Yes, this is the place that will need changes. > Ok. The most official-looking place for this documentation is on the Oracle website. I'm not sure how to go about making this change happen though. Is the change to this documentation something I can push through one of the mailing lists? Or is it perhaps part of the CSR process? > > > > >>> >>> I don't agree that the launcher should be looking for >>> "-agentlib:jdwp=help" in the command line as it's just one of many ways >>> that the debugger agent might be started (e.g. -Xrunjdwp:<options>, >>> _JAVA_TOOLS_OPTIONS, ...). > >> We can avoid that by finding a way around this line in ContinueInNewThread (java.c): > >> ``` >> return (ret != 0) ? ret : rslt; >> ``` > >> I have devised a means to do this, as outlined in the jvmti.xml change above. I put the >> changes into a recent clone of jdk/jdk, and attached the hg diff, along with an improved >> test. > > The launcher should only need to look at the return value from JNI CreateJavaVM. I don't think it should do any special handling for the JDWP or other agents (there are just too many ways to inject command lines and the launcher cannot be expected to handle all of them). > >-Alan > I agree. The change I made in response to your earlier comment is not jdwp-specific. Rather, it sets "ret" to "2" if the user has not specified an executable class in the command line. I did this because a ret value of "1" indicates an error, but we don't want to override a return code of JNI_SILENT_EXIT with ret's value if the only error was "no class was specified". So I set ret to "2" if no class is specified, and altered the aforementioned bit in ContinueInNewThread (java.c) to pick up on a ret value of 2. If the ret value is 2, and rslt is JNI_SILENT_EXIT, then we know that no error has occurred (other than "no class was specified"), and that JNI_SILENT_EXIT should be returned, as opposed to the current functionality, where it is the non-0 ret value which is returned (erroneously, in the event of a silent exit). I think my changeset explains this more concisely than I have. :) Let me know if this has just made you more confused. :) - Adam Unless stated otherwise above: IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
diff --git a/src/hotspot/share/prims/jni.cpp b/src/hotspot/share/prims/jni.cpp --- a/src/hotspot/share/prims/jni.cpp +++ b/src/hotspot/share/prims/jni.cpp @@ -3909,7 +3909,7 @@ bool can_try_again = true; result = Threads::create_vm((JavaVMInitArgs*) args, &can_try_again); - if (result == JNI_OK) { + if ((result == JNI_OK) || (result == JNI_SILENT_EXIT)) { JavaThread *thread = JavaThread::current(); assert(!thread->has_pending_exception(), "should have returned not OK"); /* thread is thread_in_vm here */ diff --git a/src/hotspot/share/prims/jvmti.xml b/src/hotspot/share/prims/jvmti.xml --- a/src/hotspot/share/prims/jvmti.xml +++ b/src/hotspot/share/prims/jvmti.xml @@ -631,8 +631,10 @@ </rationale> <p/> The return value from <code>Agent_OnLoad</code> or - <code>Agent_OnLoad_<agent-lib-name></code> is used to indicate an error. - Any value other than zero indicates an error and causes termination of the VM. + <code>Agent_OnLoad_<agent-lib-name></code> is used to indicate whether + the agent successfully initialised or not. JNI_OK indicates success, JNI_SILENT_EXIT + indicates the agent did not fully initialize but that no error occurred, and any + other value indicates an error. Non-JNI_OK return codes cause termination of the VM. </intro> <intro id="onattach" label="Agent Start-Up (Live phase)"> @@ -14811,6 +14813,12 @@ - The function may return NULL in the start phase if the can_generate_early_vmstart capability is enabled. </change> + <change date="14 February 2018" version="11.0.0"> + Added JNI_SILENT_EXIT return code for early exits without errors. + java.c's ParseArguments function now sets pret value to 2 for a NULL pwhat. Addendums: + - This allows us to clearly identify when no class was set, but no other error has occurred. + - This is undone in java.c's ContinueInNewThread method, so the surface behaviour does not change. + </change> </changehistory> </specification> diff --git a/src/hotspot/share/runtime/thread.cpp b/src/hotspot/share/runtime/thread.cpp --- a/src/hotspot/share/runtime/thread.cpp +++ b/src/hotspot/share/runtime/thread.cpp @@ -3637,6 +3637,9 @@ } jint Threads::create_vm(JavaVMInitArgs* args, bool* canTryAgain) { + /* This gets returned at the end of the method. */ + /* It allows us to complete vm initialisation and still return an error code if we want. */ + jint jniReturnCode = JNI_OK; extern void JDK_Version_init(); // Preinitialize version info. @@ -3725,7 +3728,9 @@ // Launch -agentlib/-agentpath and converted -Xrun agents if (Arguments::init_agents_at_startup()) { - create_vm_init_agents(); + if (create_vm_init_agents() == JNI_SILENT_EXIT) { + jniReturnCode = JNI_SILENT_EXIT; + } } // Initialize Threads state @@ -3991,7 +3996,7 @@ ShouldNotReachHere(); } - return JNI_OK; + return jniReturnCode; } // type for the Agent_OnLoad and JVM_OnLoad entry points @@ -4110,9 +4115,10 @@ // Create agents for -agentlib: -agentpath: and converted -Xrun // Invokes Agent_OnLoad // Called very early -- before JavaThreads exist -void Threads::create_vm_init_agents() { +jint Threads::create_vm_init_agents() { extern struct JavaVM_ main_vm; AgentLibrary* agent; + jint jniReturnCode = JNI_OK; JvmtiExport::enter_onload_phase(); @@ -4123,13 +4129,18 @@ // Invoke the Agent_OnLoad function jint err = (*on_load_entry)(&main_vm, agent->options(), NULL); if (err != JNI_OK) { - vm_exit_during_initialization("agent library failed to init", agent->name()); + if (err == JNI_SILENT_EXIT) { + jniReturnCode = JNI_SILENT_EXIT; + } else { + vm_exit_during_initialization("agent library failed to init", agent->name()); + } } } else { vm_exit_during_initialization("Could not find Agent_OnLoad function in the agent library", agent->name()); } } JvmtiExport::enter_primordial_phase(); + return jniReturnCode; } extern "C" { diff --git a/src/hotspot/share/runtime/thread.hpp b/src/hotspot/share/runtime/thread.hpp --- a/src/hotspot/share/runtime/thread.hpp +++ b/src/hotspot/share/runtime/thread.hpp @@ -2153,7 +2153,7 @@ static jint create_vm(JavaVMInitArgs* args, bool* canTryAgain); static void convert_vm_init_libraries_to_agents(); static void create_vm_init_libraries(); - static void create_vm_init_agents(); + static jint create_vm_init_agents(); static void shutdown_vm_agents(); static bool destroy_vm(); // Supported VM versions via JNI diff --git a/src/java.base/share/native/include/jni.h b/src/java.base/share/native/include/jni.h --- a/src/java.base/share/native/include/jni.h +++ b/src/java.base/share/native/include/jni.h @@ -164,6 +164,7 @@ #define JNI_ENOMEM (-4) /* not enough memory */ #define JNI_EEXIST (-5) /* VM already created */ #define JNI_EINVAL (-6) /* invalid arguments */ +#define JNI_SILENT_EXIT (-7) /* vm shut down with no error */ /* * used in ReleaseScalarArrayElements diff --git a/src/java.base/share/native/libjli/java.c b/src/java.base/share/native/libjli/java.c --- a/src/java.base/share/native/libjli/java.c +++ b/src/java.base/share/native/libjli/java.c @@ -75,6 +75,7 @@ static jboolean listModules = JNI_FALSE; static char *describeModule = NULL; static jboolean validateModules = JNI_FALSE; +static jboolean silentExit = JNI_FALSE; static const char *_program_name; static const char *_launcher_name; @@ -413,6 +414,12 @@ exit(1); } + // Exit without error if the vm has returned JNI_SILENT_EXIT. + if (silentExit) { + CHECK_EXCEPTION_LEAVE(1); + LEAVE(); + } + if (showSettings != NULL) { ShowSettings(env, showSettings); CHECK_EXCEPTION_LEAVE(1); @@ -1426,7 +1433,9 @@ if (*pwhat == NULL) { /* LM_UNKNOWN okay for options that exit */ if (!listModules && !describeModule && !validateModules) { - *pret = 1; + if (*pret != 1) { + *pret = 2; + } } } else if (mode == LM_UNKNOWN) { /* default to LM_CLASS if -m, -jar and -cp options are @@ -1477,7 +1486,10 @@ r = ifn->CreateJavaVM(pvm, (void **)penv, &args); JLI_MemFree(options); - return r == JNI_OK; + if (r == JNI_SILENT_EXIT) { + silentExit = JNI_TRUE; + } + return ((r == JNI_OK) || (r == JNI_SILENT_EXIT)); } static jclass helperClass = NULL; @@ -2314,8 +2326,17 @@ rslt = ContinueInNewThread0(JavaMain, threadStackSize, (void*)&args); /* If the caller has deemed there is an error we * simply return that, otherwise we return the value of - * the callee + * the callee. The sole exception is if ret is 2, rslt + * is 0, and silentExit is true. This indicates no class + * was specified, no error happened when starting the vm, + * and one of the options used triggered a silent exit. */ + if (ret == 2) { + if ((silentExit == JNI_TRUE) && (rslt == 0)) { + return 0; + } + ret = 1; + } return (ret != 0) ? ret : rslt; } } diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c --- a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c +++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c @@ -102,6 +102,7 @@ static void initialize(JNIEnv *env, jthread thread, EventIndex triggering_ei); static jboolean parseOptions(char *str); +static jboolean isHelp(char* options); /* * Phase 1: Initial load. @@ -204,6 +205,11 @@ jint jvmtiCompileTimeMinorVersion; jint jvmtiCompileTimeMicroVersion; + /* If we were sent a help option, we only need to print usage and return a JNI_SILENT_EXIT rc */ + if (isHelp(options)) { + return JNI_SILENT_EXIT; + } + /* See if it's already loaded */ if ( gdata!=NULL && gdata->isLoaded==JNI_TRUE ) { ERROR_MESSAGE(("Cannot load this JVM TI agent twice, check your java command line for duplicate jdwp options.")); @@ -384,7 +390,10 @@ DEF_Agent_OnUnload(JavaVM *vm) { - gdata->isLoaded = JNI_FALSE; + //If gdata was not initialised, we can't set isLoaded to false here. + if (gdata!=NULL) { + gdata->isLoaded = JNI_FALSE; + } /* Cleanup, but make sure VM is alive before using JNI, and * make sure JVMTI environment is ok before deallocating @@ -1022,9 +1031,10 @@ } /* Check for "help" BEFORE we add any environmental settings */ - if ((strcmp(options, "help")) == 0) { + /* Agent_OnLoad should have already handled this. Including here for completeness. */ + if (isHelp(options)) { printUsage(); - forceExit(0); /* Kill entire process, no core dump wanted */ + return JNI_FALSE; /* If this is a help option, we should shut down now. */ } /* These buffers are never freed */ @@ -1295,6 +1305,24 @@ return JNI_FALSE; } +/* Returns true if the options contains "help" */ +/* Useful when you want to identify a help option without parsing the entire set of options. */ +static jboolean +isHelp(char* options) +{ + /* Options being NULL will end up being an error. */ + if (options == NULL) { + options = ""; + } + + /* Check for "help" */ + if ((strcmp(options, "help")) == 0) { + printUsage(); + return JNI_TRUE; + } + return JNI_FALSE; +} + /* All normal exit doors lead here */ void debugInit_exit(jvmtiError error, const char *msg)