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

[Krystal Mo]

On 2012/10/17 19:31, 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. 
Yes, I agree. And that's exactly why I implemented 7176856 (add the JRE 
name to the error log) to do the upcall in an early stage, so that no 
upcalls are needed during crash reporting.

> 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.
>
For this kind of scenario I'd agree it's good enough to use the initial 
classpath.

Thanks,
Kris

> 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
>