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

[David Holmes]

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.

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

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