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

[Dmitry Samersoff]

David,

May be it's good to write

java_*startup*_class_path: .:/root/dir:/home

instead of just java_class_path, in case we decide to add runtime
classpath somewhere in a future.

-Dmitry

On 2012-10-17 12:47, 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.
> 
> 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


-- 
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...