Bump :)
Best Regards
Adam Farley
---- Last email ----
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
[attachment "hg_diff.txt" deleted by Adam Farley8/UK/IBM]
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
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)
