Request for review 7053130: hs_err file does not record specified CLASSPATH

[David Holmes]

Looks good to me.

Thanks,
David

On 18/10/2012 1:31 AM, harold seigel wrote:
> Here is an updated webrev.  It includes calling
> _java_class_path->value() and adds the word "(initial)" to the output.
>
> Sample hs_err... file:
>
>                  ...
>     VM Arguments:
>     jvm_args: -XX:+CrashGCForDumpingJavaThread
>     java_command: Queens
>     java_class_path (initial): .:/my/home/
>     Launcher Type: SUN_STANDARD
>
>     Environment Variables:
>     JAVA_HOME=/java/home/jdk1.8.0/
>     CLASSPATH=.:/path/way:/home
>                  ...
>
> Open webrev at http://cr.openjdk.java.net/~coleenp/Bug7053130
> <http://cr.openjdk.java.net/%7Ecoleenp/Bug7053130>
>
> Bug link at http://bugs.sun.com/view_bug.do?bug_id=7053130
>
> Thanks, Harold
>
>
> On 10/17/2012 7:31 AM, Coleen Phillimore wrote:
>> On 10/17/2012 4:47 AM, David Holmes wrote:
>>> Hi Kris,
>>>
>>> Sorry I overlooked the different potential behaviours of
>>> JvmtiEnv::AddToSystemClassLoaderSearch. What I referenced was only
>>> the onload-phase behaviour. As you say at runtime (live phase) it
>>> asks the system classloader to dynamically update its notion of the
>>> "classpath".
>>>
>>> Given we have crashed there is no way that we can safely make the
>>> upcalls to Java that would be needed to find the actual current
>>> classpath (invoke getURLs() on a URLClassLoader and process the
>>> resulting array of URL instances!). The output could indicate that
>>> this is the initial classpath.
>>
>> Yes, we should avoid upcalls to Java during crash handling.  If the
>> only way to get this class path is this value in Arguments, then
>> that's better than what we had.   Having an (initial) specified would
>> be useful.   I think the purpose is so someone can use the
>> java_command, java_args and -cp java_classpath to reproduce a failure,
>> so the initial value is good enough for most of the time for this
>> purpose.
>>
>> thanks,
>> Coleen
>>>
>>> Thanks,
>>> David
>>>
>>> On 17/10/2012 5:22 PM, Krystal Mo wrote:
>>>> Hi David,
>>>>
>>>> Comments inline:
>>>>
>>>> On 2012/10/17 12:51, David Holmes wrote:
>>>>> On 17/10/2012 7:25 AM, Krystal Mo wrote:
>>>>>> I'm okay with the change as-is, but wonder if this is a good-enough
>>>>>> tradeoff when Java agents are in the picture.
>>>>>>
>>>>>> There's a method in Java agent that could change the effective system
>>>>>> class path, namely
>>>>>> java.lang.instrument.Instrumentation.appendToSystemClassLoaderSearch(JarFile
>>>>>>
>>>>>>
>>>>>> file). It's ultimately implemented by
>>>>>> JvmtiEnv::AddToSystemClassLoaderSearch(const char* segment), in
>>>>>> prims/jvmtiEng.cpp. As you can see from the example of this function,
>>>>>> the effective system class path doesn't have to be the one stored in
>>>>>> Arguments::_java_class_path; in fact, after java.lang.System class is
>>>>>> initialized, the copy in Arguments::_java_class_path doesn't matter
>>>>>> anymore.
>>>>>> I'd like to hear other people's opinion on this issue.
>>>>>
>>>>> Arguments::_java_class_path is always the SystemProperty that holds
>>>>> java.class.path. It acts as a shortcut to that SystemProperty object
>>>>> to avoid the need to iterate through the list of system properties and
>>>>> match the key (at least within the argument processing code). The
>>>>> JVMTI code does have to do an iteration:
>>>>>
>>>>> for (SystemProperty* p = Arguments::system_properties(); p != NULL; p
>>>>> = p->next()) {
>>>>> if (strcmp("java.class.path", p->key()) == 0) {
>>>>> p->append_value(segment);
>>>>> break;
>>>>> }
>>>>> }
>>>>>
>>>>> but it appends to the SystemProperty object that
>>>>> Arguments::_java_class_path points to.
>>>>>
>>>> The JVMTI code was an example which demonstrated what I was trying to
>>>> say: the effective system class path isn't always the one stored in
>>>> Arguments::_java_class_path. That SystemProperty is only effective
>>>> during VM initialization; after the VM is initialized, all system
>>>> properties are copied to the Java side (in
>>>> java.lang.System.initProperties() -> JVM_InitProperties()), and then
>>>> the
>>>> copy on the VM side doesn't really matter anymore. The two copies could
>>>> hold different contents after initialization, and only the copy on the
>>>> Java side matters.
>>>>
>>>> Printing the contents of Arguments::_java_class_path would be good
>>>> enough for most cases, because we don't usually expect users to tweak
>>>> with the system properties initialization, so the copies of
>>>> java.class.path on both the VM and Java side should be the same. There
>>>> are exceptions to that, and the Java agent/JVMTI code I showed is an
>>>> example.
>>>>
>>>> The question is whether or not it's worth picking up the contents of
>>>> java.class.path from the Java side and print that to the hs_err
>>>> file, or
>>>> is it good enough to trade a possible loss of accuracy of
>>>> java.class.path for a simpler implementation in the VM (Harold's
>>>> version). I don't have a strong opinion on either options, just trying
>>>> to bring up the issue so that we wouldn't miss it.
>>>>
>>>>>> Nitpicking: Why did you use PropertyList_get_value() to get the
>>>>>> value of
>>>>>> _java_class_path, instead of just calling _java_class_path->value()?
>>>>>> If you're going to use PropertyList_get_value() then you don't
>>>>>> need to
>>>>>> do the null check upfront because PropertyList_get_value() does
>>>>>> that for
>>>>>> you already.
>>>>>
>>>>> Not such a nit-pick. There is no need to use PropertyList_get_value as
>>>>> it is really only intended to be used when you only know the key ie:
>>>>>
>>>>> value =
>>>>> Arguments::PropertyList_get_value(Arguments::system_properties(),
>>>>> property);
>>>>>
>>>>> For the classpath, as I said above we have a direct pointer to the
>>>>> SystemProperty instance. So the logic simplifies to:
>>>>>
>>>>> if ((_java_class_path != NULL) {
>>>>> st->print_cr("java_class_path: %s", _java_class_path->value());
>>>>> }
>>>>>
>>>>> or if you want something special for the "not-set" case:
>>>>>
>>>>> if ((_java_class_path != NULL) {
>>>>> char* path = _java_class_path->value();
>>>>> st->print_cr("java_class_path: %s", strlen(path) == 0 ? "<not set>" :
>>>>> path );
>>>>> }
>>>>>
>>>>> _java_class_path can only be NULL very early on in the VM
>>>>> initialization process, and once set its value() can not be NULL (well
>>>>> set_value(NULL) will potentially crash but it's up to set_value to add
>>>>> a guard for that).
>>>>>
>>>> Yes, I agree with David on this part.
>>>>
>>>> - Kris
>>>>
>>>>> David
>>>>> -----
>>>>>
>>>>>> - Kris
>>>>>>
>>>>>> On 2012/10/17 4:43, harold seigel wrote:
>>>>>>> Summary: Added code to write the value of the java.class.path
>>>>>>> property to the hs_err file. The new text in the hs_err file would
>>>>>>> look like this:
>>>>>>>
>>>>>>> ...
>>>>>>> VM Arguments:
>>>>>>> jvm_args: -XX:+CrashGCForDumpingJavaThread
>>>>>>> java_command: Queens
>>>>>>> java_class_path: .:/root/dir:/home
>>>>>>> Launcher Type: SUN_STANDARD
>>>>>>>
>>>>>>> Environment Variables:
>>>>>>> JAVA_HOME=/java/udrt/jdk1.8.0/
>>>>>>> CLASSPATH=.:/home/:/some/where
>>>>>>> ....
>>>>>>>
>>>>>>>
>>>>>>> Open webrev at http://cr.openjdk.java.net/~coleenp/Bug7053130
>>>>>>> <http://cr.openjdk.java.net/%7Ecoleenp/Bug7053130>
>>>>>>>
>>>>>>> Bug link at http://bugs.sun.com/view_bug.do?bug_id=7053130
>>>>>>>
>>>>>>> Tested by generating hs_err files and looking at their contents.
>>>>>>>
>>>>>>> Thanks, Harold
>>