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

[David Holmes]

Hi Rachel,

On 11/11/2016 8:31 AM, Rachel Protacio wrote:
> Ah yes, my apologies - I should have provided more context. This is the
> VM corollary of Mandy's change
> http://hg.openjdk.java.net/jdk9/dev/hotspot/rev/48fce55afe71 which
> implements the new classloader name scheme and uses it in Stack Traces.
> In the fullest instance, it could look like
>     MyClassLoader/module1 at 9.0/package1.class1
> The class loader is not listed if it is built-in, the module is not
> listed if it is unnamed, the version is not listed if it is a "non-jdk"
> module.

Ah I see. Though not at all clear why we would care about showing the 
JDK module version ??

> I appreciate your point about shortening this specific CCE message to
> not repeat information, but I think there's a stronger case for
> maintaining a VM-wide format for all ClassLoader-module-fqn strings.
> This format will be used for more messages as well, e.g. unified logging
> for class loading.  (The follow-up bug:
> https://bugs.openjdk.java.net/browse/JDK-8169559.) And most importantly,
> because there are the specific instances listed above for leaving out
> the CL or module, it could potentially be more confusing to use
> shorthand. Would the user know that the module isn't listed the second
> time because it's the same module or because it's the unnamed module?

Yes good point.

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();

---

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

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.

---

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

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
>