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

[Krystal Mo]

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