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

[David Holmes]

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