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

harold seigel harold.seigel at oracle.com
Wed Oct 17 09:36:40 PDT 2012 

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/e98efdff/attachment.html

More information about the hotspot-runtime-dev mailing list