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

Coleen Phillimore coleen.phillimore at oracle.com
Wed Oct 17 10:17:45 PDT 2012 

On 10/17/2012 12:58 PM, Krystal Mo wrote:
> Yes, got it now. Looks good to me. (But I'm not a hsx reviewer yet :-)

That's okay.   Your review is very valuable!
Thanks,
Coleen

>
> - Kris
>
> On 2012/10/18 0:36, 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
>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20121017/199ef42d/attachment-0001.html

More information about the hotspot-runtime-dev mailing list