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

[Lois Foltan]

On 11/14/2016 6:43 PM, David Holmes wrote:
> 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.

I think line #2023 in SharedRuntime::generate_class_cast_message() 
should be changed in the same manner as well?
Lois

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