RFR(M): 8165550: Add class loader names to ClassCastException message

[David Holmes]

Hi Rachel,

On 15/11/2016 7:00 AM, Rachel Protacio wrote:
> New webrev: http://cr.openjdk.java.net/~rprotacio/8165550.01/
>
> Corrected according to David's review. Comments below.

Thanks. Only one follow up that I missed the first time (and which is a 
pre-existing bug) in sharedRuntime.cpp:

1996   char* message = NEW_RESOURCE_ARRAY(char, msglen);
1997
1998   // Just return the FQN if error in allocating string
1999   if (message == NULL) {
2000     return fqn;
2001   }

If NEW_RESOURCE_ARRAY can't allocate it will not return but cause 
vm_exit_out_of_memory to be called. We should be using 
NEW_RESOURCE_ARRAY_RETURN_NULL here.

Thanks,
David
-----

>
> On 11/10/2016 6:24 PM, David Holmes wrote:
>> Okay on with the review:
>>
>> src/share/vm/classfile/moduleEntry.cpp
>>
>> +     const char* loc = "";
>> +     loc = location()->as_C_string();
>>
>> can just be:
>>
>> +     const char* loc = location()->as_C_string();
>>
> Fixed.
>> ---
>>
>> src/share/vm/runtime/sharedRuntime.cpp
>>
>> 1941 const char* class_loader_and_module_name(Klass* klass, Klass*
>> base_klass) {
>>
>> I'm unclear in what way "base_klass" is a "base" class ?? And why, if
>> it is not an instance class must be it in java.base ??
> I really meant "bottom" class, following the logic used to parse it out.
> I've moved that internal to the function now. So if the klass is an
> instance klass or instance klass array, we look for further information.
> Otherwise (if it's a primitive array), it's from java.base.
>>
>> Nit: I don't think these need to be #defines nor have such verbose names:
>>
>> 1936 #define CLASS_LOADER_NAME_DELIMITER "/"
>> 1937 #define CLASS_LOADER_NAME_DELIMITER_LENGTH
>> strlen(CLASS_LOADER_NAME_DELIMITER)
>>
>> why not just
>>
>> const char* delim = "\";
>> int delim_len = strlen(delim);
>>
>> Plus you are inconsistent with the handling of the "/" delimiter
>> versus the "@" delimiter.
> Fixed.
>>
>> ---
>>
>>  test/runtime/modules/CCE_module_msg.java
>>
>> Nit: in the catch block from line #95 can you store getMessage() into
>> a local so it isn't called 5 times. Thanks. :)
> Fixed too. Thanks!
> Rachel
>>
>> Thanks,
>> David
>>
>>> Rachel
>>>
>>> On 11/10/2016 5:02 PM, David Holmes wrote:
>>>> Hi Rachel,
>>>>
>>>> Just wondering, in the most complex case, exactly what will this new
>>>> message look like? We seem to be including module names as well as
>>>> classloader names, and of course class names.
>>>>
>>>> Arguably we should only add the additional info when they differ
>>>> between the types ie if both in same classloader and same module then
>>>> only report class name; if same loader but different module report
>>>> module:class; if different loaders then report everything
>>>> loader:module:class. Also not sure module version is needed at all in
>>>> this message.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> On 11/11/2016 7:38 AM, Rachel Protacio wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this Jigsaw enhancement which adds the new format of
>>>>> class
>>>>> loader names to VM ClassCastException messages. Includes an
>>>>> updated/expanded jtreg test.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8165550
>>>>> Open webrev: http://cr.openjdk.java.net/~rprotacio/8165550.00/
>>>>>
>>>>> Passes JPRT and jck vm and lang tests.
>>>>>
>>>>> Thanks!
>>>>> Rachel
>>>
>