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

Vladimir Kozlov vladimir.kozlov at oracle.com
Wed Oct 17 10:07:41 PDT 2012 

Good.

Vladimir

harold seigel wrote:
>   Hi Kris,
> 
> Thanks for pointing this out.  Could you take another look?  It just got 
> updated.
> 
> Harold
> 
> On 10/17/2012 11:44 AM, Krystal Mo wrote:
>> Hi Harold,
>>
>> The webrev link looks the same as the previous one. Is there a newer 
>> one somewhere?
>>
>> - Kris
>>
>> On 2012/10/17 23:31, 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
>>>>

More information about the hotspot-runtime-dev mailing list